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

[PATCH v3] 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 and M3-W Salvator-X boards.

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>
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---

Hi!

This revision reverts the MTU change if re-opening the device fails.

CU
Uli


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

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index ef8f089..402bcec 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1810,12 +1810,24 @@ 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)
 {
+	unsigned int old_mtu = ndev->mtu;
+
 	if (netif_running(ndev))
-		return -EBUSY;
+		ravb_close(ndev);
 
 	ndev->mtu = new_mtu;
 	netdev_update_features(ndev);
 
+	if (netif_running(ndev)) {
+		int err = ravb_open(ndev);
+
+		if (err) {
+			ndev->mtu = old_mtu;
+			netdev_update_features(ndev);
+			return err;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.7.4

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

[Wolfram Sang]

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

Hi Uli,

thanks for the patch.

On Tue, Aug 20, 2019 at 09:01:26PM +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 and M3-W Salvator-X boards.
> 
> 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>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Are these reviews left from v2? If so, I'd prefer to see them given
again because the logic was changed in v3.

> ---
> 
> Hi!
> 
> This revision reverts the MTU change if re-opening the device fails.
> 
> CU
> Uli
> 
> 
>  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index ef8f089..402bcec 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1810,12 +1810,24 @@ 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)
>  {
> +	unsigned int old_mtu = ndev->mtu;
> +
>  	if (netif_running(ndev))
> -		return -EBUSY;
> +		ravb_close(ndev);
>  
>  	ndev->mtu = new_mtu;
>  	netdev_update_features(ndev);
>  
> +	if (netif_running(ndev)) {
> +		int err = ravb_open(ndev);
> +
> +		if (err) {
> +			ndev->mtu = old_mtu;
> +			netdev_update_features(ndev);
> +			return err;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4
> 

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

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

[David Miller]

From: Ulrich Hecht <uli+renesas@fpond.eu>
Date: Tue, 20 Aug 2019 21:01:26 +0200

> This revision reverts the MTU change if re-opening the device fails.

But you leave the device closed if this happens.

You have to implement this properly, with a full prepare/commit sequence.

First allocate all of the necessary resources, such that you can guarantee
success.  If you cannot allocate these resources you must fail the operation
and leave the device _UP_ with the original MTU value.

If you can allocate the resource, you can fully commit to the MTU change
and return success.

You must not fail the operation in such a way that the device is left
inoperable.  But that is precisely what your patch currently does.