2008-07-09 11:10:56

by Krzysztof Halasa

[permalink] [raw]
Subject: [FIX] ARM: IXP4xx Ethernet NAPI fix

This patch removes some weirdness from IXP4xx Ethernet driver.
Ideally it should go in 2.6.26 though it's not that critical. Thanks.

Signed-off-by: Krzysztof Ha?asa <[email protected]>

diff --git a/drivers/net/arm/ixp4xx_eth.c b/drivers/net/arm/ixp4xx_eth.c
index c617b64..9b777d9 100644
--- a/drivers/net/arm/ixp4xx_eth.c
+++ b/drivers/net/arm/ixp4xx_eth.c
@@ -522,7 +522,6 @@ static int eth_poll(struct napi_struct *napi, int budget)
#endif

if ((n = queue_get_desc(rxq, port, 0)) < 0) {
- received = 0; /* No packet received */
#if DEBUG_RX
printk(KERN_DEBUG "%s: eth_poll netif_rx_complete\n",
dev->name);
@@ -543,7 +542,7 @@ static int eth_poll(struct napi_struct *napi, int budget)
printk(KERN_DEBUG "%s: eth_poll all done\n",
dev->name);
#endif
- return 0; /* all work done */
+ return received; /* all work done */
}

desc = rx_desc_ptr(port, n);


2008-07-11 14:04:29

by Jeff Garzik

[permalink] [raw]
Subject: Re: [FIX] ARM: IXP4xx Ethernet NAPI fix

Krzysztof Halasa wrote:
> This patch removes some weirdness from IXP4xx Ethernet driver.
> Ideally it should go in 2.6.26 though it's not that critical. Thanks.
>
> Signed-off-by: Krzysztof Ha?asa <[email protected]>
>
> diff --git a/drivers/net/arm/ixp4xx_eth.c b/drivers/net/arm/ixp4xx_eth.c
> index c617b64..9b777d9 100644
> --- a/drivers/net/arm/ixp4xx_eth.c
> +++ b/drivers/net/arm/ixp4xx_eth.c
> @@ -522,7 +522,6 @@ static int eth_poll(struct napi_struct *napi, int budget)
> #endif
>
> if ((n = queue_get_desc(rxq, port, 0)) < 0) {
> - received = 0; /* No packet received */
> #if DEBUG_RX
> printk(KERN_DEBUG "%s: eth_poll netif_rx_complete\n",
> dev->name);
> @@ -543,7 +542,7 @@ static int eth_poll(struct napi_struct *napi, int budget)
> printk(KERN_DEBUG "%s: eth_poll all done\n",
> dev->name);
> #endif
> - return 0; /* all work done */
> + return received; /* all work done */

applied

2008-07-11 19:21:56

by Andrew Morton

[permalink] [raw]
Subject: Re: [FIX] ARM: IXP4xx Ethernet NAPI fix

On Wed, 09 Jul 2008 13:10:32 +0200 Krzysztof Halasa <[email protected]> wrote:

> This patch removes some weirdness from IXP4xx Ethernet driver.
> Ideally it should go in 2.6.26 though it's not that critical. Thanks.
>
> Signed-off-by: Krzysztof Ha__asa <[email protected]>
>
> diff --git a/drivers/net/arm/ixp4xx_eth.c b/drivers/net/arm/ixp4xx_eth.c
> index c617b64..9b777d9 100644
> --- a/drivers/net/arm/ixp4xx_eth.c
> +++ b/drivers/net/arm/ixp4xx_eth.c
> @@ -522,7 +522,6 @@ static int eth_poll(struct napi_struct *napi, int budget)
> #endif
>
> if ((n = queue_get_desc(rxq, port, 0)) < 0) {
> - received = 0; /* No packet received */
> #if DEBUG_RX
> printk(KERN_DEBUG "%s: eth_poll netif_rx_complete\n",
> dev->name);
> @@ -543,7 +542,7 @@ static int eth_poll(struct napi_struct *napi, int budget)
> printk(KERN_DEBUG "%s: eth_poll all done\n",
> dev->name);
> #endif
> - return 0; /* all work done */
> + return received; /* all work done */
> }
>
> desc = rx_desc_ptr(port, n);

This is a functional change, and I do not believe that "fixes some
weirdness" is an adequate description of it.

Please: what was wrong with the old code? Were the effects user-visible?
How does the new code fix those problems? Stuff like that.

You hint that this might be a 2.6.26-worthy change, but we should be able
to work this out ourselves from your description of it. But the description
was no good :(

2008-07-11 19:41:37

by Krzysztof Halasa

[permalink] [raw]
Subject: Re: [FIX] ARM: IXP4xx Ethernet NAPI fix

Andrew Morton <[email protected]> writes:

>> +++ b/drivers/net/arm/ixp4xx_eth.c
>> @@ -522,7 +522,6 @@ static int eth_poll(struct napi_struct *napi, int budget)
>> #endif
>>
>> if ((n = queue_get_desc(rxq, port, 0)) < 0) {
>> - received = 0; /* No packet received */
>> #if DEBUG_RX
>> printk(KERN_DEBUG "%s: eth_poll netif_rx_complete\n",
>> dev->name);
>> @@ -543,7 +542,7 @@ static int eth_poll(struct napi_struct *napi, int budget)
>> printk(KERN_DEBUG "%s: eth_poll all done\n",
>> dev->name);
>> #endif
>> - return 0; /* all work done */
>> + return received; /* all work done */
>> }
>>
>> desc = rx_desc_ptr(port, n);
>
> This is a functional change, and I do not believe that "fixes some
> weirdness" is an adequate description of it.

Well, tried my best :-)

> Please: what was wrong with the old code?

It shouldn't do anything like that, the function has to return the
number of received packets. I.e., it was not up to the NAPI specs. Now
it does what NAPI wants, hopefully. I have no idea why did I put that
in :-(

> Were the effects user-visible?

I don't know. Actually, for me it works the same before and after the
change.
--
Krzysztof Halasa