* [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id
@ 2022-08-18 8:23 Daniel Lezcano
2022-08-18 8:23 ` [PATCH v2 2/2] thermal/of: Remove the thermal_zone_of_get_sensor_id() function Daniel Lezcano
2022-08-25 22:28 ` [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
0 siblings, 2 replies; 4+ messages in thread
From: Daniel Lezcano @ 2022-08-18 8:23 UTC (permalink / raw)
To: daniel.lezcano
Cc: rafael, linux-pm, linux-kernel, Amit Kucheria, Zhang Rui,
Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Currently the imx_sc driver is reimplementing part of the thermal zone
parsing from the thermal OF tree code to get the sensor id associated
with a thermal zone sensor.
The driver platform specific code should know what sensor is present
and not rely on the thermal zone description to do a discovery. Well
that is arguable but all the other drivers have a per platform data
telling what sensor id to use.
The imx_sc thermal driver is the only one using a different
approach. Not invalid but forcing to keep a specific function
'thermal_zone_of_get_sensor_id()' to get the sensor id for a specific
thermal zone as the self-explanatory function tells and having device
tree code inside the driver.
The thermal OF code had a rework and remains now self-encapsulated
with a register/unregister functions and their 'devm' variants, except
for the function mentioned above.
After investigating, it appears the imx_sc sensor is defined in
arch/arm64/boot/dts/freescale/imx8qxp.dtsi:
which defines the cpu-thermal zone with the id: IMX_SC_R_SYSTEM
This dtsi is included by:
- imx8qxp-ai_ml.dts
- imx8qxp-colibri.dtsi
- imx8qxp-mek.dts
The two first ones do not define more thermal zones
The third one adds the pmic-thermal0 zone with id: IMX_SC_R_PMIC_0
The thermal OF code returns -ENODEV if the thermal zone registration
with a specific id fails because the description is not available in
the DT for such a sensor id. In this case we continue with the other
ids without bailing out with an error.
So we can build for the 'fsl,imx-sc-thermal' a compatible data, an
array of sensor ids containing IMX_SC_R_SYSTEM and IMX_SC_R_PMIC_0.
The latter won't be found but that will not result in an error but a
normal case where we continue the initialization with other ids.
Just to clarify, it is what the thermal framework does and what the
other drivers are expecting: when a registration fails with -ENODEV
this is not an error but a case where the description is not found in
the device tree, that be can the entire thermal zones description or a
specific thermal zone with an unknown id.
There is one small functional change but without impact. When there is
no 'thermal-zones' description the probe function was returning
'-ENODEV', now it returns zero. When a thermal zone fails to register
with an error different from '-ENODEV', the error is detected and
returned.
Change the code accordingly and remove the OF code from the driver.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
The changes apply on the linux-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next
I don't have the platform, so I was not able to test the changes.
Changelog:
v2:
- Clarified a bit more the changes description
- Fix use-after-free and add a couple of comments to reflect the
patch description
- Put back the error message in case there is an error with the
registration
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/thermal/imx_sc_thermal.c | 68 ++++++++++++++++----------------
1 file changed, 33 insertions(+), 35 deletions(-)
diff --git a/drivers/thermal/imx_sc_thermal.c b/drivers/thermal/imx_sc_thermal.c
index 10bfa6507eb4..bba5c730b855 100644
--- a/drivers/thermal/imx_sc_thermal.c
+++ b/drivers/thermal/imx_sc_thermal.c
@@ -76,59 +76,55 @@ static const struct thermal_zone_device_ops imx_sc_thermal_ops = {
static int imx_sc_thermal_probe(struct platform_device *pdev)
{
- struct device_node *np, *child, *sensor_np;
struct imx_sc_sensor *sensor;
- int ret;
+ const int *resource_id;
+ int i, ret;
ret = imx_scu_get_handle(&thermal_ipc_handle);
if (ret)
return ret;
- np = of_find_node_by_name(NULL, "thermal-zones");
- if (!np)
- return -ENODEV;
+ resource_id = of_device_get_match_data(&pdev->dev);
+ if (!resource_id)
+ return -EINVAL;
- sensor_np = of_node_get(pdev->dev.of_node);
+ for (i = 0; resource_id[i] > 0; i++) {
- for_each_available_child_of_node(np, child) {
sensor = devm_kzalloc(&pdev->dev, sizeof(*sensor), GFP_KERNEL);
- if (!sensor) {
- of_node_put(child);
- ret = -ENOMEM;
- goto put_node;
- }
+ if (!sensor)
+ return -ENOMEM;
- ret = thermal_zone_of_get_sensor_id(child,
- sensor_np,
- &sensor->resource_id);
- if (ret < 0) {
- dev_err(&pdev->dev,
- "failed to get valid sensor resource id: %d\n",
- ret);
- of_node_put(child);
- break;
- }
+ sensor->resource_id = resource_id[i];
- sensor->tzd = devm_thermal_of_zone_register(&pdev->dev,
- sensor->resource_id,
- sensor,
- &imx_sc_thermal_ops);
+ sensor->tzd = devm_thermal_of_zone_register(&pdev->dev, sensor->resource_id,
+ sensor, &imx_sc_thermal_ops);
if (IS_ERR(sensor->tzd)) {
- dev_err(&pdev->dev, "failed to register thermal zone\n");
+ /*
+ * Save the error value before freeing the
+ * sensor pointer, otherwise we endup with a
+ * use-after-free error
+ */
ret = PTR_ERR(sensor->tzd);
- of_node_put(child);
- break;
+
+ devm_kfree(&pdev->dev, sensor);
+
+ /*
+ * The thermal framework notifies us there is
+ * no thermal zone description for such a
+ * sensor id
+ */
+ if (ret == -ENODEV)
+ continue;
+
+ dev_err(&pdev->dev, "failed to register thermal zone\n");
+ return ret;
}
if (devm_thermal_add_hwmon_sysfs(sensor->tzd))
dev_warn(&pdev->dev, "failed to add hwmon sysfs attributes\n");
}
-put_node:
- of_node_put(sensor_np);
- of_node_put(np);
-
- return ret;
+ return 0;
}
static int imx_sc_thermal_remove(struct platform_device *pdev)
@@ -136,8 +132,10 @@ static int imx_sc_thermal_remove(struct platform_device *pdev)
return 0;
}
+static int imx_sc_sensors[] = { IMX_SC_R_SYSTEM, IMX_SC_R_PMIC_0, -1 };
+
static const struct of_device_id imx_sc_thermal_table[] = {
- { .compatible = "fsl,imx-sc-thermal", },
+ { .compatible = "fsl,imx-sc-thermal", .data = imx_sc_sensors },
{}
};
MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v2 2/2] thermal/of: Remove the thermal_zone_of_get_sensor_id() function
2022-08-18 8:23 [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
@ 2022-08-18 8:23 ` Daniel Lezcano
2022-08-25 22:28 ` [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
1 sibling, 0 replies; 4+ messages in thread
From: Daniel Lezcano @ 2022-08-18 8:23 UTC (permalink / raw)
To: daniel.lezcano; +Cc: rafael, linux-pm, linux-kernel, Amit Kucheria, Zhang Rui
The function thermal_zone_of_get_sensor_id() is no longer used
anywhere, remove it.
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
drivers/thermal/thermal_of.c | 44 ------------------------------------
include/linux/thermal.h | 10 --------
2 files changed, 54 deletions(-)
diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
index fd2fb84bf246..d4b6335ace15 100644
--- a/drivers/thermal/thermal_of.c
+++ b/drivers/thermal/thermal_of.c
@@ -130,50 +130,6 @@ static int of_thermal_get_crit_temp(struct thermal_zone_device *tz,
return -EINVAL;
}
-/**
- * thermal_zone_of_get_sensor_id - get sensor ID from a DT thermal zone
- * @tz_np: a valid thermal zone device node.
- * @sensor_np: a sensor node of a valid sensor device.
- * @id: the sensor ID returned if success.
- *
- * This function will get sensor ID from a given thermal zone node and
- * the sensor node must match the temperature provider @sensor_np.
- *
- * Return: 0 on success, proper error code otherwise.
- */
-
-int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
- struct device_node *sensor_np,
- u32 *id)
-{
- struct of_phandle_args sensor_specs;
- int ret;
-
- ret = of_parse_phandle_with_args(tz_np,
- "thermal-sensors",
- "#thermal-sensor-cells",
- 0,
- &sensor_specs);
- if (ret)
- return ret;
-
- if (sensor_specs.np != sensor_np) {
- of_node_put(sensor_specs.np);
- return -ENODEV;
- }
-
- if (sensor_specs.args_count > 1)
- pr_warn("%pOFn: too many cells in sensor specifier %d\n",
- sensor_specs.np, sensor_specs.args_count);
-
- *id = sensor_specs.args_count ? sensor_specs.args[0] : 0;
-
- of_node_put(sensor_specs.np);
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(thermal_zone_of_get_sensor_id);
-
/*** functions parsing device tree nodes ***/
static int of_find_trip_id(struct device_node *np, struct device_node *trip)
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 86c24ddd5985..a5a18351a898 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -310,9 +310,6 @@ void devm_thermal_of_zone_unregister(struct device *dev, struct thermal_zone_dev
void thermal_of_zone_unregister(struct thermal_zone_device *tz);
-int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
- struct device_node *sensor_np,
- u32 *id);
#else
static inline
struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
@@ -336,13 +333,6 @@ static inline void devm_thermal_of_zone_unregister(struct device *dev,
struct thermal_zone_device *tz)
{
}
-
-static inline int thermal_zone_of_get_sensor_id(struct device_node *tz_np,
- struct device_node *sensor_np,
- u32 *id)
-{
- return -ENOENT;
-}
#endif
#ifdef CONFIG_THERMAL
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id
2022-08-18 8:23 [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
2022-08-18 8:23 ` [PATCH v2 2/2] thermal/of: Remove the thermal_zone_of_get_sensor_id() function Daniel Lezcano
@ 2022-08-25 22:28 ` Daniel Lezcano
2022-09-15 15:53 ` Daniel Lezcano
1 sibling, 1 reply; 4+ messages in thread
From: Daniel Lezcano @ 2022-08-25 22:28 UTC (permalink / raw)
To: Shawn Guo, Sascha Hauer
Cc: rafael, linux-pm, linux-kernel, Amit Kucheria, Zhang Rui,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
On 18/08/2022 10:23, Daniel Lezcano wrote:
> Currently the imx_sc driver is reimplementing part of the thermal zone
> parsing from the thermal OF tree code to get the sensor id associated
> with a thermal zone sensor.
>
> The driver platform specific code should know what sensor is present
> and not rely on the thermal zone description to do a discovery. Well
> that is arguable but all the other drivers have a per platform data
> telling what sensor id to use.
>
> The imx_sc thermal driver is the only one using a different
> approach. Not invalid but forcing to keep a specific function
> 'thermal_zone_of_get_sensor_id()' to get the sensor id for a specific
> thermal zone as the self-explanatory function tells and having device
> tree code inside the driver.
>
> The thermal OF code had a rework and remains now self-encapsulated
> with a register/unregister functions and their 'devm' variants, except
> for the function mentioned above.
>
> After investigating, it appears the imx_sc sensor is defined in
> arch/arm64/boot/dts/freescale/imx8qxp.dtsi:
>
> which defines the cpu-thermal zone with the id: IMX_SC_R_SYSTEM
>
> This dtsi is included by:
> - imx8qxp-ai_ml.dts
> - imx8qxp-colibri.dtsi
> - imx8qxp-mek.dts
>
> The two first ones do not define more thermal zones
> The third one adds the pmic-thermal0 zone with id: IMX_SC_R_PMIC_0
>
> The thermal OF code returns -ENODEV if the thermal zone registration
> with a specific id fails because the description is not available in
> the DT for such a sensor id. In this case we continue with the other
> ids without bailing out with an error.
>
> So we can build for the 'fsl,imx-sc-thermal' a compatible data, an
> array of sensor ids containing IMX_SC_R_SYSTEM and IMX_SC_R_PMIC_0.
>
> The latter won't be found but that will not result in an error but a
> normal case where we continue the initialization with other ids.
>
> Just to clarify, it is what the thermal framework does and what the
> other drivers are expecting: when a registration fails with -ENODEV
> this is not an error but a case where the description is not found in
> the device tree, that be can the entire thermal zones description or a
> specific thermal zone with an unknown id.
>
> There is one small functional change but without impact. When there is
> no 'thermal-zones' description the probe function was returning
> '-ENODEV', now it returns zero. When a thermal zone fails to register
> with an error different from '-ENODEV', the error is detected and
> returned.
>
> Change the code accordingly and remove the OF code from the driver.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> The changes apply on the linux-next branch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next
>
> I don't have the platform, so I was not able to test the changes.
>
> Changelog:
> v2:
> - Clarified a bit more the changes description
>
> - Fix use-after-free and add a couple of comments to reflect the
> patch description
>
> - Put back the error message in case there is an error with the
> registration
>
Is there any comment on this change ?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id
2022-08-25 22:28 ` [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
@ 2022-09-15 15:53 ` Daniel Lezcano
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Lezcano @ 2022-09-15 15:53 UTC (permalink / raw)
To: Shawn Guo, Sascha Hauer
Cc: rafael, linux-pm, linux-kernel, Amit Kucheria, Zhang Rui,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
Shawn, Sascha,
if there is no comment on these two patches. I'll pick them up
Thanks
-- D.
On 26/08/2022 00:28, Daniel Lezcano wrote:
> On 18/08/2022 10:23, Daniel Lezcano wrote:
>> Currently the imx_sc driver is reimplementing part of the thermal zone
>> parsing from the thermal OF tree code to get the sensor id associated
>> with a thermal zone sensor.
>>
>> The driver platform specific code should know what sensor is present
>> and not rely on the thermal zone description to do a discovery. Well
>> that is arguable but all the other drivers have a per platform data
>> telling what sensor id to use.
>>
>> The imx_sc thermal driver is the only one using a different
>> approach. Not invalid but forcing to keep a specific function
>> 'thermal_zone_of_get_sensor_id()' to get the sensor id for a specific
>> thermal zone as the self-explanatory function tells and having device
>> tree code inside the driver.
>>
>> The thermal OF code had a rework and remains now self-encapsulated
>> with a register/unregister functions and their 'devm' variants, except
>> for the function mentioned above.
>>
>> After investigating, it appears the imx_sc sensor is defined in
>> arch/arm64/boot/dts/freescale/imx8qxp.dtsi:
>>
>> which defines the cpu-thermal zone with the id: IMX_SC_R_SYSTEM
>>
>> This dtsi is included by:
>> - imx8qxp-ai_ml.dts
>> - imx8qxp-colibri.dtsi
>> - imx8qxp-mek.dts
>>
>> The two first ones do not define more thermal zones
>> The third one adds the pmic-thermal0 zone with id: IMX_SC_R_PMIC_0
>>
>> The thermal OF code returns -ENODEV if the thermal zone registration
>> with a specific id fails because the description is not available in
>> the DT for such a sensor id. In this case we continue with the other
>> ids without bailing out with an error.
>>
>> So we can build for the 'fsl,imx-sc-thermal' a compatible data, an
>> array of sensor ids containing IMX_SC_R_SYSTEM and IMX_SC_R_PMIC_0.
>>
>> The latter won't be found but that will not result in an error but a
>> normal case where we continue the initialization with other ids.
>>
>> Just to clarify, it is what the thermal framework does and what the
>> other drivers are expecting: when a registration fails with -ENODEV
>> this is not an error but a case where the description is not found in
>> the device tree, that be can the entire thermal zones description or a
>> specific thermal zone with an unknown id.
>>
>> There is one small functional change but without impact. When there is
>> no 'thermal-zones' description the probe function was returning
>> '-ENODEV', now it returns zero. When a thermal zone fails to register
>> with an error different from '-ENODEV', the error is detected and
>> returned.
>>
>> Change the code accordingly and remove the OF code from the driver.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>> The changes apply on the linux-next branch:
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next
>>
>>
>> I don't have the platform, so I was not able to test the changes.
>>
>> Changelog:
>> v2:
>> - Clarified a bit more the changes description
>>
>> - Fix use-after-free and add a couple of comments to reflect the
>> patch description
>>
>> - Put back the error message in case there is an error with the
>> registration
>>
>
> Is there any comment on this change ?
>
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-15 15:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-18 8:23 [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
2022-08-18 8:23 ` [PATCH v2 2/2] thermal/of: Remove the thermal_zone_of_get_sensor_id() function Daniel Lezcano
2022-08-25 22:28 ` [PATCH v2 1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id Daniel Lezcano
2022-09-15 15:53 ` Daniel Lezcano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).