[PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Diogo Ivo]

Remove the duplicate calls to prueth_emac_stop() and
prueth_cleanup_tx_chns() in emac_ndo_stop().

Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 411898a4f38c..cf7b73f8f450 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -1489,9 +1489,6 @@ static int emac_ndo_stop(struct net_device *ndev)
 	/* Destroying the queued work in ndo_stop() */
 	cancel_delayed_work_sync(&emac->stats_work);
 
-	/* stop PRUs */
-	prueth_emac_stop(emac);
-
 	if (prueth->emacs_initialized == 1)
 		icss_iep_exit(emac->iep);
 
@@ -1502,7 +1499,6 @@ static int emac_ndo_stop(struct net_device *ndev)
 
 	free_irq(emac->rx_chns.irq[rx_flow], emac);
 	prueth_ndev_del_tx_napi(emac, emac->tx_ch_num);
-	prueth_cleanup_tx_chns(emac);
 
 	prueth_cleanup_rx_chns(emac, &emac->rx_chns, max_rx_flows);
 	prueth_cleanup_tx_chns(emac);
-- 
2.43.0

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Simon Horman]

On Tue, Feb 06, 2024 at 03:20:51PM +0000, Diogo Ivo wrote:
> Remove the duplicate calls to prueth_emac_stop() and
> prueth_cleanup_tx_chns() in emac_ndo_stop().
> 
> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>

Hi Doigo,

I see that there are indeed duplicate calls,
but I do wonder if this is a cleanup rather than a bug:
is there a user-visible problem that this addresses?

If so, I think it would be good to spell this out in the commit message.

...

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Roger Quadros]

On 06/02/2024 17:20, Diogo Ivo wrote:
> Remove the duplicate calls to prueth_emac_stop() and
> prueth_cleanup_tx_chns() in emac_ndo_stop().
> 
> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>

Reviewed-by: Roger Quadros <rogerq@kernel.org>

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Dan Carpenter]

On Thu, Feb 08, 2024 at 10:47:00AM +0000, Simon Horman wrote:
> On Tue, Feb 06, 2024 at 03:20:51PM +0000, Diogo Ivo wrote:
> > Remove the duplicate calls to prueth_emac_stop() and
> > prueth_cleanup_tx_chns() in emac_ndo_stop().
> > 
> > Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> > Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> > Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
> 
> Hi Doigo,
> 
> I see that there are indeed duplicate calls,
> but I do wonder if this is a cleanup rather than a bug:
> is there a user-visible problem that this addresses?
> 
> If so, I think it would be good to spell this out in the commit message.
> 
> ...

So far as I can see from reviewing the code there is no user visible
effect.

rproc_shutdown() calls rproc_stop() which sets "rproc->state = RPROC_OFFLINE;"
so the second call will return be a no-op and return -EINVAL.  But the
return value is not checked so no problem.

prueth_cleanup_tx_chns() uses memset to zero out the emac->tx_chns[] so
the second call will be a no-op.

regards,
dan carpenter

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Diogo Ivo]

On 2/8/24 13:33, Dan Carpenter wrote:
> On Thu, Feb 08, 2024 at 10:47:00AM +0000, Simon Horman wrote:
>> On Tue, Feb 06, 2024 at 03:20:51PM +0000, Diogo Ivo wrote:
>>> Remove the duplicate calls to prueth_emac_stop() and
>>> prueth_cleanup_tx_chns() in emac_ndo_stop().
>>>
>>> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
>>> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
>>> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
>> Hi Doigo,
>>
>> I see that there are indeed duplicate calls,
>> but I do wonder if this is a cleanup rather than a bug:
>> is there a user-visible problem that this addresses?
>>
>> If so, I think it would be good to spell this out in the commit message.
>>
>> ...
> So far as I can see from reviewing the code there is no user visible
> effect.
>
> rproc_shutdown() calls rproc_stop() which sets "rproc->state = RPROC_OFFLINE;"
> so the second call will return be a no-op and return -EINVAL.  But the
> return value is not checked so no problem.
>
> prueth_cleanup_tx_chns() uses memset to zero out the emac->tx_chns[] so
> the second call will be a no-op.
>
> regards,
> dan carpenter

