Re: [Qemu-devel] [PATCH v2] target-arm: GIC: bug fixes for arm_gic.c

[Peter Maydell <peter.maydell@linaro.org>]

On 7 December 2012 08:07, Daniel Sangorrin <daniel.sangorrin@gmail.com> wrote:
> Sorry, it seems that I did not understand the flow in the function
> "hw/arm_gic.c:gic_dist_writeb()" and made some mistaken assumptions in
> my previous patch. Please do not apply the previous patch, and apply
> this one instead if you consider that it is correct.
>
> target-arm: fix bug in irq value on arm_gic.c
>
> Fix a small bug that was using an incorrect IRQ
> value in the function gic_dist_writeb.
>
> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@gmail.com>

Thanks -- nice catch! The code change in this patch is correct,
but there are a minor couple of formatting issues with it.
If you can fix those and resend I can apply it to arm-devs.next.

Firstly, you should send patches as emails which only include
the commit message and patch (this one has a preamble 'Sorry...'
and a quoted copy of your previous email); otherwise when they
are applied this preamble ends up in the git commit log.
Secondly, it would be good to be more specific about the
problem being fixed (something like "Fix a bug where interrupts
not set pending on the correct target CPUs when they are
triggered by writes to the Interrupt Set Enable or Set Pending
registers").

(git format-patch / git send-email send mail in the right
format.)

> ---
>  hw/arm_gic.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index f9e423f..64d4e23 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -374,7 +374,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>            value = 0xff;
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) : GIC_TARGET(irq);
> +                int mask = (irq < GIC_INTERNAL) ? (1 << cpu) :
> GIC_TARGET(irq + i);

This line is too long. If you run scripts/checkpatch.pl on your patch
you'll see that it complains about this. (Also, since your mailer
is wrapping long lines, the result is a patch that doesn't apply
cleanly or display properly in patchwork:
http://patchwork.ozlabs.org/patch/204420/
)

I'd suggest a line break after the '='.

>                  int cm = (irq < GIC_INTERNAL) ? (1 << cpu) : ALL_CPU_MASK;
>
>                  if (!GIC_TEST_ENABLED(irq + i, cm)) {
> @@ -417,7 +417,7 @@ static void gic_dist_writeb(void *opaque, hwaddr offset,
>
>          for (i = 0; i < 8; i++) {
>              if (value & (1 << i)) {
> -                GIC_SET_PENDING(irq + i, GIC_TARGET(irq));
> +                GIC_SET_PENDING(irq + i, GIC_TARGET(irq + i));
>              }
>          }
>      } else if (offset < 0x300) {
> --

If you don't have time to make these fixes that's OK -- I can
fix the patch up in my tree. But they're good practice if you
want to send more patches to QEMU in future ;-)

-- PMM