diff mbox series

[v2,1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id

Message ID 20220818082316.2717095-1-daniel.lezcano@linaro.org
State Accepted
Commit 31fd4b9db13b1877b20426e79ac7fec606587872
Headers show
Series [v2,1/2] thermal/drivers/imx_sc: Rely on the platform data to get the resource id | expand

Commit Message

Daniel Lezcano Aug. 18, 2022, 8:23 a.m. UTC
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(-)

Comments

Daniel Lezcano Aug. 25, 2022, 10:28 p.m. UTC | #1
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 ?
Daniel Lezcano Sept. 15, 2022, 3:53 p.m. UTC | #2
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 ?
> 
> 
>
diff mbox series

Patch

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);