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

Commit Message

Ulrich Hecht June 5, 2019, 3:14 p.m. UTC

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 <[e-mail]>
Reviewed-by: Niklas Söderlund <[e-mail]>
---

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

Comments

Geert Uytterhoeven June 5, 2019, 3:16 p.m. UTC | #1

On Wed, Jun 5, 2019 at 5:15 PM Ulrich Hecht <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Niklas Söderlund <[e-mail]>

Reviewed-by: Geert Uytterhoeven <[e-mail]>

Gr{oetje,eeting}s,

                        Geert

Sergei Shtylyov June 5, 2019, 5:27 p.m. UTC | #2

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 <[e-mail]>
> Reviewed-by: Niklas Söderlund <[e-mail]>

Reviewed-by: Sergei Shtylyov <[e-mail]>

MBR, Sergei

David Miller June 6, 2019, 2:08 a.m. UTC | #3

From: Ulrich Hecht <[e-mail]>
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.

Ulrich Hecht June 14, 2019, 3:28 p.m. UTC | #4

> On June 6, 2019 at 4:08 AM David Miller <[e-mail]> wrote:
> 
> 
> From: Ulrich Hecht <[e-mail]>
> 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

Patch

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