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@xxxxxxxxxxxx> --- 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@xxxxxxxxx>
... but I'll also keep that tag unless you start screaming. Thanks, Guenter
