| Message ID | 20180605071337.22915-5-joel@jms.id.au |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Misc fixes | expand |
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
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.
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>
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);
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(-)