Fw: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

Fw: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

[Stephen Hemminger]

Begin forwarded message:

Date: Thu, 10 Jul 2014 04:25:50 -0700
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?


https://bugzilla.kernel.org/show_bug.cgi?id=79821

            Bug ID: 79821
           Summary: ethernet/freescale/gianfar_ethtool.c:1584: possible
                    bad expression ?
           Product: Networking
           Version: 2.5
    Kernel Version: 3.16-rc4
          Hardware: All
                OS: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: Other
          Assignee: shemminger@linux-foundation.org
          Reporter: dcb314@hotmail.com
        Regression: No

[linux-3.16-rc4/drivers/net/ethernet/freescale/gianfar_ethtool.c:1584]: (style)
Same expression on both sides of '|'.

    for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Thu, 10 Jul 2014 05:51:00 -0700

> [linux-3.16-rc4/drivers/net/ethernet/freescale/gianfar_ethtool.c:1584]: (style)
> Same expression on both sides of '|'.
> 
>     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);

Probably this is meant to be:

diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 76d7070..f697118 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -1581,7 +1581,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
 		return -EBUSY;
 
 	/* Fill regular entries */
-	for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
+	for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].prop);
 	     i++)
 		gfar_write_filer(priv, i, tab->fe[i].ctrl, tab->fe[i].prop);
 	/* Fill the rest with fall-troughs */

But only a Gianfar expert can say for sure.

Sebastian, this is your code, please help us out.

Re: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 10 Jul 2014 17:06:10 -0700 (PDT)

> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Thu, 10 Jul 2014 05:51:00 -0700
> 
>> [linux-3.16-rc4/drivers/net/ethernet/freescale/gianfar_ethtool.c:1584]: (style)
>> Same expression on both sides of '|'.
>> 
>>     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> 
> Probably this is meant to be:
> 
> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> index 76d7070..f697118 100644
> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
> @@ -1581,7 +1581,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
>  		return -EBUSY;
>  
>  	/* Fill regular entries */
> -	for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
> +	for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].prop);
>  	     i++)
>  		gfar_write_filer(priv, i, tab->fe[i].ctrl, tab->fe[i].prop);
>  	/* Fill the rest with fall-troughs */
> 
> But only a Gianfar expert can say for sure.
> 
> Sebastian, this is your code, please help us out.

Ok, we have a problem, Sebastian's email bounces.

Anyone else who knows this chip can help us out?  We don't have a listed
maintainer for Gianfar in MAINTAINERS :-/

Re: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

[Fabio Estevam]

On Thu, Jul 10, 2014 at 9:07 PM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Thu, 10 Jul 2014 17:06:10 -0700 (PDT)
>
>> From: Stephen Hemminger <stephen@networkplumber.org>
>> Date: Thu, 10 Jul 2014 05:51:00 -0700
>>
>>> [linux-3.16-rc4/drivers/net/ethernet/freescale/gianfar_ethtool.c:1584]: (style)
>>> Same expression on both sides of '|'.
>>>
>>>     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
>>
>> Probably this is meant to be:
>>
>> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> index 76d7070..f697118 100644
>> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> @@ -1581,7 +1581,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
>>               return -EBUSY;
>>
>>       /* Fill regular entries */
>> -     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
>> +     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].prop);
>>            i++)
>>               gfar_write_filer(priv, i, tab->fe[i].ctrl, tab->fe[i].prop);
>>       /* Fill the rest with fall-troughs */
>>
>> But only a Gianfar expert can say for sure.
>>
>> Sebastian, this is your code, please help us out.
>
> Ok, we have a problem, Sebastian's email bounces.
>
> Anyone else who knows this chip can help us out?  We don't have a listed
> maintainer for Gianfar in MAINTAINERS :-/

Adding Claudiu on Cc in case he could help.

Re: [Bug 79821] New: ethernet/freescale/gianfar_ethtool.c:1584: possible bad expression ?

[Claudiu Manoil]

On 7/11/2014 3:37 AM, Fabio Estevam wrote:
> On Thu, Jul 10, 2014 at 9:07 PM, David Miller <davem@davemloft.net> wrote:
>> From: David Miller <davem@davemloft.net>
>> Date: Thu, 10 Jul 2014 17:06:10 -0700 (PDT)
>>
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>> Date: Thu, 10 Jul 2014 05:51:00 -0700
>>>
>>>> [linux-3.16-rc4/drivers/net/ethernet/freescale/gianfar_ethtool.c:1584]: (style)
>>>> Same expression on both sides of '|'.
>>>>
>>>>      for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
>>>
>>> Probably this is meant to be:
>>>
>>> diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>>> index 76d7070..f697118 100644
>>> --- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
>>> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>>> @@ -1581,7 +1581,7 @@ static int gfar_write_filer_table(struct gfar_private *priv,
>>>                return -EBUSY;
>>>
>>>        /* Fill regular entries */
>>> -     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].ctrl);
>>> +     for (; i < MAX_FILER_IDX - 1 && (tab->fe[i].ctrl | tab->fe[i].prop);
>>>             i++)
>>>                gfar_write_filer(priv, i, tab->fe[i].ctrl, tab->fe[i].prop);
>>>        /* Fill the rest with fall-troughs */
>>>
>>> But only a Gianfar expert can say for sure.
>>>
>>> Sebastian, this is your code, please help us out.
>>
>> Ok, we have a problem, Sebastian's email bounces.
>>
>> Anyone else who knows this chip can help us out?  We don't have a listed
>> maintainer for Gianfar in MAINTAINERS :-/
>
> Adding Claudiu on Cc in case he could help.
>

Hi,

Thanks for the notification, Fabio.
I'm checking the hardware manual, and looks like 0 is a valid value for 
the ctrl field, while the prop can be non-zero.  So very likely the fix 
above is correct.  Not sure why no run-time issues were reported for 
this, either a 0 crtl value is unlikely or the Rx classification feature
implemented in gianfar ethtool is hardly used.
I'll get back with a patch soon, unless someone else beats me to it.

Thanks,
Claudiu