| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0ikXJrKyHm-texq1ZnWSBqSdi-bi0NCRQ2jPTu8mJ8izA@mail.gmail.com>
Date: Mon, 26 Aug 2024 12:50:53 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: rafael@...nel.org, linux-pm@...r.kernel.org,
Jérémie Garcia <jgarcia@...libre.com>,
Alexandre Bailon <abailon@...libre.com>, Zhang Rui <rui.zhang@...el.com>,
Lukasz Luba <lukasz.luba@....com>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/2] thermal/core: Use thermal_zone_device_param
On Fri, Aug 23, 2024 at 5:43 PM Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
>
> The function thermal_zone_device_register_*() have now a significant
> number of parameters.
Which may or may not be regarded as a problem.
To me, there are two arguments for doing a change like this:
(a) A struct initialization is less error-prone than passing a long
list of arguments to a function. In the particular case of
thermal_zone_device_register_with_trips(), the last two arguments are
easy to mishandle because they are of the same type and similar
meaning.
(b) It gets rid of multiline function invocations that are hard to read.
> Simplify the parameters by extending the thermal_zone_device_param
> structure with the parameters usually used when registering the
> thermal zone.
>
> With that change we have a simpler function:
>
> thermal_zone_device_register()
>
> which can be reused in the different drivers and replace the
> duplicate thermal_zone_device_register_with_trips() and
> thermal_zone_device_register_tripless() functions.
I actually think that having a special helper for registering a
tripless zone is useful because it makes it super easy to find the
code paths doing that.
> Cc: Jérémie Garcia <jgarcia@...libre.com>
> Cc: Alexandre Bailon <abailon@...libre.com>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@...aro.org>
> ---
> drivers/thermal/thermal_core.c | 9 +++++++
> include/linux/thermal.h | 43 ++++++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index e6669aeda1ff..5869562caf9e 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1390,6 +1390,15 @@ int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp)
> }
> EXPORT_SYMBOL_GPL(thermal_zone_get_crit_temp);
>
> +struct thermal_zone_device *thermal_zone_device_register(const char *type,
> + const struct thermal_zone_params *tzp)
> +{
> + return thermal_zone_device_register_with_trips(type, tzp->trips, tzp->num_trips,
> + tzp->devdata, tzp->ops,
> + tzp, tzp->passive_delay,
> + tzp->polling_delay);
My basic concern with this approach is the copying of pointers that
may become invalid going forward to tzp.
Generally speaking, it is less than useful to hold on to copies of all
data that is only used during thermal zone registration (like a
pointer to the trips table for one example).
I would define a new struct type to hold all of the current
thermal_zone_device_register_with_trips() parameters:
struct thermal_zone_data {
const char *type,
const struct thermal_trip *trips;
int num_trips;
void *devdata;
const struct thermal_zone_device_ops *ops,
const struct thermal_zone_params *tzp;
unsigned int passive_delay;
unsigned int polling_delay;
};
and pass that to a wrapper function:
int thermal_zone_register(struct thermal_zone_data *tzdata)
{
return thermal_zone_device_register_with_trips(tzdata->type,
tzdata->trips, tzdata->num_trips,
tzdata->devdata, tzdata->ops, tzdata->tzp,
tzdata->passiva_delay, tzdata->polling_delay);
}
BTW, I don't think that the "device" part of the function name adds
any value, so I wouldn't use it.
A similar thing can be done for the tripless case:
struct thermal_tripless_zone_data {
const char *type,
void *devdata;
const struct thermal_zone_device_ops *ops,
bool no_hwmon;
};
int thermal_tripless_zone_register(thermal_tripless_zone_data *tzdata)
{
struct thermal_zone_params tzp = { .no_hwmon = tzdata->no_hwmon };
return thermal_zone_device_register_with_trips(tzdata->type, NULL, 0,
tzdata->devdata, tzdata->ops, &tzp,
0, 0);
}
And yes, I would do it so that the users of tripless thermal zones
don't need to use a full struct thermal_zone_params just in order to
pass the no_hwmon value.
> +}
> +
> /**
> * thermal_zone_device_register_with_trips() - register a new thermal zone device
> * @type: the thermal zone device type
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index b86ddca46b9e..1681b9ddd890 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -174,11 +174,45 @@ struct thermal_zone_params {
> * Used by thermal zone drivers.
> */
> int slope;
> +
> /*
> * @offset: offset of a linear temperature adjustment curve.
> * Used by thermal zone drivers (default 0).
> */
> int offset;
> +
> + /*
> + * @trips: a pointer to an array of thermal trips
> + */
> + const struct thermal_trip *trips;
> +
> + /*
> + * @num_trips: the number of trip points the thermal zone support
> + */
> + int num_trips;
> +
> + /*
> + * @devdata: private device data
> + */
> + void *devdata;
> +
> + /*
> + * @ops: standard thermal zone device callbacks
> + */
> + const struct thermal_zone_device_ops *ops;
> +
> + /*
> + * @passive_delay: number of milliseconds to wait between polls when
> + * performing passive cooling
> + */
> + unsigned int passive_delay;
> +
> + /*
> + * @polling_delay: number of milliseconds to wait between polls when checking
> + * whether trip points have been crossed (0 for interrupt
> + * driven systems)
> + */
> + unsigned int polling_delay;
> };
>
> /* Function declarations */
> @@ -218,6 +252,10 @@ void thermal_zone_set_trip_temp(struct thermal_zone_device *tz,
> int thermal_zone_get_crit_temp(struct thermal_zone_device *tz, int *temp);
>
> #ifdef CONFIG_THERMAL
> +
> +struct thermal_zone_device *thermal_zone_device_register(const char *type,
> + const struct thermal_zone_params *tzp);
> +
> struct thermal_zone_device *thermal_zone_device_register_with_trips(
> const char *type,
> const struct thermal_trip *trips,
> @@ -281,6 +319,11 @@ int thermal_zone_device_enable(struct thermal_zone_device *tz);
> int thermal_zone_device_disable(struct thermal_zone_device *tz);
> void thermal_zone_device_critical(struct thermal_zone_device *tz);
> #else
> +static inline struct thermal_zone_device *thermal_zone_device_register(
> + const char *type,
> + const struct thermal_zone_params *tzp)
> +{ return ERR_PTR(-ENODEV); }
> +
> static inline struct thermal_zone_device *thermal_zone_device_register_with_trips(
> const char *type,
> const struct thermal_trip *trips,
> --
> 2.43.0
>
Powered by blists - more mailing lists