| Message ID | 20240808-rk-drm-fix-unreg-v1-1-c30d9a754722@collabora.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] drm/rockchip: Unregister platform drivers in reverse order | expand |
Hi Cristian, At 2024-08-08 19:58:02, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote: >Move rockchip_drm_platform_driver unregistration after its sub-drivers, >which ensures all drivers are unregistered in the reverse order used >when they were registered. Would you please provied some detail information about how to reproduce this issue this patch try to fix？Or some kernel log when this issue triggered。 > >Fixes: 8820b68bd378 ("drm/rockchip: Refactor the component match logic.") >Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >--- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >index 44d769d9234d..ca7b07503fbe 100644 >--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >@@ -528,10 +528,9 @@ static int __init rockchip_drm_init(void) > > static void __exit rockchip_drm_fini(void) > { >- platform_driver_unregister(&rockchip_drm_platform_driver); >- > platform_unregister_drivers(rockchip_sub_drivers, > num_rockchip_sub_drivers); >+ platform_driver_unregister(&rockchip_drm_platform_driver); > } > > module_init(rockchip_drm_init); > >--- >base-commit: 1eb586a9782cde8e5091b9de74603e0a8386b09e >change-id: 20240702-rk-drm-fix-unreg-9f3f29996a00 >-- >Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > > >_______________________________________________ >Linux-rockchip mailing list >Linux-rockchip@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Andy, On 8/12/24 5:14 AM, Andy Yan wrote: > > Hi Cristian, > > At 2024-08-08 19:58:02, "Cristian Ciocaltea" <cristian.ciocaltea@collabora.com> wrote: >> Move rockchip_drm_platform_driver unregistration after its sub-drivers, >> which ensures all drivers are unregistered in the reverse order used >> when they were registered. > > Would you please provied some detail information about how to reproduce this > issue this patch try to fix？Or some kernel log when this issue triggered。 I submitted this patch while investigating a couple of issues encountered when tried to reload the rockchipdrm module. One was a system freeze, which eventually proved to have a different root cause and got fixed via [1]. The other one was a lockdep splat which seems to be caused by the switch to maple tree register cache in vop2 - I have a regmap workaround, not yet sure that's a proper fix. As of v6.11-rc1, reloading the module works fine, w/ or w/o this patch applied (ignoring the above mentioned splat). But I could only verify on Rock 3A, hence unregistering the drivers in the correct order should, at least, eliminate a potential source of unexpected behavior on the other boards. Regards, Cristian [1]: 9d42c3ee3ce3 ("arm64: dts: rockchip: Add missing power-domains for rk356x vop_mmu") >> Fixes: 8820b68bd378 ("drm/rockchip: Refactor the component match logic.") >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> >> --- >> drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> index 44d769d9234d..ca7b07503fbe 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c >> @@ -528,10 +528,9 @@ static int __init rockchip_drm_init(void) >> >> static void __exit rockchip_drm_fini(void) >> { >> - platform_driver_unregister(&rockchip_drm_platform_driver); >> - >> platform_unregister_drivers(rockchip_sub_drivers, >> num_rockchip_sub_drivers); >> + platform_driver_unregister(&rockchip_drm_platform_driver); >> } >> >> module_init(rockchip_drm_init); >> >> --- >> base-commit: 1eb586a9782cde8e5091b9de74603e0a8386b09e >> change-id: 20240702-rk-drm-fix-unreg-9f3f29996a00 >> -- >> Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Am Donnerstag, 8. August 2024, 13:58:02 CEST schrieb Cristian Ciocaltea: > Move rockchip_drm_platform_driver unregistration after its sub-drivers, > which ensures all drivers are unregistered in the reverse order used > when they were registered. are you sure about that? I.e. currently rockchip_drm_init() does platform_register_drivers(rockchip_sub_drivers, ...) to register the sub-drivers and only after that registers the main drm-platform-driver rockchip_drm_fini currently does the reverse of first unregistering the main drm-platform-driver and after that unregistering the array of sub- drivers. So as it stands right now, rockchip_drm_fini does already do exactly the reverse when de-registering. > Fixes: 8820b68bd378 ("drm/rockchip: Refactor the component match logic.") > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > --- > drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > index 44d769d9234d..ca7b07503fbe 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c > @@ -528,10 +528,9 @@ static int __init rockchip_drm_init(void) > > static void __exit rockchip_drm_fini(void) > { > - platform_driver_unregister(&rockchip_drm_platform_driver); > - > platform_unregister_drivers(rockchip_sub_drivers, > num_rockchip_sub_drivers); > + platform_driver_unregister(&rockchip_drm_platform_driver); > } > > module_init(rockchip_drm_init); > > --- > base-commit: 1eb586a9782cde8e5091b9de74603e0a8386b09e > change-id: 20240702-rk-drm-fix-unreg-9f3f29996a00 >
On 8/15/24 5:21 PM, Heiko Stübner wrote: > Am Donnerstag, 8. August 2024, 13:58:02 CEST schrieb Cristian Ciocaltea: >> Move rockchip_drm_platform_driver unregistration after its sub-drivers, >> which ensures all drivers are unregistered in the reverse order used >> when they were registered. > > are you sure about that? > > I.e. currently rockchip_drm_init() does > platform_register_drivers(rockchip_sub_drivers, ...) > to register the sub-drivers and only after that registers the main > drm-platform-driver > > rockchip_drm_fini currently does the reverse of first unregistering the > main drm-platform-driver and after that unregistering the array of sub- > drivers. > > > So as it stands right now, rockchip_drm_fini does already do exactly the > reverse when de-registering. Indeed, somehow I overlooked this while debugging some module unloading issues. I guess it just felt more naturally to have the subdrivers unregistered first. Out of curiosity to see if there's a common pattern for handling this, I found that most drivers do indeed unregister the subdrivers before the main platform one, but weirdly enough, some of them do also keep the same order on registration, similarily to what this patch unintentionally does: drivers/power/supply/ab8500_charger.c drivers/gpu/drm/vc4/vc4_drv.c drivers/gpu/drm/mcde/mcde_drv.c Not sure if those are potential mistakes, or maybe it doesn't really matter?! Please let me know if you have a preference for it, and I'll update the patch accordingly, otherwise let's just ignore it altogether. Thanks, Cristian
Am Donnerstag, 15. August 2024, 19:26:54 CEST schrieb Cristian Ciocaltea: > On 8/15/24 5:21 PM, Heiko Stübner wrote: > > Am Donnerstag, 8. August 2024, 13:58:02 CEST schrieb Cristian Ciocaltea: > >> Move rockchip_drm_platform_driver unregistration after its sub-drivers, > >> which ensures all drivers are unregistered in the reverse order used > >> when they were registered. > > > > are you sure about that? > > > > I.e. currently rockchip_drm_init() does > > platform_register_drivers(rockchip_sub_drivers, ...) > > to register the sub-drivers and only after that registers the main > > drm-platform-driver > > > > rockchip_drm_fini currently does the reverse of first unregistering the > > main drm-platform-driver and after that unregistering the array of sub- > > drivers. > > > > > > So as it stands right now, rockchip_drm_fini does already do exactly the > > reverse when de-registering. > > Indeed, somehow I overlooked this while debugging some module unloading > issues. I guess it just felt more naturally to have the subdrivers > unregistered first. > > Out of curiosity to see if there's a common pattern for handling this, I > found that most drivers do indeed unregister the subdrivers before the main > platform one, but weirdly enough, some of them do also keep the same order > on registration, similarily to what this patch unintentionally does: > > drivers/power/supply/ab8500_charger.c > drivers/gpu/drm/vc4/vc4_drv.c > drivers/gpu/drm/mcde/mcde_drv.c > > Not sure if those are potential mistakes, or maybe it doesn't really matter?! > > Please let me know if you have a preference for it, and I'll update the > patch accordingly, otherwise let's just ignore it altogether. in theory it shouldn't matter, simply because the component framework will only bind when all driver are present and unbind when the first driver vanishes. But I really like doing the reverse order more - so as it is now. You also wouldn't disable clocks, before trying to deactivate some device- function. So deactivating stuff in the reverse order of them getting activated is most likely less error prone.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index 44d769d9234d..ca7b07503fbe 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -528,10 +528,9 @@ static int __init rockchip_drm_init(void) static void __exit rockchip_drm_fini(void) { - platform_driver_unregister(&rockchip_drm_platform_driver); - platform_unregister_drivers(rockchip_sub_drivers, num_rockchip_sub_drivers); + platform_driver_unregister(&rockchip_drm_platform_driver); } module_init(rockchip_drm_init);
Move rockchip_drm_platform_driver unregistration after its sub-drivers, which ensures all drivers are unregistered in the reverse order used when they were registered. Fixes: 8820b68bd378 ("drm/rockchip: Refactor the component match logic.") Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- base-commit: 1eb586a9782cde8e5091b9de74603e0a8386b09e change-id: 20240702-rk-drm-fix-unreg-9f3f29996a00