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
