Re: [PATCH v1 1/1] platform/mellanox: mlxbf-pmc: Check validity of event/enable input

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

On Wed, 18 Jun 2025, Shravan Kumar Ramani wrote:

> For eventN input, check if the event is part of the event list
> supported by the block.
> For enable input, do not accept values other than 0 or 1.
> Also replace sprintf instance with snprintf.

The code changes in the patch barely match some of what is described 
here, there are major gaps in the description.

Please don't try to put multiple independent changes into the same patch 
but create a patch series, each patch having focused changelog explaining 
reasoning clearly.

Unless the change is trivial (e.g., a comment typo fix) my general 
suggestion is to first state the problem, then explain the solution (on 
general level, no need to spell out what can be trivially read from the 
patch). Even for that comment change below, I'd want it mentioned that the 
comment does not match the code, it would be not enough to say e.g. "fix 
a wrong comment" but explain why it is wrong.

Some of these changes below may need Fixes tag but given the general 
vagueness and lack of description for some of the changes, I cannot decide 
(nor will accept the patches which do not have enough explanation). Put 
any fix patch at the head of the series.

Please don't leave lines "short" in the changelog (lines cut abruptly at 
stop "."). Write real paragraphs with full length and if you want have 
more than one paragraph, leave an empty line in between.

> Signed-off-by: Shravan Kumar Ramani <shravankr@nvidia.com>
> Reviewed-by: David Thompson <davthompson@nvidia.com>
> ---
>  drivers/platform/mellanox/mlxbf-pmc.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/mellanox/mlxbf-pmc.c b/drivers/platform/mellanox/mlxbf-pmc.c
> index 900069eb186e..fcc3392ff150 100644
> --- a/drivers/platform/mellanox/mlxbf-pmc.c
> +++ b/drivers/platform/mellanox/mlxbf-pmc.c
> @@ -1215,14 +1215,14 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)
>  		return -EINVAL;
>  
>  	for (i = 0; i < size; ++i) {
> -		if (!strcmp(evt, events[i].evt_name))
> +		if (!strncmp(evt, events[i].evt_name, strlen(events[i].evt_name)))
>  			return events[i].evt_num;
>  	}
>  
>  	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;
> @@ -1799,6 +1799,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"))
> @@ -1889,6 +1892,9 @@ static ssize_t mlxbf_pmc_enable_store(struct device *dev,
>  	if (err < 0)
>  		return err;
>  
> +	if (en != 0 && en != 1)
> +		return -EINVAL;
> +
>  	if (pmc->block[blk_num].type == MLXBF_PMC_TYPE_CRSPACE) {
>  		err = mlxbf_pmc_readl(pmc->block[blk_num].mmio_base +
>  			MLXBF_PMC_CRSPACE_PERFMON_CTL(pmc->block[blk_num].counters),
> @@ -1905,9 +1911,6 @@ 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;
> 

-- 
 i.