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

[Ananth N Mavinakayanahalli]

On Wed, Jun 06, 2018 at 01:00:54PM +0930, Joel Stanley wrote:
> On 5 June 2018 at 18:49, Ananth N Mavinakayanahalli
> <ananth at 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 at 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 at linux.vnet.ibm.com>