Linux-Renesas-SoC Archive on lore.kernel.org
 help / Atom feed
* [PATCH] ravb: implement MTU change while device is up
@ 2019-05-08 15:21 Ulrich Hecht
  2019-05-08 15:59 ` Sergei Shtylyov
  2019-05-08 16:50 ` Niklas Söderlund
  0 siblings, 2 replies; 11+ messages in thread
From: Ulrich Hecht @ 2019-05-08 15:21 UTC (permalink / raw)
  To: linux-renesas-soc; +Cc: netdev, davem, 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>
---
 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


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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-08 15:21 [PATCH] ravb: implement MTU change while device is up Ulrich Hecht
@ 2019-05-08 15:59 ` Sergei Shtylyov
  2019-05-08 16:52   ` Niklas Söderlund
  2019-05-08 16:50 ` Niklas Söderlund
  1 sibling, 1 reply; 11+ messages in thread
From: Sergei Shtylyov @ 2019-05-08 15:59 UTC (permalink / raw)
  To: Ulrich Hecht, linux-renesas-soc; +Cc: netdev, davem, wsa, horms, magnus.damm

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

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-08 15:21 [PATCH] ravb: implement MTU change while device is up Ulrich Hecht
  2019-05-08 15:59 ` Sergei Shtylyov
@ 2019-05-08 16:50 ` Niklas Söderlund
  1 sibling, 0 replies; 11+ messages in thread
From: Niklas Söderlund @ 2019-05-08 16:50 UTC (permalink / raw)
  To: Ulrich Hecht; +Cc: linux-renesas-soc, netdev, davem, wsa, horms, magnus.damm

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

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-08 15:59 ` Sergei Shtylyov
@ 2019-05-08 16:52   ` Niklas Söderlund
  2019-05-09  6:57     ` Ulrich Hecht
  0 siblings, 1 reply; 11+ messages in thread
From: Niklas Söderlund @ 2019-05-08 16:52 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: Ulrich Hecht, linux-renesas-soc, netdev, davem, wsa, horms, magnus.damm

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

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-08 16:52   ` Niklas Söderlund
@ 2019-05-09  6:57     ` Ulrich Hecht
  2019-05-09 10:10       ` Simon Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Ulrich Hecht @ 2019-05-09  6:57 UTC (permalink / raw)
  To: Niklas Söderlund, Sergei Shtylyov
  Cc: Ulrich Hecht, linux-renesas-soc, netdev, davem, wsa, horms, magnus.damm


> 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

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-09  6:57     ` Ulrich Hecht
@ 2019-05-09 10:10       ` Simon Horman
  2019-05-09 15:32         ` Ulrich Hecht
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2019-05-09 10:10 UTC (permalink / raw)
  To: Ulrich Hecht
  Cc: Niklas Söderlund, Sergei Shtylyov, Ulrich Hecht,
	linux-renesas-soc, netdev, davem, wsa, magnus.damm

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.

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-09 10:10       ` Simon Horman
@ 2019-05-09 15:32         ` Ulrich Hecht
  2019-05-13 12:18           ` Simon Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Ulrich Hecht @ 2019-05-09 15:32 UTC (permalink / raw)
  To: Simon Horman
  Cc: Niklas Söderlund, Sergei Shtylyov, linux-renesas-soc,
	netdev, davem, wsa, magnus.damm


> 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

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-09 15:32         ` Ulrich Hecht
@ 2019-05-13 12:18           ` Simon Horman
  2019-05-20 12:09             ` Wolfram Sang
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2019-05-13 12:18 UTC (permalink / raw)
  To: Ulrich Hecht
  Cc: Niklas Söderlund, Sergei Shtylyov, linux-renesas-soc,
	netdev, davem, wsa, magnus.damm

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.

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-13 12:18           ` Simon Horman
@ 2019-05-20 12:09             ` Wolfram Sang
  2019-05-22 11:59               ` Simon Horman
  0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2019-05-20 12:09 UTC (permalink / raw)
  To: Simon Horman
  Cc: Ulrich Hecht, Niklas Söderlund, Sergei Shtylyov,
	linux-renesas-soc, netdev, davem, magnus.damm

[-- 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 --]

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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-20 12:09             ` Wolfram Sang
@ 2019-05-22 11:59               ` Simon Horman
  2019-05-23  1:02                 ` Ulrich Hecht
  0 siblings, 1 reply; 11+ messages in thread
From: Simon Horman @ 2019-05-22 11:59 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Ulrich Hecht, Niklas Söderlund, Sergei Shtylyov,
	linux-renesas-soc, netdev, davem, magnus.damm

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.


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

* Re: [PATCH] ravb: implement MTU change while device is up
  2019-05-22 11:59               ` Simon Horman
@ 2019-05-23  1:02                 ` Ulrich Hecht
  0 siblings, 0 replies; 11+ messages in thread
From: Ulrich Hecht @ 2019-05-23  1:02 UTC (permalink / raw)
  To: Simon Horman, Wolfram Sang
  Cc: Niklas Söderlund, Sergei Shtylyov, linux-renesas-soc,
	netdev, davem, magnus.damm


> 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

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

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 15:21 [PATCH] ravb: implement MTU change while device is up Ulrich Hecht
2019-05-08 15:59 ` Sergei Shtylyov
2019-05-08 16:52   ` Niklas Söderlund
2019-05-09  6:57     ` Ulrich Hecht
2019-05-09 10:10       ` Simon Horman
2019-05-09 15:32         ` Ulrich Hecht
2019-05-13 12:18           ` Simon Horman
2019-05-20 12:09             ` Wolfram Sang
2019-05-22 11:59               ` Simon Horman
2019-05-23  1:02                 ` Ulrich Hecht
2019-05-08 16:50 ` Niklas Söderlund

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