Fixes to mlxbf-pmc
Subsystems (2)
v3 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
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 | 28 ++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-)
Signature · 1 line
2.43.2
PATCH v3 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.
--- drivers/platform/mellanox/mlxbf-pmc.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
drivers/platform/mellanox/mlxbf-pmc.c +9 -4
@@ -1204,9 +1204,10 @@ static bool mlxbf_pmc_event_supported(const char *blk) }/* Get the event number given the name */ -static int mlxbf_pmc_get_event_num (const char *blk,const char *evt) +static int mlxbf_pmc_get_event_num (const char *blk,char *evt) {const struct mlxbf_pmc_events *events; +int len =strlen (evt);unsigned int i;size_t size;
@@ -1214,6 +1215,10 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)if (!events)return -EINVAL ; +/* Remove the trailing newline character if present */ +if (evt[len -1 ] =='\n' ) + evt[len -1 ] ='\0' ; +for (i =0 ; i < size; ++i) {if (!strcmp (evt, events[i].evt_name))return events[i].evt_num;
@@ -1681,7 +1686,7 @@ static ssize_t mlxbf_pmc_counter_show(struct device *dev,return -EINVAL ; }else if (pmc->block[blk_num].type ==MLXBF_PMC_TYPE_REGISTER ) { offset =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - attr->attr.name); + (char *)attr->attr.name);if (offset <0 )return -EINVAL ;if (mlxbf_pmc_read_reg (blk_num, offset, &value))
@@ -1730,7 +1735,7 @@ static ssize_t mlxbf_pmc_counter_store(struct device *dev,return err; }else if (pmc->block[blk_num].type ==MLXBF_PMC_TYPE_REGISTER ) { offset =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - attr->attr.name); + (char *)attr->attr.name);if (offset <0 )return -EINVAL ; err =mlxbf_pmc_write_reg (blk_num, offset, data);
@@ -1792,7 +1797,7 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,if (isalpha (buf[0 ])) { evt_num =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - buf); + (char *)buf);if (evt_num <0 )return -EINVAL ; }else {
Signature · 1 line
2.43.2
Ilpo Järvinen
maintainerRe: [PATCH v3 1/2] platform/mellanox: mlxbf-pmc: Remove newline char from event name input
Quoted 47 lines · 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 <[email protected]>Reviewed-by David Thompson <[email protected]>--- drivers/platform/mellanox/mlxbf-pmc.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)drivers/platform/mellanox/mlxbf-pmc.c +7
-2@@ -1204,9 +1204,10 @@ static bool mlxbf_pmc_event_supported(const char *blk) }/* Get the event number given the name */ -static int mlxbf_pmc_get_event_num (const char *blk,const char *evt) +static int mlxbf_pmc_get_event_num (const char *blk,char *evt) {const struct mlxbf_pmc_events *events; +int len =strlen (evt);unsigned int i;size_t size;@@ -1214,6 +1215,10 @@ static int mlxbf_pmc_get_event_num(const char *blk, const char *evt)if (!events)return -EINVAL ; +/* Remove the trailing newline character if present */ +if (evt[len -1 ] =='\n' ) + evt[len -1 ] ='\0' ; +for (i =0 ; i < size; ++i) {if (!strcmp (evt, events[i].evt_name))return events[i].evt_num;@@ -1681,7 +1686,7 @@ static ssize_t mlxbf_pmc_counter_show(struct device *dev,return -EINVAL ; }else if (pmc->block[blk_num].type ==MLXBF_PMC_TYPE_REGISTER ) { offset =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - attr->attr.name); + (char *)attr->attr.name);if (offset <0 )return -EINVAL ;if (mlxbf_pmc_read_reg (blk_num, offset, &value))
drivers/platform/mellanox/mlxbf-pmc.c static ssize_t mlxbf_pmc_counter_store(struct device *dev, · line 1738
Quoted 5 lines
drivers/platform/mellanox/mlxbf-pmc.c +1
-1@@ -1730,7 +1735,7 @@ static ssize_t mlxbf_pmc_counter_store(struct device *dev,return err; }else if (pmc->block[blk_num].type ==MLXBF_PMC_TYPE_REGISTER ) { offset =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - attr->attr.name); + (char *)attr->attr.name);
These shouldn't need any newline removal, right?
drivers/platform/mellanox/mlxbf-pmc.c static ssize_t mlxbf_pmc_event_store(struct device *dev, · line 1800
Quoted 9 lines
drivers/platform/mellanox/mlxbf-pmc.c +1
-1if (offset <0 )return -EINVAL ; err =mlxbf_pmc_write_reg (blk_num, offset, data);@@ -1792,7 +1797,7 @@ static ssize_t mlxbf_pmc_event_store(struct device *dev,if (isalpha (buf[0 ])) { evt_num =mlxbf_pmc_get_event_num (pmc->block_name[blk_num], - buf); + (char *)buf);
You should cleanup the newline here, not inside mlxbf_pmc_get_event_num() so that you can keep the argument type const.
As the input parameter is const char *, I suggest using kstrdup_and_replace() so you can modify it.
In general, casting constness away is bad practice.
Quoted 4 lines
drivers/platform/mellanox/mlxbf-pmc.c +0
-0if (evt_num <0 )return -EINVAL ; }else {
Signature · 1 line
i.
PATCH v3 2/2 platform/mellanox: mlxbf-pmc: Validate event/enable input
In reply to Shravan Kumar RamaniBefore 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 kstrtouint() in mlxbf_pmc_enable_store() to accept only valid input.
--- drivers/platform/mellanox/mlxbf-pmc.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/platform/mellanox/mlxbf-pmc.c +8 -7
@@ -1227,7 +1227,7 @@ static int mlxbf_pmc_get_event_num(const char *blk, 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;
@@ -1804,6 +1804,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" ))
@@ -1884,13 +1887,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;
@@ -1910,14 +1914,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