[alsa-devel] [PATCH] ALSA: hda - Don't power up when not powered down.

[Dylan Reid]

On Thu, Jun 21, 2012 at 10:59 PM, Takashi Iwai <tiwai at suse.de> wrote:
> At Thu, 21 Jun 2012 21:51:22 -0700,
> Dylan Reid wrote:
>>
>> After cancel_delayed_work_sync returns, the power down work either never
>> started (power_on == 1) or finished (power_on == 0).  In the former case
>> there is no need to power up again.
>>
>> Signed-off-by: Dylan Reid <dgreid at chromium.org>
>
> There is still a small window between cancel_delayed_work_sync() and
> the spinlock, so the power down might happen between them, in theory.
> (And due to the obvious reason, cancel_delayed_work_sync() can't be in
>  spinlock :)

I'm probably missing something here, but I think three scenarios could
happen during that window:

1) snd_hda_suspend or snd_hda_codec_reset, which will leave power_on =
0, and when power_up gets the spin lock it will turn back on.  I don't
think that's a problem, power_count is still > 0 so it should be on.

2) snd_hda_power_down is called and power_count goes to zero while
power_on == 0.  Might be a problem, this would not schedule new power
down work.
  Is this expected?  One thread calling power_up moving the count from
0 to 1 and another thread (that must not have called power_up) calling
power_down?
  This hole could be plugged by checking if power_count is still >0
after getting the spin lock. does that sound right?

3)  snd_hda_power_down is called and schedules new work, I think this
is OK because when hda_power_work runs it will check power_count and
do the right thing.


Thanks for the quick response,

Dylan
>
>
> thanks,
>
> Takashi
>
>> ---
>>  sound/pci/hda/hda_codec.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>> index 604699c..045b5e7 100644
>> --- a/sound/pci/hda/hda_codec.c
>> +++ b/sound/pci/hda/hda_codec.c
>> @@ -4444,6 +4444,13 @@ static void __snd_hda_power_up(struct hda_codec *codec, bool wait_power_down)
>>       cancel_delayed_work_sync(&codec->power_work);
>>
>>       spin_lock(&codec->power_lock);
>> +     /* If the power down delayed work was cancelled above before starting,
>> +      * then there is no need to go through power up here.
>> +      */
>> +     if (codec->power_on) {
>> +             spin_unlock(&codec->power_lock);
>> +             return;
>> +     }
>>       trace_hda_power_up(codec);
>>       snd_hda_update_power_acct(codec);
>>       codec->power_on = 1;
>> --
>> 1.7.9.rc0
>>