On Thu, Dec 02, 2021 at 09:53:33AM +0000, Charles Keepax wrote: > On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote: > > ...that's based on the assumption that this is all about the magic write > > sequences you're using for shutdown potentially conflicting with default > > values you've got in the cache? If it's not those then the assumption > > is that either the device was reset in which case it has reset values or > > the device was not reset and held it's previous value, in which case the > > cache sync is redundant. > This isn't quite accurate though, as whilst the device was > suspended the user may have touched ALSA controls which will have > updated the state of the cache. Thus the cache requires a sync > regardless of if the hardware turned off. Right, an actual description of an actual issue. Though how would they have touched the ALSA controls during system suspend? Userspace should halted before we start suspending devices so there shouldn't be anything new coming in from the application layer while the device is in cache only mode. The sound card as a whole should've been suspended first so nothing should be coming in from there either. > I suspect we do need to have a think about the write sequences, > but there is also a more general issue here. The sequence looks > like this: > 1) Device enters cache only mode. > 2) User writes an ALSA control causing a register to return to > its default value. > 3) Device exits cache only and does a cache sync. This is a thing that happens for runtime suspend but for runtime suspend we have good tracking of if the device lost power so we shouldn't just be marking the cache as dirty unconditionally. For system suspend there shouldn't be a need to worry about userspace changing anything, and anything coming in from the rest of the kernel should be very limited. In any case this isn't something that should be hacked around in an individual driver, like I say whatever the driver is trying to do needs to be written in a clear and obvious fashion.
Attachment:
signature.asc
Description: PGP signature