Yes, it is just a code cleanup. Is the commit message fine as it is?


Best regards,

Diogo

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Anwar, Md Danish]

On 2/6/2024 8:50 PM, Diogo Ivo wrote:
> Remove the duplicate calls to prueth_emac_stop() and
> prueth_cleanup_tx_chns() in emac_ndo_stop().
> 
> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>

Reviewed-by: MD Danish Anwar <danishanwar@ti.com>

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Diogo Ivo]

On 2/6/24 15:20, Diogo Ivo wrote:
> Remove the duplicate calls to prueth_emac_stop() and
> prueth_cleanup_tx_chns() in emac_ndo_stop().
>
> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> Fixes: 186734c15886 ("net: ti: icssg-prueth: add packet timestamping and ptp support")
> Signed-off-by: Diogo Ivo <diogo.ivo@siemens.com>
> ---
>   drivers/net/ethernet/ti/icssg/icssg_prueth.c | 4 ----
>   1 file changed, 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 411898a4f38c..cf7b73f8f450 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -1489,9 +1489,6 @@ static int emac_ndo_stop(struct net_device *ndev)
>   	/* Destroying the queued work in ndo_stop() */
>   	cancel_delayed_work_sync(&emac->stats_work);
>   
> -	/* stop PRUs */
> -	prueth_emac_stop(emac);
> -
>   	if (prueth->emacs_initialized == 1)
>   		icss_iep_exit(emac->iep);
>   
> @@ -1502,7 +1499,6 @@ static int emac_ndo_stop(struct net_device *ndev)
>   
>   	free_irq(emac->rx_chns.irq[rx_flow], emac);
>   	prueth_ndev_del_tx_napi(emac, emac->tx_ch_num);
> -	prueth_cleanup_tx_chns(emac);
>   
>   	prueth_cleanup_rx_chns(emac, &emac->rx_chns, max_rx_flows);
>   	prueth_cleanup_tx_chns(emac);

Hello,

Gentle ping on this patch.


Thank you,

Diogo

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Andrew Lunn]

> Hello,
> 
> Gentle ping on this patch.

Gentle ping's don't work for netdev. Merging patches is pretty much
driver by patchwork, so it is good to look there and see the state of
the patch.

https://patchwork.kernel.org/project/netdevbpf/patch/20240206152052.98217-1-diogo.ivo@siemens.com/

This is marked as Changes Requested.

Looking at the discussion, it seemed to conclude this is a cleanup,
not a fix. Hence the two Fixes: probably want removing.

Please repost with them removed, and the Reviewed-by's added.

You should also set the patch subject to include [net-next] to
indicate which tree this patch is for:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

       Andrew

Re: [PATCH net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop()

[Diogo Ivo]

On 2/15/24 14:53, Andrew Lunn wrote:
>> Hello,
>>
>> Gentle ping on this patch.
> Gentle ping's don't work for netdev. Merging patches is pretty much
> driver by patchwork, so it is good to look there and see the state of
> the patch.
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Fnetdevbpf%2Fpatch%2F20240206152052.98217-1-diogo.ivo%40siemens.com%2F&data=05%7C02%7Cdiogo.ivo%40siemens.com%7Cc6a5a844ef59437e85f308dc2e35cb4b%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C638436055786669669%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=CMjL%2Bkc9%2BOo0igLSnX4OzgxO3CGKP2m67afScU0pG0I%3D&reserved=0
>
> This is marked as Changes Requested.
>
> Looking at the discussion, it seemed to conclude this is a cleanup,
> not a fix. Hence the two Fixes: probably want removing.
>
> Please repost with them removed, and the Reviewed-by's added.
>
> You should also set the patch subject to include [net-next] to
> indicate which tree this patch is for:
>
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fmaintainer-netdev.html%23netdev-faq&data=05%7C02%7Cdiogo.ivo%40siemens.com%7Cc6a5a844ef59437e85f308dc2e35cb4b%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C638436055786678320%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=ZS2Gn%2F9kqM7UKL9VD8mSCXeegsx%2BXI6jvgW30XoqQ2M%3D&reserved=0
>
>         Andrew

Ok, thank you for the clarification!

I'll prepare the patch as requested.


Thanks,

Diogo