[PATCH] ravb: implement MTU change while device is up

[PATCH] ravb: implement MTU change while device is up

[Ulrich Hecht]

Uses the same method as various other drivers: shut the device down,
change the MTU, then bring it back up again.

Tested on Renesas D3 Draak board.

Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ef8f089..02c247c 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1810,13 +1810,16 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
 
 static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
 {
-	if (netif_running(ndev))
-		return -EBUSY;
+	if (!netif_running(ndev)) {
+		ndev->mtu = new_mtu;
+		netdev_update_features(ndev);
+		return 0;
+	}
 
+	ravb_close(ndev);
 	ndev->mtu = new_mtu;
-	netdev_update_features(ndev);
 
-	return 0;
+	return ravb_open(ndev);
 }
 
 static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
-- 
2.7.4

Re: [PATCH] ravb: implement MTU change while device is up

[Sergei Shtylyov]

Hello!

On 05/08/2019 06:21 PM, Ulrich Hecht wrote:

> Uses the same method as various other drivers: shut the device down,
> change the MTU, then bring it back up again.
> 
> Tested on Renesas D3 Draak board.
> 
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

   You should have CC'ed me (as an reviewer for the Renesas drivers).

> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

   What about sh_eth?

> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ef8f089..02c247c 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1810,13 +1810,16 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
>  
>  static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
>  {
> -	if (netif_running(ndev))
> -		return -EBUSY;
> +	if (!netif_running(ndev)) {
> +		ndev->mtu = new_mtu;
> +		netdev_update_features(ndev);
> +		return 0;
> +	}
>  
> +	ravb_close(ndev);
>  	ndev->mtu = new_mtu;
> -	netdev_update_features(ndev);
>  
> -	return 0;
> +	return ravb_open(ndev);

   How about the code below instead?

	if (netif_running(ndev))
		ravb_close(ndev);

 	ndev->mtu = new_mtu;
	netdev_update_features(ndev);

	if (netif_running(ndev))
		return ravb_open(ndev);

	return 0;

[...]

MBR, Sergei

Re: [PATCH] ravb: implement MTU change while device is up

[Niklas Söderlund]

Hi Ulrich,

Thanks for your patch.

On 2019-05-08 17:21:22 +0200, Ulrich Hecht wrote:
> Uses the same method as various other drivers: shut the device down,
> change the MTU, then bring it back up again.
> 
> Tested on Renesas D3 Draak board.
> 
> Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>

With or without the code relayout suggested by Sergei,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Also as he points out I used the same pattern for sh_eth while adding 
MTU configuration support so a similar patch there would be nice. I'm 
happy to see the fix to allow for changing the MTU when the device is up 
was so simple, yet I could not figure it out ;-) Nice work!

> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ef8f089..02c247c 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1810,13 +1810,16 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
>  
>  static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
>  {
> -	if (netif_running(ndev))
> -		return -EBUSY;
> +	if (!netif_running(ndev)) {
> +		ndev->mtu = new_mtu;
> +		netdev_update_features(ndev);
> +		return 0;
> +	}
>  
> +	ravb_close(ndev);
>  	ndev->mtu = new_mtu;
> -	netdev_update_features(ndev);
>  
> -	return 0;
> +	return ravb_open(ndev);
>  }
>  
>  static void ravb_set_rx_csum(struct net_device *ndev, bool enable)
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] ravb: implement MTU change while device is up

[Niklas Söderlund]

Hi Sergei,

