| Message ID | 1320871595-10304-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | afd4a65225318ec7900871ed69a46ce992637ef2 |
| Headers | show |
On 11/09/2011 02:46 PM, Peter Maydell wrote: > Fix a bug in handling the write-one-to-clear bits in the PMCR > which meant that we would always clear the bit even if the > value written was a zero. Spotted by Coverity (see bug 887883). > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> Applied. Thanks. Regards, Anthony Liguori > --- > hw/pxa2xx.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c > index bfc28a9..d38b922 100644 > --- a/hw/pxa2xx.c > +++ b/hw/pxa2xx.c > @@ -114,7 +114,9 @@ static void pxa2xx_pm_write(void *opaque, target_phys_addr_t addr, > > switch (addr) { > case PMCR: > - s->pm_regs[addr>> 2]&= 0x15& ~(value& 0x2a); > + /* Clear the write-one-to-clear bits... */ > + s->pm_regs[addr>> 2]&= ~(value& 0x2a); > + /* ...and set the plain r/w bits */ > s->pm_regs[addr>> 2] |= value& 0x15; > break; >
Hi, On 11 November 2011 20:45, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 11/09/2011 02:46 PM, Peter Maydell wrote: >> >> Fix a bug in handling the write-one-to-clear bits in the PMCR >> which meant that we would always clear the bit even if the >> value written was a zero. Spotted by Coverity (see bug 887883). >> >> Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > > Applied. Thanks. > > Regards, > > Anthony Liguori > >> --- >> hw/pxa2xx.c | 4 +++- >> 1 files changed, 3 insertions(+), 1 deletions(-) >> >> diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c >> index bfc28a9..d38b922 100644 >> --- a/hw/pxa2xx.c >> +++ b/hw/pxa2xx.c >> @@ -114,7 +114,9 @@ static void pxa2xx_pm_write(void *opaque, >> target_phys_addr_t addr, >> >> switch (addr) { >> case PMCR: >> - s->pm_regs[addr>> 2]&= 0x15& ~(value& 0x2a); >> + /* Clear the write-one-to-clear bits... */ >> + s->pm_regs[addr>> 2]&= ~(value& 0x2a); >> + /* ...and set the plain r/w bits */ >> s->pm_regs[addr>> 2] |= value& 0x15; As I was about to push these patches also, I noticed this isn't exactly setting the r/w bits. But it would work if the first line was (~value) & 0x2a instead, should I fix it this way, am I looking at it right? Cheers
On 11 November 2011 21:05, andrzej zaborowski <balrogg@gmail.com> wrote: >>> - s->pm_regs[addr>> 2]&= 0x15& ~(value& 0x2a); >>> + /* Clear the write-one-to-clear bits... */ >>> + s->pm_regs[addr>> 2]&= ~(value& 0x2a); >>> + /* ...and set the plain r/w bits */ >>> s->pm_regs[addr>> 2] |= value& 0x15; > > As I was about to push these patches also, I noticed this isn't > exactly setting the r/w bits. But it would work if the first line was > (~value) & 0x2a instead, should I fix it this way, am I looking at it > right? Rats, you're right. Your fix works (although it renders the comment wrong as it's then not just clearing the W1C bits). Alternatively add an extra line to give: /* Clear the write-one-to-clear bits... */ s->pm_regs[addr >> 2] &= ~(value & 0x2a); /* ...and set the plain r/w bits */ s->pm_regs[addr >> 2] &= ~0x15; s->pm_regs[addr>> 2] |= value & 0x15; (this is slightly different in effect from your code in that it leaves register bits [31:5] untouched where yours will always clear them.) Feel free to do whichever you think is clearest. -- PMM
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c index bfc28a9..d38b922 100644 --- a/hw/pxa2xx.c +++ b/hw/pxa2xx.c @@ -114,7 +114,9 @@ static void pxa2xx_pm_write(void *opaque, target_phys_addr_t addr, switch (addr) { case PMCR: - s->pm_regs[addr >> 2] &= 0x15 & ~(value & 0x2a); + /* Clear the write-one-to-clear bits... */ + s->pm_regs[addr >> 2] &= ~(value & 0x2a); + /* ...and set the plain r/w bits */ s->pm_regs[addr >> 2] |= value & 0x15; break;
Fix a bug in handling the write-one-to-clear bits in the PMCR which meant that we would always clear the bit even if the value written was a zero. Spotted by Coverity (see bug 887883). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/pxa2xx.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)