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