[PATCH] net: core: Fix to store new mtu setting in netdevice.

[PATCH] net: core: Fix to store new mtu setting in netdevice.

[Murali Krishna Policharla]

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);
+		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);
 
-- 
1.9.1

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Kirill Tkhai]

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);
>  
> 

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Heiner Kallweit]

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);
>>  
>>
> 

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Andrew Lunn]

> > 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

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Heiner Kallweit]

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

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Andrew Lunn]

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

RE: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Murali Krishna Policharla]

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

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Andrew Lunn]

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

RE: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Murali Krishna Policharla]

> 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

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Andrew Lunn]

> 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

RE: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Murali Krishna Policharla]

> 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

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[Heiner Kallweit]

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);
>  
> 

Re: [PATCH] net: core: Fix to store new mtu setting in netdevice.

[David Miller]

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.