Re: [bug report] thermal: add brcmstb AVS TMON driver

[Brian Norris <computersforpeace@gmail.com>]

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