[Sound-open-firmware] [PATCH 06/14] ASoC: SOF: add a power status IPC

[Guennadi Liakhovetski]

On Fri, Mar 20, 2020 at 12:19:52PM +0000, Mark Brown wrote:
> On Fri, Mar 20, 2020 at 12:52:03PM +0100, Guennadi Liakhovetski wrote:
> > On Fri, Mar 13, 2020 at 02:39:56PM +0000, Mark Brown wrote:
> > > On Thu, Mar 12, 2020 at 03:44:21PM +0100, Guennadi Liakhovetski wrote:
> 
> > > >  #endif
> > > > +	atomic_set(&sdev->reset_count, 0);
> > > >  	dev_set_drvdata(dev, sdev);
> 
> > > Do we really need to use atomics for this?  They are hard to use
> > > correctly.
> 
> > This variable is accessed from 2 contexts: it's incremented by the SOF 
> > driver, when the firmware has booted and it's read by the SOF
> > VirtIO backend vhost-be.c when receiving a resume request from the guest. 
> > Timewise the variable will only be incremented during the DSP resume / 
> > power up, while the VirtIO back end is waiting for the resume to complete in 
> > pm_runtime_get_sync(). And only after that it reads the variable. But that 
> > can happen on different CPUs. Whereas I think that runtime PM will sync 
> > caches somewhere during the process, I think it is better to access the 
> > variable in an SMP-safe way, e.g. using atomic operations.
> 
> That doesn't address my concern - to repeat, my concern is that atomics
> are hard to use correctly.  Is there no other concurrency primitive (for
> example this sounds like a completion) which can be used?

No, this isn't a completion - it's a counter. I've used atomic variables 
before, I cannot remember seeing any difficulties with their correct use 
described. Do you have a pointer?

Thinking about it, one problem I see is wrapping, it isn't currently 
handled, but that would happen after quite a few PM suspend / resume 
cycles... Still it can and should be fixed. But this isn't the concern, 
that you have?

Thanks
Guennadi