* [PATCH net-next] sh_eth: remove open coded netif_running()
@ 2023-03-21 6:58 Wolfram Sang
2023-03-21 8:22 ` Leon Romanovsky
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Wolfram Sang @ 2023-03-21 6:58 UTC (permalink / raw)
To: netdev
Cc: linux-renesas-soc, Wolfram Sang, Geert Uytterhoeven,
Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, linux-kernel
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(-)
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;
};
--
2.30.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
@ 2023-03-21 8:22 ` Leon Romanovsky
2023-03-21 14:33 ` Geert Uytterhoeven
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Leon Romanovsky @ 2023-03-21 8:22 UTC (permalink / raw)
To: Wolfram Sang
Cc: netdev, linux-renesas-soc, Geert Uytterhoeven, Sergey Shtylyov,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
2023-03-21 8:22 ` Leon Romanovsky
@ 2023-03-21 14:33 ` Geert Uytterhoeven
2023-03-21 16:58 ` Florian Fainelli
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-03-21 14:33 UTC (permalink / raw)
To: Wolfram Sang
Cc: netdev, linux-renesas-soc, Geert Uytterhoeven, Sergey Shtylyov,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
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
--
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] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
2023-03-21 8:22 ` Leon Romanovsky
2023-03-21 14:33 ` Geert Uytterhoeven
@ 2023-03-21 16:58 ` Florian Fainelli
2023-03-22 12:30 ` patchwork-bot+netdevbpf
2023-03-22 20:54 ` Sergey Shtylyov
4 siblings, 0 replies; 12+ messages in thread
From: Florian Fainelli @ 2023-03-21 16:58 UTC (permalink / raw)
To: Wolfram Sang, netdev
Cc: linux-renesas-soc, Geert Uytterhoeven, Sergey Shtylyov,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
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>
--
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
` (2 preceding siblings ...)
2023-03-21 16:58 ` Florian Fainelli
@ 2023-03-22 12:30 ` patchwork-bot+netdevbpf
2023-03-22 20:57 ` Sergey Shtylyov
2023-03-22 20:54 ` Sergey Shtylyov
4 siblings, 1 reply; 12+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-03-22 12:30 UTC (permalink / raw)
To: Wolfram Sang
Cc: netdev, linux-renesas-soc, geert+renesas, s.shtylyov, davem,
edumazet, kuba, pabeni, linux-kernel
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
` (3 preceding siblings ...)
2023-03-22 12:30 ` patchwork-bot+netdevbpf
@ 2023-03-22 20:54 ` Sergey Shtylyov
2023-03-23 8:32 ` Geert Uytterhoeven
4 siblings, 1 reply; 12+ messages in thread
From: Sergey Shtylyov @ 2023-03-22 20:54 UTC (permalink / raw)
To: Wolfram Sang, netdev
Cc: linux-renesas-soc, Geert Uytterhoeven, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-22 12:30 ` patchwork-bot+netdevbpf
@ 2023-03-22 20:57 ` Sergey Shtylyov
0 siblings, 0 replies; 12+ messages in thread
From: Sergey Shtylyov @ 2023-03-22 20:57 UTC (permalink / raw)
To: patchwork-bot+netdevbpf, Wolfram Sang
Cc: netdev, linux-renesas-soc, geert+renesas, davem, edumazet, kuba,
pabeni, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-22 20:54 ` Sergey Shtylyov
@ 2023-03-23 8:32 ` Geert Uytterhoeven
2023-03-23 16:40 ` Jakub Kicinski
2023-03-25 20:56 ` Sergey Shtylyov
0 siblings, 2 replies; 12+ messages in thread
From: Geert Uytterhoeven @ 2023-03-23 8:32 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Wolfram Sang, netdev, linux-renesas-soc, Geert Uytterhoeven,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
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
--
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] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-23 8:32 ` Geert Uytterhoeven
@ 2023-03-23 16:40 ` Jakub Kicinski
2023-03-25 20:27 ` Sergey Shtylyov
2023-03-25 20:56 ` Sergey Shtylyov
1 sibling, 1 reply; 12+ messages in thread
From: Jakub Kicinski @ 2023-03-23 16:40 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Sergey Shtylyov, Wolfram Sang, netdev, linux-renesas-soc,
Geert Uytterhoeven, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-kernel
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-23 16:40 ` Jakub Kicinski
@ 2023-03-25 20:27 ` Sergey Shtylyov
2023-03-27 8:14 ` Wolfram Sang
0 siblings, 1 reply; 12+ messages in thread
From: Sergey Shtylyov @ 2023-03-25 20:27 UTC (permalink / raw)
To: Jakub Kicinski, Geert Uytterhoeven
Cc: Wolfram Sang, netdev, linux-renesas-soc, Geert Uytterhoeven,
David S. Miller, Eric Dumazet, Paolo Abeni, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-23 8:32 ` Geert Uytterhoeven
2023-03-23 16:40 ` Jakub Kicinski
@ 2023-03-25 20:56 ` Sergey Shtylyov
1 sibling, 0 replies; 12+ messages in thread
From: Sergey Shtylyov @ 2023-03-25 20:56 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Wolfram Sang, netdev, linux-renesas-soc, Geert Uytterhoeven,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next] sh_eth: remove open coded netif_running()
2023-03-25 20:27 ` Sergey Shtylyov
@ 2023-03-27 8:14 ` Wolfram Sang
0 siblings, 0 replies; 12+ messages in thread
From: Wolfram Sang @ 2023-03-27 8:14 UTC (permalink / raw)
To: Sergey Shtylyov
Cc: Jakub Kicinski, Geert Uytterhoeven, netdev, linux-renesas-soc,
Geert Uytterhoeven, David S. Miller, Eric Dumazet, Paolo Abeni,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 225 bytes --]
> > 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-03-27 8:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-21 6:58 [PATCH net-next] sh_eth: remove open coded netif_running() Wolfram Sang
2023-03-21 8:22 ` Leon Romanovsky
2023-03-21 14:33 ` Geert Uytterhoeven
2023-03-21 16:58 ` Florian Fainelli
2023-03-22 12:30 ` patchwork-bot+netdevbpf
2023-03-22 20:57 ` Sergey Shtylyov
2023-03-22 20:54 ` Sergey Shtylyov
2023-03-23 8:32 ` Geert Uytterhoeven
2023-03-23 16:40 ` Jakub Kicinski
2023-03-25 20:27 ` Sergey Shtylyov
2023-03-27 8:14 ` Wolfram Sang
2023-03-25 20:56 ` Sergey Shtylyov
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).