Re: [PATCH 1/3] ALSA: hda - Handle error from snd_hda_power_up*()

[Takashi Iwai <tiwai@suse.de>]

On Wed, 27 Jun 2018 11:36:12 +0200,
Chris Wilson wrote:
> 
> Quoting Takashi Iwai (2018-06-27 10:10:32)
> > Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we
> > haven't dealt with the error properly in many places.  It's an unusual
> > situation but still possible.
> > 
> > This patch spots these places and adds the proper error paths.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > @@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol,
> >                 int dir = get_amp_direction(kcontrol);
> >                 unsigned long pval;
> >  
> > -               snd_hda_power_up(codec);
> > +               err = snd_hda_power_up(codec);
> > +               if (err < 0)
> > +                       return err;
> >                 mutex_lock(&codec->control_mutex);
> >                 pval = kcontrol->private_value;
> >                 kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,
> 
> Should this be deferred until pm is next acquired?
> Or are we regarding pm can only fail nonrecoverably?

The power up path is already pm_runtime_get_sync(), so it's a fatal
error.  So I suppose that the failure must be very exceptional, and no
need to complicate things too much.

> 
> > @@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
> >         long *valp = ucontrol->value.integer.value;
> >         hda_nid_t vnid = 0;
> >         int changed = 1;
> > +       int err;
> >  
> >         switch (nid) {
> >         case 0x02:
> > @@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol,
> >                 valp++;
> >         }
> >  
> > -       snd_hda_power_up(codec);
> > +       err = snd_hda_power_up(codec);
> > +       if (err < 0)
> > +               return err;
> >         ca0132_alt_dsp_volume_put(codec, vnid);
> >         mutex_lock(&codec->control_mutex);
> >         changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);
> 
> > diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> > index 5ad6c7e5f92e..b0d757cf7aab 100644
> > --- a/sound/pci/hda/patch_realtek.c
> > +++ b/sound/pci/hda/patch_realtek.c
> > @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled)
> >         pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80;
> >         if (spec->mute_led_nid) {
> >                 /* temporarily power up/down for setting VREF */
> > -               snd_hda_power_up_pm(codec);
> > +               if (snd_hda_power_up_pm(codec) < 0)
> > +                       return;
> >                 snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval);
> >                 snd_hda_power_down_pm(codec);
> >         }
> 
> Similar questions as for deferred application.
> 
> I did not find any imbalance introduced and the error is propagated or
> handled,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks!

Takashi