| Message ID | 1557328882-24307-1-git-send-email-uli+renesas@fpond.eu (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | ravb: implement MTU change while device is up | expand |
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
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 >
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
> 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
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.
> 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
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.
> > > > > > 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?
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.
> 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
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)
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(-)