[alsa-devel] [PATCH RFC/untested] hda-intel: suspend HDA codecs and the PCI device in error cases

[Guennadi Liakhovetski]

On Fri, 8 Feb 2019, Takashi Iwai wrote:

> On Fri, 08 Feb 2019 12:33:14 +0100,
> Guennadi Liakhovetski wrote:
> > 
> > Hi Takashi,
> > 
> > On Fri, 8 Feb 2019, Takashi Iwai wrote:
> > 
> > > On Fri, 08 Feb 2019 11:50:25 +0100,
> > > Guennadi Liakhovetski wrote:
> > > > 
> > > > From: Guennadi Liakhovetski <guennadi.liakhovetski at linux.intel.com>
> > > > 
> > > > If HDA probing on Intel platforms fails, all the devices, that have
> > > > previously been powered up, have to be suspended again.
> > > > 
> > > > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski at linux.intel.com>
> > > > ---
> > > > 
> > > > I came across HDA not suspending when no codec driver has successfully 
> > > > probed while working on SOF. Then I decided to look how this is handled in 
> > > > the mainline Intel HDA code and I think something like this patch is 
> > > > needed, however, I don't have a set up to test it. I haven't looked at the 
> > > > tegra HDA implementation, possibly they need something similar. Please, 
> > > > comment.
> > > 
> > > This will likely lead to the runtime PM refcount unbalance at
> > > azx_free(), I'm afraid.  chip->running is used as a flag to judge
> > > whether the runtime PM is applied or not, and if you jump to the error
> > > path, the flag won't be set.
> > 
> > Ok, understand, indeed, that would be a problem now. But isn't that also a 
> > problem, if azx_probe_continue() doesn't run at all before the azx_free()? 
> 
> In that case, it won't go suspend, so it won't need resume at that
> point.

Mmh, ok, then we have two different things there - see below.

> > Wouldn't this fix it?
> 
> Not really.  The chip->running is also checked in azx_runtime_idle().
> 
> Also, what you're trying to change is the behavior of HD-audio
> controller, not about codecs.

When talking about codecs I meant the path

azx_probe_continue() -> set_default_power_save() -> 
snd_hda_set_power_save() -> codec_set_power_save() -> 
pm_runtime_suspended()

and those codecs are first runtime-enabled via

azx_probe_continue() -> azx_probe_codecs() ->
snd_hda_codec_new() -> snd_hda_codec_device_init() ->
snd_hdac_device_init() -> pm_runtime_get_noresume()

So for symmetry they should also be runtime-disabled if probing doesn't 
complete successfully, right? And currently this isn't the case, so, there 
is a problem?

As for the controller, alright, I guess, we shouldn't touch its runtime PM 
if probe_continue() didn't succeed. So we only have to change the codec 
runtime PM status in case of a failure or have I misunderstood something 
about them and also their status is balanced in such cases?

Thanks
Guennadi

> thanks,
> 
> Takashi
> 
> > @@ -1300,7 +1300,7 @@ static int azx_free(struct azx *chip)
> >  	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> >  	struct hdac_bus *bus = azx_bus(chip);
> >  
> > -	if (azx_has_pm_runtime(chip) && chip->running)
> > +	if (azx_has_pm_runtime(chip))
> >  		pm_runtime_get_noresume(&pci->dev);
> >  	chip->running = 0;
> >  
> > 
> > Thanks
> > Guennadi
> > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > 
> > > >  sound/pci/hda/hda_intel.c | 9 +++++----
> > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > > > index e784130..d3caa37 100644
> > > > --- a/sound/pci/hda/hda_intel.c
> > > > +++ b/sound/pci/hda/hda_intel.c
> > > > @@ -2229,14 +2229,14 @@ static int azx_probe_continue(struct azx *chip)
> > > >  	/* create codec instances */
> > > >  	err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]);
> > > >  	if (err < 0)
> > > > -		goto out_free;
> > > > +		goto out_power_down;
> > > >  
> > > >  #ifdef CONFIG_SND_HDA_PATCH_LOADER
> > > >  	if (chip->fw) {
> > > >  		err = snd_hda_load_patch(&chip->bus, chip->fw->size,
> > > >  					 chip->fw->data);
> > > >  		if (err < 0)
> > > > -			goto out_free;
> > > > +			goto out_power_down;
> > > >  #ifndef CONFIG_PM
> > > >  		release_firmware(chip->fw); /* no longer needed */
> > > >  		chip->fw = NULL;
> > > > @@ -2246,12 +2246,12 @@ static int azx_probe_continue(struct azx *chip)
> > > >  	if ((probe_only[dev] & 1) == 0) {
> > > >  		err = azx_codec_configure(chip);
> > > >  		if (err < 0)
> > > > -			goto out_free;
> > > > +			goto out_power_down;
> > > >  	}
> > > >  
> > > >  	err = snd_card_register(chip->card);
> > > >  	if (err < 0)
> > > > -		goto out_free;
> > > > +		goto out_power_down;
> > > >  
> > > >  	setup_vga_switcheroo_runtime_pm(chip);
> > > >  
> > > > @@ -2260,6 +2260,7 @@ static int azx_probe_continue(struct azx *chip)
> > > >  
> > > >  	set_default_power_save(chip);
> > > >  
> > > > +out_power_down:
> > > >  	if (azx_has_pm_runtime(chip))
> > > >  		pm_runtime_put_autosuspend(&pci->dev);
> > > >  
> > > > -- 
> > > > 1.9.3
> > > > 
> > > > 
> > > 
> > 
>