[PATCH] drm/vc4: Add module dependency on hdmi-codec

[PATCH] drm/vc4: Add module dependency on hdmi-codec

[Maxime Ripard]

The VC4 HDMI controller driver relies on the HDMI codec ASoC driver. In
order to set it up properly, in vc4_hdmi_audio_init(), our HDMI driver
will register a device matching the HDMI codec driver, and then register
an ASoC card using that codec.

However, if vc4 is compiled as a module, chances are that the hdmi-codec
driver will be too. In such a case, the module loader will have a very
narrow window to load the module between the device registration and the
card registration.

If it fails to load the module in time, the card registration will fail
with EPROBE_DEFER, and we'll abort the audio initialisation,
unregistering the HDMI codec device in the process.

The next time the bind callback will be run, it's likely that we end up
missing that window again, effectively preventing vc4 to probe entirely.

In order to prevent this, we can create a soft dependency of the vc4
driver on the HDMI codec one so that we're sure the HDMI codec will be
loaded before the VC4 module is, and thus we'll never end up in the
previous situation.

Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec")
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index ffbbb454c9e8..2027063fdc30 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -490,6 +490,7 @@ module_init(vc4_drm_register);
 module_exit(vc4_drm_unregister);
 
 MODULE_ALIAS("platform:vc4-drm");
+MODULE_SOFTDEP("pre: snd-soc-hdmi-codec");
 MODULE_DESCRIPTION("Broadcom VC4 DRM Driver");
 MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
 MODULE_LICENSE("GPL v2");
-- 
2.37.1

Re: [PATCH] drm/vc4: Add module dependency on hdmi-codec

[Javier Martinez Canillas]

Hello Maxime,

On 9/2/22 16:41, Maxime Ripard wrote:
> The VC4 HDMI controller driver relies on the HDMI codec ASoC driver. In
> order to set it up properly, in vc4_hdmi_audio_init(), our HDMI driver
> will register a device matching the HDMI codec driver, and then register
> an ASoC card using that codec.
> 
> However, if vc4 is compiled as a module, chances are that the hdmi-codec
> driver will be too. In such a case, the module loader will have a very
> narrow window to load the module between the device registration and the
> card registration.
> 
> If it fails to load the module in time, the card registration will fail
> with EPROBE_DEFER, and we'll abort the audio initialisation,
> unregistering the HDMI codec device in the process.
> 
> The next time the bind callback will be run, it's likely that we end up
> missing that window again, effectively preventing vc4 to probe entirely.
> 
> In order to prevent this, we can create a soft dependency of the vc4
> driver on the HDMI codec one so that we're sure the HDMI codec will be
> loaded before the VC4 module is, and thus we'll never end up in the
> previous situation.
> 
> Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec")
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/vc4/vc4_drv.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index ffbbb454c9e8..2027063fdc30 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -490,6 +490,7 @@ module_init(vc4_drm_register);
>  module_exit(vc4_drm_unregister);
>  
>  MODULE_ALIAS("platform:vc4-drm");
> +MODULE_SOFTDEP("pre: snd-soc-hdmi-codec");

While this will solve the issue, I'm not a big fan of the MODULE_SOFTDEP()
macro to be honest. I always have the feeling that's fragile and something
that may not be updated if for example a module name changes in the future.

I wonder if instead the HDMI_CODEC_DRV_NAME platform device should not be
registered earlier and in a place that's not cleaned up in the probe error
path. For example in vc4_drm_register() instead of vc4_hdmi_audio_init().

Granted, that would register the platform device even when HDMI isn't used
which is disadvantage of doing it at vc4_hdmi_bind() time. But still, feel
that is more robust to rely on the Linux device model than module softdeps.

Having said that, I don't have a strong opinion and this patch is correct
and has been in the mailing list for a long time. So feel free to push it
if you think is the correct approach to solve this:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH] drm/vc4: Add module dependency on hdmi-codec

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3616 bytes --]

Hi Javier,

Thanks for reviewing this

On Thu, Oct 13, 2022 at 11:27:15AM +0200, Javier Martinez Canillas wrote:
> On 9/2/22 16:41, Maxime Ripard wrote:
> > The VC4 HDMI controller driver relies on the HDMI codec ASoC driver. In
> > order to set it up properly, in vc4_hdmi_audio_init(), our HDMI driver
> > will register a device matching the HDMI codec driver, and then register
> > an ASoC card using that codec.
> > 
> > However, if vc4 is compiled as a module, chances are that the hdmi-codec
> > driver will be too. In such a case, the module loader will have a very
> > narrow window to load the module between the device registration and the
> > card registration.
> > 
> > If it fails to load the module in time, the card registration will fail
> > with EPROBE_DEFER, and we'll abort the audio initialisation,
> > unregistering the HDMI codec device in the process.
> > 
> > The next time the bind callback will be run, it's likely that we end up
> > missing that window again, effectively preventing vc4 to probe entirely.
> > 
> > In order to prevent this, we can create a soft dependency of the vc4
> > driver on the HDMI codec one so that we're sure the HDMI codec will be
> > loaded before the VC4 module is, and thus we'll never end up in the
> > previous situation.
> > 
> > Fixes: 91e99e113929 ("drm/vc4: hdmi: Register HDMI codec")
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/vc4/vc4_drv.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> > index ffbbb454c9e8..2027063fdc30 100644
> > --- a/drivers/gpu/drm/vc4/vc4_drv.c
> > +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> > @@ -490,6 +490,7 @@ module_init(vc4_drm_register);
> >  module_exit(vc4_drm_unregister);
> >  
> >  MODULE_ALIAS("platform:vc4-drm");
> > +MODULE_SOFTDEP("pre: snd-soc-hdmi-codec");
> 
> While this will solve the issue, I'm not a big fan of the MODULE_SOFTDEP()
> macro to be honest. I always have the feeling that's fragile and something
> that may not be updated if for example a module name changes in the future.
> 
> I wonder if instead the HDMI_CODEC_DRV_NAME platform device should not be
> registered earlier and in a place that's not cleaned up in the probe error
> path. For example in vc4_drm_register() instead of vc4_hdmi_audio_init().
> 
> Granted, that would register the platform device even when HDMI isn't used
> which is disadvantage of doing it at vc4_hdmi_bind() time. But still, feel
> that is more robust to rely on the Linux device model than module softdeps.

Yeah, I wished we had something more robust.

Another complication stems from the fact that when we register that
device the codec ops are passed as platform_data, and we'll dereference
the device private data in those. If we do it in bind, it gets very easy
to dereference something that hasn't been allocated yet.

It will probably be NULL, but a NULL pointer dereference is already
pretty bad :)

Also, this "only" widens the window that the module loader has, but the
window is still there. MODULE_SOFTDEP, even though it relies on the name
is thus is fairly fragile, closes it entirely, for every module loading
situation (with or without udev in the system).

> Having said that, I don't have a strong opinion and this patch is correct
> and has been in the mailing list for a long time. So feel free to push it
> if you think is the correct approach to solve this:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: (subset) [PATCH] drm/vc4: Add module dependency on hdmi-codec

[Maxime Ripard]

On Fri, 2 Sep 2022 16:41:11 +0200, Maxime Ripard wrote:
> The VC4 HDMI controller driver relies on the HDMI codec ASoC driver. In
> order to set it up properly, in vc4_hdmi_audio_init(), our HDMI driver
> will register a device matching the HDMI codec driver, and then register
> an ASoC card using that codec.
> 
> However, if vc4 is compiled as a module, chances are that the hdmi-codec
> driver will be too. In such a case, the module loader will have a very
> narrow window to load the module between the device registration and the
> card registration.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime