[PATCH v1 0/4] Fixes for Tegra soctherm

[PATCH v1 0/4] Fixes for Tegra soctherm

[Wei Ni]

This series fixed some issues for Tegra soctherm

Wei Ni (4):
  thermal: tegra: continue if sensor register fails
  thermal: tegra: remove unnecessary warnings
  thermal: tegra: fix memory allocation
  thermal: tegra: fix coverity defect

 drivers/thermal/tegra/soctherm.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v1 0/4] Fixes for Tegra soctherm

[Wei Ni]

This series fixed some issues for Tegra soctherm

Wei Ni (4):
  thermal: tegra: continue if sensor register fails
  thermal: tegra: remove unnecessary warnings
  thermal: tegra: fix memory allocation
  thermal: tegra: fix coverity defect

 drivers/thermal/tegra/soctherm.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] thermal: tegra: continue if sensor register fails

[Wei Ni]

Don't bail when a sensor fails to register with the
thermal zone and allow other sensors to register.
This allows other sensors to register with thermal
framework even if one sensor fails registration.

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

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index ed28110a3535..b708300303ff 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1370,9 +1370,9 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
 							 &tegra_of_thermal_ops);
 		if (IS_ERR(z)) {
 			err = PTR_ERR(z);
-			dev_err(&pdev->dev, "failed to register sensor: %d\n",
+			dev_warn(&pdev->dev, "failed to register sensor: %d\n",
 				err);
-			goto disable_clocks;
+			continue;
 		}
 
 		zone->tz = z;
@@ -1434,6 +1434,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

[PATCH v1 1/4] thermal: tegra: continue if sensor register fails

[Wei Ni]

Don't bail when a sensor fails to register with the
thermal zone and allow other sensors to register.
This allows other sensors to register with thermal
framework even if one sensor fails registration.

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

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index ed28110a3535..b708300303ff 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -1370,9 +1370,9 @@ static int tegra_soctherm_probe(struct platform_device *pdev)
 							 &tegra_of_thermal_ops);
 		if (IS_ERR(z)) {
 			err = PTR_ERR(z);
-			dev_err(&pdev->dev, "failed to register sensor: %d\n",
+			dev_warn(&pdev->dev, "failed to register sensor: %d\n",
 				err);
-			goto disable_clocks;
+			continue;
 		}
 
 		zone->tz = z;
@@ -1434,6 +1434,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 v1 1/4] thermal: tegra: continue if sensor register fails

[Thierry Reding]

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

On Mon, Nov 05, 2018 at 05:32:31PM +0800, Wei Ni wrote:
> Don't bail when a sensor fails to register with the
> thermal zone and allow other sensors to register.
> This allows other sensors to register with thermal
> framework even if one sensor fails registration.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

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

[PATCH v1 2/4] 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 b708300303ff..39a8bda07ac4 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 v1 2/4] 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 b708300303ff..39a8bda07ac4 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

Re: [PATCH v1 2/4] thermal: tegra: remove unnecessary warnings

[Thierry Reding]

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

On Mon, Nov 05, 2018 at 05:32:32PM +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(-)

This seems overly generalized to me. Shouldn't we be checking in a more
fine-grained way for the absence of thresholds and/or sensors?

Otherwise, how are going to make the difference between the sensor not
being enabled or the device tree just missing the information?

Thierry

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

Re: [PATCH v1 2/4] thermal: tegra: remove unnecessary warnings

[Wei Ni]

On 8/11/2018 8:47 PM, Thierry Reding wrote:
> On Mon, Nov 05, 2018 at 05:32:32PM +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(-)
> 
> This seems overly generalized to me. Shouldn't we be checking in a more
> fine-grained way for the absence of thresholds and/or sensors?
> 
> Otherwise, how are going to make the difference between the sensor not
> being enabled or the device tree just missing the information?
> 
The sensor being enabled or not is controlled by device tree, if the dts
have the corresponding nodes, then the sensors should be enabled. And
the thresholds for sensor are not necessary, so I think we just need to
print out them.
BTW, in my patch 1/4, I should print out the sensor name if the sensor
not enabled and register failed.
Will update it.

> Thierry
> 

Re: [PATCH v1 2/4] thermal: tegra: remove unnecessary warnings

[Wei Ni]

