[9/9,RFC] dts: Possible FIX for set trip bits

Message ID 20180319045420.22046-10-cyril.bur@au1.ibm.com
State New
Headers show
Series
  • Coverity fixes
Related show

Commit Message

Cyril Bur March 19, 2018, 4:54 a.m.
It is possible that fixing CID 209503 will fix these FIXMEs. I do not
know this code well at all and am in absolutely no position to test.
Removing the FIXME needs much more scrutiny!!

CC: Cédric Le Goater <clg@fr.ibm.com>
CC: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
---
 hw/dts.c | 15 ---------------
 1 file changed, 15 deletions(-)

Comments

Cédric Le Goater March 19, 2018, 8:06 a.m. | #1
On 03/19/2018 05:54 AM, Cyril Bur wrote:
> It is possible that fixing CID 209503 will fix these FIXMEs. I do not
> know this code well at all and am in absolutely no position to test.
> Removing the FIXME needs much more scrutiny!!

When first support for the POWER8 DTS was added a couple of years ago, 
the trip bits always had some 'bogus' value, which raised useless 
warnings at the userspace level in hwmon. Is this fixed now ? 

What is 209503 about ? 

C.


> CC: Cédric Le Goater <clg@fr.ibm.com>
> CC: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> ---
>  hw/dts.c | 15 ---------------
>  1 file changed, 15 deletions(-)
> 
> diff --git a/hw/dts.c b/hw/dts.c
> index 2c366699..5e9e5e39 100644
> --- a/hw/dts.c
> +++ b/hw/dts.c
> @@ -187,11 +187,6 @@ static int dts_read_core_temp_p8(uint32_t pir, struct dts *dts)
>  	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
>  	      chip_id, core, dts->temp, dts->trip);
> 
> -	/*
> -	 * FIXME: The trip bits are always set ?! Just discard
> -	 * them for the moment until we understand why.
> -	 */
> -	dts->trip = 0;
>  	return 0;
>  }
> 
> @@ -228,11 +223,6 @@ static int dts_read_core_temp_p9(uint32_t pir, struct dts *dts)
>  	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
>  	      chip_id, core, dts->temp, dts->trip);
> 
> -	/*
> -	 * FIXME: The trip bits are always set ?! Just discard
> -	 * them for the moment until we understand why.
> -	 */
> -	dts->trip = 0;
>  	return 0;
>  }
> 
> @@ -367,11 +357,6 @@ static int dts_read_mem_temp(uint32_t chip_id, struct dts *dts)
>  	prlog(PR_TRACE, "DTS: Chip %x temp:%dC trip:%x\n",
>  	      chip_id, dts->temp, dts->trip);
> 
> -	/*
> -	 * FIXME: The trip bits are always set ?! Just discard
> -	 * them for the moment until we understand why.
> -	 */
> -	dts->trip = 0;
>  	return 0;
>  }
>
Cyril Bur March 19, 2018, 8:12 a.m. | #2
On Mon, 2018-03-19 at 09:06 +0100, Cédric Le Goater wrote:
> On 03/19/2018 05:54 AM, Cyril Bur wrote:
> > It is possible that fixing CID 209503 will fix these FIXMEs. I do not
> > know this code well at all and am in absolutely no position to test.
> > Removing the FIXME needs much more scrutiny!!
> 
> When first support for the POWER8 DTS was added a couple of years ago, 
> the trip bits always had some 'bogus' value, which raised useless 
> warnings at the userspace level in hwmon. Is this fixed now ? 
> 
> What is 209503 about ? 

209503 is about the dts parameter to these functions coming from
uninitialised memory which may explain bits always being set. Sorry I
probably should have cced you on the fix for 209503 as well, might have
helped with context.

Cyril

