Re: [PATCH 5/8] coresight: Remove the 'enable' field.

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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