This series polishes the code to address warnings reported by the smatch static checker.
Smatch reports a error "Function too hairy" for etm4x_sysreg_read() and etm4x_sysreg_write(). This is a trade off to avoid large code blocks, the implementation encapsulates logic using nested macros. But this is not friendly to static checker. For now, the code will be kept as is.
Signed-off-by: Leo Yan leo.yan@arm.com --- Leo Yan (2): coresight: stm: Remove redundant NULL checks coresight: perf: Use %px for printing pointers
drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++-- drivers/hwtracing/coresight/coresight-stm.c | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) --- base-commit: 408c97c4a5e0b634dcd15bf8b8808b382e888164 change-id: 20250611-arm_cs_fix_smatch_warning_v1-6c2b5e78e38a
Best regards,
container_of() cannot return NULL, so the checks for NULL pointers are unnecessary and can be safely removed.
As a result, this commit silences the following smatch warnings:
coresight-stm.c:345 stm_generic_link() warn: can 'drvdata' even be NULL? coresight-stm.c:356 stm_generic_unlink() warn: can 'drvdata' even be NULL? coresight-stm.c:387 stm_generic_set_options() warn: can 'drvdata' even be NULL? coresight-stm.c:422 stm_generic_packet() warn: can 'drvdata' even be NULL?
Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-stm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index e45c6c7204b4491e0f879bc7d5d445aa1d3118be..464b0c85c3f7d3519169d62a51e9f8c6281b5358 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -342,7 +342,7 @@ static int stm_generic_link(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm); - if (!drvdata || !drvdata->csdev) + if (!drvdata->csdev) return -EINVAL;
return coresight_enable_sysfs(drvdata->csdev); @@ -353,7 +353,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm); - if (!drvdata || !drvdata->csdev) + if (!drvdata->csdev) return;
coresight_disable_sysfs(drvdata->csdev); @@ -384,7 +384,7 @@ static long stm_generic_set_options(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm); - if (!(drvdata && coresight_get_mode(drvdata->csdev))) + if (!coresight_get_mode(drvdata->csdev)) return -EINVAL;
if (channel >= drvdata->numsp) @@ -419,7 +419,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data, struct stm_drvdata, stm); unsigned int stm_flags;
- if (!(drvdata && coresight_get_mode(drvdata->csdev))) + if (!coresight_get_mode(drvdata->csdev)) return -EACCES;
if (channel >= drvdata->numsp)
On 11/06/25 8:14 PM, Leo Yan wrote:
container_of() cannot return NULL, so the checks for NULL pointers are unnecessary and can be safely removed.
As a result, this commit silences the following smatch warnings:
coresight-stm.c:345 stm_generic_link() warn: can 'drvdata' even be NULL? coresight-stm.c:356 stm_generic_unlink() warn: can 'drvdata' even be NULL? coresight-stm.c:387 stm_generic_set_options() warn: can 'drvdata' even be NULL? coresight-stm.c:422 stm_generic_packet() warn: can 'drvdata' even be NULL?
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-stm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index e45c6c7204b4491e0f879bc7d5d445aa1d3118be..464b0c85c3f7d3519169d62a51e9f8c6281b5358 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -342,7 +342,7 @@ static int stm_generic_link(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
- if (!drvdata || !drvdata->csdev)
- if (!drvdata->csdev) return -EINVAL;
return coresight_enable_sysfs(drvdata->csdev); @@ -353,7 +353,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
- if (!drvdata || !drvdata->csdev)
- if (!drvdata->csdev) return;
coresight_disable_sysfs(drvdata->csdev); @@ -384,7 +384,7 @@ static long stm_generic_set_options(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
- if (!(drvdata && coresight_get_mode(drvdata->csdev)))
- if (!coresight_get_mode(drvdata->csdev)) return -EINVAL;
if (channel >= drvdata->numsp) @@ -419,7 +419,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data, struct stm_drvdata, stm); unsigned int stm_flags;
- if (!(drvdata && coresight_get_mode(drvdata->csdev)))
- if (!coresight_get_mode(drvdata->csdev)) return -EACCES;
if (channel >= drvdata->numsp)
Seems to be a sensible clean up.
Reviewed-by: Anshuman Khandual anshuman.khandual@arm.com
On Thu, 12 Jun 2025 at 06:19, Anshuman Khandual anshuman.khandual@arm.com wrote:
On 11/06/25 8:14 PM, Leo Yan wrote:
container_of() cannot return NULL, so the checks for NULL pointers are unnecessary and can be safely removed.
As a result, this commit silences the following smatch warnings:
coresight-stm.c:345 stm_generic_link() warn: can 'drvdata' even be NULL? coresight-stm.c:356 stm_generic_unlink() warn: can 'drvdata' even be NULL? coresight-stm.c:387 stm_generic_set_options() warn: can 'drvdata' even be NULL? coresight-stm.c:422 stm_generic_packet() warn: can 'drvdata' even be NULL?
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-stm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index e45c6c7204b4491e0f879bc7d5d445aa1d3118be..464b0c85c3f7d3519169d62a51e9f8c6281b5358 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -342,7 +342,7 @@ static int stm_generic_link(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
if (!drvdata || !drvdata->csdev)
if (!drvdata->csdev) return -EINVAL;
I'd agree that the container_of() cannot return NULL, but what happens if stm_data is NULL? Perhaps the NULL check was on the wrong parameter?
Mike
return coresight_enable_sysfs(drvdata->csdev);@@ -353,7 +353,7 @@ static void stm_generic_unlink(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
if (!drvdata || !drvdata->csdev)
if (!drvdata->csdev) return; coresight_disable_sysfs(drvdata->csdev);@@ -384,7 +384,7 @@ static long stm_generic_set_options(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
if (!(drvdata && coresight_get_mode(drvdata->csdev)))
if (!coresight_get_mode(drvdata->csdev)) return -EINVAL; if (channel >= drvdata->numsp)@@ -419,7 +419,7 @@ static ssize_t notrace stm_generic_packet(struct stm_data *stm_data, struct stm_drvdata, stm); unsigned int stm_flags;
if (!(drvdata && coresight_get_mode(drvdata->csdev)))
if (!coresight_get_mode(drvdata->csdev)) return -EACCES; if (channel >= drvdata->numsp)Seems to be a sensible clean up.
Reviewed-by: Anshuman Khandual anshuman.khandual@arm.com
On Thu, Jun 12, 2025 at 09:28:42AM +0100, Mike Leach wrote:
On Thu, 12 Jun 2025 at 06:19, Anshuman Khandual anshuman.khandual@arm.com wrote:
On 11/06/25 8:14 PM, Leo Yan wrote:
container_of() cannot return NULL, so the checks for NULL pointers are unnecessary and can be safely removed.
As a result, this commit silences the following smatch warnings:
coresight-stm.c:345 stm_generic_link() warn: can 'drvdata' even be NULL? coresight-stm.c:356 stm_generic_unlink() warn: can 'drvdata' even be NULL? coresight-stm.c:387 stm_generic_set_options() warn: can 'drvdata' even be NULL? coresight-stm.c:422 stm_generic_packet() warn: can 'drvdata' even be NULL?
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-stm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index e45c6c7204b4491e0f879bc7d5d445aa1d3118be..464b0c85c3f7d3519169d62a51e9f8c6281b5358 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -342,7 +342,7 @@ static int stm_generic_link(struct stm_data *stm_data, { struct stm_drvdata *drvdata = container_of(stm_data, struct stm_drvdata, stm);
if (!drvdata || !drvdata->csdev)
if (!drvdata->csdev) return -EINVAL;
I'd agree that the container_of() cannot return NULL, but what happens if stm_data is NULL? Perhaps the NULL check was on the wrong parameter?
The pointer "stm_data" should be safe. The data structure is allocated in probe and released in the remove, the structure "stm_data" is safely accessed during runtime.
If stm_data really has NULL pointer issue, we should already receive alarms for kernel oops :)
Thanks, Leo
Use "%px" to print a pointer, which is better than casting the pointer to unsigned long and printing it with the "%lx" specifier.
Note, the printing format will be updated as 64-bit value:
# cat /sys/devices/cs_etm/sinks/trbe0 0x000000003744496a
This commit dismisses the following smatch warnings:
coresight-etm-perf.c:854 etm_perf_sink_name_show() warn: argument 4 to %lx specifier is cast from pointer coresight-etm-perf.c:946 etm_perf_cscfg_event_show() warn: argument 4 to %lx specifier is cast from pointer
Signed-off-by: Leo Yan leo.yan@arm.com --- drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index f1551c08ecb20ebd7feab82dbaee3176e6bcc999..f677c08233ba1a28b277674662c6e6db904873dd 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -851,7 +851,7 @@ static ssize_t etm_perf_sink_name_show(struct device *dev, struct dev_ext_attribute *ea;
ea = container_of(dattr, struct dev_ext_attribute, attr); - return scnprintf(buf, PAGE_SIZE, "0x%lx\n", (unsigned long)(ea->var)); + return scnprintf(buf, PAGE_SIZE, "0x%px\n", ea->var); }
static struct dev_ext_attribute * @@ -943,7 +943,7 @@ static ssize_t etm_perf_cscfg_event_show(struct device *dev, struct dev_ext_attribute *ea;
ea = container_of(dattr, struct dev_ext_attribute, attr); - return scnprintf(buf, PAGE_SIZE, "configid=0x%lx\n", (unsigned long)(ea->var)); + return scnprintf(buf, PAGE_SIZE, "configid=0x%px\n", ea->var); }
int etm_perf_add_symlink_cscfg(struct device *dev, struct cscfg_config_desc *config_desc)
On 11/06/25 8:14 PM, Leo Yan wrote:
Use "%px" to print a pointer, which is better than casting the pointer to unsigned long and printing it with the "%lx" specifier.
Note, the printing format will be updated as 64-bit value:
# cat /sys/devices/cs_etm/sinks/trbe0 0x000000003744496a
But what was it before this patch is applied ? Just wondering if there are sysfs changes that might be visible to the user space.
This commit dismisses the following smatch warnings:
coresight-etm-perf.c:854 etm_perf_sink_name_show() warn: argument 4 to %lx specifier is cast from pointer coresight-etm-perf.c:946 etm_perf_cscfg_event_show() warn: argument 4 to %lx specifier is cast from pointer
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index f1551c08ecb20ebd7feab82dbaee3176e6bcc999..f677c08233ba1a28b277674662c6e6db904873dd 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -851,7 +851,7 @@ static ssize_t etm_perf_sink_name_show(struct device *dev, struct dev_ext_attribute *ea; ea = container_of(dattr, struct dev_ext_attribute, attr);
- return scnprintf(buf, PAGE_SIZE, "0x%lx\n", (unsigned long)(ea->var));
- return scnprintf(buf, PAGE_SIZE, "0x%px\n", ea->var);
} static struct dev_ext_attribute * @@ -943,7 +943,7 @@ static ssize_t etm_perf_cscfg_event_show(struct device *dev, struct dev_ext_attribute *ea; ea = container_of(dattr, struct dev_ext_attribute, attr);
- return scnprintf(buf, PAGE_SIZE, "configid=0x%lx\n", (unsigned long)(ea->var));
- return scnprintf(buf, PAGE_SIZE, "configid=0x%px\n", ea->var);
} int etm_perf_add_symlink_cscfg(struct device *dev, struct cscfg_config_desc *config_desc)
On Thu, Jun 12, 2025 at 11:14:00AM +0530, Anshuman Khandual wrote:
On 11/06/25 8:14 PM, Leo Yan wrote:
Use "%px" to print a pointer, which is better than casting the pointer to unsigned long and printing it with the "%lx" specifier.
Note, the printing format will be updated as 64-bit value:
# cat /sys/devices/cs_etm/sinks/trbe0 0x000000003744496a
But what was it before this patch is applied ? Just wondering if there are sysfs changes that might be visible to the user space.
Good question. Before this patch, the printing does not pad with leading zeros, so the output result is:
# cat /sys/devices/cs_etm/sinks/trbe0 0x3744496a
The perf tool in user space extracts the lower 32-bit value with the specifier "%x". I verified that this would be fine that the tool can read out the hash value properly.
ret = perf_pmu__scan_file(pmu, path, "%x", &hash);
Thanks, Leo
This commit dismisses the following smatch warnings:
coresight-etm-perf.c:854 etm_perf_sink_name_show() warn: argument 4 to %lx specifier is cast from pointer coresight-etm-perf.c:946 etm_perf_cscfg_event_show() warn: argument 4 to %lx specifier is cast from pointer
Signed-off-by: Leo Yan leo.yan@arm.com
drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index f1551c08ecb20ebd7feab82dbaee3176e6bcc999..f677c08233ba1a28b277674662c6e6db904873dd 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -851,7 +851,7 @@ static ssize_t etm_perf_sink_name_show(struct device *dev, struct dev_ext_attribute *ea; ea = container_of(dattr, struct dev_ext_attribute, attr);
- return scnprintf(buf, PAGE_SIZE, "0x%lx\n", (unsigned long)(ea->var));
- return scnprintf(buf, PAGE_SIZE, "0x%px\n", ea->var);
} static struct dev_ext_attribute * @@ -943,7 +943,7 @@ static ssize_t etm_perf_cscfg_event_show(struct device *dev, struct dev_ext_attribute *ea; ea = container_of(dattr, struct dev_ext_attribute, attr);
- return scnprintf(buf, PAGE_SIZE, "configid=0x%lx\n", (unsigned long)(ea->var));
- return scnprintf(buf, PAGE_SIZE, "configid=0x%px\n", ea->var);
} int etm_perf_add_symlink_cscfg(struct device *dev, struct cscfg_config_desc *config_desc)
On Wed, 11 Jun 2025 15:44:00 +0100, Leo Yan wrote:
This series polishes the code to address warnings reported by the smatch static checker.
Smatch reports a error "Function too hairy" for etm4x_sysreg_read() and etm4x_sysreg_write(). This is a trade off to avoid large code blocks, the implementation encapsulates logic using nested macros. But this is not friendly to static checker. For now, the code will be kept as is.
[...]
Applied, thanks!
[1/2] coresight: stm: Remove redundant NULL checks https://git.kernel.org/coresight/c/c33c8fc1 [2/2] coresight: perf: Use %px for printing pointers https://git.kernel.org/coresight/c/7b8584a4
Best regards,
-
Anshuman Khandual -
Leo Yan -
Mike Leach -
Suzuki K Poulose