Re: [PATCH 1/8] coresight: Fix issue where a source device's helpers aren't disabled

[Suzuki K Poulose]

On 13/12/2023 13:54, James Clark wrote:
> On 12/12/2023 17:44, Suzuki K Poulose wrote:
> > Hi James
> >
> > On 12/12/2023 15:53, James Clark wrote:
> > > The linked commit reverts the change that accidentally used some sysfs
> > > enable/disable functions from Perf which broke the refcounting, but it
> > > also removes the fact that the sysfs disable function disabled the
> > > helpers.
> >
> >
> > > Add a new wrapper function that does both which is used by both Perf and
> > > sysfs, and label the sysfs disable function appropriately. The naming of
> > > all of the functions will be tidied up later to avoid this happening
> > > again.
> > >
> > > Fixes: 287e82cf69aa ("coresight: Fix crash when Perf and sysfs modes
> > > are used concurrently")
> >
> > But we still don't "enable" the helpers from perf mode with this patch.
> > i.e., we use source_ops()->enable directly. So, I guess this patch
> > doesn't fix a bug as such. But that said, it would be good to
> > enable/disable helpers for sources, in perf mode.
> >
> > Suzuki
>
> We do, it happens in coresight_enable_path() which Perf uses. I added
> the comment below about that.

Ah, I see. That indeed is a bit confusing. And I think all users of 
coresight_enable_path() enables the source right after. So, I don't
see any point in having it separate. That said, this fix makes sense
and apologies for the confusion. We could may be cleanup the 
enable_path() to enable the source too, in a separate patch.

Suzuki


>   [...]
>
> > >    +/*
> > > + * Helper function to call source_ops(csdev)->disable and also
> > > disable the
> > > + * helpers.
> > > + *
> > > + * There is an imbalance between coresight_enable_path() and
> > > + * coresight_disable_path(). Enabling also enables the source's
> > > helpers as part
> > > + * of the path, but disabling always skips the first item in the path
> > > (which is
> > > + * the source), so sources and their helpers don't get disabled as
> > > part of that
> > > + * function and we need the extra step here.
> > > + */
>
> The reason coresight_disable_path() skips the first item is because it's
> used after errors where a path is only partially enabled and it unwinds,
> skipping the last item, because the last item didn't enable.
>
> It seemed a bit more intrusive to change that, and it's already working.