On 8/11/2018 8:47 PM, Thierry Reding wrote:
> On Mon, Nov 05, 2018 at 05:32:32PM +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(-)
> 
> This seems overly generalized to me. Shouldn't we be checking in a more
> fine-grained way for the absence of thresholds and/or sensors?
> 
> Otherwise, how are going to make the difference between the sensor not
> being enabled or the device tree just missing the information?
> 
The sensor being enabled or not is controlled by device tree, if the dts
have the corresponding nodes, then the sensors should be enabled. And
the thresholds for sensor are not necessary, so I think we just need to
print out them.
BTW, in my patch 1/4, I should print out the sensor name if the sensor
not enabled and register failed.
Will update it.

> Thierry
> 

[PATCH v1 3/4] 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>
---
 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 39a8bda07ac4..3042837364e8 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 v1 3/4] 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>
---
 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 39a8bda07ac4..3042837364e8 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

Re: [PATCH v1 3/4] thermal: tegra: fix memory allocation

[Thierry Reding]

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

On Mon, Nov 05, 2018 at 05:32:33PM +0800, Wei Ni wrote:
> Fix memory allocation to store the pointers to
> thermal_zone_device.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Thierry Reding <treding@nvidia.com>

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

[PATCH v1 4/4] thermal: tegra: fix coverity defect

[Wei Ni]