On 2019-05-08 18:59:01 +0300, Sergei Shtylyov wrote:
> Hello!
> 
> On 05/08/2019 06:21 PM, Ulrich Hecht wrote:
> 
> > Uses the same method as various other drivers: shut the device down,
> > change the MTU, then bring it back up again.
> > 
> > Tested on Renesas D3 Draak board.
> > 
> > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> 
>    You should have CC'ed me (as an reviewer for the Renesas drivers).
> 
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> 
>    What about sh_eth?
> 
> > 
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index ef8f089..02c247c 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -1810,13 +1810,16 @@ static int ravb_do_ioctl(struct net_device *ndev, struct ifreq *req, int cmd)
> >  
> >  static int ravb_change_mtu(struct net_device *ndev, int new_mtu)
> >  {
> > -	if (netif_running(ndev))
> > -		return -EBUSY;
> > +	if (!netif_running(ndev)) {
> > +		ndev->mtu = new_mtu;
> > +		netdev_update_features(ndev);
> > +		return 0;
> > +	}
> >  
> > +	ravb_close(ndev);
> >  	ndev->mtu = new_mtu;
> > -	netdev_update_features(ndev);
> >  
> > -	return 0;
> > +	return ravb_open(ndev);
> 
>    How about the code below instead?
> 
> 	if (netif_running(ndev))
> 		ravb_close(ndev);
> 
>  	ndev->mtu = new_mtu;
> 	netdev_update_features(ndev);

Is there a need to call netdev_update_features() even if the if is not 
running?

> 
> 	if (netif_running(ndev))
> 		return ravb_open(ndev);
> 
> 	return 0;
> 
> [...]
> 
> MBR, Sergei

-- 
Regards,
Niklas Söderlund

Re: [PATCH] ravb: implement MTU change while device is up

[Ulrich Hecht]

> On May 8, 2019 at 6:52 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote:
> 
> 
> Hi Sergei,
> 
> On 2019-05-08 18:59:01 +0300, Sergei Shtylyov wrote:
> > Hello!
> > 
> > On 05/08/2019 06:21 PM, Ulrich Hecht wrote:
> > 
> > > Uses the same method as various other drivers: shut the device down,
> > > change the MTU, then bring it back up again.
> > > 
> > > Tested on Renesas D3 Draak board.
> > > 
> > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > 
> >    You should have CC'ed me (as an reviewer for the Renesas drivers).

Sorry, will do next time.

> > 
> >    How about the code below instead?
> > 
> > 	if (netif_running(ndev))
> > 		ravb_close(ndev);
> > 
> >  	ndev->mtu = new_mtu;
> > 	netdev_update_features(ndev);
> 
> Is there a need to call netdev_update_features() even if the if is not 
> running?

In my testing, it didn't seem so.

CU
Uli

Re: [PATCH] ravb: implement MTU change while device is up

[Simon Horman]

On Thu, May 09, 2019 at 08:57:44AM +0200, Ulrich Hecht wrote:
> 
> > On May 8, 2019 at 6:52 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote:
> > 
> > 
> > Hi Sergei,
> > 
> > On 2019-05-08 18:59:01 +0300, Sergei Shtylyov wrote:
> > > Hello!
> > > 
> > > On 05/08/2019 06:21 PM, Ulrich Hecht wrote:
> > > 
> > > > Uses the same method as various other drivers: shut the device down,
> > > > change the MTU, then bring it back up again.
> > > > 
> > > > Tested on Renesas D3 Draak board.
> > > > 
> > > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > > 
> > >    You should have CC'ed me (as an reviewer for the Renesas drivers).
> 
> Sorry, will do next time.
> 
> > > 
> > >    How about the code below instead?
> > > 
> > > 	if (netif_running(ndev))
> > > 		ravb_close(ndev);
> > > 
> > >  	ndev->mtu = new_mtu;
> > > 	netdev_update_features(ndev);
> > 
> > Is there a need to call netdev_update_features() even if the if is not 
> > running?
> 
> In my testing, it didn't seem so.

That may be because your testing doesn't cover cases where it would make
any difference.

Re: [PATCH] ravb: implement MTU change while device is up

[Ulrich Hecht]

> On May 9, 2019 at 12:10 PM Simon Horman <horms@verge.net.au> wrote:
> 
> 
> On Thu, May 09, 2019 at 08:57:44AM +0200, Ulrich Hecht wrote:
> > 
> > > On May 8, 2019 at 6:52 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote:
> > > 
> > > 
> > > Hi Sergei,
> > > 
> > > On 2019-05-08 18:59:01 +0300, Sergei Shtylyov wrote:
> > > > Hello!
> > > > 
> > > > On 05/08/2019 06:21 PM, Ulrich Hecht wrote:
> > > > 
> > > > > Uses the same method as various other drivers: shut the device down,
> > > > > change the MTU, then bring it back up again.
> > > > > 
> > > > > Tested on Renesas D3 Draak board.
> > > > > 
> > > > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > > > 
> > > >    You should have CC'ed me (as an reviewer for the Renesas drivers).
> > 
> > Sorry, will do next time.
> > 
> > > > 
> > > >    How about the code below instead?
> > > > 
> > > > 	if (netif_running(ndev))
> > > > 		ravb_close(ndev);
> > > > 
> > > >  	ndev->mtu = new_mtu;
> > > > 	netdev_update_features(ndev);
> > > 
> > > Is there a need to call netdev_update_features() even if the if is not 
> > > running?
> > 
> > In my testing, it didn't seem so.
> 
> That may be because your testing doesn't cover cases where it would make
> any difference.

Cases other than changing the MTU while the device is up?

CU
Uli

Re: [PATCH] ravb: implement MTU change while device is up

[Simon Horman]

On Thu, May 09, 2019 at 05:32:21PM +0200, Ulrich Hecht wrote:
> 
> > On May 9, 2019 at 12:10 PM Simon Horman <horms@verge.net.au> wrote:
> > 
> > 
> > On Thu, May 09, 2019 at 08:57:44AM +0200, Ulrich Hecht wrote:
> > > 
> > > > On May 8, 2019 at 6:52 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote:
> > > > 
> > > > 
> > > > Hi Sergei,
> > > > 
> > > > On 2019-05-08 18:59:01 +0300, Sergei Shtylyov wrote:
> > > > > Hello!
> > > > > 
> > > > > On 05/08/2019 06:21 PM, Ulrich Hecht wrote:
> > > > > 
> > > > > > Uses the same method as various other drivers: shut the device down,
> > > > > > change the MTU, then bring it back up again.
> > > > > > 
> > > > > > Tested on Renesas D3 Draak board.
> > > > > > 
> > > > > > Signed-off-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > > > > 
> > > > >    You should have CC'ed me (as an reviewer for the Renesas drivers).
> > > 
> > > Sorry, will do next time.
> > > 
> > > > > 
> > > > >    How about the code below instead?
> > > > > 
> > > > > 	if (netif_running(ndev))
> > > > > 		ravb_close(ndev);
> > > > > 
> > > > >  	ndev->mtu = new_mtu;
> > > > > 	netdev_update_features(ndev);
> > > > 
> > > > Is there a need to call netdev_update_features() even if the if is not 
> > > > running?
> > > 
> > > In my testing, it didn't seem so.
> > 
> > That may be because your testing doesn't cover cases where it would make
> > any difference.
> 
> Cases other than changing the MTU while the device is up?

I was thinking of cases where listeners are registered for the
notifier that netdev_update_features() triggers.

Re: [PATCH] ravb: implement MTU change while device is up

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]


