* [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 related [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: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
* 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
end of thread, other threads:[~2019-05-23 1:02 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).