diff mbox series

[net-next] ravb: Fix "failed to switch device to config mode" message during unbind

Message ID 20221213095938.1280861-1-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] ravb: Fix "failed to switch device to config mode" message during unbind | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 12 of 12 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 13 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Biju Das Dec. 13, 2022, 9:59 a.m. UTC
This patch fixes the error "ravb 11c20000.ethernet eth0: failed to switch
device to config mode" during unbind.

We are doing register access after pm_runtime_put_sync().

We usually do cleanup in reverse order of init. Currently in
remove(), the "pm_runtime_put_sync" is not in reverse order.

Probe
	reset_control_deassert(rstc);
	pm_runtime_enable(&pdev->dev);
	pm_runtime_get_sync(&pdev->dev);

remove
	pm_runtime_put_sync(&pdev->dev);
	unregister_netdev(ndev);
	..
	ravb_mdio_release(priv);
	pm_runtime_disable(&pdev->dev);

Consider the call to unregister_netdev()
unregister_netdev->unregister_netdevice_queue->rollback_registered_many
that calls the below functions which access the registers after
pm_runtime_put_sync()
 1) ravb_get_stats
 2) ravb_close

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Cc: stable@vger.kernel.org
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Leon Romanovsky Dec. 14, 2022, 7:07 a.m. UTC | #1
On Tue, Dec 13, 2022 at 09:59:38AM +0000, Biju Das wrote:
> This patch fixes the error "ravb 11c20000.ethernet eth0: failed to switch
> device to config mode" during unbind.
> 
> We are doing register access after pm_runtime_put_sync().
> 
> We usually do cleanup in reverse order of init. Currently in
> remove(), the "pm_runtime_put_sync" is not in reverse order.
> 
> Probe
> 	reset_control_deassert(rstc);
> 	pm_runtime_enable(&pdev->dev);
> 	pm_runtime_get_sync(&pdev->dev);
> 
> remove
> 	pm_runtime_put_sync(&pdev->dev);
> 	unregister_netdev(ndev);
> 	..
> 	ravb_mdio_release(priv);
> 	pm_runtime_disable(&pdev->dev);
> 
> Consider the call to unregister_netdev()
> unregister_netdev->unregister_netdevice_queue->rollback_registered_many
> that calls the below functions which access the registers after
> pm_runtime_put_sync()
>  1) ravb_get_stats
>  2) ravb_close
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

I don't know how you came to this fixes line, but the more correct one
is c156633f1353 ("Renesas Ethernet AVB driver proper")

Ant the title should need to be "PATCH net".

When you resend the patch, feel free to add my tag.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Biju Das Dec. 14, 2022, 8:07 a.m. UTC | #2
Hi Leon Romanovsky,

Thanks for the feedback.

> Subject: Re: [PATCH net-next] ravb: Fix "failed to switch device to config
> mode" message during unbind
> 
> On Tue, Dec 13, 2022 at 09:59:38AM +0000, Biju Das wrote:
> > This patch fixes the error "ravb 11c20000.ethernet eth0: failed to
> > switch device to config mode" during unbind.
> >
> > We are doing register access after pm_runtime_put_sync().
> >
> > We usually do cleanup in reverse order of init. Currently in remove(),
> > the "pm_runtime_put_sync" is not in reverse order.
> >
> > Probe
> > 	reset_control_deassert(rstc);
> > 	pm_runtime_enable(&pdev->dev);
> > 	pm_runtime_get_sync(&pdev->dev);
> >
> > remove
> > 	pm_runtime_put_sync(&pdev->dev);
> > 	unregister_netdev(ndev);
> > 	..
> > 	ravb_mdio_release(priv);
> > 	pm_runtime_disable(&pdev->dev);
> >
> > Consider the call to unregister_netdev()
> > unregister_netdev->unregister_netdevice_queue->rollback_registered_man
> > y that calls the below functions which access the registers after
> > pm_runtime_put_sync()
> >  1) ravb_get_stats
> >  2) ravb_close
> >
> > Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> 
> I don't know how you came to this fixes line, but the more correct one is
> c156633f1353 ("Renesas Ethernet AVB driver proper")

I got the details from [1]. The file name is renamed immediately after c156633f1353.

So from Stable backporting point I feel [1] is better.

What do you think?

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/ethernet/renesas/ravb_main.c?h=next-20221214&id=a0d2f20650e81407d8e51ad2cbdc492861c74e9c

