Message ID | 1345373950-16276-1-git-send-email-bfennell@skynet.ie |
---|---|
State | New |
Headers | show |
On Sun, Aug 19, 2012 at 10:59 AM, Brendan Fennell <bfennell@skynet.ie> wrote: > > Signed-off-by: Brendan Fennell <bfennell@skynet.ie> > --- > hw/pl190.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/hw/pl190.c b/hw/pl190.c > index cb50afb..eddb531 100644 > --- a/hw/pl190.c > +++ b/hw/pl190.c > @@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, > current priority level to that of the current interrupt. */ > for (i = 0; i < s->priority; i++) > { > - if ((s->level | s->soft_level) & s->prio_mask[i]) > + /* Ensure that 'i' is current highest priority interrupt on exit */ > + if ((s->level | s->soft_level) & s->prio_mask[i+1]) Missing braces and spaces around '+', please read CODING_STYLE. > break; > } > /* Reading this value with no pending interrupts is undefined. > -- > 1.7.2.5 > >
On 19 August 2012 11:59, Brendan Fennell <bfennell@skynet.ie> wrote: > > Signed-off-by: Brendan Fennell <bfennell@skynet.ie> > --- > hw/pl190.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/hw/pl190.c b/hw/pl190.c > index cb50afb..eddb531 100644 > --- a/hw/pl190.c > +++ b/hw/pl190.c > @@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, > current priority level to that of the current interrupt. */ > for (i = 0; i < s->priority; i++) > { > - if ((s->level | s->soft_level) & s->prio_mask[i]) > + /* Ensure that 'i' is current highest priority interrupt on exit */ > + if ((s->level | s->soft_level) & s->prio_mask[i+1]) > break; > } > /* Reading this value with no pending interrupts is undefined. > -- > 1.7.2.5 The technical content of this patch looks correct to me, and I've tested it on a versatilepb Linux image. (Presumably Linux doesn't make use of different vector addresses/priorities, which is why we haven't noticed this bug before now.) As Blue says, you need to fix the coding style issues (you can run your patch through scripts/checkpatch.pl to help with this). checkpatch is probably going to end up getting you to fix the indent on the whole for() loop, which is fine -- we usually fix up the coding style locally when we make a change. (the key bits of coding style here are 4 space indent, open-brace on same line as 'for' and 'if', braces mandatory even for single line 'if' bodies.) I also think we could improve on the comment text. Here's my suggestion (replaces the current just-above-the-loop comment): /* Read vector address at the start of an ISR. Increases the * current priority level to that of the current interrupt. * * Since an enabled interrupt X at priority P causes prio_mask[Y] * to have bit X set for all Y > P, this loop will stop with * i == the priority of the highest priority set interrupt. */ thanks -- PMM
On Mon, 20 Aug 2012, Peter Maydell wrote: > On 19 August 2012 11:59, Brendan Fennell <bfennell@skynet.ie> wrote: >> >> Signed-off-by: Brendan Fennell <bfennell@skynet.ie> >> --- >> hw/pl190.c | 3 ++- >> 1 files changed, 2 insertions(+), 1 deletions(-) >> >> diff --git a/hw/pl190.c b/hw/pl190.c >> index cb50afb..eddb531 100644 >> --- a/hw/pl190.c >> +++ b/hw/pl190.c >> @@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, >> current priority level to that of the current interrupt. */ >> for (i = 0; i < s->priority; i++) >> { >> - if ((s->level | s->soft_level) & s->prio_mask[i]) >> + /* Ensure that 'i' is current highest priority interrupt on exit */ >> + if ((s->level | s->soft_level) & s->prio_mask[i+1]) >> break; >> } >> /* Reading this value with no pending interrupts is undefined. >> -- >> 1.7.2.5 > > The technical content of this patch looks correct to me, and I've tested > it on a versatilepb Linux image. (Presumably Linux doesn't make use > of different vector addresses/priorities, which is why we haven't noticed > this bug before now.) > > As Blue says, you need to fix the coding style issues (you can run > your patch through scripts/checkpatch.pl to help with this). checkpatch > is probably going to end up getting you to fix the indent on the > whole for() loop, which is fine -- we usually fix up the coding style > locally when we make a change. (the key bits of coding style here are > 4 space indent, open-brace on same line as 'for' and 'if', braces > mandatory even for single line 'if' bodies.) Thanks for the helpful feedback - checkpatch.pl does indeed point out the coding style problems in the patch. > > I also think we could improve on the comment text. Here's my > suggestion (replaces the current just-above-the-loop comment): > > /* Read vector address at the start of an ISR. Increases the > * current priority level to that of the current interrupt. > * > * Since an enabled interrupt X at priority P causes prio_mask[Y] > * to have bit X set for all Y > P, this loop will stop with > * i == the priority of the highest priority set interrupt. > */ That explains the situation more clearly, thanks. V3 to follow. > > thanks > -- PMM > >
diff --git a/hw/pl190.c b/hw/pl190.c index cb50afb..eddb531 100644 --- a/hw/pl190.c +++ b/hw/pl190.c @@ -120,7 +120,8 @@ static uint64_t pl190_read(void *opaque, target_phys_addr_t offset, current priority level to that of the current interrupt. */ for (i = 0; i < s->priority; i++) { - if ((s->level | s->soft_level) & s->prio_mask[i]) + /* Ensure that 'i' is current highest priority interrupt on exit */ + if ((s->level | s->soft_level) & s->prio_mask[i+1]) break; } /* Reading this value with no pending interrupts is undefined.
Signed-off-by: Brendan Fennell <bfennell@skynet.ie> --- hw/pl190.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)