[PATCH 1/2] thermal: tegra-bpmp: Handle offline zones

[PATCH 1/2] thermal: tegra-bpmp: Handle offline zones

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

Thermal zones located in power domains may not be accessible when
the domain is powergated. In this situation, reading the temperature
will return -BPMP_EFAULT and the temperature is considered to be
-256C for calculating trips.

For smooth operation, for offline zones, return -EAGAIN when reading
the temperature and allow registration of zones even if they are
offline during probe.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/thermal/tegra/tegra-bpmp-thermal.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/tegra/tegra-bpmp-thermal.c b/drivers/thermal/tegra/tegra-bpmp-thermal.c
index c76e1ea62c8a..628b18818ae9 100644
--- a/drivers/thermal/tegra/tegra-bpmp-thermal.c
+++ b/drivers/thermal/tegra/tegra-bpmp-thermal.c
@@ -52,6 +52,8 @@ static int __tegra_bpmp_thermal_get_temp(struct tegra_bpmp_thermal_zone *zone,
 	err = tegra_bpmp_transfer(zone->tegra->bpmp, &msg);
 	if (err)
 		return err;
+	if (msg.rx.ret == -BPMP_EFAULT)
+		return -EAGAIN;
 	if (msg.rx.ret)
 		return -EINVAL;
 
@@ -257,7 +259,12 @@ static int tegra_bpmp_thermal_probe(struct platform_device *pdev)
 		zone->tegra = tegra;
 
 		err = __tegra_bpmp_thermal_get_temp(zone, &temp);
-		if (err < 0) {
+
+		/*
+		 * Sensors in powergated domains may temporarily fail to be read
+		 * (-EAGAIN), but will become accessible when the domain is powered on.
+		 */
+		if (err < 0 && err != -EAGAIN) {
 			devm_kfree(&pdev->dev, zone);
 			continue;
 		}
-- 
2.39.0

[PATCH 2/2] thermal: tegra-bpmp: Always (re)program trip temperatures

[Mikko Perttunen]

From: Mikko Perttunen <mperttunen@nvidia.com>

In the rare case that calculation of trip temperatures would result
in the same trip temperatures that were previously programmed, the
thermal core skips calling .set_trips. However, presently, if it is
not called, we may end up with no trip temperatures programmed at all.

To avoid this, make set_trips a no-op and in places where it would be
called, instead unconditionally program trip temperatures to the last
specified temperatures.

This also fixes the situation where a trip is triggered between
registering a thermal zone and registering the trip MRQ handler, in
which case we would also get stuck.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/thermal/tegra/tegra-bpmp-thermal.c | 33 +++++++++++++++++++---
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/thermal/tegra/tegra-bpmp-thermal.c b/drivers/thermal/tegra/tegra-bpmp-thermal.c
index 628b18818ae9..9f69dbe1c7d4 100644
--- a/drivers/thermal/tegra/tegra-bpmp-thermal.c
+++ b/drivers/thermal/tegra/tegra-bpmp-thermal.c
@@ -67,9 +67,8 @@ static int tegra_bpmp_thermal_get_temp(struct thermal_zone_device *tz, int *out_
 	return __tegra_bpmp_thermal_get_temp(tz->devdata, out_temp);
 }
 
-static int tegra_bpmp_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
+static int tegra_bpmp_thermal_program_trips(struct tegra_bpmp_thermal_zone *zone)
 {
-	struct tegra_bpmp_thermal_zone *zone = tz->devdata;
 	struct mrq_thermal_host_to_bpmp_request req;
 	struct tegra_bpmp_message msg;
 	int err;
@@ -78,8 +77,10 @@ static int tegra_bpmp_thermal_set_trips(struct thermal_zone_device *tz, int low,
 	req.type = CMD_THERMAL_SET_TRIP;
 	req.set_trip.zone = zone->idx;
 	req.set_trip.enabled = true;
-	req.set_trip.low = low;
-	req.set_trip.high = high;
+	mutex_lock(&zone->tzd->lock);
+	req.set_trip.low = zone->tzd->prev_low_trip;
+	req.set_trip.high = zone->tzd->prev_high_trip;
+	mutex_unlock(&zone->tzd->lock);
 
 	memset(&msg, 0, sizeof(msg));
 	msg.mrq = MRQ_THERMAL;
@@ -95,14 +96,31 @@ static int tegra_bpmp_thermal_set_trips(struct thermal_zone_device *tz, int low,
 	return 0;
 }
 
+static int tegra_bpmp_thermal_set_trips(struct thermal_zone_device *tz, int low, int high)
+{
+	return 0;
+}
+
 static void tz_device_update_work_fn(struct work_struct *work)
 {
 	struct tegra_bpmp_thermal_zone *zone;
+	int err;
 
 	zone = container_of(work, struct tegra_bpmp_thermal_zone,
 			    tz_device_update_work);
 
+	/* Recalculates trip temperatures. */
 	thermal_zone_device_update(zone->tzd, THERMAL_TRIP_VIOLATED);
+
+	/*
+	 * Program trip temperatures. We must do this outside `set_trips`
+	 * since thermal core may skip calling it if the trip temperatures
+	 * are unchanged.
+	 */
+	err = tegra_bpmp_thermal_program_trips(zone);
+	if (err)
+		dev_err(zone->tegra->dev, "failed to update trip temperatures for zone '%s': %d\n",
+			zone->tzd->type, err);
 }
 
 static void bpmp_mrq_thermal(unsigned int mrq, struct tegra_bpmp_channel *ch,
@@ -293,6 +311,13 @@ static int tegra_bpmp_thermal_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	for (i = 0; i < tegra->num_zones; i++) {
+		err = tegra_bpmp_thermal_program_trips(tegra->zones[i]);
+		if (err)
+			dev_err(&pdev->dev, "failed to set trip temperatures for zone '%s': %d\n",
+				tzd->type, err);
+	}
+
 	platform_set_drvdata(pdev, tegra);
 
 	return 0;
-- 
2.39.0

Re: [PATCH 2/2] thermal: tegra-bpmp: Always (re)program trip temperatures

[Thierry Reding]

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

On Tue, Feb 07, 2023 at 03:56:09PM +0200, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
> 
> In the rare case that calculation of trip temperatures would result
> in the same trip temperatures that were previously programmed, the
> thermal core skips calling .set_trips.

That seems like an appropriate optimization.

> However, presently, if it is not called, we may end up with no trip
> temperatures programmed at all.

I have a hard time understanding when this would happen. prev_low_trip
and prev_high_trip are -INT_MAX and INT_MAX, respectively, so these are
unlikely to be the result of anything we compute at runtime, based on
temperatures specified in DT, for example.

So I would expect ->set_trips() to get called at least once when the
thermal zones are first registered. Are you saying there are cases where
->set_trips() doesn't get called at all?

> To avoid this, make set_trips a no-op and in places where it would be
> called, instead unconditionally program trip temperatures to the last
> specified temperatures.

Again, this seems more like a workaround for an issue that exists
elsewhere. If ->set_trips() doesn't always get called when it should be,
then that's what we should fix.

> This also fixes the situation where a trip is triggered between
> registering a thermal zone and registering the trip MRQ handler, in
> which case we would also get stuck.

Could this be fixed by requesting the MRQ prior to registering the
zones? That seems like the more appropriate fix for this issue. It's
similar to how we typically register IRQ handlers before enabling a
device to make sure we don't miss any interrupts.

Thierry

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

Re: [PATCH 2/2] thermal: tegra-bpmp: Always (re)program trip temperatures

[Mikko Perttunen]

On 2/8/23 12:43, Thierry Reding wrote:
> On Tue, Feb 07, 2023 at 03:56:09PM +0200, Mikko Perttunen wrote:
>> From: Mikko Perttunen <mperttunen@nvidia.com>
>>
>> In the rare case that calculation of trip temperatures would result
>> in the same trip temperatures that were previously programmed, the
>> thermal core skips calling .set_trips.
> 
> That seems like an appropriate optimization.
> 
>> However, presently, if it is not called, we may end up with no trip
>> temperatures programmed at all.
> 
> I have a hard time understanding when this would happen. prev_low_trip
> and prev_high_trip are -INT_MAX and INT_MAX, respectively, so these are
> unlikely to be the result of anything we compute at runtime, based on
> temperatures specified in DT, for example.

Consider:

Temperature is 45C.
set_trips is called with low=40C high=50C. We program accordingly.
Temperature goes to 55C. Trip point triggers.
Before execution gets to CPU, temperature returns to 45C.
CPU gets the MRQ, calls into thermal core to update.
Thermal core notices that temperature is 45C and sets again the same 
low=40C high=50C trip points, does not call set_trips.
No trip point is programmed to BPMP and we never get trips again.

The above, of course, is rather unlikely to happen, but theoretically 
possible nevertheless.

Alternatively, where I discovered the issue originally, was the issue 
described in the last paragraph of the commit message; see below.

> 
> So I would expect ->set_trips() to get called at least once when the
> thermal zones are first registered. Are you saying there are cases where
> ->set_trips() doesn't get called at all?

No, not saying that. It will get called when registering the zone 
initially, but see below.

> 
>> To avoid this, make set_trips a no-op and in places where it would be
>> called, instead unconditionally program trip temperatures to the last
>> specified temperatures.
> 
> Again, this seems more like a workaround for an issue that exists
> elsewhere. If ->set_trips() doesn't always get called when it should be,
> then that's what we should fix.

I think it depends on what the interpretation is with set_trips. If the 
interpretation is that the the trips configured in the hardware are 
persistent (not disabled when a trip occurs), then the current 
implementation and this patch make sense. Otherwise a change in the 
thermal core would make sense.

> 
>> This also fixes the situation where a trip is triggered between
>> registering a thermal zone and registering the trip MRQ handler, in
>> which case we would also get stuck.
> 
> Could this be fixed by requesting the MRQ prior to registering the
> zones? That seems like the more appropriate fix for this issue. It's
> similar to how we typically register IRQ handlers before enabling a
> device to make sure we don't miss any interrupts.

I considered that -- there are two reasons I didn't go for it:

1. It doesn't solve the race condition described in the first part of 
the message
2. To handle the incoming MRQ, zone->tzd needs to be set. But we only 
get tzd from the zone registration call, and already before that call 
returns, set_trips has been called and we might have received an MRQ. I 
tested using a completion object to block in the MRQ handler until the 
initialization completes, but that's pretty ugly as well.

> 
> Thierry

Thanks,
Mikko

Re: [PATCH 2/2] thermal: tegra-bpmp: Always (re)program trip temperatures

[Thierry Reding]

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

On Wed, Feb 08, 2023 at 05:35:22PM +0200, Mikko Perttunen wrote:
> On 2/8/23 12:43, Thierry Reding wrote:
> > On Tue, Feb 07, 2023 at 03:56:09PM +0200, Mikko Perttunen wrote:
> > > From: Mikko Perttunen <mperttunen@nvidia.com>
> > > 
> > > In the rare case that calculation of trip temperatures would result
> > > in the same trip temperatures that were previously programmed, the
> > > thermal core skips calling .set_trips.
> > 
> > That seems like an appropriate optimization.
> > 
> > > However, presently, if it is not called, we may end up with no trip
> > > temperatures programmed at all.
> > 
> > I have a hard time understanding when this would happen. prev_low_trip
> > and prev_high_trip are -INT_MAX and INT_MAX, respectively, so these are
> > unlikely to be the result of anything we compute at runtime, based on
> > temperatures specified in DT, for example.
> 
> Consider:
> 
> Temperature is 45C.
> set_trips is called with low=40C high=50C. We program accordingly.
> Temperature goes to 55C. Trip point triggers.
> Before execution gets to CPU, temperature returns to 45C.
> CPU gets the MRQ, calls into thermal core to update.
> Thermal core notices that temperature is 45C and sets again the same low=40C
> high=50C trip points, does not call set_trips.
> No trip point is programmed to BPMP and we never get trips again.

So does this mean that trip points in BPMP are "one-shot". That is, once
they are triggered, BPMP will automatically delete them? And we actively
need to reprogram them to trigger again?

Perhaps a better alternative would be to force the previous temperatures
to be invalid when the trip point triggers to ensure they always get
reprogrammed. Or perhaps we could add a flag to the thermal subsystem to
mark "one-shot" triggers so that the core skips over the temperature
check and always reprograms.

> The above, of course, is rather unlikely to happen, but theoretically
> possible nevertheless.
> 
> Alternatively, where I discovered the issue originally, was the issue
> described in the last paragraph of the commit message; see below.
> 
> > 
> > So I would expect ->set_trips() to get called at least once when the
> > thermal zones are first registered. Are you saying there are cases where
> > ->set_trips() doesn't get called at all?
> 
> No, not saying that. It will get called when registering the zone initially,
> but see below.
> 
> > 
> > > To avoid this, make set_trips a no-op and in places where it would be
> > > called, instead unconditionally program trip temperatures to the last
> > > specified temperatures.
> > 
> > Again, this seems more like a workaround for an issue that exists
> > elsewhere. If ->set_trips() doesn't always get called when it should be,
> > then that's what we should fix.
> 
> I think it depends on what the interpretation is with set_trips. If the
> interpretation is that the the trips configured in the hardware are
> persistent (not disabled when a trip occurs), then the current
> implementation and this patch make sense. Otherwise a change in the thermal
> core would make sense.
> 
> > 
> > > This also fixes the situation where a trip is triggered between
> > > registering a thermal zone and registering the trip MRQ handler, in
> > > which case we would also get stuck.
> > 
> > Could this be fixed by requesting the MRQ prior to registering the
> > zones? That seems like the more appropriate fix for this issue. It's
> > similar to how we typically register IRQ handlers before enabling a
> > device to make sure we don't miss any interrupts.
> 
> I considered that -- there are two reasons I didn't go for it:
> 
> 1. It doesn't solve the race condition described in the first part of the
> message

These are two different problems, though, so trying to swat them with
the same fix is not likely the best solution.

> 2. To handle the incoming MRQ, zone->tzd needs to be set. But we only get
> tzd from the zone registration call, and already before that call returns,
> set_trips has been called and we might have received an MRQ. I tested using
> a completion object to block in the MRQ handler until the initialization
> completes, but that's pretty ugly as well.

Sounds to me like we may need to split the registration and activation
steps. I recall discussions about similar issues in other subsystems.

With interrupts this can sometimes be worked around by installing
handlers and keeping interrupts masked and once unmasked they will
immediately trigger and cause the handler to run. We probably don't
want that for BPMP MRQs, though.

Thierry

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

Re: [PATCH 1/2] thermal: tegra-bpmp: Handle offline zones

[Thierry Reding]

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

On Tue, Feb 07, 2023 at 03:56:08PM +0200, Mikko Perttunen wrote:
> From: Mikko Perttunen <mperttunen@nvidia.com>
> 
> Thermal zones located in power domains may not be accessible when
> the domain is powergated. In this situation, reading the temperature
> will return -BPMP_EFAULT and the temperature is considered to be
> -256C for calculating trips.

Where's that -256C being set? I only see THERMAL_TEMP_INVALID being set
as the default for a zone, but that's not -274C, not -256C. If that's
the temperature that you're referring to, it might be better to state
that we rely on the default temperature rather than any specific number.

Thierry

> 
> For smooth operation, for offline zones, return -EAGAIN when reading
> the temperature and allow registration of zones even if they are
> offline during probe.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/thermal/tegra/tegra-bpmp-thermal.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/tegra/tegra-bpmp-thermal.c b/drivers/thermal/tegra/tegra-bpmp-thermal.c
> index c76e1ea62c8a..628b18818ae9 100644
> --- a/drivers/thermal/tegra/tegra-bpmp-thermal.c
> +++ b/drivers/thermal/tegra/tegra-bpmp-thermal.c
> @@ -52,6 +52,8 @@ static int __tegra_bpmp_thermal_get_temp(struct tegra_bpmp_thermal_zone *zone,
>  	err = tegra_bpmp_transfer(zone->tegra->bpmp, &msg);
>  	if (err)
>  		return err;
> +	if (msg.rx.ret == -BPMP_EFAULT)
> +		return -EAGAIN;
>  	if (msg.rx.ret)
>  		return -EINVAL;
>  
> @@ -257,7 +259,12 @@ static int tegra_bpmp_thermal_probe(struct platform_device *pdev)
>  		zone->tegra = tegra;
>  
>  		err = __tegra_bpmp_thermal_get_temp(zone, &temp);
> -		if (err < 0) {
> +
> +		/*
> +		 * Sensors in powergated domains may temporarily fail to be read
> +		 * (-EAGAIN), but will become accessible when the domain is powered on.
> +		 */
> +		if (err < 0 && err != -EAGAIN) {
>  			devm_kfree(&pdev->dev, zone);
>  			continue;
>  		}
> -- 
> 2.39.0
> 

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

Re: [PATCH 1/2] thermal: tegra-bpmp: Handle offline zones

[Mikko Perttunen]

On 2/8/23 12:31, Thierry Reding wrote:
> On Tue, Feb 07, 2023 at 03:56:08PM +0200, Mikko Perttunen wrote:
>> From: Mikko Perttunen <mperttunen@nvidia.com>
>>
>> Thermal zones located in power domains may not be accessible when
>> the domain is powergated. In this situation, reading the temperature
>> will return -BPMP_EFAULT and the temperature is considered to be
>> -256C for calculating trips.
> 
> Where's that -256C being set? I only see THERMAL_TEMP_INVALID being set
> as the default for a zone, but that's not -274C, not -256C. If that's
> the temperature that you're referring to, it might be better to state
> that we rely on the default temperature rather than any specific number.
> 
> Thierry

It is based on BPMP's internal behavior.

Mikko

> 
>>
>> For smooth operation, for offline zones, return -EAGAIN when reading
>> the temperature and allow registration of zones even if they are
>> offline during probe.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>   drivers/thermal/tegra/tegra-bpmp-thermal.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/tegra/tegra-bpmp-thermal.c b/drivers/thermal/tegra/tegra-bpmp-thermal.c
>> index c76e1ea62c8a..628b18818ae9 100644
>> --- a/drivers/thermal/tegra/tegra-bpmp-thermal.c
>> +++ b/drivers/thermal/tegra/tegra-bpmp-thermal.c
>> @@ -52,6 +52,8 @@ static int __tegra_bpmp_thermal_get_temp(struct tegra_bpmp_thermal_zone *zone,
>>   	err = tegra_bpmp_transfer(zone->tegra->bpmp, &msg);
>>   	if (err)
>>   		return err;
>> +	if (msg.rx.ret == -BPMP_EFAULT)
>> +		return -EAGAIN;
>>   	if (msg.rx.ret)
>>   		return -EINVAL;
>>   
>> @@ -257,7 +259,12 @@ static int tegra_bpmp_thermal_probe(struct platform_device *pdev)
>>   		zone->tegra = tegra;
>>   
>>   		err = __tegra_bpmp_thermal_get_temp(zone, &temp);
>> -		if (err < 0) {
>> +
>> +		/*
>> +		 * Sensors in powergated domains may temporarily fail to be read
>> +		 * (-EAGAIN), but will become accessible when the domain is powered on.
>> +		 */
>> +		if (err < 0 && err != -EAGAIN) {
>>   			devm_kfree(&pdev->dev, zone);
>>   			continue;
>>   		}
>> -- 
>> 2.39.0
>>

Re: [PATCH 1/2] thermal: tegra-bpmp: Handle offline zones

[Thierry Reding]

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

On Wed, Feb 08, 2023 at 05:35:48PM +0200, Mikko Perttunen wrote:
> On 2/8/23 12:31, Thierry Reding wrote:
> > On Tue, Feb 07, 2023 at 03:56:08PM +0200, Mikko Perttunen wrote:
> > > From: Mikko Perttunen <mperttunen@nvidia.com>
> > > 
> > > Thermal zones located in power domains may not be accessible when
> > > the domain is powergated. In this situation, reading the temperature
> > > will return -BPMP_EFAULT and the temperature is considered to be
> > > -256C for calculating trips.
> > 
> > Where's that -256C being set? I only see THERMAL_TEMP_INVALID being set
> > as the default for a zone, but that's not -274C, not -256C. If that's
> > the temperature that you're referring to, it might be better to state
> > that we rely on the default temperature rather than any specific number.
> > 
> > Thierry
> 
> It is based on BPMP's internal behavior.

Okay, maybe clarify that part of the sentence then. Could be something
like:

	... will return -BPMP_EFAULT. When evaluating trips, BPMP will
	internally use -256C as the temperature for offline zones.

Thierry

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