| Message ID | 20230321065826.2044-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ce1fdb065695f49ef6f126d35c1abbfe645d62d5 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] sh_eth: remove open coded netif_running() | expand |
On Tue, Mar 21, 2023 at 07:58:26AM +0100, Wolfram Sang wrote: > It had a purpose back in the days, but today we have a handy helper. > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Based on 6.3-rc3 and tested on a Renesas Lager board (R-Car H2). > > drivers/net/ethernet/renesas/sh_eth.c | 6 +----- > drivers/net/ethernet/renesas/sh_eth.h | 1 - > 2 files changed, 1 insertion(+), 6 deletions(-) > Thanks, Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
On Tue, Mar 21, 2023 at 7:58 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > It had a purpose back in the days, but today we have a handy helper. > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> No regressions seen on R-Car M2-W, RZ/A1H, and RZ/A2M. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On 3/20/23 23:58, Wolfram Sang wrote: > It had a purpose back in the days, but today we have a handy helper. > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 21 Mar 2023 07:58:26 +0100 you wrote: > It had a purpose back in the days, but today we have a handy helper. > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Based on 6.3-rc3 and tested on a Renesas Lager board (R-Car H2). > > [...] Here is the summary with links: - [net-next] sh_eth: remove open coded netif_running() https://git.kernel.org/netdev/net-next/c/ce1fdb065695 You are awesome, thank you!
On 3/21/23 9:58 AM, Wolfram Sang wrote: > It had a purpose back in the days, but today we have a handy helper. Well, the is_opened flag doesn't get set/cleared at the same time as __LINK_STATE_START. I'm not sure they are interchangeable... > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Based on 6.3-rc3 and tested on a Renesas Lager board (R-Car H2). > > drivers/net/ethernet/renesas/sh_eth.c | 6 +----- > drivers/net/ethernet/renesas/sh_eth.h | 1 - > 2 files changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c > index d8ec729825be..2d9787231099 100644 > --- a/drivers/net/ethernet/renesas/sh_eth.c > +++ b/drivers/net/ethernet/renesas/sh_eth.c > @@ -2441,8 +2441,6 @@ static int sh_eth_open(struct net_device *ndev) > > netif_start_queue(ndev); > > - mdp->is_opened = 1; > - __LINK_STATE_START gets set before the ndo_open() method call, so before the RPM call that enbales the clocks... > return ret; > > out_free_irq: > @@ -2565,7 +2563,7 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev) > if (mdp->cd->no_tx_cntrs) > return &ndev->stats; > > - if (!mdp->is_opened) > + if (!netif_running(ndev)) > return &ndev->stats; I guess mdp->is_opened is checked here to avoid reading the counter regs when RPM hasn't been called yet to enable the clocks... [...] MBR, Sergey
On 3/22/23 3:30 PM, patchwork-bot+netdevbpf@kernel.org wrote: [...] > This patch was applied to netdev/net-next.git (main) > by Paolo Abeni <pabeni@redhat.com>: > > On Tue, 21 Mar 2023 07:58:26 +0100 you wrote: >> It had a purpose back in the days, but today we have a handy helper. >> >> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> >> Based on 6.3-rc3 and tested on a Renesas Lager board (R-Car H2). >> >> [...] > > Here is the summary with links: > - [net-next] sh_eth: remove open coded netif_running() > https://git.kernel.org/netdev/net-next/c/ce1fdb065695 > > You are awesome, thank you! I don't think this needed to be merged circumventing my review. The patch was posted yesterday... MBR, Sergey
Hi Sergey, On Wed, Mar 22, 2023 at 9:54 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > On 3/21/23 9:58 AM, Wolfram Sang wrote: > > It had a purpose back in the days, but today we have a handy helper. > > Well, the is_opened flag doesn't get set/cleared at the same time as > __LINK_STATE_START. I'm not sure they are interchangeable... > > > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- a/drivers/net/ethernet/renesas/sh_eth.c > > +++ b/drivers/net/ethernet/renesas/sh_eth.c > > @@ -2441,8 +2441,6 @@ static int sh_eth_open(struct net_device *ndev) > > > > netif_start_queue(ndev); > > > > - mdp->is_opened = 1; > > - > > __LINK_STATE_START gets set before the ndo_open() method call, so > before the RPM call that enbales the clocks... > > > return ret; > > > > out_free_irq: > > @@ -2565,7 +2563,7 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev) > > if (mdp->cd->no_tx_cntrs) > > return &ndev->stats; > > > > - if (!mdp->is_opened) > > + if (!netif_running(ndev)) > > return &ndev->stats; > > I guess mdp->is_opened is checked here to avoid reading the counter > regs when RPM hasn't been called yet to enable the clocks... Exactly, cfr. commit 7fa2955ff70ce453 ("sh_eth: Fix sleeping function called from invalid context"). So you mean sh_eth_get_stats() can now be called after setting __LINK_STATE_START, but before RPM has enabled the clocks? Is there some protection against parallel execution of ndo_open() and get_stats()? Gr{oetje,eeting}s, Geert
On Thu, 23 Mar 2023 09:32:27 +0100 Geert Uytterhoeven wrote: > Is there some protection against parallel execution of ndo_open() > and get_stats()? Nope - one is under rtnl_lock, the other under just RCU, IIRC. So this patch just makes the race worse, but it was already racy before.
On 3/23/23 7:40 PM, Jakub Kicinski wrote: [...] >> Is there some protection against parallel execution of ndo_open() >> and get_stats()? > > Nope - one is under rtnl_lock, the other under just RCU, IIRC. > So this patch just makes the race worse, but it was already > racy before. How about reverting it then? MBR, Sergey
Hello! On 3/23/23 11:32 AM, Geert Uytterhoeven wrote: [...] >>> It had a purpose back in the days, but today we have a handy helper. >> >> Well, the is_opened flag doesn't get set/cleared at the same time as >> __LINK_STATE_START. I'm not sure they are interchangeable... >> >>> Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> >>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > >>> --- a/drivers/net/ethernet/renesas/sh_eth.c >>> +++ b/drivers/net/ethernet/renesas/sh_eth.c >>> @@ -2441,8 +2441,6 @@ static int sh_eth_open(struct net_device *ndev) >>> >>> netif_start_queue(ndev); >>> >>> - mdp->is_opened = 1; >>> - >> >> __LINK_STATE_START gets set before the ndo_open() method call, so >> before the RPM call that enbales the clocks... >> >>> return ret; >>> >>> out_free_irq: >>> @@ -2565,7 +2563,7 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev) >>> if (mdp->cd->no_tx_cntrs) >>> return &ndev->stats; >>> >>> - if (!mdp->is_opened) >>> + if (!netif_running(ndev)) >>> return &ndev->stats; >> >> I guess mdp->is_opened is checked here to avoid reading the counter >> regs when RPM hasn't been called yet to enable the clocks... > > Exactly, cfr. commit 7fa2955ff70ce453 ("sh_eth: Fix sleeping function > called from invalid context"). Yeah, pm_runtime_get_sync() couldn't be called in this case as netstat_show() invoked read_lock() that ensued calling preempt_disable()... > So you mean sh_eth_get_stats() can now be called after setting > __LINK_STATE_START, but before RPM has enabled the clocks? Yes, probably... > Is there some protection against parallel execution of ndo_open() > and get_stats()? Haven't seen it (yet?)... > Gr{oetje,eeting}s, > > Geert MBR, Sergey
> > Nope - one is under rtnl_lock, the other under just RCU, IIRC. > > So this patch just makes the race worse, but it was already > > racy before. > > How about reverting it then? Agreed. Will send a revert.
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c index d8ec729825be..2d9787231099 100644 --- a/drivers/net/ethernet/renesas/sh_eth.c +++ b/drivers/net/ethernet/renesas/sh_eth.c @@ -2441,8 +2441,6 @@ static int sh_eth_open(struct net_device *ndev) netif_start_queue(ndev); - mdp->is_opened = 1; - return ret; out_free_irq: @@ -2565,7 +2563,7 @@ static struct net_device_stats *sh_eth_get_stats(struct net_device *ndev) if (mdp->cd->no_tx_cntrs) return &ndev->stats; - if (!mdp->is_opened) + if (!netif_running(ndev)) return &ndev->stats; sh_eth_update_stat(ndev, &ndev->stats.tx_dropped, TROCR); @@ -2614,8 +2612,6 @@ static int sh_eth_close(struct net_device *ndev) /* Free all the skbuffs in the Rx queue and the DMA buffer. */ sh_eth_ring_free(ndev); - mdp->is_opened = 0; - pm_runtime_put(&mdp->pdev->dev); return 0; diff --git a/drivers/net/ethernet/renesas/sh_eth.h b/drivers/net/ethernet/renesas/sh_eth.h index a5c07c6ff44a..f56dbc8a064a 100644 --- a/drivers/net/ethernet/renesas/sh_eth.h +++ b/drivers/net/ethernet/renesas/sh_eth.h @@ -560,7 +560,6 @@ struct sh_eth_private { unsigned no_ether_link:1; unsigned ether_link_active_low:1; - unsigned is_opened:1; unsigned wol_enabled:1; };
It had a purpose back in the days, but today we have a handy helper. Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Based on 6.3-rc3 and tested on a Renesas Lager board (R-Car H2). drivers/net/ethernet/renesas/sh_eth.c | 6 +----- drivers/net/ethernet/renesas/sh_eth.h | 1 - 2 files changed, 1 insertion(+), 6 deletions(-)