Re: [Sound-open-firmware] [PATCH 06/14] ASoC: SOF: add a power status IPC
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
On Fri, Mar 20, 2020 at 02:27:32PM +0100, Guennadi Liakhovetski wrote:
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?
Actually I'd even say this isn't a problem. I think it's safe to say, you won't suspend and resume your audio interface more often than every 10 seconds. That makes under 3200000 cycles per year. Even with 31 bits for a signed integer that makes more than 600 years. I think we're safe. Thanks Guennadi
participants (1)
-
Guennadi Liakhovetski