Skip to contentSkip to conversation

Fixes to mlxbf-pmc

PATCH In review Shravan Kumar Ramani 2 participants 4 messages Jul 2, 2025 – Jul 7, 2025

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

v4 cover Fixes to mlxbf-pmc

This submission contains 2 patches:

Patch 1 fixes an issue with matching an input event_name string with the list of supported events in the event_list due to the trailing newline char.

Patch 2 adds checks to validate the input for event and enable fields, and prevents the user from writing invalid values to these fields.

v1 -> v2
Split single patch into 2 patches addressing each fix separately

v2 -> v3
Patch 1: Remove the trailing newline character before comparing strings Patch 2: Use kstrtobool() instead of kstrtouint() which would validate the input

v3 -> v4
Patch 1: Use kstrdup_and_replace() to remove the newline char.

Shravan Kumar Ramani (2):
platform/mellanox: mlxbf-pmc: Remove newline char from event name
input
platform/mellanox: mlxbf-pmc: Validate event/enable input

 drivers/platform/mellanox/mlxbf-pmc.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)
Signature · 1 line
2.43.2
#1 lore raw

PATCH v4 1/2 platform/mellanox: mlxbf-pmc: Remove newline char from event name input

Since the input string passed via the command line appends a newline char, it needs to be removed before comparison with the event_list.

Fixes 1a218d312e65 ("platform/mellanox: mlxbf-pmc: Add Mellanox BlueField PMC driver")
---
 drivers/platform/mellanox/mlxbf-pmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/platform/mellanox/mlxbf-pmc.c +9 -1
index 900069eb186e..9cc3d4ca53cf 100644
@@ -15,6 +15,7 @@
 #include <linux/hwmon.h>
 #include <linux/platform_device.h>
 #include <linux/string.h>
+#include <linux/string_helpers.h>
 #include <uapi/linux/psci.h>
 
 #define MLXBF_PMC_WRITE_REG_32 0x82000009
@@ -1784,6 +1785,7 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,
 		attr, struct mlxbf_pmc_attribute, dev_attr);
 	unsigned int blk_num, cnt_num;
 	bool is_l3 = false;
+	char *evt_name;
 	int evt_num;
 	int err;
 
@@ -1791,8 +1793,14 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,
 	cnt_num = attr_event->index;
 
 	if (isalpha(buf[0])) {
+		/* Remove the trailing newline character if present */
+		evt_name = kstrdup_and_replace(buf, '\n', '\0', GFP_KERNEL);
+		if (!evt_name)
+			return -ENOMEM;
+
 		evt_num = mlxbf_pmc_get_event_num(pmc->block_name[blk_num],
-						  buf);
+						  evt_name);
+		kfree(evt_name);
 		if (evt_num < 0)
 			return -EINVAL;
 	} else {
Signature · 1 line
2.43.2
#2 lore raw

PATCH v4 2/2 platform/mellanox: mlxbf-pmc: Validate event/enable input

In reply to Shravan Kumar Ramani

Before programming the event info, validate the event number received as input by checking if it exists in the event_list. Also fix a typo in the comment for mlxbf_pmc_get_event_name() to correctly mention that it returns the event name when taking the event number as input, and not the other way round. For setting the enable value, the input should be 0 or 1 only. Use kstrtobool() in place of kstrtoint() in mlxbf_pmc_enable_store() to accept only valid input.

Fixes 423c3361855c ("platform/mellanox: mlxbf-pmc: Add support for BlueField-3")
---
 drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/platform/mellanox/mlxbf-pmc.c +8 -7
index 9cc3d4ca53cf..9aa8de1122e5 100644
@@ -1223,7 +1223,7 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)
 	return -ENODEV;
 }
 
-/* Get the event number given the name */
+/* Get the event name given the number */
 static char *mlxbf_pmc_get_event_name(const char *blk, u32 evt)
 {
 	const struct mlxbf_pmc_events *events;
@@ -1807,6 +1807,9 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,
 		err = kstrtouint(buf, 0, &evt_num);
 		if (err < 0)
 			return err;
+
+		if (!mlxbf_pmc_get_event_name(pmc->block_name[blk_num], evt_num))
+			return -EINVAL;
 	}
 
 	if (strstr(pmc->block_name[blk_num], "l3cache"))