> 
> Ant the title should need to be "PATCH net".
> 
> When you resend the patch, feel free to add my tag.
> 
> Thanks,
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
Leon Romanovsky Dec. 14, 2022, 8:12 a.m. UTC | #3
On Wed, Dec 14, 2022 at 08:07:55AM +0000, Biju Das wrote:
> Hi Leon Romanovsky,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH net-next] ravb: Fix "failed to switch device to config
> > mode" message during unbind
> > 
> > On Tue, Dec 13, 2022 at 09:59:38AM +0000, Biju Das wrote:
> > > This patch fixes the error "ravb 11c20000.ethernet eth0: failed to
> > > switch device to config mode" during unbind.
> > >
> > > We are doing register access after pm_runtime_put_sync().
> > >
> > > We usually do cleanup in reverse order of init. Currently in remove(),
> > > the "pm_runtime_put_sync" is not in reverse order.
> > >
> > > Probe
> > > 	reset_control_deassert(rstc);
> > > 	pm_runtime_enable(&pdev->dev);
> > > 	pm_runtime_get_sync(&pdev->dev);
> > >
> > > remove
> > > 	pm_runtime_put_sync(&pdev->dev);
> > > 	unregister_netdev(ndev);
> > > 	..
> > > 	ravb_mdio_release(priv);
> > > 	pm_runtime_disable(&pdev->dev);
> > >
> > > Consider the call to unregister_netdev()
> > > unregister_netdev->unregister_netdevice_queue->rollback_registered_man
> > > y that calls the below functions which access the registers after
> > > pm_runtime_put_sync()
> > >  1) ravb_get_stats
> > >  2) ravb_close
> > >
> > > Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> > 
> > I don't know how you came to this fixes line, but the more correct one is
> > c156633f1353 ("Renesas Ethernet AVB driver proper")
> 
> I got the details from [1]. The file name is renamed immediately after c156633f1353.
> 
> So from Stable backporting point I feel [1] is better.

No, you should use correct tag from the beginning, @stable will figure it.

Thanks
Biju Das Dec. 14, 2022, 8:15 a.m. UTC | #4
> Subject: Re: [PATCH net-next] ravb: Fix "failed to switch device to config
> mode" message during unbind
> 
> On Wed, Dec 14, 2022 at 08:07:55AM +0000, Biju Das wrote:
> > Hi Leon Romanovsky,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH net-next] ravb: Fix "failed to switch device to
> > > config mode" message during unbind
> > >
> > > On Tue, Dec 13, 2022 at 09:59:38AM +0000, Biju Das wrote:
> > > > This patch fixes the error "ravb 11c20000.ethernet eth0: failed to
> > > > switch device to config mode" during unbind.
> > > >
> > > > We are doing register access after pm_runtime_put_sync().
> > > >
> > > > We usually do cleanup in reverse order of init. Currently in
> > > > remove(), the "pm_runtime_put_sync" is not in reverse order.
> > > >
> > > > Probe
> > > > 	reset_control_deassert(rstc);
> > > > 	pm_runtime_enable(&pdev->dev);
> > > > 	pm_runtime_get_sync(&pdev->dev);
> > > >
> > > > remove
> > > > 	pm_runtime_put_sync(&pdev->dev);
> > > > 	unregister_netdev(ndev);
> > > > 	..
> > > > 	ravb_mdio_release(priv);
> > > > 	pm_runtime_disable(&pdev->dev);
> > > >
> > > > Consider the call to unregister_netdev()
> > > > unregister_netdev->unregister_netdevice_queue->rollback_registered
> > > > _man y that calls the below functions which access the registers
> > > > after
> > > > pm_runtime_put_sync()
> > > >  1) ravb_get_stats
> > > >  2) ravb_close
> > > >
> > > > Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> > >
> > > I don't know how you came to this fixes line, but the more correct
> > > one is
> > > c156633f1353 ("Renesas Ethernet AVB driver proper")
> >
> > I got the details from [1]. The file name is renamed immediately after
> c156633f1353.
> >
> > So from Stable backporting point I feel [1] is better.
> 
> No, you should use correct tag from the beginning, @stable will figure it.

OK will send v2 with PATCH net and correct fixes tag.

Cheers,
Biju
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 33f723a9f471..b4e0fc7f65bd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2903,12 +2903,12 @@  static int ravb_remove(struct platform_device *pdev)
 			  priv->desc_bat_dma);
 	/* Set reset mode */
 	ravb_write(ndev, CCC_OPC_RESET, CCC);
-	pm_runtime_put_sync(&pdev->dev);
 	unregister_netdev(ndev);
 	if (info->nc_queues)
 		netif_napi_del(&priv->napi[RAVB_NC]);
 	netif_napi_del(&priv->napi[RAVB_BE]);
 	ravb_mdio_release(priv);
+	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	reset_control_assert(priv->rstc);
 	free_netdev(ndev);