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

[Takashi Iwai]

On Fri, 08 Feb 2019 13:14:06 +0100,
Guennadi Liakhovetski wrote:
> 
> 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?

It's disabled in the counterpart, snd_hdac_device_exit().

> 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?

Actually, I don't care much about the power consumption in the driver
failure path as long as it's balanced.  Ideally we should turn off
everything and sets to the minimum, but this is already an abnormal
state, so you can't expect everything perfect in that state.

If there is a scenario that brings the improper PM refcount, we must
fix it, yes.  Or, we can simplify the code a lot, I'd love to apply,
too.  But for other optimization, it's in a much lower priority,
honestly speaking...


thanks,

Takashi


> 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
> > > > > 
> > > > > 
> > > > 
> > > 
> > 
>