Re: [Intel-gfx] [PATCH v2] ALSA: hda/i915 - avoid hung task timeout in i915 wait

[Takashi Iwai <tiwai@suse.de>]

On Wed, 09 Mar 2022 09:36:54 +0100,
Tvrtko Ursulin wrote:
> 
> 
> On 08/03/2022 17:27, Kai Vehmanen wrote:
> > If kernel is built with hung task detection enabled and
> > CONFIG_DEFAULT_HUNG_TASK_TIMEOUT set to less than 60 seconds,
> > snd_hdac_i915_init() will trigger the hung task timeout in case i915 is
> > not available and taint the kernel.
> >
> > Split the 60sec wait into a loop of smaller waits to avoid this.
> >
> > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > Co-developed-by: Ramalingam C <ramalingam.c@intel.com>
> > Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> > ---
> >   sound/hda/hdac_i915.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > Changes V1->V2:
> >   - address local variable naming issue raised by Amadeusz
> >     and use Takashi's proposal
> >
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index 454474ac5716..aa48bed7baf7 100644
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -143,7 +143,7 @@ static bool i915_gfx_present(void)
> >   int snd_hdac_i915_init(struct hdac_bus *bus)
> >   {
> >   	struct drm_audio_component *acomp;
> > -	int err;
> > +	int err, i;
> >     	if (!i915_gfx_present())
> >   		return -ENODEV;
> > @@ -159,9 +159,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
> >   	if (!acomp->ops) {
> >   		if (!IS_ENABLED(CONFIG_MODULES) ||
> >   		    !request_module("i915")) {
> > -			/* 60s timeout */
> 
> Where does this 60s come from and why is the fix to work around
> DEFAULT_HUNG_TASK_TIMEOUT in a hacky way deemed okay?

The 60s timeout comes from the fact that the binding with i915 *might*
be mandatory for HD-audio driver on some platforms, while the binding
couldn't be achieved depending on the dynamic configuration change or
any other reasons, so we don't want to block forver.  And, basically
the hung check is false-positive, and if there is a better way to mark
to skip the hung check, we'd take it. But currently this seems to be
the easiest workaround for avoiding the false-positive checks.

> For instance
> would limiting the wait here to whatever the kconfig is set to be an
> option?

No, the hunk task timeout can be changed dynamically via procfs, hence
the fixed Kconfig won't help at all.


Takashi