| Message ID | 20170818113558.71928-1-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Aug 18, 2017 at 12:35:58PM +0100, Salil Mehta wrote: > This patch adds the following support to the HNS3 driver: > 1. Support to change the Maximum Transmission Unit of a > netdevice and of a port in hardware . > 2. Initializes the supported MTU range for the netdevice. > > Signed-off-by: lipeng <lipeng321@huawei.com> > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > --- > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++++++++++++++++++++++ > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 1 + > 2 files changed, 47 insertions(+) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > index e731f87..8e3711e 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > @@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, > return ret; > } > > +static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu) > +{ > + struct hns3_nic_priv *priv = netdev_priv(netdev); > + struct hnae3_handle *h = priv->ae_handle; > + bool if_running = netif_running(netdev); > + int ret; > + > + /* no change in MTU */ > + if (new_mtu == netdev->mtu) > + return 0; Hi Salil It is worth reading the core code: http://elixir.free-electrons.com/linux/latest/source/net/core/dev.c#L6713 + > + if (!h->ae_algo->ops->set_mtu) > + return -ENOTSUPP; > + > + /* if this was called with netdev up then bring netdevice down */ > + if (if_running) { > + (void)hns3_nic_net_stop(netdev); > + msleep(100); > + } > + > + ret = h->ae_algo->ops->set_mtu(h, new_mtu); > + if (ret) { > + netdev_err(netdev, "failed to change MTU in hardware %d\n", > + ret); > + return ret; > + } > + > + /* assign newly changed mtu to netdevice as well */ > + netdev->mtu = new_mtu; http://elixir.free-electrons.com/linux/latest/source/net/core/dev.c#L6698 > + > + /* if the netdev was running earlier, bring it up again */ > + if (if_running) { > + if (hns3_nic_net_open(netdev)) { > + netdev_err(netdev, "MTU, couldnt up netdev again\n"); > + ret = -EINVAL; > + } > + } > + > + return ret; > +} > + > static const struct net_device_ops hns3_nic_netdev_ops = { > .ndo_open = hns3_nic_net_open, > .ndo_stop = hns3_nic_net_stop, > .ndo_start_xmit = hns3_nic_net_xmit, > .ndo_set_mac_address = hns3_nic_net_set_mac_address, > + .ndo_change_mtu = hns3_nic_change_mtu, > .ndo_set_features = hns3_nic_set_features, > .ndo_get_stats64 = hns3_nic_get_stats64, > .ndo_setup_tc = hns3_nic_setup_tc, > @@ -2752,6 +2794,10 @@ static int hns3_client_init(struct hnae3_handle *handle) > goto out_reg_netdev_fail; > } > > + /* MTU range: 68 - 9706 */ > + netdev->min_mtu = ETH_MIN_MTU; http://elixir.free-electrons.com/linux/latest/source/net/ethernet/eth.c#L361 > + netdev->max_mtu = HNS3_MAX_MTU - (ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN); > + > return ret; > > out_reg_netdev_fail: > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > index a6e8f15..7e87461 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > @@ -76,6 +76,7 @@ enum hns3_nic_state { > #define HNS3_RING_NAME_LEN 16 > #define HNS3_BUFFER_SIZE_2048 2048 > #define HNS3_RING_MAX_PENDING 32768 > +#define HNS3_MAX_MTU 9728 It seems odd that it does not already exists somewhere. The core does not enforce the MTU. You could be passed a frame which is bigger. So you should check before trying to pass something to the hardware which the hardware cannot handle. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Friday, August 18, 2017 2:31 PM > To: Salil Mehta > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); lipeng (Y); > dan.carpenter@oracle.com; mehta.salil.lnk@gmail.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > rdma@vger.kernel.org; Linuxarm > Subject: Re: [PATCH net-next] net: hns3: Add support to change MTU in > hardware & netdev > > On Fri, Aug 18, 2017 at 12:35:58PM +0100, Salil Mehta wrote: > > This patch adds the following support to the HNS3 driver: > > 1. Support to change the Maximum Transmission Unit of a > > netdevice and of a port in hardware . > > 2. Initializes the supported MTU range for the netdevice. > > > > Signed-off-by: lipeng <lipeng321@huawei.com> > > Signed-off-by: Salil Mehta <salil.mehta@huawei.com> > > --- > > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 > ++++++++++++++++++++++ > > .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 1 + > > 2 files changed, 47 insertions(+) > > > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > > index e731f87..8e3711e 100644 > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c > > @@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct > net_device *netdev, int vf, u16 vlan, > > return ret; > > } > > > > +static int hns3_nic_change_mtu(struct net_device *netdev, int > new_mtu) > > +{ > > + struct hns3_nic_priv *priv = netdev_priv(netdev); > > + struct hnae3_handle *h = priv->ae_handle; > > + bool if_running = netif_running(netdev); > > + int ret; > > + > > + /* no change in MTU */ > > + if (new_mtu == netdev->mtu) > > + return 0; > > Hi Salil > > It is worth reading the core code: > > http://elixir.free- > electrons.com/linux/latest/source/net/core/dev.c#L6713 Right. Looks like it is already being done since 4.10-rc1 because of the patches Floated by Jarod Wilson <jarod@redhat.com>. Link: https://lkml.org/lkml/2016/10/13/270 Will remove this duplicate check! Thanks Salil > + > > + if (!h->ae_algo->ops->set_mtu) > > + return -ENOTSUPP; > > + > > + /* if this was called with netdev up then bring netdevice down */ > > + if (if_running) { > > + (void)hns3_nic_net_stop(netdev); > > + msleep(100); > > + } > > + > > + ret = h->ae_algo->ops->set_mtu(h, new_mtu); > > + if (ret) { > > + netdev_err(netdev, "failed to change MTU in hardware %d\n", > > + ret); > > + return ret; > > + } > > + > > + /* assign newly changed mtu to netdevice as well */ > > + netdev->mtu = new_mtu; > > http://elixir.free- > electrons.com/linux/latest/source/net/core/dev.c#L6698 Again, it is being done by core now since 4.10-rc1. Link: https://lkml.org/lkml/2016/10/13/270 Will remove this duplicate code! Thanks Salil > > > + > > + /* if the netdev was running earlier, bring it up again */ > > + if (if_running) { > > + if (hns3_nic_net_open(netdev)) { > > + netdev_err(netdev, "MTU, couldnt up netdev again\n"); > > + ret = -EINVAL; > > + } > > + } > > + > > + return ret; > > +} > > + > > static const struct net_device_ops hns3_nic_netdev_ops = { > > .ndo_open = hns3_nic_net_open, > > .ndo_stop = hns3_nic_net_stop, > > .ndo_start_xmit = hns3_nic_net_xmit, > > .ndo_set_mac_address = hns3_nic_net_set_mac_address, > > + .ndo_change_mtu = hns3_nic_change_mtu, > > .ndo_set_features = hns3_nic_set_features, > > .ndo_get_stats64 = hns3_nic_get_stats64, > > .ndo_setup_tc = hns3_nic_setup_tc, > > @@ -2752,6 +2794,10 @@ static int hns3_client_init(struct > hnae3_handle *handle) > > goto out_reg_netdev_fail; > > } > > > > + /* MTU range: 68 - 9706 */ > > + netdev->min_mtu = ETH_MIN_MTU; > > http://elixir.free- > electrons.com/linux/latest/source/net/ethernet/eth.c#L361 Supported range of Min and Max MTU should be at the discretion of the driver. Therefore, initialization looks fine to me. I could not clearly understand the problem being highlighted over here. Could you further clarify? Thanks Salil > > > + netdev->max_mtu = HNS3_MAX_MTU - (ETH_HLEN + ETH_FCS_LEN + > VLAN_HLEN); > > + > > return ret; > > > > out_reg_netdev_fail: > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > > index a6e8f15..7e87461 100644 > > --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h > > @@ -76,6 +76,7 @@ enum hns3_nic_state { > > #define HNS3_RING_NAME_LEN 16 > > #define HNS3_BUFFER_SIZE_2048 2048 > > #define HNS3_RING_MAX_PENDING 32768 > > +#define HNS3_MAX_MTU 9728 > > It seems odd that it does not already exists somewhere. The core does > not enforce the MTU. You could be passed a frame which is bigger. So > you should check before trying to pass something to the hardware which > the hardware cannot handle. There is a check already in place for this as well since 4.10-rc1. But perhaps this time there is no change required as it is being taken care by the core. Thanks to sharp eyes of Jarod. Link: https://lkml.org/lkml/2016/10/13/270 [...] if (dev->max_mtu > 0 && new_mtu > dev->max_mtu) { net_err_ratelimited("%s: Invalid MTU %d requested, hw max %d\n", dev->name, new_mtu, dev->max_mtu); [...] I think I missed this patch entirely so those rest above checks & assignments were repeated. Thanks Salil > > Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > + /* MTU range: 68 - 9706 */ > > > + netdev->min_mtu = ETH_MIN_MTU; > > > > http://elixir.free- > > electrons.com/linux/latest/source/net/ethernet/eth.c#L361 > Supported range of Min and Max MTU should be at the discretion > of the driver. Therefore, initialization looks fine to me. > > I could not clearly understand the problem being highlighted > over here. Could you further clarify? This is already setting min_mtu to ETH_MIN_MTU. There is no need for you to set it. > > > #define HNS3_RING_MAX_PENDING 32768 > > > +#define HNS3_MAX_MTU 9728 > > > > It seems odd that it does not already exists somewhere. The core does > > not enforce the MTU. You could be passed a frame which is bigger. So > > you should check before trying to pass something to the hardware which > > the hardware cannot handle. > There is a check already in place for this as well since 4.10-rc1. Yes, the core will check when changing the MTU. But when passing frames to be transmitted, it does not check the frame fits the MTU. DSA actually makes use of this, when passing frames to an Ethernet switch attached to the interface. We need to add an extra header to the frame, which makes the frame bigger than the MTU. Most Ethernet drivers are happy with this, they send the frame. Other reject it, and we have had to make driver changes. And some just explode :-( If 9728 is a hard limit for your device, you should check when passed a frame to make sure it is actually <= 9728 bytes in length. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Friday, August 18, 2017 4:04 PM > To: Salil Mehta > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); lipeng (Y); > dan.carpenter@oracle.com; mehta.salil.lnk@gmail.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux- > rdma@vger.kernel.org; Linuxarm > Subject: Re: [PATCH net-next] net: hns3: Add support to change MTU in > hardware & netdev > > > > > + /* MTU range: 68 - 9706 */ > > > > + netdev->min_mtu = ETH_MIN_MTU; > > > > > > http://elixir.free- > > > electrons.com/linux/latest/source/net/ethernet/eth.c#L361 > > Supported range of Min and Max MTU should be at the discretion > > of the driver. Therefore, initialization looks fine to me. > > > > I could not clearly understand the problem being highlighted > > over here. Could you further clarify? > > This is already setting min_mtu to ETH_MIN_MTU. There is no need for > you to set it. I grep'ed entire code and could see min and max MTUs being set by the Respective Ethernet driver code. I also verified by the original patch floated by the Jarod where he did that Change across all the drivers https://patchwork.kernel.org/patch/9387361/ for example, file: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) netdev->priv_flags |= IFF_UNICAST_FLT; + /* MTU range: 81 - 9600 */ + netdev->min_mtu = 81; + netdev->max_mtu = MAX_MTU; Many such changes are present in the above mentioned patch. Hope I am not missing anything there? Thanks Salil > > > > > #define HNS3_RING_MAX_PENDING 32768 > > > > +#define HNS3_MAX_MTU 9728 > > > > > > It seems odd that it does not already exists somewhere. The core > does > > > not enforce the MTU. You could be passed a frame which is bigger. > So > > > you should check before trying to pass something to the hardware > which > > > the hardware cannot handle. > > There is a check already in place for this as well since 4.10-rc1. > > Yes, the core will check when changing the MTU. > > But when passing frames to be transmitted, it does not check the frame > fits the MTU. DSA actually makes use of this, when passing frames to > an Ethernet switch attached to the interface. We need to add an extra > header to the frame, which makes the frame bigger than the MTU. Most > Ethernet drivers are happy with this, they send the frame. Other > reject it, and we have had to make driver changes. And some just > explode :-( I see. IMHO HNS3 is currently limited by maximum buffer per descriptor which is 64k. I am sure such frames would get dropped in the hardware itself and which I guess should be more preferable than dropping in driver since it saves you some precious cpu cycles? So I am not able to appreciate the presence of such a MTU check in the live data-path. Maybe I am missing something here? Thanks Salil > > If 9728 is a hard limit for your device, you should check when passed > a frame to make sure it is actually <= 9728 bytes in length. > > Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> for example, > file: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > static int init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > > netdev->priv_flags |= IFF_UNICAST_FLT; > > + /* MTU range: 81 - 9600 */ > + netdev->min_mtu = 81; > + netdev->max_mtu = MAX_MTU; In this cause, the driver is not using the default values. So it sets them. Anyway, try it. After your alloc_etherdev_mqs(), print the value of min_mtu. It should already be set to MIN_ETH_MTU. > I see. IMHO HNS3 is currently limited by maximum buffer per descriptor > which is 64k. I am sure such frames would get dropped in the hardware > itself and which I guess should be more preferable than dropping in > driver since it saves you some precious cpu cycles? If you hardware handles this, then you don't need to do anything. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew > -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Friday, August 18, 2017 5:02 PM > To: Salil Mehta > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); lipeng (Y); > mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm > Subject: Re: [PATCH net-next] net: hns3: Add support to change MTU in > hardware & netdev > > > for example, > > file: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > > static int init_one(struct pci_dev *pdev, const struct pci_device_id > *ent) > > > > netdev->priv_flags |= IFF_UNICAST_FLT; > > > > + /* MTU range: 81 - 9600 */ > > + netdev->min_mtu = 81; > > + netdev->max_mtu = MAX_MTU; > > In this cause, the driver is not using the default values. So it sets > them. > > Anyway, try it. After your alloc_etherdev_mqs(), print the value of > min_mtu. It should already be set to MIN_ETH_MTU. I understand your point. In this case, I would like to keep the range being set by the driver just to be more explicit. So for now keep this initialization in the driver? Thanks Salil > > > I see. IMHO HNS3 is currently limited by maximum buffer per > descriptor > > which is 64k. I am sure such frames would get dropped in the hardware > > itself and which I guess should be more preferable than dropping in > > driver since it saves you some precious cpu cycles? > > If you hardware handles this, then you don't need to do anything. Fine. Thanks! Salil > > Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andrew, > > -----Original Message----- > > From: Andrew Lunn [mailto:andrew@lunn.ch] > > Sent: Friday, August 18, 2017 5:02 PM > > To: Salil Mehta > > Cc: davem@davemloft.net; Zhuangyuzeng (Yisen); lipeng (Y); > > mehta.salil.lnk@gmail.com; netdev@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-rdma@vger.kernel.org; Linuxarm > > Subject: Re: [PATCH net-next] net: hns3: Add support to change MTU in > > hardware & netdev > > > > > for example, > > > file: drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c > > > static int init_one(struct pci_dev *pdev, const struct > pci_device_id > > *ent) > > > > > > netdev->priv_flags |= IFF_UNICAST_FLT; > > > > > > + /* MTU range: 81 - 9600 */ > > > + netdev->min_mtu = 81; > > > + netdev->max_mtu = MAX_MTU; > > > > In this cause, the driver is not using the default values. So it sets > > them. > > > > Anyway, try it. After your alloc_etherdev_mqs(), print the value of > > min_mtu. It should already be set to MIN_ETH_MTU. > I understand your point. In this case, I would like to keep the > range being set by the driver just to be more explicit. > So for now keep this initialization in the driver? Removed the min_mtu initialization from the driver code and added a comment over it to be explicit. Please have a look at the V2 patch. Thanks Salil -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c index e731f87..8e3711e 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c @@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, return ret; } +static int hns3_nic_change_mtu(struct net_device *netdev, int new_mtu) +{ + struct hns3_nic_priv *priv = netdev_priv(netdev); + struct hnae3_handle *h = priv->ae_handle; + bool if_running = netif_running(netdev); + int ret; + + /* no change in MTU */ + if (new_mtu == netdev->mtu) + return 0; + + if (!h->ae_algo->ops->set_mtu) + return -ENOTSUPP; + + /* if this was called with netdev up then bring netdevice down */ + if (if_running) { + (void)hns3_nic_net_stop(netdev); + msleep(100); + } + + ret = h->ae_algo->ops->set_mtu(h, new_mtu); + if (ret) { + netdev_err(netdev, "failed to change MTU in hardware %d\n", + ret); + return ret; + } + + /* assign newly changed mtu to netdevice as well */ + netdev->mtu = new_mtu; + + /* if the netdev was running earlier, bring it up again */ + if (if_running) { + if (hns3_nic_net_open(netdev)) { + netdev_err(netdev, "MTU, couldnt up netdev again\n"); + ret = -EINVAL; + } + } + + return ret; +} + static const struct net_device_ops hns3_nic_netdev_ops = { .ndo_open = hns3_nic_net_open, .ndo_stop = hns3_nic_net_stop, .ndo_start_xmit = hns3_nic_net_xmit, .ndo_set_mac_address = hns3_nic_net_set_mac_address, + .ndo_change_mtu = hns3_nic_change_mtu, .ndo_set_features = hns3_nic_set_features, .ndo_get_stats64 = hns3_nic_get_stats64, .ndo_setup_tc = hns3_nic_setup_tc, @@ -2752,6 +2794,10 @@ static int hns3_client_init(struct hnae3_handle *handle) goto out_reg_netdev_fail; } + /* MTU range: 68 - 9706 */ + netdev->min_mtu = ETH_MIN_MTU; + netdev->max_mtu = HNS3_MAX_MTU - (ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN); + return ret; out_reg_netdev_fail: diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h index a6e8f15..7e87461 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h @@ -76,6 +76,7 @@ enum hns3_nic_state { #define HNS3_RING_NAME_LEN 16 #define HNS3_BUFFER_SIZE_2048 2048 #define HNS3_RING_MAX_PENDING 32768 +#define HNS3_MAX_MTU 9728 #define HNS3_BD_SIZE_512_TYPE 0 #define HNS3_BD_SIZE_1024_TYPE 1