[PATCH net-next] sh_eth: remove open coded netif_running()

[PATCH net-next] sh_eth: remove open coded netif_running()

[Wolfram Sang]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Leon Romanovsky]

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>

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Geert Uytterhoeven]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Florian Fainelli]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[patchwork-bot+netdevbpf]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Sergey Shtylyov]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Sergey Shtylyov]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Geert Uytterhoeven]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Jakub Kicinski]

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.

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Sergey Shtylyov]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Sergey Shtylyov]

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

Re: [PATCH net-next] sh_eth: remove open coded netif_running()

[Wolfram Sang]

[-- 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 --]