2009-12-15 18:40:24

by Roel Kluin

Subject: [PATCH] gianfar: Fix tests of unsigneds in gfar_parse_group()

interruptTransmit, -Receive and -Error are unsigned so the tests did not work.

This also changes that if irq_of_parse_and_map(np, 0) returns NO_IRQ, that
gfar_parse_group() returns an -EINVAL.

Signed-off-by: Roel Kluin <[email protected]>
---
Found using coccinelle: http://coccinelle.lip6.fr/

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 6850dc0..c411682 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -522,6 +522,7 @@ static int gfar_parse_group(struct device_node *np,
{
u32 *queue_mask;
u64 addr, size;
+ int irq;

addr = of_translate_address(np,
of_get_address(np, 0, &size, NULL));
@@ -530,20 +531,22 @@ static int gfar_parse_group(struct device_node *np,
if (!priv->gfargrp[priv->num_grps].regs)
return -ENOMEM;

- priv->gfargrp[priv->num_grps].interruptTransmit =
- irq_of_parse_and_map(np, 0);
+ irq = irq_of_parse_and_map(np, 0);
+ if (irq < 0)
+ return -EINVAL;
+ priv->gfargrp[priv->num_grps].interruptTransmit = irq;

/* If we aren't the FEC we have multiple interrupts */
if (model && strcasecmp(model, "FEC")) {
- priv->gfargrp[priv->num_grps].interruptReceive =
- irq_of_parse_and_map(np, 1);
- priv->gfargrp[priv->num_grps].interruptError =
- irq_of_parse_and_map(np,2);
- if (priv->gfargrp[priv->num_grps].interruptTransmit < 0 ||
- priv->gfargrp[priv->num_grps].interruptReceive < 0 ||
- priv->gfargrp[priv->num_grps].interruptError < 0) {
+ irq = irq_of_parse_and_map(np, 1);
+ if (irq < 0)
return -EINVAL;
- }
+ priv->gfargrp[priv->num_grps].interruptReceive = irq;
+
+ irq = irq_of_parse_and_map(np, 2);
+ if (irq < 0)
+ return -EINVAL;
+ priv->gfargrp[priv->num_grps].interruptError = irq;
}

priv->gfargrp[priv->num_grps].grp_id = priv->num_grps;

2009-12-16 05:02:52

by Kumar Gopalpet-B05799

Subject: RE: [PATCH] gianfar: Fix tests of unsigneds in gfar_parse_group()

>interruptTransmit, -Receive and -Error are unsigned so the
>tests did not work.
>
>This also changes that if irq_of_parse_and_map(np, 0) returns
>NO_IRQ, that
>gfar_parse_group() returns an -EINVAL.
>
>Signed-off-by: Roel Kluin <[email protected]>
>---
>Found using coccinelle: http://coccinelle.lip6.fr/
>
>diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
>index 6850dc0..c411682 100644
>--- a/drivers/net/gianfar.c
>+++ b/drivers/net/gianfar.c
>@@ -522,6 +522,7 @@ static int gfar_parse_group(struct
>device_node *np, {
> u32 *queue_mask;
> u64 addr, size;
>+ int irq;
>
> addr = of_translate_address(np,
> of_get_address(np, 0, &size, NULL)); @@
>-530,20 +531,22 @@ static int gfar_parse_group(struct device_node *np,
> if (!priv->gfargrp[priv->num_grps].regs)
> return -ENOMEM;
>
>- priv->gfargrp[priv->num_grps].interruptTransmit =
>- irq_of_parse_and_map(np, 0);
>+ irq = irq_of_parse_and_map(np, 0);
>+ if (irq < 0)
>+ return -EINVAL;
>+ priv->gfargrp[priv->num_grps].interruptTransmit = irq;
>
> /* If we aren't the FEC we have multiple interrupts */
> if (model && strcasecmp(model, "FEC")) {
>- priv->gfargrp[priv->num_grps].interruptReceive =
>- irq_of_parse_and_map(np, 1);
>- priv->gfargrp[priv->num_grps].interruptError =
>- irq_of_parse_and_map(np,2);
>- if
>(priv->gfargrp[priv->num_grps].interruptTransmit < 0 ||
>-
>priv->gfargrp[priv->num_grps].interruptReceive < 0 ||
>-
>priv->gfargrp[priv->num_grps].interruptError < 0) {
>+ irq = irq_of_parse_and_map(np, 1);
>+ if (irq < 0)
> return -EINVAL;
>- }
>+ priv->gfargrp[priv->num_grps].interruptReceive = irq;
>+
>+ irq = irq_of_parse_and_map(np, 2);
>+ if (irq < 0)
>+ return -EINVAL;
>+ priv->gfargrp[priv->num_grps].interruptError = irq;
> }
>
> priv->gfargrp[priv->num_grps].grp_id = priv->num_grps;


Won't it be better to change types of interruptTransmit, -Receive and
-Error
in struct gfar_priv_grp itself. Something like this:

struct gfar_priv_grp {
....
- unsigned int interruptTransmit;
- unsigned int interruptReceive;
- unsigned int interruptError;

+ unsigned int interruptTransmit;
+ unsigned int interruptReceive;
+ unsigned int interruptError;
....
}

--

Thanks
Sandeep

2009-12-16 05:05:06

by Kumar Gopalpet-B05799

Subject: RE: [PATCH] gianfar: Fix tests of unsigneds in gfar_parse_group()

>Won't it be better to change types of interruptTransmit,
>-Receive and -Error in struct gfar_priv_grp itself. Something
>like this:
>
>struct gfar_priv_grp {
>....
>- unsigned int interruptTransmit;
>- unsigned int interruptReceive;
>- unsigned int interruptError;
>
>+ unsigned int interruptTransmit;
>+ unsigned int interruptReceive;
>+ unsigned int interruptError;
>....
>}
>

Sorry for my earlier mail, What I meant is something like this:

struct gfar_priv_grp {
....
- unsigned int interruptTransmit;
- unsigned int interruptReceive;
- unsigned int interruptError;

+ int interruptTransmit;
+ int interruptReceive;
+ int interruptError;
....
}

--

Thanks
Sandeep

2009-12-16 15:48:24

by Roel Kluin

Subject: Re: [PATCH] gianfar: Fix tests of unsigneds in gfar_parse_group()

On Wed, Dec 16, 2009 at 6:04 AM, Kumar Gopalpet-B05799
<[email protected]> wrote:
>
>>Won't it be better to change types of interruptTransmit,
>>-Receive and -Error in  struct gfar_priv_grp itself. Something
>>like this:

> struct gfar_priv_grp {
> ....
> -       unsigned int interruptTransmit;
> -       unsigned int interruptReceive;
> -       unsigned int interruptError;
>
> +       int interruptTransmit;
> +       int interruptReceive;
> +       int interruptError;
> ....
> }
>
> --
>
> Thanks
> Sandeep

I don't think that's better. Only the error returned is signed,
If none occurred the irq thereafter is unsigned.

Thanks,

Roel