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

[PATCH v2] 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>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---

Hi!

This revision incorporates Simon's and Sergei's suggestions, namely calling
netdev_update_features() whether the device is up or not. Thanks to
reviewers!

CU
Uli


 drivers/net/ethernet/renesas/ravb_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ef8f089..00427e7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1811,11 +1811,14 @@ 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;
+		ravb_close(ndev);
 
 	ndev->mtu = new_mtu;
 	netdev_update_features(ndev);
 
+	if (netif_running(ndev))
+		return ravb_open(ndev);
+
 	return 0;
 }
 
-- 
2.7.4

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

[Geert Uytterhoeven]

On Wed, Jun 5, 2019 at 5:15 PM Ulrich Hecht <uli+renesas@fpond.eu> 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>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Sergei Shtylyov]

Hello!

On 06/05/2019 06:14 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>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

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

[David Miller]

From: Ulrich Hecht <uli+renesas@fpond.eu>
Date: Wed,  5 Jun 2019 17:14:20 +0200

> @@ -1811,11 +1811,14 @@ 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;
> +		ravb_close(ndev);
>  
>  	ndev->mtu = new_mtu;
>  	netdev_update_features(ndev);
>  
> +	if (netif_running(ndev))
> +		return ravb_open(ndev);
> +

And if ravb_open() fails?  The user sees a failure, but to the user the failure
means the MTU change can't be done, yet the device has the new MTU set still.

This really is terrible behavior.

If you must do a prepare/commit kind of sequence for this to work properly if
you are going to go down the road of taking the device down to change the MTU
when the device is UP.

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

[Ulrich Hecht]

> On June 6, 2019 at 4:08 AM David Miller <davem@davemloft.net> wrote:
> 
> 
> From: Ulrich Hecht <uli+renesas@fpond.eu>
> Date: Wed,  5 Jun 2019 17:14:20 +0200
> 
> > @@ -1811,11 +1811,14 @@ 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;
> > +		ravb_close(ndev);
> >  
> >  	ndev->mtu = new_mtu;
> >  	netdev_update_features(ndev);
> >  
> > +	if (netif_running(ndev))
> > +		return ravb_open(ndev);
> > +
> 
> And if ravb_open() fails?  The user sees a failure, but to the user the failure
> means the MTU change can't be done, yet the device has the new MTU set still.
> 
> This really is terrible behavior.
> 
> If you must do a prepare/commit kind of sequence for this to work properly if
> you are going to go down the road of taking the device down to change the MTU
> when the device is UP.

So would rolling back the MTU change in case of a re-open failure be acceptable?

If so, is there additional action that needs to be taken if a device unexpectedly goes down as a side-effect of an MTU change?

CU
Uli