Re: [PATCH 06/14] ASoC: SOF: add a power status IPC

[Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>]

Hi Mark,

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.

> >  #include "ops.h"
> > @@ -617,6 +618,9 @@ int snd_sof_run_firmware(struct snd_sof_dev *sdev)
> >  	/* fw boot is complete. Update the active cores mask */
> >  	sdev->enabled_cores_mask = init_core_mask;
> >  
> > +	/* increment reset count */
> > +	atomic_add(1, &sdev->reset_count);
> > +
> 
> We at no point seem to read from this reset counter?  I can't figure out
> from this commit what it's doing.

It's used in vhost-be.c (patch 10/14).

Thanks
Guennadi