[PATCH v3 0/3] Fixes for Tegra soctherm

[PATCH v3 0/3] Fixes for Tegra soctherm

[Wei Ni]

This series fixed some issues for Tegra soctherm

Main changes from v2:
1. add codes to parse sensor id to avoid registration
failure.

Main changes from v1:
1. Acked by Thierry Reding <treding@nvidia.com> for the patch
"thermal: tegra: fix memory allocation".
2. Print out the sensor name when register failed.
2. Remove patch "thermal: tegra: fix coverity defect"

Wei Ni (3):
  thermal: tegra: remove unnecessary warnings
  thermal: tegra: fix memory allocation
  thermal: tegra: parse sensor id before sensor register

 drivers/thermal/tegra/soctherm.c | 54 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

-- 
2.7.4

[PATCH v3 1/3] thermal: tegra: remove unnecessary warnings

[Wei Ni]

Convert warnings to info as not all platforms may
have all the thresholds and sensors enabled.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/thermal/tegra/soctherm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index ed28110a3535..55cc1f2f6a45 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -550,7 +550,7 @@ static int tegra_soctherm_set_hwtrips(struct device *dev,
 
 	ret = tz->ops->get_crit_temp(tz, &temperature);
 	if (ret) {
-		dev_warn(dev, "thermtrip: %s: missing critical temperature\n",
+		dev_info(dev, "thermtrip: %s: missing critical temperature\n",
 			 sg->name);
 		goto set_throttle;
 	}
@@ -569,7 +569,7 @@ static int tegra_soctherm_set_hwtrips(struct device *dev,
 set_throttle:
 	ret = get_hot_temp(tz, &trip, &temperature);
 	if (ret) {
-		dev_warn(dev, "throttrip: %s: missing hot temperature\n",
+		dev_info(dev, "throttrip: %s: missing hot temperature\n",
 			 sg->name);
 		return 0;
 	}
@@ -600,7 +600,7 @@ static int tegra_soctherm_set_hwtrips(struct device *dev,
 	}
 
 	if (i == THROTTLE_SIZE)
-		dev_warn(dev, "throttrip: %s: missing throttle cdev\n",
+		dev_info(dev, "throttrip: %s: missing throttle cdev\n",
 			 sg->name);
 
 	return 0;
-- 
2.7.4

[PATCH v3 2/3] thermal: tegra: fix memory allocation

[Wei Ni]

Fix memory allocation to store the pointers to
thermal_zone_device.

Signed-off-by: Wei Ni <wni@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>
---
 drivers/thermal/tegra/soctherm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 55cc1f2f6a45..375cadbc24cd 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1339,7 +1339,7 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
 	}
 
 	tegra->thermctl_tzs = devm_kcalloc(&pdev->dev,
-					   soc->num_ttgs, sizeof(*z),
+					   soc->num_ttgs, sizeof(z),
 					   GFP_KERNEL);
 	if (!tegra->thermctl_tzs)
 		return -ENOMEM;
-- 
2.7.4

[PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Wei Ni]

Since different platforms may not support all 4
sensors, so the sensor registration may be failed.
Add codes to parse dt to find sensor id which
need to be registered. So that the registration
can be successful on all platform.

Signed-off-by: Wei Ni <wni@nvidia.com>
---
 drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 375cadbc24cd..79e4628224d7 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
 	tegra_soctherm_throttle(&pdev->dev);
 }
 
+static bool tegra_soctherm_find_sensor_id(int sensor_id)
+{
+	int id;
+	bool ret = false;
+	struct of_phandle_args sensor_specs;
+	struct device_node *np, *sensor_np;
+
+	np = of_find_node_by_name(NULL, "thermal-zones");
+	if (!np)
+		return ret;
+
+	sensor_np = of_get_next_child(np, NULL);
+	for_each_available_child_of_node(np, sensor_np) {
+		if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
+						 "#thermal-sensor-cells",
+						 0, &sensor_specs))
+			continue;
+
+		if (sensor_specs.args_count != 1) {
+			WARN(sensor_specs.args_count > 1,
+			     "%s: wrong cells in sensor specifier %d\n",
+			     sensor_specs.np->name, sensor_specs.args_count);
+			continue;
+		} else {
+			id = sensor_specs.args[0];
+			if (sensor_id == id) {
+				ret = true;
+				break;
+			}
+		}
+	}
+
+	of_node_put(np);
+	of_node_put(sensor_np);
+
+	return ret;
+}
+
 static const struct of_device_id tegra_soctherm_of_match[] = {
 #ifdef CONFIG_ARCH_TEGRA_124_SOC
 	{
@@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
 		zone->sg = soc->ttgs[i];
 		zone->ts = tegra;
 
+		if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
+			continue;
 		z = devm_thermal_zone_of_sensor_register(&pdev->dev,
 							 soc->ttgs[i]->id, zone,
 							 &tegra_of_thermal_ops);
 		if (IS_ERR(z)) {
 			err = PTR_ERR(z);
-			dev_err(&pdev->dev, "failed to register sensor: %d\n",
-				err);
+			dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
+				soc->ttgs[i]->name, err);
 			goto disable_clocks;
 		}
 
@@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
 		struct thermal_zone_device *tz;
 
 		tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
+		if (!tz)
+			continue;
 		err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
 		if (err) {
 			dev_err(&pdev->dev,
-- 
2.7.4

Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Wei Ni]

Hi Daniel,
I updated my patch to parse the sensor id, please take a look.

Wei.

On 28/11/2018 1:44 PM, Wei Ni wrote:
> Since different platforms may not support all 4
> sensors, so the sensor registration may be failed.
> Add codes to parse dt to find sensor id which
> need to be registered. So that the registration
> can be successful on all platform.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 375cadbc24cd..79e4628224d7 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
>  	tegra_soctherm_throttle(&pdev->dev);
>  }
>  
> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
> +{
> +	int id;
> +	bool ret = false;
> +	struct of_phandle_args sensor_specs;
> +	struct device_node *np, *sensor_np;
> +
> +	np = of_find_node_by_name(NULL, "thermal-zones");
> +	if (!np)
> +		return ret;
> +
> +	sensor_np = of_get_next_child(np, NULL);
> +	for_each_available_child_of_node(np, sensor_np) {
> +		if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> +						 "#thermal-sensor-cells",
> +						 0, &sensor_specs))
> +			continue;
> +
> +		if (sensor_specs.args_count != 1) {
> +			WARN(sensor_specs.args_count > 1,
> +			     "%s: wrong cells in sensor specifier %d\n",
> +			     sensor_specs.np->name, sensor_specs.args_count);
> +			continue;
> +		} else {
> +			id = sensor_specs.args[0];
> +			if (sensor_id == id) {
> +				ret = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	of_node_put(np);
> +	of_node_put(sensor_np);
> +
> +	return ret;
> +}
> +
>  static const struct of_device_id tegra_soctherm_of_match[] = {
>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
>  	{
> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>  		zone->sg = soc->ttgs[i];
>  		zone->ts = tegra;
>  
> +		if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
> +			continue;
>  		z = devm_thermal_zone_of_sensor_register(&pdev->dev,
>  							 soc->ttgs[i]->id, zone,
>  							 &tegra_of_thermal_ops);
>  		if (IS_ERR(z)) {
>  			err = PTR_ERR(z);
> -			dev_err(&pdev->dev, "failed to register sensor: %d\n",
> -				err);
> +			dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
> +				soc->ttgs[i]->name, err);
>  			goto disable_clocks;
>  		}
>  
> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
>  		struct thermal_zone_device *tz;
>  
>  		tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
> +		if (!tz)
> +			continue;
>  		err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
>  		if (err) {
>  			dev_err(&pdev->dev,
> 

Re: [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On Wed, Nov 28, 2018 at 01:44:35PM +0800, Wei Ni wrote:
> Convert warnings to info as not all platforms may
> have all the thresholds and sensors enabled.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

If these are all optional anyway, why even keep the informational
messages? While those may not show up as warnings in the log, they will
still be unconditional noise in the kernel log. Do we really want that,
or should we just trust that the DT is correct and shut up if optional
thresholds and sensors are not present?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3656 bytes --]

On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> Since different platforms may not support all 4
> sensors, so the sensor registration may be failed.
> Add codes to parse dt to find sensor id which
> need to be registered. So that the registration
> can be successful on all platform.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 375cadbc24cd..79e4628224d7 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
>  	tegra_soctherm_throttle(&pdev->dev);
>  }
>  
> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
> +{
> +	int id;

You might want to make this and the sensor_id parameter unsigned int to
match the signedness of the ID in the specifier arguments and the sensor
groups.

Thierry

> +	bool ret = false;
> +	struct of_phandle_args sensor_specs;
> +	struct device_node *np, *sensor_np;
> +
> +	np = of_find_node_by_name(NULL, "thermal-zones");
> +	if (!np)
> +		return ret;
> +
> +	sensor_np = of_get_next_child(np, NULL);
> +	for_each_available_child_of_node(np, sensor_np) {

Aren't we leaking np here? I think we need of_node_put() after
of_get_next_child() to make sure the reference to the "thermal-zones"
node is properly released.

> +		if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
> +						 "#thermal-sensor-cells",
> +						 0, &sensor_specs))
> +			continue;
> +
> +		if (sensor_specs.args_count != 1) {
> +			WARN(sensor_specs.args_count > 1,
> +			     "%s: wrong cells in sensor specifier %d\n",
> +			     sensor_specs.np->name, sensor_specs.args_count);
> +			continue;

This is odd. You check for args_count != 1 but then WARN on args_count >
1. Shouldn't both of these conditions be the same?

> +		} else {

Also, since the above has "continue;", we don't really need the else
block.

> +			id = sensor_specs.args[0];
> +			if (sensor_id == id) {

It might not be worth to store the ID in a separate variable, we could
just do:

	if (sensor_specs.args[0] == sensor_id)

Thierry
> +				ret = true;
> +				break;
> +			}
> +		}
> +	}
> +
> +	of_node_put(np);
> +	of_node_put(sensor_np);
> +
> +	return ret;
> +}
> +
>  static const struct of_device_id tegra_soctherm_of_match[] = {
>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
>  	{
> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>  		zone->sg = soc->ttgs[i];
>  		zone->ts = tegra;
>  
> +		if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
> +			continue;
>  		z = devm_thermal_zone_of_sensor_register(&pdev->dev,

I'd would prefer a blank line after the if block for readability.

>  							 soc->ttgs[i]->id, zone,
>  							 &tegra_of_thermal_ops);
>  		if (IS_ERR(z)) {
>  			err = PTR_ERR(z);
> -			dev_err(&pdev->dev, "failed to register sensor: %d\n",
> -				err);
> +			dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
> +				soc->ttgs[i]->name, err);
>  			goto disable_clocks;
>  		}
>  
> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
>  		struct thermal_zone_device *tz;
>  
>  		tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
> +		if (!tz)
> +			continue;
>  		err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);

Same here:

		if (!tz)
			continue;

		err = ...

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Wei Ni]

On 28/11/2018 6:25 PM, Thierry Reding wrote:
> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
>> Since different platforms may not support all 4
>> sensors, so the sensor registration may be failed.
>> Add codes to parse dt to find sensor id which
>> need to be registered. So that the registration
>> can be successful on all platform.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/thermal/tegra/soctherm.c | 46 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 375cadbc24cd..79e4628224d7 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -1224,6 +1224,44 @@ static void soctherm_init(struct platform_device *pdev)
>>  	tegra_soctherm_throttle(&pdev->dev);
>>  }
>>  
>> +static bool tegra_soctherm_find_sensor_id(int sensor_id)
>> +{
>> +	int id;
> 
> You might want to make this and the sensor_id parameter unsigned int to
> match the signedness of the ID in the specifier arguments and the sensor
> groups.

Ok, will change it.

> 
> Thierry
> 
>> +	bool ret = false;
>> +	struct of_phandle_args sensor_specs;
>> +	struct device_node *np, *sensor_np;
>> +
>> +	np = of_find_node_by_name(NULL, "thermal-zones");
>> +	if (!np)
>> +		return ret;
>> +
>> +	sensor_np = of_get_next_child(np, NULL);
>> +	for_each_available_child_of_node(np, sensor_np) {
> 
> Aren't we leaking np here? I think we need of_node_put() after
> of_get_next_child() to make sure the reference to the "thermal-zones"
> node is properly released.

No, we will not leak np here. Because the
for_each_available_child_of_node will call
of_get_next_available_child(), which will call of_node_put(prev) to
decrease refcount of the prev node. So we just need to of_node_put the
last node after break from this for block.
> 
>> +		if (of_parse_phandle_with_args(sensor_np, "thermal-sensors",
>> +						 "#thermal-sensor-cells",
>> +						 0, &sensor_specs))
>> +			continue;
>> +
>> +		if (sensor_specs.args_count != 1) {
>> +			WARN(sensor_specs.args_count > 1,
>> +			     "%s: wrong cells in sensor specifier %d\n",
>> +			     sensor_specs.np->name, sensor_specs.args_count);
>> +			continue;
> 
> This is odd. You check for args_count != 1 but then WARN on args_count >
> 1. Shouldn't both of these conditions be the same?

Sorry, it's my mistake, will fix it.

> 
>> +		} else {
> 
> Also, since the above has "continue;", we don't really need the else
> block.

Will fix it.
> 
>> +			id = sensor_specs.args[0];
>> +			if (sensor_id == id) {
> 
> It might not be worth to store the ID in a separate variable, we could
> just do:
> 
> 	if (sensor_specs.args[0] == sensor_id)
> 
> Thierry

Sure, will fix it.

>> +				ret = true;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	of_node_put(np);
We decrease refcount of the last np.

>> +	of_node_put(sensor_np);
>> +
>> +	return ret;
>> +}
>> +
>>  static const struct of_device_id tegra_soctherm_of_match[] = {
>>  #ifdef CONFIG_ARCH_TEGRA_124_SOC
>>  	{
>> @@ -1365,13 +1403,15 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
>>  		zone->sg = soc->ttgs[i];
>>  		zone->ts = tegra;
>>  
>> +		if (!tegra_soctherm_find_sensor_id(soc->ttgs[i]->id))
>> +			continue;
>>  		z = devm_thermal_zone_of_sensor_register(&pdev->dev,
> 
> I'd would prefer a blank line after the if block for readability.

Sure, will update it.

> 
>>  							 soc->ttgs[i]->id, zone,
>>  							 &tegra_of_thermal_ops);
>>  		if (IS_ERR(z)) {
>>  			err = PTR_ERR(z);
>> -			dev_err(&pdev->dev, "failed to register sensor: %d\n",
>> -				err);
>> +			dev_err(&pdev->dev, "failed to register sensor %s: %d\n",
>> +				soc->ttgs[i]->name, err);
>>  			goto disable_clocks;
>>  		}
>>  
>> @@ -1434,6 +1474,8 @@ static int __maybe_unused soctherm_resume(struct device *dev)
>>  		struct thermal_zone_device *tz;
>>  
>>  		tz = tegra->thermctl_tzs[soc->ttgs[i]->id];
>> +		if (!tz)
>> +			continue;
>>  		err = tegra_soctherm_set_hwtrips(dev, soc->ttgs[i], tz);
> 
> Same here:
> 
> 		if (!tz)
> 			continue;
> 
> 		err = ...
> 
> Thierry

Will update it.

> 

Re: [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings

[Wei Ni]

On 28/11/2018 6:12 PM, Thierry Reding wrote:
> On Wed, Nov 28, 2018 at 01:44:35PM +0800, Wei Ni wrote:
>> Convert warnings to info as not all platforms may
>> have all the thresholds and sensors enabled.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/thermal/tegra/soctherm.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> If these are all optional anyway, why even keep the informational
> messages? While those may not show up as warnings in the log, they will
> still be unconditional noise in the kernel log. Do we really want that,
> or should we just trust that the DT is correct and shut up if optional
> thresholds and sensors are not present?
> 
> Thierry

I think we can trust the DT files, but these trip points are important,
it's better to notify user about these information. And user also can
change log level to control these messages.

Thanks.
Wei.
> 

Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1731 bytes --]

On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
> On 28/11/2018 6:25 PM, Thierry Reding wrote:
> > On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
[...]
> >> +	bool ret = false;
> >> +	struct of_phandle_args sensor_specs;
> >> +	struct device_node *np, *sensor_np;
> >> +
> >> +	np = of_find_node_by_name(NULL, "thermal-zones");
> >> +	if (!np)
> >> +		return ret;
> >> +
> >> +	sensor_np = of_get_next_child(np, NULL);
> >> +	for_each_available_child_of_node(np, sensor_np) {
> > 
> > Aren't we leaking np here? I think we need of_node_put() after
> > of_get_next_child() to make sure the reference to the "thermal-zones"
> > node is properly released.
> 
> No, we will not leak np here. Because the
> for_each_available_child_of_node will call
> of_get_next_available_child(), which will call of_node_put(prev) to
> decrease refcount of the prev node. So we just need to of_node_put the
> last node after break from this for block.

Okay, looks like I misinterpreted what you were doing there. I thought
that for_each_available_child_of_node() took the child as first argument
and the parent as second and therefore np would be overwritten by the
first assignment in the macro.

But looking at this more closely I think there's something else wrong
here. for_each_available_child_of_node() is defined as:

	for_each_available_child_of_node(parent, child)

so in the above, np will be the parent and sensor_np the child. Why do
you have to do

	sensor_np = of_get_next_child(np, NULL);

? That's already done as part of the loop in the macro, right? So does
that not mean we get two references and we leak the first one? Can the
above not simply been dropped?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register

[Wei Ni]

On 30/11/2018 1:13 AM, Thierry Reding wrote:
> On Thu, Nov 29, 2018 at 01:55:02PM +0800, Wei Ni wrote:
>> On 28/11/2018 6:25 PM, Thierry Reding wrote:
>>> On Wed, Nov 28, 2018 at 01:44:37PM +0800, Wei Ni wrote:
> [...]
>>>> +	bool ret = false;
>>>> +	struct of_phandle_args sensor_specs;
>>>> +	struct device_node *np, *sensor_np;
>>>> +
>>>> +	np = of_find_node_by_name(NULL, "thermal-zones");
>>>> +	if (!np)
>>>> +		return ret;
>>>> +
>>>> +	sensor_np = of_get_next_child(np, NULL);
>>>> +	for_each_available_child_of_node(np, sensor_np) {
>>>
>>> Aren't we leaking np here? I think we need of_node_put() after
>>> of_get_next_child() to make sure the reference to the "thermal-zones"
>>> node is properly released.
>>
>> No, we will not leak np here. Because the
>> for_each_available_child_of_node will call
>> of_get_next_available_child(), which will call of_node_put(prev) to
>> decrease refcount of the prev node. So we just need to of_node_put the
>> last node after break from this for block.
> 
> Okay, looks like I misinterpreted what you were doing there. I thought
> that for_each_available_child_of_node() took the child as first argument
> and the parent as second and therefore np would be overwritten by the
> first assignment in the macro.
> 
> But looking at this more closely I think there's something else wrong
> here. for_each_available_child_of_node() is defined as:
> 
> 	for_each_available_child_of_node(parent, child)
> 
> so in the above, np will be the parent and sensor_np the child. Why do
> you have to do
> 
> 	sensor_np = of_get_next_child(np, NULL);
> 
> ? That's already done as part of the loop in the macro, right? So does
> that not mean we get two references and we leak the first one? Can the
> above not simply been dropped?

It's so sorry, it's my mistake, we should remove this line, it was my
develop code, forgot to remove it.
Will fix it in next version.

Thanks.

> 
> Thierry
>