| Message ID | 20200410221236.6484-2-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/6] thermal: hwmon: Replace the call the thermal_cdev_update() | expand |
On 4/10/20 3:12 PM, Daniel Lezcano wrote: > The function thermal_cdev_upadte is called from the throttling misspelled > functions in the governors not from the cooling device itself. > > The cooling device is set to its maximum state and then updated. Even > if I don't get the purpose of probing the pwm-fan to its maximum > cooling state, we can replace the thermal_cdev_update() call to the > internal set_cur_state() function directly. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/hwmon/pwm-fan.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index 30b7b3ea8836..a654ecdf21ab 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev) > if (ret) > return ret; > > - ctx->pwm_fan_state = ctx->pwm_fan_max_state; > if (IS_ENABLED(CONFIG_THERMAL)) { > cdev = devm_thermal_of_cooling_device_register(dev, > dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops); > @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > return ret; > } > ctx->cdev = cdev; > - thermal_cdev_update(cdev); > + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state); So far the function would only change the state if the new state is not equal to the old state. This was the case because pwm_fan_state was set to pwm_fan_max_state, and the call to thermal_cdev_update() and thus pwm_fan_set_cur_state() would do nothing except update statistics. The old code _assumed_ that the current state is pwm_fan_max_state. The new code enforces it. That is a substantial semantic change, and it is not really reflected in the commit message. Is that really what you want ? If so, the commit message needs to state that and explain the rationale. Thanks, Guenter > } > > return 0; >
On 11/04/2020 03:32, Guenter Roeck wrote: > On 4/10/20 3:12 PM, Daniel Lezcano wrote: >> The function thermal_cdev_upadte is called from the throttling > > misspelled > >> functions in the governors not from the cooling device itself. >> >> The cooling device is set to its maximum state and then updated. Even >> if I don't get the purpose of probing the pwm-fan to its maximum >> cooling state, we can replace the thermal_cdev_update() call to the >> internal set_cur_state() function directly. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> --- >> drivers/hwmon/pwm-fan.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c >> index 30b7b3ea8836..a654ecdf21ab 100644 >> --- a/drivers/hwmon/pwm-fan.c >> +++ b/drivers/hwmon/pwm-fan.c >> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - ctx->pwm_fan_state = ctx->pwm_fan_max_state; >> if (IS_ENABLED(CONFIG_THERMAL)) { >> cdev = devm_thermal_of_cooling_device_register(dev, >> dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops); >> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev) >> return ret; >> } >> ctx->cdev = cdev; >> - thermal_cdev_update(cdev); >> + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state); > > So far the function would only change the state if the new > state is not equal to the old state. This was the case because > pwm_fan_state was set to pwm_fan_max_state, and the call to > thermal_cdev_update() and thus pwm_fan_set_cur_state() would > do nothing except update statistics. The old code _assumed_ > that the current state is pwm_fan_max_state. The new code > enforces it. That is a substantial semantic change, and it > is not really reflected in the commit message. Is that really > what you want ? If so, the commit message needs to state that > and explain the rationale. Well, to be honest I'm not getting the rational of calling thermal_cdev_update(cdev) right after devm_thermal_of_cooling_device_register() neither setting pwm_fan_state to pwm_fan_max_state. Do we have the guarantee there is at this point a thermal instance making the target state working when thermal_cdev_update is called? Are we sure a thermal_cdev_update(cdev) is actually right here?
On 4/11/20 9:45 AM, Daniel Lezcano wrote: > On 11/04/2020 03:32, Guenter Roeck wrote: >> On 4/10/20 3:12 PM, Daniel Lezcano wrote: >>> The function thermal_cdev_upadte is called from the throttling >> >> misspelled >> >>> functions in the governors not from the cooling device itself. >>> >>> The cooling device is set to its maximum state and then updated. Even >>> if I don't get the purpose of probing the pwm-fan to its maximum >>> cooling state, we can replace the thermal_cdev_update() call to the >>> internal set_cur_state() function directly. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> --- >>> drivers/hwmon/pwm-fan.c | 3 +-- >>> 1 file changed, 1 insertion(+), 2 deletions(-) >>> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c >>> index 30b7b3ea8836..a654ecdf21ab 100644 >>> --- a/drivers/hwmon/pwm-fan.c >>> +++ b/drivers/hwmon/pwm-fan.c >>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev) >>> if (ret) >>> return ret; >>> >>> - ctx->pwm_fan_state = ctx->pwm_fan_max_state; >>> if (IS_ENABLED(CONFIG_THERMAL)) { >>> cdev = devm_thermal_of_cooling_device_register(dev, >>> dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops); >>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev) >>> return ret; >>> } >>> ctx->cdev = cdev; >>> - thermal_cdev_update(cdev); >>> + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state); >> >> So far the function would only change the state if the new >> state is not equal to the old state. This was the case because >> pwm_fan_state was set to pwm_fan_max_state, and the call to >> thermal_cdev_update() and thus pwm_fan_set_cur_state() would >> do nothing except update statistics. The old code _assumed_ >> that the current state is pwm_fan_max_state. The new code >> enforces it. That is a substantial semantic change, and it >> is not really reflected in the commit message. Is that really >> what you want ? If so, the commit message needs to state that >> and explain the rationale. > > Well, to be honest I'm not getting the rational of calling > thermal_cdev_update(cdev) right after > devm_thermal_of_cooling_device_register() neither setting pwm_fan_state > to pwm_fan_max_state. > Good question. The author might know/recall. Maybe the idea was that thermal would update the state to a lower state shortly thereafter. > Do we have the guarantee there is at this point a thermal instance > making the target state working when thermal_cdev_update is called? > > Are we sure a thermal_cdev_update(cdev) is actually right here? > I don't know. I am not exactly familiar with thermal subsystem particulars. I do recall seeing similar code in other drivers, though. Either case, your patch does change functionality, and we should not do that without understanding its impact. Thanks Guenter
On 11/04/2020 19:26, Guenter Roeck wrote: > On 4/11/20 9:45 AM, Daniel Lezcano wrote: >> On 11/04/2020 03:32, Guenter Roeck wrote: >>> On 4/10/20 3:12 PM, Daniel Lezcano wrote: >>>> The function thermal_cdev_upadte is called from the throttling >>> >>> misspelled >>> >>>> functions in the governors not from the cooling device itself. >>>> >>>> The cooling device is set to its maximum state and then updated. Even >>>> if I don't get the purpose of probing the pwm-fan to its maximum >>>> cooling state, we can replace the thermal_cdev_update() call to the >>>> internal set_cur_state() function directly. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> --- >>>> drivers/hwmon/pwm-fan.c | 3 +-- >>>> 1 file changed, 1 insertion(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c >>>> index 30b7b3ea8836..a654ecdf21ab 100644 >>>> --- a/drivers/hwmon/pwm-fan.c >>>> +++ b/drivers/hwmon/pwm-fan.c >>>> @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev) >>>> if (ret) >>>> return ret; >>>> >>>> - ctx->pwm_fan_state = ctx->pwm_fan_max_state; >>>> if (IS_ENABLED(CONFIG_THERMAL)) { >>>> cdev = devm_thermal_of_cooling_device_register(dev, >>>> dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops); >>>> @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev) >>>> return ret; >>>> } >>>> ctx->cdev = cdev; >>>> - thermal_cdev_update(cdev); >>>> + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state); >>> >>> So far the function would only change the state if the new >>> state is not equal to the old state. This was the case because >>> pwm_fan_state was set to pwm_fan_max_state, and the call to >>> thermal_cdev_update() and thus pwm_fan_set_cur_state() would >>> do nothing except update statistics. The old code _assumed_ >>> that the current state is pwm_fan_max_state. The new code >>> enforces it. That is a substantial semantic change, and it >>> is not really reflected in the commit message. Is that really >>> what you want ? If so, the commit message needs to state that >>> and explain the rationale. >> >> Well, to be honest I'm not getting the rational of calling >> thermal_cdev_update(cdev) right after >> devm_thermal_of_cooling_device_register() neither setting pwm_fan_state >> to pwm_fan_max_state. >> > Good question. The author might know/recall. Maybe the idea was that > thermal would update the state to a lower state shortly thereafter. > >> Do we have the guarantee there is at this point a thermal instance >> making the target state working when thermal_cdev_update is called? >> >> Are we sure a thermal_cdev_update(cdev) is actually right here? >> > I don't know. I am not exactly familiar with thermal subsystem > particulars. I do recall seeing similar code in other drivers, though. This call is done only in the governors actually. > Either case, your patch does change functionality, and we should not > do that without understanding its impact. Right, so I've been hacking my board, added a pwm-fan and binded the thermal zone to it. As expected, the call to thermal_cdev_update() is not needed. ctx->pwm_fan_state = ctx->pwm_fan_max_state; intializes to a max value (in my case it is 3). Right after it calls thermal_cdev_update() which fails to find any instance active because we are at init time and then calls set_cur_state with the target state set to zero and passing through a stats usage for nothing. The ctx->pwm_fan_state is only used by the cooling device ops, so I don't see any reason why it is set to pwm_fan_max_state before the compilation condition. May be there is something subtle here. Lukasz ? Is there any reason why thermal_cdev_update() was called here ? IMO, this function is a governor thing and it must be removed from the cooling device.
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 30b7b3ea8836..a654ecdf21ab 100644 --- a/drivers/hwmon/pwm-fan.c +++ b/drivers/hwmon/pwm-fan.c @@ -372,7 +372,6 @@ static int pwm_fan_probe(struct platform_device *pdev) if (ret) return ret; - ctx->pwm_fan_state = ctx->pwm_fan_max_state; if (IS_ENABLED(CONFIG_THERMAL)) { cdev = devm_thermal_of_cooling_device_register(dev, dev->of_node, "pwm-fan", ctx, &pwm_fan_cooling_ops); @@ -384,7 +383,7 @@ static int pwm_fan_probe(struct platform_device *pdev) return ret; } ctx->cdev = cdev; - thermal_cdev_update(cdev); + pwm_fan_set_cur_state(cdev, ctx->pwm_fan_max_state); } return 0;
The function thermal_cdev_upadte is called from the throttling functions in the governors not from the cooling device itself. The cooling device is set to its maximum state and then updated. Even if I don't get the purpose of probing the pwm-fan to its maximum cooling state, we can replace the thermal_cdev_update() call to the internal set_cur_state() function directly. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/hwmon/pwm-fan.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)