Hi,
On Tue, 23 Sept 2025 at 02:49, Jie Gan <jie.gan(a)oss.qualcomm.com> wrote:
>
>
>
> On 9/23/2025 1:31 AM, Carl Worth wrote:
> > Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> >> From: Carl Worth <carl(a)os.amperecomputing.com>
> >>
> >> The handle is essential for retrieving the AUX_EVENT of each CPU and is
> >> required in perf mode. It has been added to the coresight_path so that
> >> dependent devices can access it from the path when needed.
> >
> > I'd still like to have the original command I used to trigger the bug in
> > the commit message. I really like having reproduction steps captured in
> > commit messages when I look back at commits in the future. So, that was:
> >
> > perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
> >
>
> Sure, I’ll include your commit message in the formal patch series, I
> think it's V3 since you have submitted two versions, if you're okay with
> me sending it out.
>
> >> /**
> >> * struct coresight_path - data needed by enable/disable path
> >> - * @path_list: path from source to sink.
> >> - * @trace_id: trace_id of the whole path.
> >> + * @path_list: path from source to sink.
> >> + * @trace_id: trace_id of the whole path.
> >> + * struct perf_output_handle: handle of the aux_event.
> >> */
> >
> > Fixing to "@handle" was mentioned in another comment already.
> >
> > Something about the above still feels a little off to me. It feels like
> > we're throwing new data into a structure just because it happens to be
> > conveniently at hand for the code paths we're needing, and not because
> > it really _belongs_ there.
> >
>
This data is perf specific - not path generic; so I agree that this
structure should go elsewhere.
I would suggest in the csdev (coresight_device) structure itself. We
already have some sink specific data in here e.g. perf_sink_id_map.
This could then be set/clear in the functions coresight-etm-perf.c
file, where there is a significant amount of code dealing with the
perf handle and ensuring it is valid and in scope.
This can then be set only when appropriate - for source / sink devices
and only when in perf mode, and avoid the need to pass the handle
around as API call parameters.
Regards
Mike.
> The core idea behind coresight_path is that it can hold all the data
> potentially needed by any device along the path.
>
> For example with the path ETM->Link->ETR->CATU:
>
> All the mentioned devices operate by forming a path, for which the
> system constructs a coresight_path. This 'path' is then passed to each
> device along the route, allowing any device to directly access the
> required data from coresight_path instead of receiving it as a separate
> argument.
>
> Imagine a device that requires more than two or three arguments, and you
> want to pass them through coresight_enable_path or similar functions...
>
> For certain coresight_path instances, we may not need the handle or
> other parameters. Since these values are initialized, it's acceptable to
> leave them as NULL or 0.
>
>
> > Or, maybe it's the right place for it, and the cause of my concern is
> > that "path" is an overly-narrow name in struct coresight_path?
> >
>
> It defines the direction of data flow—serving as the path for trace data.
>
> Thanks,
> Jie
>
> > But if a renaming of this structure would improve the code, I'd also be
> > fine with that happening in a subsequent commit, so I won't try to hold
> > up the current series based on that.
> >
> > -Carl
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
On Thu, Sep 25, 2025 at 11:03:42AM -0400, Sean Anderson wrote:
> Panics can occur at any time, so taking locks may cause a deadlock (such
> as if the panicking CPU held the lock). coresight_panic_cb uses
> bus_for_each_dev, but that calls bus_to_subsys which takes
> bus_kset->list_lock.
>
> Instead of registering a single panic notifier and iterating over
> coresight devices, register a panic notifier for each coresight device
> that requires it (letting the atomic notifier list handle iteration).
> atomic_notifier_chain_unregister will just return -ENOENT if a notifier
> block isn't on the list, so it's safe to call when we haven't registered
> a notifier.
>
> Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> Signed-off-by: Sean Anderson <sean.anderson(a)linux.dev>
Reviewed-by: Leo Yan <leo.yan(a)arm.com>
Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> On 9/23/2025 1:31 AM, Carl Worth wrote:
>> I'd still like to have the original command I used to trigger the bug in
>> the commit message. I really like having reproduction steps captured in
>> commit messages when I look back at commits in the future. So, that was:
>>
>> perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
>
> Sure, I’ll include your commit message in the formal patch series, I
> think it's V3 since you have submitted two versions, if you're okay with
> me sending it out.
Yes. Please do. And thank you.
> The core idea behind coresight_path is that it can hold all the data
> potentially needed by any device along the path.
>
> For example with the path ETM->Link->ETR->CATU:
OK. That makes sense to me. The name of coresight_path definitely threw
me off, since I interpreted it as being a description of the path, not a
container for data to be consumed along the path.
-Carl
On 24/09/2025 12:06, Greg KH wrote:
> On Wed, Sep 24, 2025 at 09:22:42AM +0100, Suzuki K Poulose wrote:
>> Hi Greg
>>
>> Please find the updated pull request to fix the invalid commit reference in
>> the fixes tag, in the original pull request [0]
>>
>> Apologies for the inconvenience.
>>
>> [0] https://lkml.kernel.org/r/20250922125907.2268152-1-suzuki.poulose@arm.com
>>
>> Kindly pull,
>>
>> Suzuki
>>
>> ---
>>
>> The following changes since commit 1b237f190eb3d36f52dffe07a40b5eb210280e00:
>>
>> Linux 6.17-rc3 (2025-08-24 12:04:12 -0400)
>>
>> are available in the Git repository at:
>>
>> git//git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-next-v6.18-v2
>
> That is not a valid "url" to pull from, how did you create this?
Dang! I did use the git request-pull, but have to manually replace the
"ssh" URL with git:// :-( and I messed up.
>
> I've added the missing ':', but you might want to check your scripts to
> verify you aren't doing something odd.
Will take care of it, thanks.
Suzuki
Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> Update the sink_enable functions to accept coresight_path instead of
> a generic void *data, as coresight_path encapsulates all the necessary
> data required by devices along the path.
>
> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
Tested-by: Carl Worth <carl(a)os.amperecomputing.com>
Reviewed-by: Carl Worth <carl(a)os.amperecomputing.com>
-Carl
Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> Update the helper_enable and helper_disable functions to accept
> coresight_path instead of a generic void *data, as coresight_path
> encapsulates all the necessary data required by devices along the path.
>
> Signed-off-by: Jie Gan <jie.gan(a)oss.qualcomm.com>
Tested-by: Carl Worth <carl(a)os.amperecomputing.com>
Reviewed-by: Carl Worth <carl(a)os.amperecomputing.com>
-Carl
Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> From: Carl Worth <carl(a)os.amperecomputing.com>
>
> The handle is essential for retrieving the AUX_EVENT of each CPU and is
> required in perf mode. It has been added to the coresight_path so that
> dependent devices can access it from the path when needed.
I'd still like to have the original command I used to trigger the bug in
the commit message. I really like having reproduction steps captured in
commit messages when I look back at commits in the future. So, that was:
perf record -e cs_etm//k -C 0-9 dd if=/dev/zero of=/dev/null
> /**
> * struct coresight_path - data needed by enable/disable path
> - * @path_list: path from source to sink.
> - * @trace_id: trace_id of the whole path.
> + * @path_list: path from source to sink.
> + * @trace_id: trace_id of the whole path.
> + * struct perf_output_handle: handle of the aux_event.
> */
Fixing to "@handle" was mentioned in another comment already.
Something about the above still feels a little off to me. It feels like
we're throwing new data into a structure just because it happens to be
conveniently at hand for the code paths we're needing, and not because
it really _belongs_ there.
Or, maybe it's the right place for it, and the cause of my concern is
that "path" is an overly-narrow name in struct coresight_path?
But if a renaming of this structure would improve the code, I'd also be
fine with that happening in a subsequent commit, so I won't try to hold
up the current series based on that.
-Carl
Jie Gan <jie.gan(a)oss.qualcomm.com> writes:
> I think it's better to explain my ideas with codes, so I directly created the
> patch series for sharing my solution. Please let me know if it's
> offend you.
Thanks, Jie! I'm not offended at all. My primary goal is the improvement
of our shared code base, and this is helpful for that. And I agree with
you that code can bring a lot of clarity to the discussion.
I've tested the series and it works and fixes the bug. I'll comment
specifically on each patch separately.
-Carl