* [bug report] thermal: add brcmstb AVS TMON driver
@ 2017-09-06 8:22 Dan Carpenter
2017-09-06 17:00 ` Brian Norris
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2017-09-06 8:22 UTC (permalink / raw)
To: computersforpeace; +Cc: linux-pm
Hello Brian Norris,
The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
Aug 9, 2017, leads to the following static checker warning:
drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)'
drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)'
drivers/thermal/broadcom/brcmstb_thermal.c
274 static int brcmstb_set_trips(void *data, int low, int high)
275 {
276 struct brcmstb_thermal_priv *priv = data;
277
278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
279
280 if (low) {
281 if (low > INT_MAX)
^^^^^^^^^^^^^
Never true
282 low = INT_MAX;
283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
285 } else {
286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
287 }
288
289 if (high < ULONG_MAX) {
^^^^^^^^^^^^^^^^
Always true
290 if (high > INT_MAX)
^^^^^^^^^^^^^^
Never true
291 high = INT_MAX;
292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
294 } else {
295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
296 }
297
298 return 0;
299 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [bug report] thermal: add brcmstb AVS TMON driver 2017-09-06 8:22 [bug report] thermal: add brcmstb AVS TMON driver Dan Carpenter @ 2017-09-06 17:00 ` Brian Norris 2017-09-06 18:08 ` Florian Fainelli 0 siblings, 1 reply; 8+ messages in thread From: Brian Norris @ 2017-09-06 17:00 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-pm, Doug Berger, Florian Fainelli On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote: > Hello Brian Norris, Hi Dan! Or Dan's robots. > The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from I'll blame Doug or Florian :) Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the tree where I first wrote this driver (before 'set_trips' was even merged upstream). We can probably simplify this now. But I'll leave that to Doug. Brian > Aug 9, 2017, leads to the following static checker warning: > > drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips() > warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)' > > drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips() > warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)' > > drivers/thermal/broadcom/brcmstb_thermal.c > 274 static int brcmstb_set_trips(void *data, int low, int high) > 275 { > 276 struct brcmstb_thermal_priv *priv = data; > 277 > 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); > 279 > 280 if (low) { > 281 if (low > INT_MAX) > ^^^^^^^^^^^^^ > Never true > > 282 low = INT_MAX; > 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); > 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); > 285 } else { > 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); > 287 } > 288 > 289 if (high < ULONG_MAX) { > ^^^^^^^^^^^^^^^^ > Always true > > 290 if (high > INT_MAX) > ^^^^^^^^^^^^^^ > Never true > > 291 high = INT_MAX; > 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); > 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); > 294 } else { > 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); > 296 } > 297 > 298 return 0; > 299 } > > > regards, > dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver 2017-09-06 17:00 ` Brian Norris @ 2017-09-06 18:08 ` Florian Fainelli 2017-09-06 18:37 ` Brian Norris 0 siblings, 1 reply; 8+ messages in thread From: Florian Fainelli @ 2017-09-06 18:08 UTC (permalink / raw) To: Brian Norris, Dan Carpenter; +Cc: linux-pm, Doug Berger, colin.king On 09/06/2017 10:00 AM, Brian Norris wrote: > On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote: >> Hello Brian Norris, > > Hi Dan! Or Dan's robots. > >> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from > > I'll blame Doug or Florian :) > > Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the > tree where I first wrote this driver (before 'set_trips' was even merged > upstream). We can probably simplify this now. That is exactly what happened yes :) and Colin already sent a fix for that: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html > > But I'll leave that to Doug. > > Brian > >> Aug 9, 2017, leads to the following static checker warning: >> >> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips() >> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)' >> >> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips() >> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)' >> >> drivers/thermal/broadcom/brcmstb_thermal.c >> 274 static int brcmstb_set_trips(void *data, int low, int high) >> 275 { >> 276 struct brcmstb_thermal_priv *priv = data; >> 277 >> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); >> 279 >> 280 if (low) { >> 281 if (low > INT_MAX) >> ^^^^^^^^^^^^^ >> Never true >> >> 282 low = INT_MAX; >> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); >> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); >> 285 } else { >> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); >> 287 } >> 288 >> 289 if (high < ULONG_MAX) { >> ^^^^^^^^^^^^^^^^ >> Always true >> >> 290 if (high > INT_MAX) >> ^^^^^^^^^^^^^^ >> Never true >> >> 291 high = INT_MAX; >> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); >> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); >> 294 } else { >> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); >> 296 } >> 297 >> 298 return 0; >> 299 } >> >> >> regards, >> dan carpenter -- Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver 2017-09-06 18:08 ` Florian Fainelli @ 2017-09-06 18:37 ` Brian Norris 2017-09-06 19:33 ` Doug Berger 0 siblings, 1 reply; 8+ messages in thread From: Brian Norris @ 2017-09-06 18:37 UTC (permalink / raw) To: Florian Fainelli; +Cc: Dan Carpenter, linux-pm, Doug Berger, colin.king Hi, On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote: > On 09/06/2017 10:00 AM, Brian Norris wrote: > > On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote: > >> Hello Brian Norris, > > > > Hi Dan! Or Dan's robots. > > > >> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from > > > > I'll blame Doug or Florian :) > > > > Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the > > tree where I first wrote this driver (before 'set_trips' was even merged > > upstream). We can probably simplify this now. > > That is exactly what happened yes :) and Colin already sent a fix for that: > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html Colin didn't fix all the problems. See below: > > > > But I'll leave that to Doug. > > > > Brian > > > >> Aug 9, 2017, leads to the following static checker warning: > >> > >> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips() > >> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)' > >> > >> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips() > >> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)' > >> > >> drivers/thermal/broadcom/brcmstb_thermal.c > >> 274 static int brcmstb_set_trips(void *data, int low, int high) > >> 275 { > >> 276 struct brcmstb_thermal_priv *priv = data; > >> 277 > >> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); > >> 279 > >> 280 if (low) { > >> 281 if (low > INT_MAX) > >> ^^^^^^^^^^^^^ > >> Never true > >> > >> 282 low = INT_MAX; > >> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); > >> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); > >> 285 } else { > >> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); > >> 287 } > >> 288 > >> 289 if (high < ULONG_MAX) { > >> ^^^^^^^^^^^^^^^^ > >> Always true We didn't expect this one ^^^ and Colin didn't fix it. We expected a "max" value to turn the trip point off (in the 'else' branch). But we can never possibly reach the 'else'. Maybe my expectations are off now (I'm not really that familiar with the current state of the thermal framework), but at any rate, we shouldn't be leaving dead code. I'll make a quick comment on Colin's patch too, so this doesn't get lost. Brian > >> 290 if (high > INT_MAX) > >> ^^^^^^^^^^^^^^ > >> Never true > >> > >> 291 high = INT_MAX; > >> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); > >> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); > >> 294 } else { > >> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); > >> 296 } > >> 297 > >> 298 return 0; > >> 299 } > >> > >> > >> regards, > >> dan carpenter > > > -- > Florian ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver 2017-09-06 18:37 ` Brian Norris @ 2017-09-06 19:33 ` Doug Berger 2017-09-06 19:46 ` Markus Mayer 0 siblings, 1 reply; 8+ messages in thread From: Doug Berger @ 2017-09-06 19:33 UTC (permalink / raw) To: Brian Norris, Florian Fainelli, Markus Mayer Cc: Dan Carpenter, linux-pm, colin.king On 09/06/2017 11:37 AM, Brian Norris wrote: > Hi, > > On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote: >> On 09/06/2017 10:00 AM, Brian Norris wrote: >>> On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote: >>>> Hello Brian Norris, >>> >>> Hi Dan! Or Dan's robots. >>> >>>> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from >>> >>> I'll blame Doug or Florian :) I of course have to blame Markus ;) >>> >>> Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the >>> tree where I first wrote this driver (before 'set_trips' was even merged >>> upstream). We can probably simplify this now. >> >> That is exactly what happened yes :) and Colin already sent a fix for that: >> >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html > > Colin didn't fix all the problems. See below: > >>> >>> But I'll leave that to Doug. >>> >>> Brian >>> >>>> Aug 9, 2017, leads to the following static checker warning: >>>> >>>> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips() >>>> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)' >>>> >>>> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips() >>>> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)' >>>> >>>> drivers/thermal/broadcom/brcmstb_thermal.c >>>> 274 static int brcmstb_set_trips(void *data, int low, int high) >>>> 275 { >>>> 276 struct brcmstb_thermal_priv *priv = data; >>>> 277 >>>> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); >>>> 279 >>>> 280 if (low) { >>>> 281 if (low > INT_MAX) >>>> ^^^^^^^^^^^^^ >>>> Never true >>>> >>>> 282 low = INT_MAX; >>>> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); >>>> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); >>>> 285 } else { >>>> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); >>>> 287 } >>>> 288 >>>> 289 if (high < ULONG_MAX) { >>>> ^^^^^^^^^^^^^^^^ >>>> Always true > > We didn't expect this one ^^^ and Colin didn't fix it. We expected a > "max" value to turn the trip point off (in the 'else' branch). But we > can never possibly reach the 'else'. This actually appears to be the heart of the matter. As stated above, the set_trips patch API that this logic was developed for was different from the version of the patch ultimately merged upstream and the brcmstb_thermal driver logic was not corrected before being submitted upstream. The key change appears to be as follows: Old API - unsigned long ints and "If there is no trip point above or below the current temperature, the passed trip temperature will be ULONG_MAX or 0 respectively. New API - ints and "If there is no trip point above or below the current temperature, the passed trip temperature will be -INT_MAX or INT_MAX respectively." So there are latent logic errors in addition to the "dead code" errors flagged here. > > Maybe my expectations are off now (I'm not really that familiar with the > current state of the thermal framework), but at any rate, we shouldn't > be leaving dead code. > > I'll make a quick comment on Colin's patch too, so this doesn't get > lost. > > Brian > >>>> 290 if (high > INT_MAX) >>>> ^^^^^^^^^^^^^^ >>>> Never true >>>> >>>> 291 high = INT_MAX; >>>> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); >>>> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); >>>> 294 } else { >>>> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); >>>> 296 } >>>> 297 >>>> 298 return 0; >>>> 299 } >>>> >>>> >>>> regards, >>>> dan carpenter >> >> >> -- >> Florian I'll leave it to Markus to correct the logic, unless he wants to punt it back to me :). Thanks! Doug ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver 2017-09-06 19:33 ` Doug Berger @ 2017-09-06 19:46 ` Markus Mayer 2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer 0 siblings, 1 reply; 8+ messages in thread From: Markus Mayer @ 2017-09-06 19:46 UTC (permalink / raw) To: Doug Berger Cc: Brian Norris, Florian Fainelli, Markus Mayer, Dan Carpenter, Power Management List, colin.king On 6 September 2017 at 16:33, Doug Berger <opendmb@gmail.com> wrote: > On 09/06/2017 11:37 AM, Brian Norris wrote: >> Hi, >> >> On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote: >>> On 09/06/2017 10:00 AM, Brian Norris wrote: >>>> On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote: >>>>> Hello Brian Norris, >>>> >>>> Hi Dan! Or Dan's robots. >>>> >>>>> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from >>>> >>>> I'll blame Doug or Florian :) > > I of course have to blame Markus ;) Tried fleeing to the other side of the continent, but it caught up with me anyway. ;-) >>>> >>>> Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the >>>> tree where I first wrote this driver (before 'set_trips' was even merged >>>> upstream). We can probably simplify this now. >>> >>> That is exactly what happened yes :) and Colin already sent a fix for > that: >>> >>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html >> >> Colin didn't fix all the problems. See below: >> >>>> >>>> But I'll leave that to Doug. >>>> >>>> Brian >>>> >>>>> Aug 9, 2017, leads to the following static checker warning: >>>>> >>>>> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips() >>>>> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max >> s32max)' >>>>> >>>>> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips() >>>>> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max >> s32max)' >>>>> >>>>> drivers/thermal/broadcom/brcmstb_thermal.c >>>>> 274 static int brcmstb_set_trips(void *data, int low, int high) >>>>> 275 { >>>>> 276 struct brcmstb_thermal_priv *priv = data; >>>>> 277 >>>>> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, > high); >>>>> 279 >>>>> 280 if (low) { >>>>> 281 if (low > INT_MAX) >>>>> ^^^^^^^^^^^^^ >>>>> Never true >>>>> >>>>> 282 low = INT_MAX; >>>>> 283 avs_tmon_set_trip_temp(priv, > TMON_TRIP_TYPE_LOW, low); >>>>> 284 avs_tmon_trip_enable(priv, > TMON_TRIP_TYPE_LOW, 1); >>>>> 285 } else { >>>>> 286 avs_tmon_trip_enable(priv, > TMON_TRIP_TYPE_LOW, 0); >>>>> 287 } >>>>> 288 >>>>> 289 if (high < ULONG_MAX) { >>>>> ^^^^^^^^^^^^^^^^ >>>>> Always true >> >> We didn't expect this one ^^^ and Colin didn't fix it. We expected a >> "max" value to turn the trip point off (in the 'else' branch). But we >> can never possibly reach the 'else'. > This actually appears to be the heart of the matter. > > As stated above, the set_trips patch API that this logic was developed > for was different from the version of the patch ultimately merged > upstream and the brcmstb_thermal driver logic was not corrected before > being submitted upstream. > > The key change appears to be as follows: > > Old API - unsigned long ints and "If there is no trip point above or > below the current temperature, the passed trip temperature will be > ULONG_MAX or 0 respectively. > > New API - ints and "If there is no trip point above or below the current > temperature, the passed trip temperature will be -INT_MAX or INT_MAX > respectively." > > So there are latent logic errors in addition to the "dead code" errors > flagged here. > >> >> Maybe my expectations are off now (I'm not really that familiar with the >> current state of the thermal framework), but at any rate, we shouldn't >> be leaving dead code. >> >> I'll make a quick comment on Colin's patch too, so this doesn't get >> lost. >> >> Brian >> >>>>> 290 if (high > INT_MAX) >>>>> ^^^^^^^^^^^^^^ >>>>> Never true >>>>> >>>>> 291 high = INT_MAX; >>>>> 292 avs_tmon_set_trip_temp(priv, > TMON_TRIP_TYPE_HIGH, high); >>>>> 293 avs_tmon_trip_enable(priv, > TMON_TRIP_TYPE_HIGH, 1); >>>>> 294 } else { >>>>> 295 avs_tmon_trip_enable(priv, > TMON_TRIP_TYPE_HIGH, 0); >>>>> 296 } >>>>> 297 >>>>> 298 return 0; >>>>> 299 } >>>>> >>>>> >>>>> regards, >>>>> dan carpenter >>> >>> >>> -- >>> Florian > > I'll leave it to Markus to correct the logic, unless he wants to punt it > back to me :). I'll fix it up when I get back home next week. Cheers, -Markus ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] thermal: brcmstb: disable trip points properly when needed 2017-09-06 19:46 ` Markus Mayer @ 2017-09-14 23:19 ` Markus Mayer 2017-09-14 23:25 ` Markus Mayer 0 siblings, 1 reply; 8+ messages in thread From: Markus Mayer @ 2017-09-14 23:19 UTC (permalink / raw) To: Zhang Rui, Eduardo Valentin, Doug Berger, Brian Norris, Florian Fainelli, Dan Carpenter, Colin King Cc: Markus Mayer, Power Management List, ARM Kernel List, Linux Kernel Mailing List From: Markus Mayer <mmayer@broadcom.com> The code checking for low and high temperature points was still based on an earlier implementation of the driver. It wasn't working properly with the data types currently being used. We fix this by disabling the high trip point if our high temperature is INT_MAX and disabling the low trip point if our low temperature is -INT_MAX (or lower). Signed-off-by: Markus Mayer <mmayer@broadcom.com> --- Here is my patch for the issue described. Please let me know if I should be squashing this into the driver patch and re-submit the entire series. drivers/thermal/broadcom/brcmstb_thermal.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c index 87b8e7a..1919f91 100644 --- a/drivers/thermal/broadcom/brcmstb_thermal.c +++ b/drivers/thermal/broadcom/brcmstb_thermal.c @@ -277,22 +277,23 @@ static int brcmstb_set_trips(void *data, int low, int high) dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); - if (low) { - if (low > INT_MAX) - low = INT_MAX; + /* + * Disable low-temp if "low" is too small. As per thermal framework + * API, we use -INT_MAX rather than INT_MIN. + */ + if (low <= -INT_MAX) { + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); + } else { avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); - } else { - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); } - if (high < ULONG_MAX) { - if (high > INT_MAX) - high = INT_MAX; + /* Disable high-temp if "high" is too big. */ + if (high == INT_MAX) { + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); + } else { avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); - } else { - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); } return 0; -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: brcmstb: disable trip points properly when needed 2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer @ 2017-09-14 23:25 ` Markus Mayer 0 siblings, 0 replies; 8+ messages in thread From: Markus Mayer @ 2017-09-14 23:25 UTC (permalink / raw) To: Markus Mayer Cc: Zhang Rui, Eduardo Valentin, Doug Berger, Brian Norris, Florian Fainelli, Dan Carpenter, Colin King, Power Management List, ARM Kernel List, Linux Kernel Mailing List On 14 September 2017 at 16:19, Markus Mayer <code@mmayer.net> wrote: > From: Markus Mayer <mmayer@broadcom.com> > > The code checking for low and high temperature points was still based > on an earlier implementation of the driver. It wasn't working properly > with the data types currently being used. > > We fix this by disabling the high trip point if our high temperature is > INT_MAX and disabling the low trip point if our low temperature is -INT_MAX > (or lower). > > Signed-off-by: Markus Mayer <mmayer@broadcom.com> > --- > > Here is my patch for the issue described. Please let me know if I should be > squashing this into the driver patch and re-submit the entire series. Looks like my attempt at adding this patch to the existing thread may not have worked out so well. Here's a link: https://marc.info/?l=linux-pm&m=150472721929919&w=2 > drivers/thermal/broadcom/brcmstb_thermal.c | 21 +++++++++++---------- > 1 file changed, 11 insertions(+), 10 deletions(-) > > diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c > index 87b8e7a..1919f91 100644 > --- a/drivers/thermal/broadcom/brcmstb_thermal.c > +++ b/drivers/thermal/broadcom/brcmstb_thermal.c > @@ -277,22 +277,23 @@ static int brcmstb_set_trips(void *data, int low, int high) > > dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high); > > - if (low) { > - if (low > INT_MAX) > - low = INT_MAX; > + /* > + * Disable low-temp if "low" is too small. As per thermal framework > + * API, we use -INT_MAX rather than INT_MIN. > + */ > + if (low <= -INT_MAX) { > + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); > + } else { > avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low); > avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1); > - } else { > - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0); > } > > - if (high < ULONG_MAX) { > - if (high > INT_MAX) > - high = INT_MAX; > + /* Disable high-temp if "high" is too big. */ > + if (high == INT_MAX) { > + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); > + } else { > avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high); > avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1); > - } else { > - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0); > } > > return 0; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-14 23:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-06 8:22 [bug report] thermal: add brcmstb AVS TMON driver Dan Carpenter 2017-09-06 17:00 ` Brian Norris 2017-09-06 18:08 ` Florian Fainelli 2017-09-06 18:37 ` Brian Norris 2017-09-06 19:33 ` Doug Berger 2017-09-06 19:46 ` Markus Mayer 2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer 2017-09-14 23:25 ` Markus Mayer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox