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 <redacted>
Signed-off-by: Salil Mehta <redacted>
---
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++++++++++++++++++++++
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 1 +
2 files changed, 47 insertions(+)
@@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan,returnret;}+staticinthns3_nic_change_mtu(structnet_device*netdev,intnew_mtu)+{+structhns3_nic_priv*priv=netdev_priv(netdev);+structhnae3_handle*h=priv->ae_handle;+boolif_running=netif_running(netdev);+intret;++/* no change in MTU */+if(new_mtu==netdev->mtu)+return0;++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);+returnret;+}++/* 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;+}+}++returnret;+}+staticconststructnet_device_opshns3_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)gotoout_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);+returnret;out_reg_netdev_fail:
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-18 13:31:44
On Fri, Aug 18, 2017 at 12:35:58PM +0100, Salil Mehta wrote:
quoted hunk
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 <redacted>
Signed-off-by: Salil Mehta <redacted>
---
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46 ++++++++++++++++++++++
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.h | 1 +
2 files changed, 47 insertions(+)
@@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan,returnret;}+staticinthns3_nic_change_mtu(structnet_device*netdev,intnew_mtu)+{+structhns3_nic_priv*priv=netdev_priv(netdev);+structhnae3_handle*h=priv->ae_handle;+boolif_running=netif_running(netdev);+intret;++/* no change in MTU */+if(new_mtu==netdev->mtu)+return0;
+ 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;
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
-----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:
quoted
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 <redacted>
Signed-off-by: Salil Mehta <redacted>
---
.../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
@@ -1278,11 +1278,53 @@ static int hns3_ndo_set_vf_vlan(struct
net_device *netdev, int vf, u16 vlan,
quoted
return ret; }+static int hns3_nic_change_mtu(struct net_device *netdev, int
new_mtu)
quoted
+{+ 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 [off-list ref].
Link: https://lkml.org/lkml/2016/10/13/270
Will remove this duplicate check!
Thanks
Salil
+
quoted
+ 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;
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
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
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.
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
It seems odd that it does not already exists somewhere. The core
does
quoted
quoted
not enforce the MTU. You could be passed a frame which is bigger.
So
quoted
quoted
you should check before trying to pass something to the hardware
which
quoted
quoted
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andrew Lunn <andrew@lunn.ch> Date: 2017-08-18 16:02:04
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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
quoted
I see. IMHO HNS3 is currently limited by maximum buffer per
descriptor
quoted
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.
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