| Message ID | 1423241948-31981-7-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On Fri, Feb 06, 2015 at 05:59:06PM +0100, Lukasz Majewski wrote: > It was necessary to decouple code handling writing to sysfs from the one > responsible for setting PWM of the fan. > Due to that, new __set_pwm() method was extracted, which is responsible for > only setting new PWM duty cycle. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > Changes for v2: > - None > Changes for v3: > - The commit headline has been reedited. > --- > drivers/hwmon/pwm-fan.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index 1991d903..870e100 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -33,21 +33,15 @@ struct pwm_fan_ctx { > unsigned char pwm_value; > }; > > -static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, > - const char *buf, size_t count) > +static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > { > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > - unsigned long pwm, duty; > - ssize_t ret; > - > - if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) > - return -EINVAL; > - > - mutex_lock(&ctx->lock); > + unsigned long duty; > + int ret; > > if (ctx->pwm_value == pwm) > - goto exit_set_pwm_no_change; > + return 0; > Why did you move this check outside the lock ? With this change there is no guarantee that pwm_value wasn't changed while waiting for the lock. Guenter > + mutex_lock(&ctx->lock); > if (pwm == 0) { > pwm_disable(ctx->pwm); > goto exit_set_pwm; > @@ -66,13 +60,28 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, > > exit_set_pwm: > ctx->pwm_value = pwm; > -exit_set_pwm_no_change: > - ret = count; > exit_set_pwm_err: > mutex_unlock(&ctx->lock); > return ret; > } > > +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > + unsigned long pwm; > + int ret; > + > + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) > + return -EINVAL; > + > + ret = __set_pwm(ctx, pwm); > + if (ret) > + return ret; > + > + return count; > +} > + > static ssize_t show_pwm(struct device *dev, > struct device_attribute *attr, char *buf) > { > -- > 2.0.0.rc2 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 6 Feb 2015 10:27:25 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On Fri, Feb 06, 2015 at 05:59:06PM +0100, Lukasz Majewski wrote: > > It was necessary to decouple code handling writing to sysfs from > > the one responsible for setting PWM of the fan. > > Due to that, new __set_pwm() method was extracted, which is > > responsible for only setting new PWM duty cycle. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > Changes for v2: > > - None > > Changes for v3: > > - The commit headline has been reedited. > > --- > > drivers/hwmon/pwm-fan.c | 35 ++++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > index 1991d903..870e100 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > @@ -33,21 +33,15 @@ struct pwm_fan_ctx { > > unsigned char pwm_value; > > }; > > > > -static ssize_t set_pwm(struct device *dev, struct device_attribute > > *attr, > > - const char *buf, size_t count) > > +static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > > { > > - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > - unsigned long pwm, duty; > > - ssize_t ret; > > - > > - if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) > > - return -EINVAL; > > - > > - mutex_lock(&ctx->lock); > > + unsigned long duty; > > + int ret; > > > > if (ctx->pwm_value == pwm) > > - goto exit_set_pwm_no_change; > > + return 0; > > > Why did you move this check outside the lock ? With this change there > is no guarantee that pwm_value wasn't changed while waiting for the > lock. Grrr. You are obviously right here. I will fix this. Thanks for spotting Best regards, Lukasz Majewski > > Guenter > > > + mutex_lock(&ctx->lock); > > if (pwm == 0) { > > pwm_disable(ctx->pwm); > > goto exit_set_pwm; > > @@ -66,13 +60,28 @@ static ssize_t set_pwm(struct device *dev, > > struct device_attribute *attr, > > exit_set_pwm: > > ctx->pwm_value = pwm; > > -exit_set_pwm_no_change: > > - ret = count; > > exit_set_pwm_err: > > mutex_unlock(&ctx->lock); > > return ret; > > } > > > > +static ssize_t set_pwm(struct device *dev, struct device_attribute > > *attr, > > + const char *buf, size_t count) > > +{ > > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); > > + unsigned long pwm; > > + int ret; > > + > > + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) > > + return -EINVAL; > > + > > + ret = __set_pwm(ctx, pwm); > > + if (ret) > > + return ret; > > + > > + return count; > > +} > > + > > static ssize_t show_pwm(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > -- > > 2.0.0.rc2 > >
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 1991d903..870e100 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -33,21 +33,15 @@ struct pwm_fan_ctx { unsigned char pwm_value; }; -static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, - const char *buf, size_t count) +static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) { - struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); - unsigned long pwm, duty; - ssize_t ret; - - if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) - return -EINVAL; - - mutex_lock(&ctx->lock); + unsigned long duty; + int ret; if (ctx->pwm_value == pwm) - goto exit_set_pwm_no_change; + return 0; + mutex_lock(&ctx->lock); if (pwm == 0) { pwm_disable(ctx->pwm); goto exit_set_pwm; @@ -66,13 +60,28 @@ static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, exit_set_pwm: ctx->pwm_value = pwm; -exit_set_pwm_no_change: - ret = count; exit_set_pwm_err: mutex_unlock(&ctx->lock); return ret; } +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); + unsigned long pwm; + int ret; + + if (kstrtoul(buf, 10, &pwm) || pwm > MAX_PWM) + return -EINVAL; + + ret = __set_pwm(ctx, pwm); + if (ret) + return ret; + + return count; +} + static ssize_t show_pwm(struct device *dev, struct device_attribute *attr, char *buf) {
It was necessary to decouple code handling writing to sysfs from the one responsible for setting PWM of the fan. Due to that, new __set_pwm() method was extracted, which is responsible for only setting new PWM duty cycle. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- Changes for v2: - None Changes for v3: - The commit headline has been reedited. --- drivers/hwmon/pwm-fan.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)