Re: Adding movable PCIe BARs support in snd_hda_intel

[Takashi Iwai <tiwai@suse.de>]

On Thu, 25 Mar 2021 17:13:41 +0100,
Andrey Grodzovsky wrote:
> 
> 
> 
> On 2021-03-25 2:58 a.m., Takashi Iwai wrote:
> > On Wed, 24 Mar 2021 22:43:24 +0100,
> > Andrey Grodzovsky wrote:
> >>
> >> Few comments -
> >>
> >> 1) Why we don't use snd_power_wait_and_ref in patch 3 in the common
> >> handler ?
> >> Don't we want the PCI rescan sequence to 'wait for' any in flight
> >> taks that might be accessing registers and not only read/write/tlv
> >> accesses ?
> >
> > Right, we don't need to block all ioctls, but only the ones that may
> > access the hardware.  So basically the patches 3-5 can be dropped if
> > we take the patch 6.  The current patch was written on top of the
> > previous series, that's why it has both.
> >
> >> 2) Possible deadlock -
> >> In azx_rescan_prepare - you put the card into SNDRV_CTL_POWER_D3hot
> >> first and then 'wait for' all in flight tasks with the refcount.
> >> The in flight tasks on the other hand, using snd_power_wait_and_ref,
> >> may have already bumped up the refcount and now 'wait for' the card
> >> to go into SNDRV_CTL_POWER_D0 which can't happen since PCI rescan
> >> waits for the refocunt to drop to 0 before proceeding.
> >>
> >> Instead of snd_power_wait_and_ref can't we just call snd_power_ref
> >> in common IOCTL before checking for power_state != SNDRV_CTL_POWER_D0 ?
> >> Or is it because you don't want to fail IOCTLs ?
> >
> > No, this is the intended behavior and should work as-is because
> > snd_power_wait_and_ref() drops the refcount in the loop before
> > sleeping.  The inc before the state check is a must for covering the
> > possible race, and ditto for changing the power_state to D3hot before
> > syncing.
> 
> Ohh, missed the refcount dec in the wait loop... My bad.
> 
> On boot the latest patch-set was throwing refcount warnings since
> u are not supposed to inc from zerro count and so I fixed
> with the attached patch. That way seems to work fine.

Ah, I completely forgot about the oddness of refcount_t.
Then I guess it's simpler to replace with atomic_*() instead.


thanks,

Takashi