On Fri, Nov 27, 2020 at 12:42:47AM +0900, Dongjin Kim wrote:
> On Wed, Nov 25, 2020 at 06:05:36PM -0800, Guenter Roeck wrote:
> > On Thu, Nov 26, 2020 at 01:32:42AM +0900, Dongjin Kim wrote:
> > > This patch adds to new attribute 'pwm1_enable' to change the fan speed
> > > control method as documented in 'Documentation/hwmon/sysfs-interface'.
> > >
> > > Signed-off-by: Dongjin Kim <tobetter@xxxxxxxxx>
> >
> > The new attribute needs to be documented in
> > Documentation/hwmon/pwm-fan.rst, with supported values.
> >
> > > ---
> > > drivers/hwmon/pwm-fan.c | 52 ++++++++++++++++++++++++++++++++++++-----
> > > 1 file changed, 46 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > > index 1f63807c0399..834275309421 100644
> > > --- a/drivers/hwmon/pwm-fan.c
> > > +++ b/drivers/hwmon/pwm-fan.c
> > > @@ -39,6 +39,7 @@ struct pwm_fan_ctx {
> > > unsigned int pwm_fan_max_state;
> > > unsigned int *pwm_fan_cooling_levels;
> > > struct thermal_cooling_device *cdev;
> > > + int enable;
> > > };
> > >
> > > /* This handler assumes self resetting edge triggered interrupt. */
> > > @@ -76,6 +77,10 @@ static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
> > > struct pwm_state state = { };
> > >
> > > mutex_lock(&ctx->lock);
> > > +
> > > + if (ctx->enable == 0)
> > > + pwm = MAX_PWM;
> > > +
> > > if (ctx->pwm_value == pwm)
> > > goto exit_set_pwm_err;
> > >
> > > @@ -137,11 +142,42 @@ static ssize_t rpm_show(struct device *dev,
> > > return sprintf(buf, "%u\n", ctx->rpm);
> > > }
> > >
> > > +static ssize_t enable_store(struct device *dev,
> > > + struct device_attribute *attr,
> > > + const char *buf, size_t count)
> > > +{
> > > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > > + int err;
> > > + unsigned long val;
> > > +
> > > + err = kstrtoul(buf, 10, &val);
> > > + if (err)
> > > + return err;
> > > +
> >
> > 'val' must be validated and only accept permitted values.
> Sure.
> >
> > > + mutex_lock(&ctx->lock);
> > > + ctx->enable = val;
> > > + mutex_unlock(&ctx->lock);
> > > +
> > > + err = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[ctx->pwm_fan_state]);
> > > +
> > > + return err ? err : count;
> > > +}
> > > +
> > > +static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
> > > + char *buf)
> > > +{
> > > + struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > > +
> > > + return sprintf(buf, "%u\n", ctx->enable);
> > > +}
> > > +
> > > static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
> > > +static SENSOR_DEVICE_ATTR_RW(pwm1_enable, enable, 0);
> > > static SENSOR_DEVICE_ATTR_RO(fan1_input, rpm, 0);
> > >
> > > static struct attribute *pwm_fan_attrs[] = {
> > > &sensor_dev_attr_pwm1.dev_attr.attr,
> > > + &sensor_dev_attr_pwm1_enable.dev_attr.attr,
> > > &sensor_dev_attr_fan1_input.dev_attr.attr,
> > > NULL,
> > > };
> > > @@ -153,7 +189,7 @@ static umode_t pwm_fan_attrs_visible(struct kobject *kobj, struct attribute *a,
> > > struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> > >
> > > /* Hide fan_input in case no interrupt is available */
> > > - if (n == 1 && ctx->irq <= 0)
> > > + if (n == 2 && ctx->irq <= 0)
> > > return 0;
> > >
> > > return a->mode;
> > > @@ -200,7 +236,7 @@ static int
> > > pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> > > {
> > > struct pwm_fan_ctx *ctx = cdev->devdata;
> > > - int ret;
> > > + int ret = 0;
> > >
> > > if (!ctx || (state > ctx->pwm_fan_max_state))
> > > return -EINVAL;
> > > @@ -208,10 +244,12 @@ pwm_fan_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state)
> > > if (state == ctx->pwm_fan_state)
> > > return 0;
> > >
> > > - ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> > > - if (ret) {
> > > - dev_err(&cdev->device, "Cannot set pwm!\n");
> > > - return ret;
> > > + if (ctx->enable >= 2) {
> >
> > What is "automatic" here ? I don't see how this driver or the underlying
> > pwm controller/chip would support automatic fan speed control. This is
> > completely independent of thermal control: thermal device support does _not_
> > imply or suggest "automatic" fan speed control from a pwm chip perspective.
> >
> My understanding of 'automatic' is to set the fan speed by a thermal device
> with the trip points, it changes the cooling state and change the fan speed.
> Please correct me if I am wrong.
Yes, you are wrong. "automatic" means the controller performs pwm control
automatically, on its own, without interference by the Linux kernel or userspace.
It is also autonomous to thermal subsystem control. In general, "automatic" and
thermal control by the Linux kernel contradict each other: "automatic" implies
that the device is _not_ under thermal subsystem control. That would, from
a device perspective, be "manual".
>
> > This makes me also very concerned about this attribute in the first please:
> > All it does, in its current implementation, is to disable thermal device
> > control. That is not the idea here, and it doesn't make any sense to me.
> > On the contrary, all I can find is that it is dangerous.
> >
> I don't intend to change the thermal control itself in 'manual' mode, but does
> not change the fan speed with given cooling state. In 'automatic' mode, the
> fan speed will be changed by the thermal device.
>
> > Please describe in detail what you think this attribute is supposed to
> > accomplish, and why you think it is needed or even makes sense. Overriding
> > thermal control doesn't make sense to me: If you want that, just disable
> > thermal control, or don't register this device as cooling device in the
> > first place. Other than that, the whole driver implies manual pwm control.
> > That means pwm can already be "disabled" by writing a 0 pwm value. An
> > additional attributes doesn't add value and only makes the driver ABI
> > unnecessarily complex. So please prepare a detailed rationale to convince
> > me otherwise.
> >
> I am using this driver to run a fan on ARM SBC and thought 'fancontrol' can
> help to map the fan speed with pwm value if necessary instead of using the pwm
> values in a device tree by writing '1' to 'pwm1_enable'. When this driver is
> not registered as a cooling device, as you suggested, OS should provide the
> default fancontrol config to run fully in manual mode if my understanding is
> correct. Based on this what I am thinking is that OS images for ARM SBC should
> have a fan control service or a tool if it allows user to change the fan speed.
> But this could be very various for all SBC, I would prefer to provide the
> default cooling device in the device tree to prevent the absence of fan
> control due to the missing or wrong config.
'fancontrol' and in-kernel thermal management should not both run at the same
time. In-kernel thermal control would in general be preferred, and should be
configurable through the thermal subsystem. Either case, we can't use hwmon
attributes to switch between the two methods for thermal control. If
'fancontrol' is to be used, the thermal subsystem should be completely
disabled. However, again, I would not recommend doing that since it is much more
comprehensive than 'fancontrol'.
Guenter
>
> > We can have a separate discussion if disabling a pwm controller using the
> > hwmon ABI should disable that pwm controller from a thermal (cooling) device
> > perspective. If so, we'll need to think about the implications. We can not
> > just disable thermal cooling device support without telling the thermal
> > subsystem that this means of temperature control doesn't work anymore.
> > That should, however, be a completely separate discussion, independent
> > of this driver, and it should include thermal subsystem maintainers
> > and the thermal subsystem mailing list.
> Ok, thank you for your advice and review. :)
>
> Dongjin.
> >
> > Thanks,
> > Guenter
> >
> > > + ret = __set_pwm(ctx, ctx->pwm_fan_cooling_levels[state]);
> > > + if (ret) {
> > > + dev_err(&cdev->device, "Cannot set pwm!\n");
> > > + return ret;
> > > + }
> > > }
> > >
> > > ctx->pwm_fan_state = state;
> > > @@ -298,6 +336,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
> > > if (IS_ERR(ctx->pwm))
> > > return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n");
> > >
> > > + ctx->enable = 2;
> > > +
> > > platform_set_drvdata(pdev, ctx);
> > >
> > > ctx->irq = platform_get_irq_optional(pdev, 0);
> > > --
> > > 2.25.1
> > >
