| Message ID | 20240206152052.98217-1-diogo.ivo@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net] net: ti: icssg-prueth: Remove duplicate cleanup calls in emac_ndo_stop() | expand |
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. ...
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>
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
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
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>
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
> 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
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
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);
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(-)