> > > > > >    How about the code below instead?
> > > > > > 
> > > > > > 	if (netif_running(ndev))
> > > > > > 		ravb_close(ndev);
> > > > > > 
> > > > > >  	ndev->mtu = new_mtu;
> > > > > > 	netdev_update_features(ndev);
> > > > > 
> > > > > Is there a need to call netdev_update_features() even if the if is not 
> > > > > running?
> > > > 
> > > > In my testing, it didn't seem so.
> > > 
> > > That may be because your testing doesn't cover cases where it would make
> > > any difference.
> > 
> > Cases other than changing the MTU while the device is up?
> 
> I was thinking of cases where listeners are registered for the
> notifier that netdev_update_features() triggers.

Where are we here? Is this a blocker?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ravb: implement MTU change while device is up

[Simon Horman]

On Mon, May 20, 2019 at 02:09:54PM +0200, Wolfram Sang wrote:
> 
> > > > > > >    How about the code below instead?
> > > > > > > 
> > > > > > > 	if (netif_running(ndev))
> > > > > > > 		ravb_close(ndev);
> > > > > > > 
> > > > > > >  	ndev->mtu = new_mtu;
> > > > > > > 	netdev_update_features(ndev);
> > > > > > 
> > > > > > Is there a need to call netdev_update_features() even if the if is not 
> > > > > > running?
> > > > > 
> > > > > In my testing, it didn't seem so.
> > > > 
> > > > That may be because your testing doesn't cover cases where it would make
> > > > any difference.
> > > 
> > > Cases other than changing the MTU while the device is up?
> > 
> > I was thinking of cases where listeners are registered for the
> > notifier that netdev_update_features() triggers.
> 
> Where are we here? Is this a blocker?

I don't think this is a blocker but I would lean towards leaving
netdev_update_features() in unless we are certain its not needed.

Re: [PATCH] ravb: implement MTU change while device is up

[Ulrich Hecht]

> On May 22, 2019 at 1:59 PM Simon Horman <horms@verge.net.au> wrote:
> 
> 
> On Mon, May 20, 2019 at 02:09:54PM +0200, Wolfram Sang wrote:
> > 
> > > > > > > >    How about the code below instead?
> > > > > > > > 
> > > > > > > > 	if (netif_running(ndev))
> > > > > > > > 		ravb_close(ndev);
> > > > > > > > 
> > > > > > > >  	ndev->mtu = new_mtu;
> > > > > > > > 	netdev_update_features(ndev);
> > > > > > > 
> > > > > > > Is there a need to call netdev_update_features() even if the if is not 
> > > > > > > running?
> > > > > > 
> > > > > > In my testing, it didn't seem so.
> > > > > 
> > > > > That may be because your testing doesn't cover cases where it would make
> > > > > any difference.
> > > > 
> > > > Cases other than changing the MTU while the device is up?
> > > 
> > > I was thinking of cases where listeners are registered for the
> > > notifier that netdev_update_features() triggers.
> > 
> > Where are we here? Is this a blocker?
> 
> I don't think this is a blocker but I would lean towards leaving
> netdev_update_features() in unless we are certain its not needed.
>

I have read through the code and have indeed not found any indication that netdev_update_features() or something equivalent is called implicitly when the device is closed and reopened. I'll send a v2.

CU
Uli