* [PATCH v3 0/3] Fixes for Tegra soctherm
@ 2018-11-28 5:44 Wei Ni
2018-11-28 5:44 ` [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings Wei Ni
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Wei Ni @ 2018-11-28 5:44 UTC (permalink / raw)
To: thierry.reding, daniel.lezcano, linux-tegra
Cc: rui.zhang, edubezval, srikars, linux-kernel, 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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings 2018-11-28 5:44 [PATCH v3 0/3] Fixes for Tegra soctherm Wei Ni @ 2018-11-28 5:44 ` Wei Ni 2018-11-28 10:12 ` Thierry Reding 2018-11-28 5:44 ` [PATCH v3 2/3] thermal: tegra: fix memory allocation Wei Ni 2018-11-28 5:44 ` [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register Wei Ni 2 siblings, 1 reply; 11+ messages in thread From: Wei Ni @ 2018-11-28 5:44 UTC (permalink / raw) To: thierry.reding, daniel.lezcano, linux-tegra Cc: rui.zhang, edubezval, srikars, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings 2018-11-28 5:44 ` [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings Wei Ni @ 2018-11-28 10:12 ` Thierry Reding 2018-11-29 6:43 ` Wei Ni 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2018-11-28 10:12 UTC (permalink / raw) To: Wei Ni Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings 2018-11-28 10:12 ` Thierry Reding @ 2018-11-29 6:43 ` Wei Ni 0 siblings, 0 replies; 11+ messages in thread From: Wei Ni @ 2018-11-29 6:43 UTC (permalink / raw) To: Thierry Reding Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/3] thermal: tegra: fix memory allocation 2018-11-28 5:44 [PATCH v3 0/3] Fixes for Tegra soctherm Wei Ni 2018-11-28 5:44 ` [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings Wei Ni @ 2018-11-28 5:44 ` Wei Ni 2018-11-28 5:44 ` [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register Wei Ni 2 siblings, 0 replies; 11+ messages in thread From: Wei Ni @ 2018-11-28 5:44 UTC (permalink / raw) To: thierry.reding, daniel.lezcano, linux-tegra Cc: rui.zhang, edubezval, srikars, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-28 5:44 [PATCH v3 0/3] Fixes for Tegra soctherm Wei Ni 2018-11-28 5:44 ` [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings Wei Ni 2018-11-28 5:44 ` [PATCH v3 2/3] thermal: tegra: fix memory allocation Wei Ni @ 2018-11-28 5:44 ` Wei Ni 2018-11-28 5:55 ` Wei Ni 2018-11-28 10:25 ` Thierry Reding 2 siblings, 2 replies; 11+ messages in thread From: Wei Ni @ 2018-11-28 5:44 UTC (permalink / raw) To: thierry.reding, daniel.lezcano, linux-tegra Cc: rui.zhang, edubezval, srikars, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-28 5:44 ` [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register Wei Ni @ 2018-11-28 5:55 ` Wei Ni 2018-11-28 10:25 ` Thierry Reding 1 sibling, 0 replies; 11+ messages in thread From: Wei Ni @ 2018-11-28 5:55 UTC (permalink / raw) To: thierry.reding, daniel.lezcano, linux-tegra Cc: rui.zhang, edubezval, srikars, linux-kernel 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, > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-28 5:44 ` [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register Wei Ni 2018-11-28 5:55 ` Wei Ni @ 2018-11-28 10:25 ` Thierry Reding 2018-11-29 5:55 ` Wei Ni 1 sibling, 1 reply; 11+ messages in thread From: Thierry Reding @ 2018-11-28 10:25 UTC (permalink / raw) To: Wei Ni Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-28 10:25 ` Thierry Reding @ 2018-11-29 5:55 ` Wei Ni 2018-11-29 17:13 ` Thierry Reding 0 siblings, 1 reply; 11+ messages in thread From: Wei Ni @ 2018-11-29 5:55 UTC (permalink / raw) To: Thierry Reding Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel 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. > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-29 5:55 ` Wei Ni @ 2018-11-29 17:13 ` Thierry Reding 2018-11-30 3:10 ` Wei Ni 0 siblings, 1 reply; 11+ messages in thread From: Thierry Reding @ 2018-11-29 17:13 UTC (permalink / raw) To: Wei Ni Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register 2018-11-29 17:13 ` Thierry Reding @ 2018-11-30 3:10 ` Wei Ni 0 siblings, 0 replies; 11+ messages in thread From: Wei Ni @ 2018-11-30 3:10 UTC (permalink / raw) To: Thierry Reding Cc: daniel.lezcano, linux-tegra, rui.zhang, edubezval, srikars, linux-kernel 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-11-30 3:10 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-28 5:44 [PATCH v3 0/3] Fixes for Tegra soctherm Wei Ni 2018-11-28 5:44 ` [PATCH v3 1/3] thermal: tegra: remove unnecessary warnings Wei Ni 2018-11-28 10:12 ` Thierry Reding 2018-11-29 6:43 ` Wei Ni 2018-11-28 5:44 ` [PATCH v3 2/3] thermal: tegra: fix memory allocation Wei Ni 2018-11-28 5:44 ` [PATCH v3 3/3] thermal: tegra: parse sensor id before sensor register Wei Ni 2018-11-28 5:55 ` Wei Ni 2018-11-28 10:25 ` Thierry Reding 2018-11-29 5:55 ` Wei Ni 2018-11-29 17:13 ` Thierry Reding 2018-11-30 3:10 ` Wei Ni
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).