@@ -1887,13 +1890,14 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,
 {
 	struct mlxbf_pmc_attribute *attr_enable = container_of(
 		attr, struct mlxbf_pmc_attribute, dev_attr);
-	unsigned int en, blk_num;
+	unsigned int blk_num;
 	u32 word;
 	int err;
+	bool en;
 
 	blk_num = attr_enable->nr;
 
-	err = kstrtouint(buf, 0, &en);
+	err = kstrtobool(buf, &en);
 	if (err < 0)
 		return err;
 
@@ -1913,14 +1917,11 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,
 			MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters),
 			MLXBF_PMC_WRITE_REG_32, word);
 	} else {
-		if (en && en != 1)
-			return -EINVAL;
-
 		err = mlxbf_pmc_config_l3_counters(blk_num, false, !!en);
 		if (err)
 			return err;
 
-		if (en == 1) {
+		if (en) {
 			err = mlxbf_pmc_config_l3_counters(blk_num, true, false);
 			if (err)
 				return err;
Signature · 1 line
2.43.2
maintainer
#3 lore raw

Re: [PATCH v4 2/2] platform/mellanox: mlxbf-pmc: Validate event/enable input

Quoted 61 lines · Shravan Kumar Ramani

Before programming the event info, validate the event number received as input by checking if it exists in the event_list. Also fix a typo in the comment for mlxbf_pmc_get_event_name() to correctly mention that it returns the event name when taking the event number as input, and not the other way round. For setting the enable value, the input should be 0 or 1 only. Use kstrtobool() in place of kstrtoint() in mlxbf_pmc_enable_store() to accept only valid input.

Fixes 423c3361855c ("platform/mellanox: mlxbf-pmc: Add support for BlueField-3")
---
 drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/platform/mellanox/mlxbf-pmc.c +7 -5
index 9cc3d4ca53cf..9aa8de1122e5 100644
@@ -1223,7 +1223,7 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)
 	return -ENODEV;
 }
 
-/* Get the event number given the name */
+/* Get the event name given the number */
 static char *mlxbf_pmc_get_event_name(const char *blk, u32 evt)
 {
 	const struct mlxbf_pmc_events *events;
@@ -1807,6 +1807,9 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,
 		err = kstrtouint(buf, 0, &evt_num);
 		if (err < 0)
 			return err;
+
+		if (!mlxbf_pmc_get_event_name(pmc->block_name[blk_num], evt_num))
+			return -EINVAL;
 	}
 
 	if (strstr(pmc->block_name[blk_num], "l3cache"))
@@ -1887,13 +1890,14 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,
 {
 	struct mlxbf_pmc_attribute *attr_enable = container_of(
 		attr, struct mlxbf_pmc_attribute, dev_attr);
-	unsigned int en, blk_num;
+	unsigned int blk_num;
 	u32 word;
 	int err;
+	bool en;
 
 	blk_num = attr_enable->nr;
 
-	err = kstrtouint(buf, 0, &en);
+	err = kstrtobool(buf, &en);
 	if (err < 0)
 		return err;
 
@@ -1913,14 +1917,11 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,
 			MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters),
 			MLXBF_PMC_WRITE_REG_32, word);
 	} else {
-		if (en && en != 1)
-			return -EINVAL;

drivers/platform/mellanox/mlxbf-pmc.c static ssize_t mlxbf_pmc_enable_store(struct device *dev, · line 1927

Quoted 10 lines
drivers/platform/mellanox/mlxbf-pmc.c +1 -2
-
 		err = mlxbf_pmc_config_l3_counters(blk_num, false, !!en);
 		if (err)
 			return err;
 
-		if (en == 1) {
+		if (en) {
 			err = mlxbf_pmc_config_l3_counters(blk_num, true, false);
 			if (err)
 				return err;

Hi,

I've applied this series to the review-ilpo-fixes branch but I had to split the kstrbool() change to own commit, it's very apparent these two changes should have been separate right from the start (and I even asked you to split this earlier).

Whenever making changes, especially fixes, please try hard put separate changes into own patches. That should be done even if the changes touch same file, and they may even look similar such as here, both are doing "input validation", but the cases were still clearly different.

It's easier to review, justify in the changelog, etc. when the change is very focused on a single problem only.

Signature · 1 line
 i.