[4/4] psi: Properly mask errors in SEMR

Message ID 20180605071337.22915-5-joel@jms.id.au
State Accepted
Headers show
Series
  • Misc fixes
Related show

Commit Message

Joel Stanley June 5, 2018, 7:13 a.m.
It looks like this code intended to read PSIHB SEMR, mask out some of
the values, and write it back. Instead it writes the mask to the
register.

Found using scan-build.

Fixes: 39addc6a0f1f ("PSI: Reorganize PSI link down handling code")
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 hw/psi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ananth N Mavinakayanahalli June 5, 2018, 9:19 a.m. | #1
On Tue, Jun 05, 2018 at 04:43:37PM +0930, Joel Stanley wrote:
> It looks like this code intended to read PSIHB SEMR, mask out some of
> the values, and write it back. Instead it writes the mask to the
> register.
> 
> Found using scan-build.
> 
> Fixes: 39addc6a0f1f ("PSI: Reorganize PSI link down handling code")
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  hw/psi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/psi.c b/hw/psi.c
> index f7d8cd9a459d..f5168ba96765 100644
> --- a/hw/psi.c
> +++ b/hw/psi.c
> @@ -74,7 +74,7 @@ void psi_disable_link(struct psi *psi)
> 
>  		/* Mask errors in SEMR */
>  		reg = in_be64(psi->regs + PSIHB_SEMR);
> -		reg = ((0xfffull << 36) | (0xfffull << 20));
> +		reg &= ((0xfffull << 36) | (0xfffull << 20));
>  		out_be64(psi->regs + PSIHB_SEMR, reg);
>  		printf("PSI: SEMR set to %llx\n", reg);

This is actually the PSIHBCR error mask register, bits of which when set
will prevent the corresponding PSIHBCR bits to be set. The existing
logic is fine for this case...

Ananth
Joel Stanley June 6, 2018, 3:30 a.m. | #2
On 5 June 2018 at 18:49, Ananth N Mavinakayanahalli
<ananth@linux.vnet.ibm.com> wrote:
> On Tue, Jun 05, 2018 at 04:43:37PM +0930, Joel Stanley wrote:
>> It looks like this code intended to read PSIHB SEMR, mask out some of
>> the values, and write it back. Instead it writes the mask to the
>> register.
>>
>> Found using scan-build.
>>
>> Fixes: 39addc6a0f1f ("PSI: Reorganize PSI link down handling code")
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>  hw/psi.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/psi.c b/hw/psi.c
>> index f7d8cd9a459d..f5168ba96765 100644
>> --- a/hw/psi.c
>> +++ b/hw/psi.c
>> @@ -74,7 +74,7 @@ void psi_disable_link(struct psi *psi)
>>
>>               /* Mask errors in SEMR */
>>               reg = in_be64(psi->regs + PSIHB_SEMR);
>> -             reg = ((0xfffull << 36) | (0xfffull << 20));
>> +             reg &= ((0xfffull << 36) | (0xfffull << 20));
>>               out_be64(psi->regs + PSIHB_SEMR, reg);
>>               printf("PSI: SEMR set to %llx\n", reg);
>
> This is actually the PSIHBCR error mask register, bits of which when set
> will prevent the corresponding PSIHBCR bits to be set.

Okay, that makes sense.

> The existing
> logic is fine for this case...

The existing logic reads the value from PSIHB_SEMR, and then
overwrites it with a mask. That doesn't sound correct to me.
Ananth N Mavinakayanahalli June 7, 2018, 4:42 a.m. | #3
On Wed, Jun 06, 2018 at 01:00:54PM +0930, Joel Stanley wrote:
> On 5 June 2018 at 18:49, Ananth N Mavinakayanahalli
> <ananth@linux.vnet.ibm.com> wrote:
> > On Tue, Jun 05, 2018 at 04:43:37PM +0930, Joel Stanley wrote:
> >> It looks like this code intended to read PSIHB SEMR, mask out some of
> >> the values, and write it back. Instead it writes the mask to the
> >> register.
> >>
> >> Found using scan-build.
> >>
> >> Fixes: 39addc6a0f1f ("PSI: Reorganize PSI link down handling code")
> >> Signed-off-by: Joel Stanley <joel@jms.id.au>
> >> ---
> >>  hw/psi.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/psi.c b/hw/psi.c
> >> index f7d8cd9a459d..f5168ba96765 100644
> >> --- a/hw/psi.c
> >> +++ b/hw/psi.c
> >> @@ -74,7 +74,7 @@ void psi_disable_link(struct psi *psi)
> >>
> >>               /* Mask errors in SEMR */
> >>               reg = in_be64(psi->regs + PSIHB_SEMR);
> >> -             reg = ((0xfffull << 36) | (0xfffull << 20));
> >> +             reg &= ((0xfffull << 36) | (0xfffull << 20));
> >>               out_be64(psi->regs + PSIHB_SEMR, reg);
> >>               printf("PSI: SEMR set to %llx\n", reg);
> >
> > This is actually the PSIHBCR error mask register, bits of which when set
> > will prevent the corresponding PSIHBCR bits to be set.
> 
> Okay, that makes sense.
> 
> > The existing
> > logic is fine for this case...
> 
> The existing logic reads the value from PSIHB_SEMR, and then
> overwrites it with a mask. That doesn't sound correct to me.

What I meant to say was the value in the register does not change over
time. Your patch is fine, but it does not make a difference in this
case.

Stewart,
If you prefer Joel's change, you have my ack...

Acked-by: Ananth N Mavinakayanahalli <ananth@linux.vnet.ibm.com>

Patch

diff --git a/hw/psi.c b/hw/psi.c
index f7d8cd9a459d..f5168ba96765 100644
--- a/hw/psi.c
+++ b/hw/psi.c
@@ -74,7 +74,7 @@  void psi_disable_link(struct psi *psi)
 
 		/* Mask errors in SEMR */
 		reg = in_be64(psi->regs + PSIHB_SEMR);
-		reg = ((0xfffull << 36) | (0xfffull << 20));
+		reg &= ((0xfffull << 36) | (0xfffull << 20));
 		out_be64(psi->regs + PSIHB_SEMR, reg);
 		printf("PSI: SEMR set to %llx\n", reg);