On 19/01/2024 09:59, James Clark wrote:
On 08/01/2024 14:42, Suzuki K Poulose wrote:
Hi James
+Cc: Tao Zhang quic_taozha@quicinc.com +Cc: Mao Jinlong quic_jinlmao@quicinc.com
On 12/12/2023 15:54, James Clark wrote:
'enable', which probably should have been 'enabled', is only ever read in the core code in relation to controlling sources, and specifically only sources in sysfs mode. Confusingly it's not labelled as such and relying on it can be a source of bugs like the one fixed by commit 078dbba3f0c9 ("coresight: Fix crash when Perf and sysfs modes are used concurrently").
Most importantly, it can only be used when the coresight_mutex is held which is only done when enabling and disabling paths in sysfs mode, and not Perf mode.
I think we may be able to relax this a bit for the syfs. The sole reason for holding the mutex is for the "build_path" (and get_enabled_sink) where we need to make sure that no devices are removed/added. We hold necessary refcount on the device and the module (via coresight_grab_device()). After which, we should be able to release the mutex and perform the rest without it in coresight_enable()
After looking at the per-sink trace ID maps a bit more, I'm not sure if it will be worth the mental effort and risk to relax the sysfs locking right now.
We also currently have other things like writing to the global tracer_path which are outside of build_path/get_enabled_sink. But for the per-sink maps change we'll also have some tracking for sysfs mode about which sink map was used for each source and sink. And this state will be accessed across multiple sources, and after building the path, so it makes sense to leave the locking as-is for now IMO.
I also can't see a realistic gain from doing it, most sysfs use cases would be done from a single threaded script. Maybe in the future we could do the change to move the per-device locks into struct coresight_device, and then the core code can use them for things that need to be locked, but don't need the full coresight_mutex. And then that would also work for the per-sink case. But at the moment each device has its own lock so that's difficult.
Ok, we could come back to this after the per-sink trace id pool work. My observation was about the inconsistency between the perf vs sysfs mode as you mentioned in the above code.
Suzuki