Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] ravb: implement MTU change while device is up
@ 2019-06-05 15:14 Ulrich Hecht
  2019-06-05 15:16 ` Geert Uytterhoeven
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Ulrich Hecht @ 2019-06-05 15:14 UTC (permalink / raw)
  To: linux-renesas-soc
  Cc: netdev, davem, sergei.shtylyov, niklas.soderlund, wsa, horms,
	magnus.damm, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] ravb: implement MTU change while device is up
  2019-06-05 15:14 [PATCH v2] ravb: implement MTU change while device is up Ulrich Hecht
@ 2019-06-05 15:16 ` Geert Uytterhoeven
  2019-06-05 17:27 ` Sergei Shtylyov
  2019-06-06  2:08 ` David Miller
  2 siblings, 0 replies; 5+ messages in thread
From: Geert Uytterhoeven @ 2019-06-05 15:16 UTC (permalink / raw)
  To: Ulrich Hecht
  Cc: Linux-Renesas, netdev, David S. Miller, Sergei Shtylyov,
	Niklas Söderlund, Wolfram Sang, Simon Horman, Magnus Damm

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] ravb: implement MTU change while device is up
  2019-06-05 15:14 [PATCH v2] ravb: implement MTU change while device is up Ulrich Hecht
  2019-06-05 15:16 ` Geert Uytterhoeven
@ 2019-06-05 17:27 ` Sergei Shtylyov
  2019-06-06  2:08 ` David Miller
  2 siblings, 0 replies; 5+ messages in thread
From: Sergei Shtylyov @ 2019-06-05 17:27 UTC (permalink / raw)
  To: Ulrich Hecht, linux-renesas-soc
  Cc: netdev, davem, niklas.soderlund, wsa, horms, magnus.damm

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] ravb: implement MTU change while device is up
  2019-06-05 15:14 [PATCH v2] ravb: implement MTU change while device is up Ulrich Hecht
  2019-06-05 15:16 ` Geert Uytterhoeven
  2019-06-05 17:27 ` Sergei Shtylyov
@ 2019-06-06  2:08 ` David Miller
  2019-06-14 15:28   ` Ulrich Hecht
  2 siblings, 1 reply; 5+ messages in thread
From: David Miller @ 2019-06-06  2:08 UTC (permalink / raw)
  To: uli+renesas
  Cc: linux-renesas-soc, netdev, sergei.shtylyov, niklas.soderlund,
	wsa, horms, magnus.damm

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] ravb: implement MTU change while device is up
  2019-06-06  2:08 ` David Miller
@ 2019-06-14 15:28   ` Ulrich Hecht
  0 siblings, 0 replies; 5+ messages in thread
From: Ulrich Hecht @ 2019-06-14 15:28 UTC (permalink / raw)
  To: David Miller, uli+renesas
  Cc: linux-renesas-soc, netdev, sergei.shtylyov, niklas.soderlund,
	wsa, horms, magnus.damm


> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05 15:14 [PATCH v2] ravb: implement MTU change while device is up Ulrich Hecht
2019-06-05 15:16 ` Geert Uytterhoeven
2019-06-05 17:27 ` Sergei Shtylyov
2019-06-06  2:08 ` David Miller
2019-06-14 15:28   ` Ulrich Hecht

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox