| Message ID | 20240701212348.1670617-2-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (amc6821) Various improvements | expand |
Hi Guenter, On 7/1/24 11:23 PM, Guenter Roeck wrote: > The pwm value range is well defined from 0..255. Don't accept > any values outside this range. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Use kstrtou8() instead of kstrtol() where possible. > Limit range of pwm1_auto_point_pwm to 0..254 in patch 1 > instead of limiting it later, and do not accept invalid > values for the attribute. > > drivers/hwmon/amc6821.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c > index 9b02b304c2f5..eb2d5592a41a 100644 > --- a/drivers/hwmon/amc6821.c > +++ b/drivers/hwmon/amc6821.c > @@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev, > { > struct amc6821_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > - long val; > - int ret = kstrtol(buf, 10, &val); > + u8 val; > + int ret = kstrtou8(buf, 10, &val); > if (ret) > return ret; > > mutex_lock(&data->update_lock); > - data->pwm1 = clamp_val(val , 0, 255); > + data->pwm1 = val; > i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1); > mutex_unlock(&data->update_lock); > return count; > @@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev, > struct amc6821_data *data = dev_get_drvdata(dev); > struct i2c_client *client = data->client; > int dpwm; > - long val; > - int ret = kstrtol(buf, 10, &val); > + u8 val; > + int ret = kstrtou8(buf, 10, &val); > if (ret) > return ret; > > + if (val > 254) Would have appreciated a comment as to why it's 254. My understanding is that the subsystem requires no overlap between multiple pwm_auto_points? 0 being 0 and 2 being 255, we need 1 to be 255? Actually, that doesn't explain why we allow 0 here, so maybe I'm just clueless :) The change itself though: Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
On 7/3/24 07:29, Quentin Schulz wrote: > Hi Guenter, > > On 7/1/24 11:23 PM, Guenter Roeck wrote: >> The pwm value range is well defined from 0..255. Don't accept >> any values outside this range. >> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> v2: Use kstrtou8() instead of kstrtol() where possible. >> Limit range of pwm1_auto_point_pwm to 0..254 in patch 1 >> instead of limiting it later, and do not accept invalid >> values for the attribute. >> >> drivers/hwmon/amc6821.c | 15 +++++++++------ >> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c >> index 9b02b304c2f5..eb2d5592a41a 100644 >> --- a/drivers/hwmon/amc6821.c >> +++ b/drivers/hwmon/amc6821.c >> @@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev, >> { >> struct amc6821_data *data = dev_get_drvdata(dev); >> struct i2c_client *client = data->client; >> - long val; >> - int ret = kstrtol(buf, 10, &val); >> + u8 val; >> + int ret = kstrtou8(buf, 10, &val); >> if (ret) >> return ret; >> mutex_lock(&data->update_lock); >> - data->pwm1 = clamp_val(val , 0, 255); >> + data->pwm1 = val; >> i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1); >> mutex_unlock(&data->update_lock); >> return count; >> @@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev, >> struct amc6821_data *data = dev_get_drvdata(dev); >> struct i2c_client *client = data->client; >> int dpwm; >> - long val; >> - int ret = kstrtol(buf, 10, &val); >> + u8 val; >> + int ret = kstrtou8(buf, 10, &val); >> if (ret) >> return ret; >> >> + if (val > 254) > > Would have appreciated a comment as to why it's 254. My understanding is that the subsystem requires no overlap between multiple pwm_auto_points? 0 being 0 and 2 being 255, we need 1 to be 255? > No idea, really, I just took it from the original code. I don't find a hint in the code suggesting why 255 would be worse than 0. > Actually, that doesn't explain why we allow 0 here, so maybe I'm just clueless :) > Yes, agreed, that doesn't really make sense. I'll change the upper limit to 255. > The change itself though: > Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> > ... but I'll also keep that tag unless you start screaming. Thanks, Guenter
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c index 9b02b304c2f5..eb2d5592a41a 100644 --- a/drivers/hwmon/amc6821.c +++ b/drivers/hwmon/amc6821.c @@ -355,13 +355,13 @@ static ssize_t pwm1_store(struct device *dev, { struct amc6821_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; - long val; - int ret = kstrtol(buf, 10, &val); + u8 val; + int ret = kstrtou8(buf, 10, &val); if (ret) return ret; mutex_lock(&data->update_lock); - data->pwm1 = clamp_val(val , 0, 255); + data->pwm1 = val; i2c_smbus_write_byte_data(client, AMC6821_REG_DCY, data->pwm1); mutex_unlock(&data->update_lock); return count; @@ -558,13 +558,16 @@ static ssize_t pwm1_auto_point_pwm_store(struct device *dev, struct amc6821_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; int dpwm; - long val; - int ret = kstrtol(buf, 10, &val); + u8 val; + int ret = kstrtou8(buf, 10, &val); if (ret) return ret; + if (val > 254) + return -EINVAL; + mutex_lock(&data->update_lock); - data->pwm1_auto_point_pwm[1] = clamp_val(val, 0, 254); + data->pwm1_auto_point_pwm[1] = val; if (i2c_smbus_write_byte_data(client, AMC6821_REG_DCY_LOW_TEMP, data->pwm1_auto_point_pwm[1])) { dev_err(&client->dev, "Register write error, aborting.\n");
The pwm value range is well defined from 0..255. Don't accept any values outside this range. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: Use kstrtou8() instead of kstrtol() where possible. Limit range of pwm1_auto_point_pwm to 0..254 in patch 1 instead of limiting it later, and do not accept invalid values for the attribute. drivers/hwmon/amc6821.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)