| Message ID | 20200528192051.28034-3-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/11] acpi: thermal: Fix error handling in the register function | expand |
On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote: > Prepare for storing mode in struct thermal_zone_device. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/acpi/thermal.c | 27 +++++++++---------- > drivers/platform/x86/acerhdf.c | 8 ++++-- > .../intel/int340x_thermal/int3400_thermal.c | 18 +++++-------- > 3 files changed, 25 insertions(+), 28 deletions(-) > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 6de8066ca1e7..fb46070c66d8 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -172,7 +172,7 @@ struct acpi_thermal { > struct acpi_thermal_trips trips; > struct acpi_handle_list devices; > struct thermal_zone_device *thermal_zone; > - int tz_enabled; > + enum thermal_device_mode mode; > int kelvin_offset; /* in millidegrees */ > struct work_struct thermal_check_work; > }; > @@ -500,7 +500,7 @@ static void acpi_thermal_check(void *data) > { > struct acpi_thermal *tz = data; > > - if (!tz->tz_enabled) > + if (tz->mode != THERMAL_DEVICE_ENABLED) > return; > > thermal_zone_device_update(tz->thermal_zone, > @@ -534,8 +534,7 @@ static int thermal_get_mode(struct thermal_zone_device *thermal, > if (!tz) > return -EINVAL; > > - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : > - THERMAL_DEVICE_DISABLED; > + *mode = tz->mode; > > return 0; > } > @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct acpi_thermal *tz = thermal->devdata; > - int enable; > > if (!tz) > return -EINVAL; > > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; Personally I find this check unnecessary: The enum has no other values, and it is verifyable that the callers provide the enum and not some other value. > /* > * enable/disable thermal management from ACPI thermal driver > */ > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = 1; > - else if (mode == THERMAL_DEVICE_DISABLED) { > - enable = 0; > + if (mode == THERMAL_DEVICE_DISABLED) > pr_warn("thermal zone will be disabled\n"); > - } else > - return -EINVAL; > > - if (enable != tz->tz_enabled) { > - tz->tz_enabled = enable; > + if (mode != tz->mode) { > + tz->mode = mode; > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "%s kernel ACPI thermal control\n", > - tz->tz_enabled ? "Enable" : "Disable")); > + tz->mode == THERMAL_DEVICE_ENABLED ? > + "Enable" : "Disable")); > acpi_thermal_check(tz); > } > return 0; > @@ -915,7 +912,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > goto remove_dev_link; > } > > - tz->tz_enabled = 1; > + tz->mode = THERMAL_DEVICE_ENABLED; > > dev_info(&tz->device->dev, "registered as thermal_zone%d\n", > tz->thermal_zone->id); > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c > index 8cc86f4e3ac1..830a8b060e74 100644 > --- a/drivers/platform/x86/acerhdf.c > +++ b/drivers/platform/x86/acerhdf.c > @@ -68,6 +68,7 @@ static int kernelmode = 1; > #else > static int kernelmode; > #endif > +static enum thermal_device_mode thermal_mode; > > static unsigned int interval = 10; > static unsigned int fanon = 60000; > @@ -397,6 +398,7 @@ static inline void acerhdf_revert_to_bios_mode(void) > { > acerhdf_change_fanstate(ACERHDF_FAN_AUTO); > kernelmode = 0; > + thermal_mode = THERMAL_DEVICE_DISABLED; > if (thz_dev) > thz_dev->polling_delay = 0; > pr_notice("kernel mode fan control OFF\n"); > @@ -404,6 +406,7 @@ static inline void acerhdf_revert_to_bios_mode(void) > static inline void acerhdf_enable_kernelmode(void) > { > kernelmode = 1; > + thermal_mode = THERMAL_DEVICE_ENABLED; > > thz_dev->polling_delay = interval*1000; > thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); > @@ -416,8 +419,7 @@ static int acerhdf_get_mode(struct thermal_zone_device *thermal, > if (verbose) > pr_notice("kernel mode fan control %d\n", kernelmode); > > - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED > - : THERMAL_DEVICE_DISABLED; > + *mode = thermal_mode; > > return 0; > } > @@ -739,6 +741,8 @@ static int __init acerhdf_register_thermal(void) > if (IS_ERR(cl_dev)) > return -EINVAL; > > + thermal_mode = kernelmode ? > + THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; > thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, > &acerhdf_dev_ops, > &acerhdf_zone_params, 0, > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > index 0b3a62655843..e84faaadff87 100644 > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > @@ -48,7 +48,7 @@ struct int3400_thermal_priv { > struct acpi_device *adev; > struct platform_device *pdev; > struct thermal_zone_device *thermal; > - int mode; > + enum thermal_device_mode mode; > int art_count; > struct art *arts; > int trt_count; > @@ -395,24 +395,20 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct int3400_thermal_priv *priv = thermal->devdata; > - bool enable; > int result = 0; > > if (!priv) > return -EINVAL; > > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = true; > - else if (mode == THERMAL_DEVICE_DISABLED) > - enable = false; > - else > + if (mode != THERMAL_DEVICE_ENABLED && > + mode != THERMAL_DEVICE_DISABLED) > return -EINVAL; Same as above. > > - if (enable != priv->mode) { > - priv->mode = enable; > + if (mode != priv->mode) { > + priv->mode = mode; > result = int3400_thermal_run_osc(priv->adev->handle, > - priv->current_uuid_index, > - enable); > + priv->current_uuid_index, > + mode == THERMAL_DEVICE_ENABLED); > } > > evaluate_odvp(priv);
Hi Guenter, W dniu 29.05.2020 o 16:48, Guenter Roeck pisze: > On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote: >> Prepare for storing mode in struct thermal_zone_device. >> >> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >> --- >> drivers/acpi/thermal.c | 27 +++++++++---------- >> drivers/platform/x86/acerhdf.c | 8 ++++-- >> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++-------- >> 3 files changed, 25 insertions(+), 28 deletions(-) <snip> >> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal, >> enum thermal_device_mode mode) >> { >> struct acpi_thermal *tz = thermal->devdata; >> - int enable; >> >> if (!tz) >> return -EINVAL; >> >> + if (mode != THERMAL_DEVICE_DISABLED && >> + mode != THERMAL_DEVICE_ENABLED) >> + return -EINVAL; > > Personally I find this check unnecessary: The enum has no other values, > and it is verifyable that the callers provide the enum and not some other > value. It is getting removed in PATCH 10/11. >> + if (mode != THERMAL_DEVICE_ENABLED && >> + mode != THERMAL_DEVICE_DISABLED) >> return -EINVAL; > > Same as above. ditto.
On 5/29/20 8:13 AM, Andrzej Pietrasiewicz wrote: > Hi Guenter, > > W dniu 29.05.2020 o 16:48, Guenter Roeck pisze: >> On Thu, May 28, 2020 at 09:20:42PM +0200, Andrzej Pietrasiewicz wrote: >>> Prepare for storing mode in struct thermal_zone_device. >>> >>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >>> --- >>> drivers/acpi/thermal.c | 27 +++++++++---------- >>> drivers/platform/x86/acerhdf.c | 8 ++++-- >>> .../intel/int340x_thermal/int3400_thermal.c | 18 +++++-------- >>> 3 files changed, 25 insertions(+), 28 deletions(-) > > <snip> > >>> @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal, >>> enum thermal_device_mode mode) >>> { >>> struct acpi_thermal *tz = thermal->devdata; >>> - int enable; >>> if (!tz) >>> return -EINVAL; >>> + if (mode != THERMAL_DEVICE_DISABLED && >>> + mode != THERMAL_DEVICE_ENABLED) >>> + return -EINVAL; >> >> Personally I find this check unnecessary: The enum has no other values, >> and it is verifyable that the callers provide the enum and not some other >> value. > > It is getting removed in PATCH 10/11. > > >>> + if (mode != THERMAL_DEVICE_ENABLED && >>> + mode != THERMAL_DEVICE_DISABLED) >>> return -EINVAL; >> >> Same as above. > > ditto. Hmm, I think that would be better done with this patch. But I guess that is a bit of PoV, so Reviewed-by: Guenter Roeck <linux@roeck-us.net> since I don't have any other objections/observations. Guenter
28. Mai 2020 21:21, "Andrzej Pietrasiewicz" <andrzej.p@collabora.com> schrieb: > Prepare for storing mode in struct thermal_zone_device. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- [...] > drivers/platform/x86/acerhdf.c | 8 ++++-- Acked-by: Peter Kaestle <peter@piie.net>
On 5/28/20 9:20 PM, Andrzej Pietrasiewicz wrote: > Prepare for storing mode in struct thermal_zone_device. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > --- > drivers/acpi/thermal.c | 27 +++++++++---------- > drivers/platform/x86/acerhdf.c | 8 ++++-- > .../intel/int340x_thermal/int3400_thermal.c | 18 +++++-------- > 3 files changed, 25 insertions(+), 28 deletions(-) > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 6de8066ca1e7..fb46070c66d8 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -172,7 +172,7 @@ struct acpi_thermal { > struct acpi_thermal_trips trips; > struct acpi_handle_list devices; > struct thermal_zone_device *thermal_zone; > - int tz_enabled; > + enum thermal_device_mode mode; > int kelvin_offset; /* in millidegrees */ > struct work_struct thermal_check_work; > }; > @@ -500,7 +500,7 @@ static void acpi_thermal_check(void *data) > { > struct acpi_thermal *tz = data; > > - if (!tz->tz_enabled) > + if (tz->mode != THERMAL_DEVICE_ENABLED) > return; > > thermal_zone_device_update(tz->thermal_zone, > @@ -534,8 +534,7 @@ static int thermal_get_mode(struct thermal_zone_device *thermal, > if (!tz) > return -EINVAL; > > - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : > - THERMAL_DEVICE_DISABLED; > + *mode = tz->mode; > > return 0; > } > @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct acpi_thermal *tz = thermal->devdata; > - int enable; > > if (!tz) > return -EINVAL; > > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; > /* > * enable/disable thermal management from ACPI thermal driver > */ > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = 1; > - else if (mode == THERMAL_DEVICE_DISABLED) { > - enable = 0; > + if (mode == THERMAL_DEVICE_DISABLED) > pr_warn("thermal zone will be disabled\n"); > - } else > - return -EINVAL; > > - if (enable != tz->tz_enabled) { > - tz->tz_enabled = enable; > + if (mode != tz->mode) { > + tz->mode = mode; > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "%s kernel ACPI thermal control\n", > - tz->tz_enabled ? "Enable" : "Disable")); > + tz->mode == THERMAL_DEVICE_ENABLED ? > + "Enable" : "Disable")); > acpi_thermal_check(tz); > } > return 0; > @@ -915,7 +912,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > goto remove_dev_link; > } > > - tz->tz_enabled = 1; > + tz->mode = THERMAL_DEVICE_ENABLED; > > dev_info(&tz->device->dev, "registered as thermal_zone%d\n", > tz->thermal_zone->id); > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c > index 8cc86f4e3ac1..830a8b060e74 100644 > --- a/drivers/platform/x86/acerhdf.c > +++ b/drivers/platform/x86/acerhdf.c > @@ -68,6 +68,7 @@ static int kernelmode = 1; > #else > static int kernelmode; > #endif > +static enum thermal_device_mode thermal_mode; > > static unsigned int interval = 10; > static unsigned int fanon = 60000; > @@ -397,6 +398,7 @@ static inline void acerhdf_revert_to_bios_mode(void) > { > acerhdf_change_fanstate(ACERHDF_FAN_AUTO); > kernelmode = 0; > + thermal_mode = THERMAL_DEVICE_DISABLED; > if (thz_dev) > thz_dev->polling_delay = 0; > pr_notice("kernel mode fan control OFF\n"); > @@ -404,6 +406,7 @@ static inline void acerhdf_revert_to_bios_mode(void) > static inline void acerhdf_enable_kernelmode(void) > { > kernelmode = 1; > + thermal_mode = THERMAL_DEVICE_ENABLED; > > thz_dev->polling_delay = interval*1000; > thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); > @@ -416,8 +419,7 @@ static int acerhdf_get_mode(struct thermal_zone_device *thermal, > if (verbose) > pr_notice("kernel mode fan control %d\n", kernelmode); > > - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED > - : THERMAL_DEVICE_DISABLED; > + *mode = thermal_mode; > > return 0; > } > @@ -739,6 +741,8 @@ static int __init acerhdf_register_thermal(void) > if (IS_ERR(cl_dev)) > return -EINVAL; > > + thermal_mode = kernelmode ? > + THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; > thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, > &acerhdf_dev_ops, > &acerhdf_zone_params, 0, > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > index 0b3a62655843..e84faaadff87 100644 > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > @@ -48,7 +48,7 @@ struct int3400_thermal_priv { > struct acpi_device *adev; > struct platform_device *pdev; > struct thermal_zone_device *thermal; > - int mode; > + enum thermal_device_mode mode; > int art_count; > struct art *arts; > int trt_count; > @@ -395,24 +395,20 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct int3400_thermal_priv *priv = thermal->devdata; > - bool enable; > int result = 0; > > if (!priv) > return -EINVAL; > > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = true; > - else if (mode == THERMAL_DEVICE_DISABLED) > - enable = false; > - else > + if (mode != THERMAL_DEVICE_ENABLED && > + mode != THERMAL_DEVICE_DISABLED) > return -EINVAL; > > - if (enable != priv->mode) { > - priv->mode = enable; > + if (mode != priv->mode) { > + priv->mode = mode; > result = int3400_thermal_run_osc(priv->adev->handle, > - priv->current_uuid_index, > - enable); > + priv->current_uuid_index, > + mode == THERMAL_DEVICE_ENABLED); > } > > evaluate_odvp(priv); >
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 6de8066ca1e7..fb46070c66d8 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -172,7 +172,7 @@ struct acpi_thermal { struct acpi_thermal_trips trips; struct acpi_handle_list devices; struct thermal_zone_device *thermal_zone; - int tz_enabled; + enum thermal_device_mode mode; int kelvin_offset; /* in millidegrees */ struct work_struct thermal_check_work; }; @@ -500,7 +500,7 @@ static void acpi_thermal_check(void *data) { struct acpi_thermal *tz = data; - if (!tz->tz_enabled) + if (tz->mode != THERMAL_DEVICE_ENABLED) return; thermal_zone_device_update(tz->thermal_zone, @@ -534,8 +534,7 @@ static int thermal_get_mode(struct thermal_zone_device *thermal, if (!tz) return -EINVAL; - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : - THERMAL_DEVICE_DISABLED; + *mode = tz->mode; return 0; } @@ -544,27 +543,25 @@ static int thermal_set_mode(struct thermal_zone_device *thermal, enum thermal_device_mode mode) { struct acpi_thermal *tz = thermal->devdata; - int enable; if (!tz) return -EINVAL; + if (mode != THERMAL_DEVICE_DISABLED && + mode != THERMAL_DEVICE_ENABLED) + return -EINVAL; /* * enable/disable thermal management from ACPI thermal driver */ - if (mode == THERMAL_DEVICE_ENABLED) - enable = 1; - else if (mode == THERMAL_DEVICE_DISABLED) { - enable = 0; + if (mode == THERMAL_DEVICE_DISABLED) pr_warn("thermal zone will be disabled\n"); - } else - return -EINVAL; - if (enable != tz->tz_enabled) { - tz->tz_enabled = enable; + if (mode != tz->mode) { + tz->mode = mode; ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%s kernel ACPI thermal control\n", - tz->tz_enabled ? "Enable" : "Disable")); + tz->mode == THERMAL_DEVICE_ENABLED ? + "Enable" : "Disable")); acpi_thermal_check(tz); } return 0; @@ -915,7 +912,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) goto remove_dev_link; } - tz->tz_enabled = 1; + tz->mode = THERMAL_DEVICE_ENABLED; dev_info(&tz->device->dev, "registered as thermal_zone%d\n", tz->thermal_zone->id); diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c index 8cc86f4e3ac1..830a8b060e74 100644 --- a/drivers/platform/x86/acerhdf.c +++ b/drivers/platform/x86/acerhdf.c @@ -68,6 +68,7 @@ static int kernelmode = 1; #else static int kernelmode; #endif +static enum thermal_device_mode thermal_mode; static unsigned int interval = 10; static unsigned int fanon = 60000; @@ -397,6 +398,7 @@ static inline void acerhdf_revert_to_bios_mode(void) { acerhdf_change_fanstate(ACERHDF_FAN_AUTO); kernelmode = 0; + thermal_mode = THERMAL_DEVICE_DISABLED; if (thz_dev) thz_dev->polling_delay = 0; pr_notice("kernel mode fan control OFF\n"); @@ -404,6 +406,7 @@ static inline void acerhdf_revert_to_bios_mode(void) static inline void acerhdf_enable_kernelmode(void) { kernelmode = 1; + thermal_mode = THERMAL_DEVICE_ENABLED; thz_dev->polling_delay = interval*1000; thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); @@ -416,8 +419,7 @@ static int acerhdf_get_mode(struct thermal_zone_device *thermal, if (verbose) pr_notice("kernel mode fan control %d\n", kernelmode); - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED - : THERMAL_DEVICE_DISABLED; + *mode = thermal_mode; return 0; } @@ -739,6 +741,8 @@ static int __init acerhdf_register_thermal(void) if (IS_ERR(cl_dev)) return -EINVAL; + thermal_mode = kernelmode ? + THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, &acerhdf_dev_ops, &acerhdf_zone_params, 0, diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 0b3a62655843..e84faaadff87 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -48,7 +48,7 @@ struct int3400_thermal_priv { struct acpi_device *adev; struct platform_device *pdev; struct thermal_zone_device *thermal; - int mode; + enum thermal_device_mode mode; int art_count; struct art *arts; int trt_count; @@ -395,24 +395,20 @@ static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, enum thermal_device_mode mode) { struct int3400_thermal_priv *priv = thermal->devdata; - bool enable; int result = 0; if (!priv) return -EINVAL; - if (mode == THERMAL_DEVICE_ENABLED) - enable = true; - else if (mode == THERMAL_DEVICE_DISABLED) - enable = false; - else + if (mode != THERMAL_DEVICE_ENABLED && + mode != THERMAL_DEVICE_DISABLED) return -EINVAL; - if (enable != priv->mode) { - priv->mode = enable; + if (mode != priv->mode) { + priv->mode = mode; result = int3400_thermal_run_osc(priv->adev->handle, - priv->current_uuid_index, - enable); + priv->current_uuid_index, + mode == THERMAL_DEVICE_ENABLED); } evaluate_odvp(priv);
Prepare for storing mode in struct thermal_zone_device. Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> --- drivers/acpi/thermal.c | 27 +++++++++---------- drivers/platform/x86/acerhdf.c | 8 ++++-- .../intel/int340x_thermal/int3400_thermal.c | 18 +++++-------- 3 files changed, 25 insertions(+), 28 deletions(-)