* [PATCH v3] coresight: fix missing error code when trace ID is invalid
@ 2026-05-11 8:36 Jie Gan
2026-05-11 9:19 ` Richard Cheng
0 siblings, 1 reply; 6+ messages in thread
From: Jie Gan @ 2026-05-11 8:36 UTC (permalink / raw)
To: Suzuki K Poulose, Mike Leach, James Clark, Leo Yan,
Alexander Shishkin, Tingwei Zhang
Cc: coresight, linux-arm-kernel, linux-kernel, Jie Gan
When coresight_path_assign_trace_id() cannot assign a valid trace ID,
coresight_enable_sysfs() takes the err_path goto with ret still 0,
returning success to the caller despite no trace session being started.
Fix this by changing coresight_path_assign_trace_id() to return int.
Move the IS_VALID_CS_TRACE_ID() check inside the function so it returns
-EINVAL on failure and 0 on success. Update coresight_enable_sysfs() to
check the return value directly instead of inspecting path->trace_id
after the call.
The other caller in coresight-etm-perf.c discards the return value and
continues to check path->trace_id via IS_VALID_CS_TRACE_ID() directly.
This is unaffected: on failure path->trace_id is no longer written, so
it remains 0, which IS_VALID_CS_TRACE_ID() rejects the same as before.
Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path")
Reviewed-by: James Clark <james.clark@linaro.org>
Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com>
---
Changes in v3:
- directly return the value for clear expression.
- Link to v2: https://lore.kernel.org/r/20260509-fix-trace-id-error-v2-1-c900bcbab3e9@oss.qualcomm.com
Changes in v2:
- Refactor the coresight_path_assign_trace_id function.
- Link to v1: https://lore.kernel.org/r/20260508-fix-trace-id-error-v1-1-5f11a5456fdf@oss.qualcomm.com
---
drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++++----
drivers/hwtracing/coresight/coresight-priv.h | 2 +-
drivers/hwtracing/coresight/coresight-sysfs.c | 4 ++--
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 46f247f73cf6..254db91a8ac9 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -739,8 +739,8 @@ static int coresight_get_trace_id(struct coresight_device *csdev,
* Call this after creating the path and before enabling it. This leaves
* the trace ID set on the path, or it remains 0 if it couldn't be assigned.
*/
-void coresight_path_assign_trace_id(struct coresight_path *path,
- enum cs_mode mode)
+int coresight_path_assign_trace_id(struct coresight_path *path,
+ enum cs_mode mode)
{
struct coresight_device *sink = coresight_get_sink(path);
struct coresight_node *nd;
@@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path,
* Non 0 is either success or fail.
*/
if (trace_id != 0) {
- path->trace_id = trace_id;
- return;
+ if (IS_VALID_CS_TRACE_ID(trace_id)) {
+ path->trace_id = trace_id;
+ return 0;
+ }
+
+ return -EINVAL;
}
}
+
+ return -EINVAL;
}
/**
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index 1ea882dffd70..34c7e792adbd 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -153,7 +153,7 @@ int coresight_make_links(struct coresight_device *orig,
void coresight_remove_links(struct coresight_device *orig,
struct coresight_connection *conn);
u32 coresight_get_sink_id(struct coresight_device *csdev);
-void coresight_path_assign_trace_id(struct coresight_path *path,
+int coresight_path_assign_trace_id(struct coresight_path *path,
enum cs_mode mode);
#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index d2a6ed8bcc74..b6a870399e83 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -211,8 +211,8 @@ int coresight_enable_sysfs(struct coresight_device *csdev)
goto out;
}
- coresight_path_assign_trace_id(path, CS_MODE_SYSFS);
- if (!IS_VALID_CS_TRACE_ID(path->trace_id))
+ ret = coresight_path_assign_trace_id(path, CS_MODE_SYSFS);
+ if (ret)
goto err_path;
ret = coresight_enable_path(path, CS_MODE_SYSFS);
---
base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90
change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1
Best regards,
--
Jie Gan <jie.gan@oss.qualcomm.com>
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid 2026-05-11 8:36 [PATCH v3] coresight: fix missing error code when trace ID is invalid Jie Gan @ 2026-05-11 9:19 ` Richard Cheng 2026-05-11 9:27 ` Jie Gan 0 siblings, 1 reply; 6+ messages in thread From: Richard Cheng @ 2026-05-11 9:19 UTC (permalink / raw) To: Jie Gan Cc: Suzuki K Poulose, Mike Leach, James Clark, Leo Yan, Alexander Shishkin, Tingwei Zhang, coresight, linux-arm-kernel, linux-kernel On Mon, May 11, 2026 at 04:36:18PM +0800, Jie Gan wrote: > When coresight_path_assign_trace_id() cannot assign a valid trace ID, > coresight_enable_sysfs() takes the err_path goto with ret still 0, > returning success to the caller despite no trace session being started. > > Fix this by changing coresight_path_assign_trace_id() to return int. > Move the IS_VALID_CS_TRACE_ID() check inside the function so it returns > -EINVAL on failure and 0 on success. Update coresight_enable_sysfs() to > check the return value directly instead of inspecting path->trace_id > after the call. > > The other caller in coresight-etm-perf.c discards the return value and > continues to check path->trace_id via IS_VALID_CS_TRACE_ID() directly. > This is unaffected: on failure path->trace_id is no longer written, so > it remains 0, which IS_VALID_CS_TRACE_ID() rejects the same as before. > > Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") > Reviewed-by: James Clark <james.clark@linaro.org> > Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> > --- > Changes in v3: > - directly return the value for clear expression. > - Link to v2: https://lore.kernel.org/r/20260509-fix-trace-id-error-v2-1-c900bcbab3e9@oss.qualcomm.com > > Changes in v2: > - Refactor the coresight_path_assign_trace_id function. > - Link to v1: https://lore.kernel.org/r/20260508-fix-trace-id-error-v1-1-5f11a5456fdf@oss.qualcomm.com > --- > drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++++---- > drivers/hwtracing/coresight/coresight-priv.h | 2 +- > drivers/hwtracing/coresight/coresight-sysfs.c | 4 ++-- > 3 files changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > index 46f247f73cf6..254db91a8ac9 100644 > --- a/drivers/hwtracing/coresight/coresight-core.c > +++ b/drivers/hwtracing/coresight/coresight-core.c > @@ -739,8 +739,8 @@ static int coresight_get_trace_id(struct coresight_device *csdev, > * Call this after creating the path and before enabling it. This leaves > * the trace ID set on the path, or it remains 0 if it couldn't be assigned. > */ > -void coresight_path_assign_trace_id(struct coresight_path *path, > - enum cs_mode mode) > +int coresight_path_assign_trace_id(struct coresight_path *path, > + enum cs_mode mode) > { > struct coresight_device *sink = coresight_get_sink(path); > struct coresight_node *nd; > @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, > * Non 0 is either success or fail. > */ > if (trace_id != 0) { > - path->trace_id = trace_id; > - return; > + if (IS_VALID_CS_TRACE_ID(trace_id)) { > + path->trace_id = trace_id; > + return 0; > + } > + > + return -EINVAL; > } > } > + > + return -EINVAL; > } > > /** > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h > index 1ea882dffd70..34c7e792adbd 100644 > --- a/drivers/hwtracing/coresight/coresight-priv.h > +++ b/drivers/hwtracing/coresight/coresight-priv.h > @@ -153,7 +153,7 @@ int coresight_make_links(struct coresight_device *orig, > void coresight_remove_links(struct coresight_device *orig, > struct coresight_connection *conn); > u32 coresight_get_sink_id(struct coresight_device *csdev); > -void coresight_path_assign_trace_id(struct coresight_path *path, > +int coresight_path_assign_trace_id(struct coresight_path *path, > enum cs_mode mode); Hi Jie, Thanks for your patch, I'm thinking will "__must_check" should be added so in the future the next caller won't accidently introduce this class of bug ? Best regards, Richard Cheng. > > #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X) > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > index d2a6ed8bcc74..b6a870399e83 100644 > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > @@ -211,8 +211,8 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > goto out; > } > > - coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > + ret = coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > + if (ret) > goto err_path; > > ret = coresight_enable_path(path, CS_MODE_SYSFS); > > --- > base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90 > change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1 > > Best regards, > -- > Jie Gan <jie.gan@oss.qualcomm.com> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid 2026-05-11 9:19 ` Richard Cheng @ 2026-05-11 9:27 ` Jie Gan 2026-05-11 10:26 ` Richard Cheng 2026-05-11 14:45 ` Leo Yan 0 siblings, 2 replies; 6+ messages in thread From: Jie Gan @ 2026-05-11 9:27 UTC (permalink / raw) To: Richard Cheng Cc: Suzuki K Poulose, Mike Leach, James Clark, Leo Yan, Alexander Shishkin, Tingwei Zhang, coresight, linux-arm-kernel, linux-kernel On 5/11/2026 5:19 PM, Richard Cheng wrote: > On Mon, May 11, 2026 at 04:36:18PM +0800, Jie Gan wrote: >> When coresight_path_assign_trace_id() cannot assign a valid trace ID, >> coresight_enable_sysfs() takes the err_path goto with ret still 0, >> returning success to the caller despite no trace session being started. >> >> Fix this by changing coresight_path_assign_trace_id() to return int. >> Move the IS_VALID_CS_TRACE_ID() check inside the function so it returns >> -EINVAL on failure and 0 on success. Update coresight_enable_sysfs() to >> check the return value directly instead of inspecting path->trace_id >> after the call. >> >> The other caller in coresight-etm-perf.c discards the return value and >> continues to check path->trace_id via IS_VALID_CS_TRACE_ID() directly. >> This is unaffected: on failure path->trace_id is no longer written, so >> it remains 0, which IS_VALID_CS_TRACE_ID() rejects the same as before. >> >> Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") >> Reviewed-by: James Clark <james.clark@linaro.org> >> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >> --- >> Changes in v3: >> - directly return the value for clear expression. >> - Link to v2: https://lore.kernel.org/r/20260509-fix-trace-id-error-v2-1-c900bcbab3e9@oss.qualcomm.com >> >> Changes in v2: >> - Refactor the coresight_path_assign_trace_id function. >> - Link to v1: https://lore.kernel.org/r/20260508-fix-trace-id-error-v1-1-5f11a5456fdf@oss.qualcomm.com >> --- >> drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++++---- >> drivers/hwtracing/coresight/coresight-priv.h | 2 +- >> drivers/hwtracing/coresight/coresight-sysfs.c | 4 ++-- >> 3 files changed, 13 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c >> index 46f247f73cf6..254db91a8ac9 100644 >> --- a/drivers/hwtracing/coresight/coresight-core.c >> +++ b/drivers/hwtracing/coresight/coresight-core.c >> @@ -739,8 +739,8 @@ static int coresight_get_trace_id(struct coresight_device *csdev, >> * Call this after creating the path and before enabling it. This leaves >> * the trace ID set on the path, or it remains 0 if it couldn't be assigned. >> */ >> -void coresight_path_assign_trace_id(struct coresight_path *path, >> - enum cs_mode mode) >> +int coresight_path_assign_trace_id(struct coresight_path *path, >> + enum cs_mode mode) >> { >> struct coresight_device *sink = coresight_get_sink(path); >> struct coresight_node *nd; >> @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, >> * Non 0 is either success or fail. >> */ >> if (trace_id != 0) { >> - path->trace_id = trace_id; >> - return; >> + if (IS_VALID_CS_TRACE_ID(trace_id)) { >> + path->trace_id = trace_id; >> + return 0; >> + } >> + >> + return -EINVAL; >> } >> } >> + >> + return -EINVAL; >> } >> >> /** >> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h >> index 1ea882dffd70..34c7e792adbd 100644 >> --- a/drivers/hwtracing/coresight/coresight-priv.h >> +++ b/drivers/hwtracing/coresight/coresight-priv.h >> @@ -153,7 +153,7 @@ int coresight_make_links(struct coresight_device *orig, >> void coresight_remove_links(struct coresight_device *orig, >> struct coresight_connection *conn); >> u32 coresight_get_sink_id(struct coresight_device *csdev); >> -void coresight_path_assign_trace_id(struct coresight_path *path, >> +int coresight_path_assign_trace_id(struct coresight_path *path, >> enum cs_mode mode); > > Hi Jie, > > Thanks for your patch, > > I'm thinking will "__must_check" should be added so in the future the next caller won't accidently > introduce this class of bug ? > Hi Richard, The return value has been ignored in perf mode. It will introduce noisy by adding __must_check. So I think its better without __must_check? Thanks, Jie > Best regards, > Richard Cheng. > >> >> #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X) >> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c >> index d2a6ed8bcc74..b6a870399e83 100644 >> --- a/drivers/hwtracing/coresight/coresight-sysfs.c >> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c >> @@ -211,8 +211,8 @@ int coresight_enable_sysfs(struct coresight_device *csdev) >> goto out; >> } >> >> - coresight_path_assign_trace_id(path, CS_MODE_SYSFS); >> - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) >> + ret = coresight_path_assign_trace_id(path, CS_MODE_SYSFS); >> + if (ret) >> goto err_path; >> >> ret = coresight_enable_path(path, CS_MODE_SYSFS); >> >> --- >> base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90 >> change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1 >> >> Best regards, >> -- >> Jie Gan <jie.gan@oss.qualcomm.com> >> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid 2026-05-11 9:27 ` Jie Gan @ 2026-05-11 10:26 ` Richard Cheng 2026-05-11 14:45 ` Leo Yan 1 sibling, 0 replies; 6+ messages in thread From: Richard Cheng @ 2026-05-11 10:26 UTC (permalink / raw) To: Jie Gan Cc: Suzuki K Poulose, Mike Leach, James Clark, Leo Yan, Alexander Shishkin, Tingwei Zhang, coresight, linux-arm-kernel, linux-kernel On Mon, May 11, 2026 at 05:27:10PM +0800, Jie Gan wrote: > > > On 5/11/2026 5:19 PM, Richard Cheng wrote: > > On Mon, May 11, 2026 at 04:36:18PM +0800, Jie Gan wrote: > > > When coresight_path_assign_trace_id() cannot assign a valid trace ID, > > > coresight_enable_sysfs() takes the err_path goto with ret still 0, > > > returning success to the caller despite no trace session being started. > > > > > > Fix this by changing coresight_path_assign_trace_id() to return int. > > > Move the IS_VALID_CS_TRACE_ID() check inside the function so it returns > > > -EINVAL on failure and 0 on success. Update coresight_enable_sysfs() to > > > check the return value directly instead of inspecting path->trace_id > > > after the call. > > > > > > The other caller in coresight-etm-perf.c discards the return value and > > > continues to check path->trace_id via IS_VALID_CS_TRACE_ID() directly. > > > This is unaffected: on failure path->trace_id is no longer written, so > > > it remains 0, which IS_VALID_CS_TRACE_ID() rejects the same as before. > > > > > > Fixes: d87d76d823d1 ("Coresight: Allocate trace ID after building the path") > > > Reviewed-by: James Clark <james.clark@linaro.org> > > > Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> > > > --- > > > Changes in v3: > > > - directly return the value for clear expression. > > > - Link to v2: https://lore.kernel.org/r/20260509-fix-trace-id-error-v2-1-c900bcbab3e9@oss.qualcomm.com > > > > > > Changes in v2: > > > - Refactor the coresight_path_assign_trace_id function. > > > - Link to v1: https://lore.kernel.org/r/20260508-fix-trace-id-error-v1-1-5f11a5456fdf@oss.qualcomm.com > > > --- > > > drivers/hwtracing/coresight/coresight-core.c | 14 ++++++++++---- > > > drivers/hwtracing/coresight/coresight-priv.h | 2 +- > > > drivers/hwtracing/coresight/coresight-sysfs.c | 4 ++-- > > > 3 files changed, 13 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c > > > index 46f247f73cf6..254db91a8ac9 100644 > > > --- a/drivers/hwtracing/coresight/coresight-core.c > > > +++ b/drivers/hwtracing/coresight/coresight-core.c > > > @@ -739,8 +739,8 @@ static int coresight_get_trace_id(struct coresight_device *csdev, > > > * Call this after creating the path and before enabling it. This leaves > > > * the trace ID set on the path, or it remains 0 if it couldn't be assigned. > > > */ > > > -void coresight_path_assign_trace_id(struct coresight_path *path, > > > - enum cs_mode mode) > > > +int coresight_path_assign_trace_id(struct coresight_path *path, > > > + enum cs_mode mode) > > > { > > > struct coresight_device *sink = coresight_get_sink(path); > > > struct coresight_node *nd; > > > @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, > > > * Non 0 is either success or fail. > > > */ > > > if (trace_id != 0) { > > > - path->trace_id = trace_id; > > > - return; > > > + if (IS_VALID_CS_TRACE_ID(trace_id)) { > > > + path->trace_id = trace_id; > > > + return 0; > > > + } > > > + > > > + return -EINVAL; > > > } > > > } > > > + > > > + return -EINVAL; > > > } > > > /** > > > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h > > > index 1ea882dffd70..34c7e792adbd 100644 > > > --- a/drivers/hwtracing/coresight/coresight-priv.h > > > +++ b/drivers/hwtracing/coresight/coresight-priv.h > > > @@ -153,7 +153,7 @@ int coresight_make_links(struct coresight_device *orig, > > > void coresight_remove_links(struct coresight_device *orig, > > > struct coresight_connection *conn); > > > u32 coresight_get_sink_id(struct coresight_device *csdev); > > > -void coresight_path_assign_trace_id(struct coresight_path *path, > > > +int coresight_path_assign_trace_id(struct coresight_path *path, > > > enum cs_mode mode); > > > > Hi Jie, > > > > Thanks for your patch, > > > > I'm thinking will "__must_check" should be added so in the future the next caller won't accidently > > introduce this class of bug ? > > > > Hi Richard, > > The return value has been ignored in perf mode. It will introduce noisy by > adding __must_check. So I think its better without __must_check? > > Thanks, > Jie > Hi Jie, Ahh thanks for the head up, my intention was adding a compile-time check there to be safe, no need for __must_check here then. If any new callers were to appear in the future we'll see how it goes. Thanks. Reviewed-by: Richard Cheng <icheng@nvidia.com> Best regards, Richard Cheng. > > Best regards, > > Richard Cheng. > > > > > #if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X) > > > diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c > > > index d2a6ed8bcc74..b6a870399e83 100644 > > > --- a/drivers/hwtracing/coresight/coresight-sysfs.c > > > +++ b/drivers/hwtracing/coresight/coresight-sysfs.c > > > @@ -211,8 +211,8 @@ int coresight_enable_sysfs(struct coresight_device *csdev) > > > goto out; > > > } > > > - coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > > > - if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > > > + ret = coresight_path_assign_trace_id(path, CS_MODE_SYSFS); > > > + if (ret) > > > goto err_path; > > > ret = coresight_enable_path(path, CS_MODE_SYSFS); > > > > > > --- > > > base-commit: 17c7841d09ee7d33557fd075562d9289b6018c90 > > > change-id: 20260508-fix-trace-id-error-dbfdd4d8f2d1 > > > > > > Best regards, > > > -- > > > Jie Gan <jie.gan@oss.qualcomm.com> > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid 2026-05-11 9:27 ` Jie Gan 2026-05-11 10:26 ` Richard Cheng @ 2026-05-11 14:45 ` Leo Yan 2026-05-12 0:54 ` Jie Gan 1 sibling, 1 reply; 6+ messages in thread From: Leo Yan @ 2026-05-11 14:45 UTC (permalink / raw) To: Jie Gan Cc: Richard Cheng, Suzuki K Poulose, Mike Leach, James Clark, Alexander Shishkin, Tingwei Zhang, coresight, linux-arm-kernel, linux-kernel On Mon, May 11, 2026 at 05:27:10PM +0800, Jie Gan wrote: [...] > > > @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, > > > * Non 0 is either success or fail. > > > */ > > > if (trace_id != 0) { > > > - path->trace_id = trace_id; > > > - return; > > > + if (IS_VALID_CS_TRACE_ID(trace_id)) { > > > + path->trace_id = trace_id; > > > + return 0; > > > + } > > > + > > > + return -EINVAL; I'd advocate a bit early exit style, like: /* 0 means the device has no ID assignment, so keep searching */ if (trace_id == 0) continue; if (!IS_VALID_CS_TRACE_ID(trace_id)) return -EINVAL; path->trace_id = trace_id; return 0; Early exit can reduce indentation depth, and it handles simple cases first and then the complex logic. In some cases (maye not this case), we may benefit a bit from compiler optimization [1]. [1] https://xania.org/202512/18-partial-inlining [...] > The return value has been ignored in perf mode. It will introduce noisy by > adding __must_check. So I think its better without __must_check? Wouldn't it need to update perf mode as well? Regarding __must_check, I searched Documentation but didn't find guidance on when it should be used. I don't want to use this annotation randomly (some functions use it and some not), this will be hard for everyone to follow up. IMO, it's fine not to use __must_check here. I would leave this to Suzuki and other maintainers if have different opinions. Thanks, Leo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] coresight: fix missing error code when trace ID is invalid 2026-05-11 14:45 ` Leo Yan @ 2026-05-12 0:54 ` Jie Gan 0 siblings, 0 replies; 6+ messages in thread From: Jie Gan @ 2026-05-12 0:54 UTC (permalink / raw) To: Leo Yan Cc: Richard Cheng, Suzuki K Poulose, Mike Leach, James Clark, Alexander Shishkin, Tingwei Zhang, coresight, linux-arm-kernel, linux-kernel On 5/11/2026 10:45 PM, Leo Yan wrote: > On Mon, May 11, 2026 at 05:27:10PM +0800, Jie Gan wrote: > > [...] > >>>> @@ -755,10 +755,16 @@ void coresight_path_assign_trace_id(struct coresight_path *path, >>>> * Non 0 is either success or fail. >>>> */ >>>> if (trace_id != 0) { >>>> - path->trace_id = trace_id; >>>> - return; >>>> + if (IS_VALID_CS_TRACE_ID(trace_id)) { >>>> + path->trace_id = trace_id; >>>> + return 0; >>>> + } >>>> + >>>> + return -EINVAL; > > I'd advocate a bit early exit style, like: > > /* 0 means the device has no ID assignment, so keep searching */ > if (trace_id == 0) > continue; > > if (!IS_VALID_CS_TRACE_ID(trace_id)) > return -EINVAL; > > path->trace_id = trace_id; > return 0; > > Early exit can reduce indentation depth, and it handles simple cases > first and then the complex logic. In some cases (maye not this case), > we may benefit a bit from compiler optimization [1]. > Thanks, that's a good suggestion and much simpler than my solution. > [1] https://xania.org/202512/18-partial-inlining > > [...] > >> The return value has been ignored in perf mode. It will introduce noisy by >> adding __must_check. So I think its better without __must_check? > > Wouldn't it need to update perf mode as well? I will also update the perf mode for consistent usage. Thanks, Jie > > Regarding __must_check, I searched Documentation but didn't find > guidance on when it should be used. I don't want to use this annotation > randomly (some functions use it and some not), this will be hard for > everyone to follow up. > > IMO, it's fine not to use __must_check here. I would leave this to > Suzuki and other maintainers if have different opinions. > > Thanks, > Leo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-05-12 0:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-05-11 8:36 [PATCH v3] coresight: fix missing error code when trace ID is invalid Jie Gan 2026-05-11 9:19 ` Richard Cheng 2026-05-11 9:27 ` Jie Gan 2026-05-11 10:26 ` Richard Cheng 2026-05-11 14:45 ` Leo Yan 2026-05-12 0:54 ` Jie Gan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox