[PATCH v4 0/2] Fixes to mlxbf-pmc

[PATCH v4 0/2] Fixes to mlxbf-pmc

[Shravan Kumar Ramani]

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(-)

-- 
2.43.2

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

[Shravan Kumar Ramani]

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")
Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
Reviewed-by: David Thompson <davthompson@nvidia.com>
---
 drivers/platform/mellanox/mlxbf-pmc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c
index 900069eb186e..9cc3d4ca53cf 100644
--- a/drivers/platform/mellanox/mlxbf-pmc.c
+++ b/drivers/platform/mellanox/mlxbf-pmc.c
@@ -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 {
-- 
2.43.2

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

[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")
Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
Reviewed-by: David Thompson <davthompson@nvidia.com>
---
 drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c
index 9cc3d4ca53cf..9aa8de1122e5 100644
--- a/drivers/platform/mellanox/mlxbf-pmc.c
+++ b/drivers/platform/mellanox/mlxbf-pmc.c
@@ -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;
-- 
2.43.2

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

[Ilpo Järvinen]

On Wed, 2 Jul 2025, Shravan Kumar Ramani wrote:

> 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")
> Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> ---
>  drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c
> index 9cc3d4ca53cf..9aa8de1122e5 100644
> --- a/drivers/platform/mellanox/mlxbf-pmc.c
> +++ b/drivers/platform/mellanox/mlxbf-pmc.c
> @@ -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;
> 

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.

-- 
 i.