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