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

[Suzuki K Poulose]

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@xxxxxxxxxxx>
> > +Cc: Mao Jinlong <quic_jinlmao@xxxxxxxxxxx>
> >
> > 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