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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
Thanks!
Maxime
Attachment:
signature.asc
Description: PGP signature
