Re: [PATCH 1/1] drm/i915: move audio component intialization before audio driver use it

[Lucas De Marchi <lucas.demarchi@intel.com>]

On Tue, Apr 10, 2018 at 10:58:28AM -0300, Jani Nikula wrote:
> On Tue, 10 Apr 2018, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi@intel.com> wrote:
> >>>On Tue, 10 Apr 2018, "Shi, Yang A" <yang.a.shi@intel.com> wrote:
> >>>> issue: snd_soc_skl meet "failed to add i915 component master (-19)"
> >>>> when platform don't connect any display output.
> >>>>
> >>>> i915 do initialization before than skl_probe, but if there is no
> >>>> display output connect, in function drm_dp_dpcd_access, there is a 32
> >>>> retry for aux i2c transactions. It will meet timeout and do usleep.

You should not really rely on "module is loaded, I can use it". It may
not be bound to the device yet in case of async probe for example.
You should rather wait for the service you want to use from the driver
to be ready. See documentation of __request_module(). 

> >>>> Then skl_probe function will be scheduled. It will call
> >>>> snd_hdac_i915_init, and it will meet "failed to add i915 component
> >>>> master" error.  And whole snd_soc_skl initialization will be failed,
> >>>> audio can't work normally anymore.

Humn... I fail to see how the usleep() would cause any of this. If the
probe is synchronous (see below) we an eventual call to request_module()
will/should wait for the first probe to finish nonetheless.
To me it seems the error is elsewhere.

> >>>>
> >>>> So i915 driver need to move intel_audio_init at the beginning of
> >>>> intel_modeset_init. This will make sure i915_audio_component_init
> >>>> process before snd_hdac_i915_init call it.
> >>>
> >>>We do intel_audio_init() and register the audio component when we are
> >>>ready to handle the audio component calls. We are ready at
> >>>i915_driver_register(). We are not ready at intel_modeset_init().
> >>>
> >>>BR,
> >>>Jani.
> >>
> >> Thanks to comments my patch.
> >> After I check the whole driver code, I think all ops in
> >> i915_audio_component_ops should be ready at the beginning of function
> >> intel_modeset_init. So can we move intel_audio_init as early as we
> >> can.
> >
> > No, that's not true. Just as an example, dev_priv->cdclk.hw.cdclk hasn't
> > been initialized.
> >
> >> Would you like to suggest a better place to do intel_audio_init?
> >
> > I think the call is already where it is supposed to be. We expose
> > ourselves to the rest of the system when we are ready. If it takes long,
> > it takes long. I think you have a race in your driver, and you need to
> > deal with it properly in your driver.
> >
> > In snd_hdac_i915_init(), I don't think there are any guarantees that the
> > request_module() call is the one actually probing i915. We might already

This should not be relevant in this case. Even if the module is already
mid-probe, request_module() will only return after modprobe returns
(since request_module forces UMH_WAIT_PROC).

If a second call races with the first, it will try to find the module in
the module list and end up waiting for it to complete. See
add_unformed_module().

> > be mid-probe. You don't even check or log request_module() return value.

Checking the return value of request_module() is absolutely the first
thing to do. It's plain broken otherwise... there are many reasons why a
call to request_module() could fail. If your driver is being loaded from
a work queue, if the modprobe binary is not where it should be, if
there's a bug in modprobe, etc etc etc.

> >
> > I'm also not 100% sure at what point of driver loading request_module()
> > returns. I think it's when the module init hook returns, which should be
> > all right, but again, I don't think you can count on that if it isn't
> > your request_module() that actually probes i915.

You can, except if the pci probe is async... In this case modprobe would
return before the rest of the initialization. I'm not totally sure about
this without giving it a deeper look. But then, moving the call anywhere
in that function should produce the same results with enough tries.

> >
> > I think the patch at hand is a hack that reduces the window for the
> > race, and not a real fix. Moreover, it makes the i915 audio component
> > code fragile by introducing tricky probe order dependencies that we've
> > been systematically trying to reduce by placing the call where it is
> > now.
> >
> > Cc: Lucas for any further input on module probing.
> 
> Apparently there was also a bug in some version of kmod/modprobe which
> could have lead to what you're experiencing. Are you running the fixed
> version? See [1].

Yep, this might explain as well. But only if i915 is builtin rather than
a module. Is this the case here?

Lucas De Marchi

> 
> BR,
> Jani.
> 
> 
> [1] https://github.com/lucasdemarchi/kmod/commit/fd44a98ae2eb5eb32161088954ab21e58e19dfc4
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx