| Message ID | 1546324934-17555-1-git-send-email-murali.policharla@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: core: Fix to store new mtu setting in netdevice. | expand |
On 01.01.2019 09:42, Murali Krishna Policharla wrote: > Store newly configured mtu settings in the netdevice after mtu > configuration is successful to the dsa switch. > > Fixes: 2315dc91a5 ("net: make dev_set_mtu() honor notification return code") > Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > --- > net/core/dev.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 722d50d..58617aa 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -7586,12 +7586,15 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) > int __dev_set_mtu(struct net_device *dev, int new_mtu) > { > const struct net_device_ops *ops = dev->netdev_ops; > + int ret = 0; > > if (ops->ndo_change_mtu) > - return ops->ndo_change_mtu(dev, new_mtu); Is there a .ndo_change_mtu callback, which does not assign a new mtu itself? > + ret = ops->ndo_change_mtu(dev, new_mtu); > > - dev->mtu = new_mtu; > - return 0; > + if (ret >= 0) > + dev->mtu = new_mtu; > + > + return ret; > } > EXPORT_SYMBOL(__dev_set_mtu); > >
On Tue, Jan 01, 2019 at 12:12:14PM +0530, Murali Krishna Policharla wrote: > Store newly configured mtu settings in the netdevice after mtu > configuration is successful to the dsa switch. Hi Murali Please could you give more details. net/dsa/slave.c does not have a ndo_change_mtu function, so i don't know what you mean by the reference to DSA. Thanks Andrew
On 01.01.2019 07:42, Murali Krishna Policharla wrote: > Store newly configured mtu settings in the netdevice after mtu > configuration is successful to the dsa switch. > At first: good that this is fixed, so far each network driver had to do the "dev->mtu = new_mtu" in its ndo_change_mtu callback what can be removed now. I think the commit description could be improved, the change isn't specific to dsa switches at all. And the patch should be annotated "net" because it's a fix. > Fixes: 2315dc91a5 ("net: make dev_set_mtu() honor notification return code") > Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > --- > net/core/dev.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 722d50d..58617aa 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -7586,12 +7586,15 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) > int __dev_set_mtu(struct net_device *dev, int new_mtu) > { > const struct net_device_ops *ops = dev->netdev_ops; > + int ret = 0; > > if (ops->ndo_change_mtu) > - return ops->ndo_change_mtu(dev, new_mtu); > + ret = ops->ndo_change_mtu(dev, new_mtu); > > - dev->mtu = new_mtu; > - return 0; > + if (ret >= 0) > + dev->mtu = new_mtu; > + > + return ret; > } > EXPORT_SYMBOL(__dev_set_mtu); > >
On 01.01.2019 08:54, Kirill Tkhai wrote: > On 01.01.2019 09:42, Murali Krishna Policharla wrote: >> Store newly configured mtu settings in the netdevice after mtu >> configuration is successful to the dsa switch. >> >> Fixes: 2315dc91a5 ("net: make dev_set_mtu() honor notification return code") >> Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com> >> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> >> --- >> net/core/dev.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/net/core/dev.c b/net/core/dev.c >> index 722d50d..58617aa 100644 >> --- a/net/core/dev.c >> +++ b/net/core/dev.c >> @@ -7586,12 +7586,15 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) >> int __dev_set_mtu(struct net_device *dev, int new_mtu) >> { >> const struct net_device_ops *ops = dev->netdev_ops; >> + int ret = 0; >> >> if (ops->ndo_change_mtu) >> - return ops->ndo_change_mtu(dev, new_mtu); > > Is there a .ndo_change_mtu callback, which does not assign a new mtu itself? > So far all drivers have to do it themselves. But IMO this is more a workaround for the core not doing it. It's something the core should do. Now we can remove this from drivers. >> + ret = ops->ndo_change_mtu(dev, new_mtu); >> >> - dev->mtu = new_mtu; >> - return 0; >> + if (ret >= 0) >> + dev->mtu = new_mtu; >> + >> + return ret; >> } >> EXPORT_SYMBOL(__dev_set_mtu); >> >> >
Hi Andrew,
Currently net/dsa/slave.c does not have
ndo_change_mtu function. But shortly I will be submitting a separate
patch outside this fix that has ndo_change_mtu function support added to
DSA switch. As part of testing the newly added ndo_change_mtu function
for DSA switch it uncovered that new mtu size is not being updated to
netdevice structure. This patch fixes this issue and updates new mtu size
to netdevice structure.
Hope this clarifies, let me know if you need any further information.
Regards
Murali
-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch]
Sent: Tuesday, January 1, 2019 2:05 PM
To: Murali Krishna Policharla
Cc: davem@davemloft.net; amritha.nambiar@intel.com; ecree@solarflare.com;
ktkhai@virtuozzo.com; alexander.h.duyck@intel.com; netdev@vger.kernel.org;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.
On Tue, Jan 01, 2019 at 12:12:14PM +0530, Murali Krishna Policharla wrote:
> Store newly configured mtu settings in the netdevice after mtu
> configuration is successful to the dsa switch.
Hi Murali
Please could you give more details. net/dsa/slave.c does not have a
ndo_change_mtu function, so i don't know what you mean by the
reference to DSA.
Thanks
Andrew
From: Murali Krishna Policharla <murali.policharla@broadcom.com> Date: Tue, 1 Jan 2019 12:12:14 +0530 > Store newly configured mtu settings in the netdevice after mtu > configuration is successful to the dsa switch. > > Fixes: 2315dc91a5 ("net: make dev_set_mtu() honor notification return code") > Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> It is entirely premature to mention DSA in this commit message when the DSA code lacks the callback yet.
On Tue, Jan 01, 2019 at 03:18:51PM +0530, Murali Krishna Policharla wrote: > Hi Andrew, > Currently net/dsa/slave.c does not have > ndo_change_mtu function. But shortly I will be submitting a separate > patch outside this fix that has ndo_change_mtu function support added to > DSA switch. As part of testing the newly added ndo_change_mtu function > for DSA switch it uncovered that new mtu size is not being updated to > netdevice structure. This patch fixes this issue and updates new mtu size > to netdevice structure. > > Hope this clarifies, let me know if you need any further information. Hi Murali Thanks for the explanation. However, i looked at the patch you listed as 'fixes'. I don't see what came before that setting the mtu when an ndo_change_mtu function is provided. It seems to me, if you provide an ndo_change_mtu, it has to do the assignment. I don't think you are fixing anything here. If you do want to move the assignment into the core, please review all the MAC drivers which implement ndo_change_mtu and remove the assignment from them. Thanks Andrew
> > Is there a .ndo_change_mtu callback, which does not assign a new mtu itself? > > > So far all drivers have to do it themselves. But IMO this is more a workaround > for the core not doing it. It's something the core should do. > Now we can remove this from drivers. Hi Heiner I think somebody first needs to review all the ndo_change_mtu implementations and check that none do something funny like round to multiple of 2 or 4 to satisfy DMA restrictions, etc. If there is such a thing, we cannot easily move this into the core. Andrew
On 02.01.2019 00:36, Andrew Lunn wrote: >>> Is there a .ndo_change_mtu callback, which does not assign a new mtu itself? >>> >> So far all drivers have to do it themselves. But IMO this is more a workaround >> for the core not doing it. It's something the core should do. >> Now we can remove this from drivers. > > Hi Heiner > > I think somebody first needs to review all the ndo_change_mtu > implementations and check that none do something funny like round to > multiple of 2 or 4 to satisfy DMA restrictions, etc. If there is such > a thing, we cannot easily move this into the core. > > Andrew > . > Good point. I briefly grepped over all drivers and it's not that many drivers not using the standard assignment dev->mtu = new_mtu. Some are doing things like dev->mtu = max(new_mtu, xx) what could be achieved easier with an appropriate max_mtu setting. But that's a different story. And I was under the impression that such things had been checked because the patch had a "Reviewed-by" from Florian (although he wasn't on cc). Heiner
> Hi Andrew, > Currently net/dsa/slave.c does not have > ndo_change_mtu function. But shortly I will be submitting a separate > patch outside this fix that has ndo_change_mtu function support added to > DSA switch. As part of testing the newly added ndo_change_mtu function > for DSA switch it uncovered that new mtu size is not being updated to > netdevice structure. This patch fixes this issue and updates new mtu size > to netdevice structure. > > Hope this clarifies, let me know if you need any further information. > Hi Murali > Thanks for the explanation. > However, i looked at the patch you listed as 'fixes'. I don't see what > came before that setting the mtu when an ndo_change_mtu function is > provided. It seems to me, if you provide an ndo_change_mtu, it has to > do the assignment. I don't think you are fixing anything here. > If you do want to move the assignment into the core, please review all > the MAC drivers which implement ndo_change_mtu and remove the > assignment from them. > Thanks > Andrew Hi Andrew/Heiner Thanks for the feedback. This patch fixes a case where ndo_change_mtu function is provided but the callback function is not storing mtu to netdevice structure. In the discussion thread for this patch it is mentioned that there is a possibility of ndo_change_mtu function to manipulate the mtu size and then store the modified value to netdevice structure. For me it appears this is incorrect operation since mtu value is being modified and it is not storing the value that user has configured. In another use case that is discussed user configures mtu value that exceeds the max/min permitted value. In this case an error should be returned to the user rather than modifying/storing only the permitted value. Also any modifications done to mtu in the callback function for DMA operations/restrictions should be transparent to user. As per my understanding ndo_change_mtu callback function can modify and use the mtu value that suits for the respective device operation, but only user configured mtu size should be stored in netdevice structure and this is done in the core driver with this fix. Please let me know your inputs. Thanks Murali
> Hi Andrew/Heiner > > Thanks for the feedback. This patch > fixes a case where ndo_change_mtu function is provided but the callback > function is not storing mtu to netdevice structure. Hi Murali At the moment, any driver which implements ndo_change_mtu MUST set ndev->mtu. It is a nice clean definition, easy for any driver write to understand. What you are proposing is that the core will set ndev->mtu. That is fine, but again we need a nice clean definition. The drivers MUST NOT set ndev->mtu. If you plan to make this core change, please also change all the drivers as well, so we have very clear semantics of what is expected. It would also be good to extend the comment in include/linux/netdevice.h: * int (*ndo_change_mtu)(struct net_device *dev, int new_mtu); * Called when a user wants to change the Maximum Transfer Unit * of a device. Adding something like: Should only program the hardware with the new MTU. To give the hint that the core is doing all the validation and modifying ndev->mtu. I had one other thought. Please also take a look at how stacked devices work. I've not looked, but i expect there are cases where a lower device calls into an upper device to inform it the MTU has changed. When does this call happen? Does the MTU of the lower device need to of already changed before this call is made? Are there similar cases of an upper device calling down to the lower device? You need to be careful here, you are changing exactly when the ndev->mtu value changes, and so could be introducing bugs if you don't do a proper code review. Andrew
> Hi Andrew/Heiner > > Thanks for the feedback. This patch > fixes a case where ndo_change_mtu function is provided but the callback > function is not storing mtu to netdevice structure. > Hi Murali > At the moment, any driver which implements ndo_change_mtu MUST set > ndev->mtu. It is a nice clean definition, easy for any driver write to > understand. Hi Andrew Since drivers implementing ndo_change_mtu callback function are following this approach. Will go with the existing approach and modify the ndo_change_mtu callback function that I will be adding to store mtu to netdevice structure. Thanks Murali
diff --git a/net/core/dev.c b/net/core/dev.c index 722d50d..58617aa 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7586,12 +7586,15 @@ int dev_change_flags(struct net_device *dev, unsigned int flags) int __dev_set_mtu(struct net_device *dev, int new_mtu) { const struct net_device_ops *ops = dev->netdev_ops; + int ret = 0; if (ops->ndo_change_mtu) - return ops->ndo_change_mtu(dev, new_mtu); + ret = ops->ndo_change_mtu(dev, new_mtu); - dev->mtu = new_mtu; - return 0; + if (ret >= 0) + dev->mtu = new_mtu; + + return ret; } EXPORT_SYMBOL(__dev_set_mtu);
Store newly configured mtu settings in the netdevice after mtu configuration is successful to the dsa switch. Fixes: 2315dc91a5 ("net: make dev_set_mtu() honor notification return code") Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> --- net/core/dev.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)