It had a purpose back in the days, but today we have a handy helper.
Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
>
> 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 <[email protected]>
On Tue, Mar 21, 2023 at 7:58 AM Wolfram Sang
<[email protected]> wrote:
> It had a purpose back in the days, but today we have a handy helper.
>
> Reported-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
Reviewed-by: Geert Uytterhoeven <[email protected]>
No regressions seen on R-Car M2-W, RZ/A1H, and RZ/A2M.
Tested-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
Hello:
This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <[email protected]>:
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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
>
> 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
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 <[email protected]>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
>
> 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, [email protected] wrote:
[...]
> This patch was applied to netdev/net-next.git (main)
> by Paolo Abeni <[email protected]>:
>
> 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 <[email protected]>
>> Signed-off-by: Wolfram Sang <[email protected]>
>> ---
>>
>> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Wolfram Sang <[email protected]>
> > --- 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 -- [email protected]
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
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 <[email protected]>
>>> Signed-off-by: Wolfram Sang <[email protected]>
>
>>> --- 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.