Fix dereference dev before null check.

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

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 3042837364e8..96527df91f2a 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -397,7 +397,7 @@ static int throttrip_program(struct device *dev,
 			     struct soctherm_throt_cfg *stc,
 			     int trip_temp)
 {
-	struct tegra_soctherm *ts = dev_get_drvdata(dev);
+	struct tegra_soctherm *ts;
 	int temp, cpu_throt, gpu_throt;
 	unsigned int throt;
 	u32 r, reg_off;
@@ -405,6 +405,8 @@ static int throttrip_program(struct device *dev,
 	if (!sg || !stc || !stc->init)
 		return -EINVAL;
 
+	ts = dev_get_drvdata(dev);
+
 	temp = enforce_temp_range(dev, trip_temp) / ts->soc->thresh_grain;
 
 	/* Hardcode LIGHT on LEVEL1 and HEAVY on LEVEL2 */
-- 
2.7.4

[PATCH v1 4/4] thermal: tegra: fix coverity defect

[Wei Ni]

Fix dereference dev before null check.

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

diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
index 3042837364e8..96527df91f2a 100644
--- a/drivers/thermal/tegra/soctherm.c
+++ b/drivers/thermal/tegra/soctherm.c
@@ -397,7 +397,7 @@ static int throttrip_program(struct device *dev,
 			     struct soctherm_throt_cfg *stc,
 			     int trip_temp)
 {
-	struct tegra_soctherm *ts = dev_get_drvdata(dev);
+	struct tegra_soctherm *ts;
 	int temp, cpu_throt, gpu_throt;
 	unsigned int throt;
 	u32 r, reg_off;
@@ -405,6 +405,8 @@ static int throttrip_program(struct device *dev,
 	if (!sg || !stc || !stc->init)
 		return -EINVAL;
 
+	ts = dev_get_drvdata(dev);
+
 	temp = enforce_temp_range(dev, trip_temp) / ts->soc->thresh_grain;
 
 	/* Hardcode LIGHT on LEVEL1 and HEAVY on LEVEL2 */
-- 
2.7.4

Re: [PATCH v1 4/4] thermal: tegra: fix coverity defect

[Thierry Reding]

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

On Mon, Nov 05, 2018 at 05:32:34PM +0800, Wei Ni wrote:
> Fix dereference dev before null check.
> 
> Signed-off-by: Wei Ni <wni@nvidia.com>
> ---
>  drivers/thermal/tegra/soctherm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
> index 3042837364e8..96527df91f2a 100644
> --- a/drivers/thermal/tegra/soctherm.c
> +++ b/drivers/thermal/tegra/soctherm.c
> @@ -397,7 +397,7 @@ static int throttrip_program(struct device *dev,
>  			     struct soctherm_throt_cfg *stc,
>  			     int trip_temp)
>  {
> -	struct tegra_soctherm *ts = dev_get_drvdata(dev);
> +	struct tegra_soctherm *ts;
>  	int temp, cpu_throt, gpu_throt;
>  	unsigned int throt;
>  	u32 r, reg_off;
> @@ -405,6 +405,8 @@ static int throttrip_program(struct device *dev,
>  	if (!sg || !stc || !stc->init)
>  		return -EINVAL;
>  
> +	ts = dev_get_drvdata(dev);

I think coverity is wrong. How is dev ever going to be NULL in this
case? We allocate all of these struct tegra_thermctl_zone structures in
tegra_soctherm_probe() and assign zone->dev = &pdev->dev, which can
never be NULL.

And even if it could, the code would've crashed earlier in
tegra_soctherm_probe() already.

Furthermore, I fail to see how your patch would fix the defect. None of
the checks in the conditional above actually check the dev value.

Thierry

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

Re: [PATCH v1 4/4] thermal: tegra: fix coverity defect

[Wei Ni]

On 8/11/2018 8:37 PM, Thierry Reding wrote:
> On Mon, Nov 05, 2018 at 05:32:34PM +0800, Wei Ni wrote:
>> Fix dereference dev before null check.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/thermal/tegra/soctherm.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 3042837364e8..96527df91f2a 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -397,7 +397,7 @@ static int throttrip_program(struct device *dev,
>>  			     struct soctherm_throt_cfg *stc,
>>  			     int trip_temp)
>>  {
>> -	struct tegra_soctherm *ts = dev_get_drvdata(dev);
>> +	struct tegra_soctherm *ts;
>>  	int temp, cpu_throt, gpu_throt;
>>  	unsigned int throt;
>>  	u32 r, reg_off;
>> @@ -405,6 +405,8 @@ static int throttrip_program(struct device *dev,
>>  	if (!sg || !stc || !stc->init)
>>  		return -EINVAL;
>>  
>> +	ts = dev_get_drvdata(dev);
> 
> I think coverity is wrong. How is dev ever going to be NULL in this
> case? We allocate all of these struct tegra_thermctl_zone structures in
> tegra_soctherm_probe() and assign zone->dev = &pdev->dev, which can
> never be NULL.
> 
> And even if it could, the code would've crashed earlier in
> tegra_soctherm_probe() already.
> 
> Furthermore, I fail to see how your patch would fix the defect. None of
> the checks in the conditional above actually check the dev value.
> 
Yes, you are right, we doesn't need this change. The driver would not
pass null dev in any case.
And this driver already had a change "1fba81cc09bd thermal: tegra:
remove null check for dev pointer" which remove this "dev" checking.

Thank.
Wei.

> Thierry
> 

Re: [PATCH v1 4/4] thermal: tegra: fix coverity defect

[Wei Ni]

On 8/11/2018 8:37 PM, Thierry Reding wrote:
> On Mon, Nov 05, 2018 at 05:32:34PM +0800, Wei Ni wrote:
>> Fix dereference dev before null check.
>>
>> Signed-off-by: Wei Ni <wni@nvidia.com>
>> ---
>>  drivers/thermal/tegra/soctherm.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/tegra/soctherm.c b/drivers/thermal/tegra/soctherm.c
>> index 3042837364e8..96527df91f2a 100644
>> --- a/drivers/thermal/tegra/soctherm.c
>> +++ b/drivers/thermal/tegra/soctherm.c
>> @@ -397,7 +397,7 @@ static int throttrip_program(struct device *dev,
>>  			     struct soctherm_throt_cfg *stc,
>>  			     int trip_temp)
>>  {
>> -	struct tegra_soctherm *ts = dev_get_drvdata(dev);
>> +	struct tegra_soctherm *ts;
>>  	int temp, cpu_throt, gpu_throt;
>>  	unsigned int throt;
>>  	u32 r, reg_off;
>> @@ -405,6 +405,8 @@ static int throttrip_program(struct device *dev,
>>  	if (!sg || !stc || !stc->init)
>>  		return -EINVAL;
>>  
>> +	ts = dev_get_drvdata(dev);
> 
> I think coverity is wrong. How is dev ever going to be NULL in this
> case? We allocate all of these struct tegra_thermctl_zone structures in
> tegra_soctherm_probe() and assign zone->dev = &pdev->dev, which can
> never be NULL.
> 
> And even if it could, the code would've crashed earlier in
> tegra_soctherm_probe() already.
> 
> Furthermore, I fail to see how your patch would fix the defect. None of
> the checks in the conditional above actually check the dev value.
> 
Yes, you are right, we doesn't need this change. The driver would not
pass null dev in any case.
And this driver already had a change "1fba81cc09bd thermal: tegra:
remove null check for dev pointer" which remove this "dev" checking.

Thank.
Wei.

> Thierry
>