> 
> C.
> 
> 
> > CC: Cédric Le Goater <clg@fr.ibm.com>
> > CC: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com>
> > ---
> >  hw/dts.c | 15 ---------------
> >  1 file changed, 15 deletions(-)
> > 
> > diff --git a/hw/dts.c b/hw/dts.c
> > index 2c366699..5e9e5e39 100644
> > --- a/hw/dts.c
> > +++ b/hw/dts.c
> > @@ -187,11 +187,6 @@ static int dts_read_core_temp_p8(uint32_t pir, struct dts *dts)
> >  	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
> >  	      chip_id, core, dts->temp, dts->trip);
> > 
> > -	/*
> > -	 * FIXME: The trip bits are always set ?! Just discard
> > -	 * them for the moment until we understand why.
> > -	 */
> > -	dts->trip = 0;
> >  	return 0;
> >  }
> > 
> > @@ -228,11 +223,6 @@ static int dts_read_core_temp_p9(uint32_t pir, struct dts *dts)
> >  	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
> >  	      chip_id, core, dts->temp, dts->trip);
> > 
> > -	/*
> > -	 * FIXME: The trip bits are always set ?! Just discard
> > -	 * them for the moment until we understand why.
> > -	 */
> > -	dts->trip = 0;
> >  	return 0;
> >  }
> > 
> > @@ -367,11 +357,6 @@ static int dts_read_mem_temp(uint32_t chip_id, struct dts *dts)
> >  	prlog(PR_TRACE, "DTS: Chip %x temp:%dC trip:%x\n",
> >  	      chip_id, dts->temp, dts->trip);
> > 
> > -	/*
> > -	 * FIXME: The trip bits are always set ?! Just discard
> > -	 * them for the moment until we understand why.
> > -	 */
> > -	dts->trip = 0;
> >  	return 0;
> >  }
> > 
> 
>
Cédric Le Goater March 19, 2018, 10:48 a.m. | #3
On 03/19/2018 09:12 AM, Cyril Bur wrote:
> On Mon, 2018-03-19 at 09:06 +0100, Cédric Le Goater wrote:
>> On 03/19/2018 05:54 AM, Cyril Bur wrote:
>>> It is possible that fixing CID 209503 will fix these FIXMEs. I do not
>>> know this code well at all and am in absolutely no position to test.
>>> Removing the FIXME needs much more scrutiny!!
>>
>> When first support for the POWER8 DTS was added a couple of years ago, 
>> the trip bits always had some 'bogus' value, which raised useless 
>> warnings at the userspace level in hwmon. Is this fixed now ? 
>>
>> What is 209503 about ? 
> 
> 209503 is about the dts parameter to these functions coming from
> uninitialised memory which may explain bits always being set. 

hmm, I don't see where the dts could be uninitialized. Anyhow, 
you can check the raw DTS values from userspace with this command :

	getscom <0x10000000 | 0x50000 | (core_id << 24)>

which returns something like 023f023f022f0000 on a palmetto.

C.

Patch

diff --git a/hw/dts.c b/hw/dts.c
index 2c366699..5e9e5e39 100644
--- a/hw/dts.c
+++ b/hw/dts.c
@@ -187,11 +187,6 @@  static int dts_read_core_temp_p8(uint32_t pir, struct dts *dts)
 	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
 	      chip_id, core, dts->temp, dts->trip);
 
-	/*
-	 * FIXME: The trip bits are always set ?! Just discard
-	 * them for the moment until we understand why.
-	 */
-	dts->trip = 0;
 	return 0;
 }
 
@@ -228,11 +223,6 @@  static int dts_read_core_temp_p9(uint32_t pir, struct dts *dts)
 	prlog(PR_TRACE, "DTS: Chip %x Core %x temp:%dC trip:%x\n",
 	      chip_id, core, dts->temp, dts->trip);
 
-	/*
-	 * FIXME: The trip bits are always set ?! Just discard
-	 * them for the moment until we understand why.
-	 */
-	dts->trip = 0;
 	return 0;
 }
 
@@ -367,11 +357,6 @@  static int dts_read_mem_temp(uint32_t chip_id, struct dts *dts)
 	prlog(PR_TRACE, "DTS: Chip %x temp:%dC trip:%x\n",
 	      chip_id, dts->temp, dts->trip);
 
-	/*
-	 * FIXME: The trip bits are always set ?! Just discard
-	 * them for the moment until we understand why.
-	 */
-	dts->trip = 0;
 	return 0;
 }