On 19 August 2012 11:59, Brendan Fennell <bfenn...@skynet.ie> wrote:
>
> Signed-off-by: Brendan Fennell <bfenn...@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
