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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.