netdev - Re: [PATCH 1/1] KS8695: update ksp->next_rx_desc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Mar 2010 10:39:30 +0100
From:	Yegor Yefremov <yegorslists@...glemail.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	netdev <netdev@...r.kernel.org>, davem@...emloft.net
Subject: Re: [PATCH 1/1] KS8695: update ksp->next_rx_desc_read at the end of 
	rx loop

On Tue, Mar 16, 2010 at 4:52 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> Le mardi 16 mars 2010 à 16:42 +0100, Yegor Yefremov a écrit :
>> KS8695: update ksp->next_rx_desc_read at the end of rx loop
>>
>> There is no need to adjust the next rx descriptor after each packet,
>> so do it only once at the end of the routine.
>>
>> Signed-off-by: Yegor Yefremov <yegorslists@...glemail.com>
>>
>> Index: linux-2.6.34-rc1/drivers/net/arm/ks8695net.c
>> ===================================================================
>> --- linux-2.6.34-rc1.orig/drivers/net/arm/ks8695net.c
>> +++ linux-2.6.34-rc1/drivers/net/arm/ks8695net.c
>> @@ -538,12 +538,13 @@ rx_finished:
>>                        */
>>                       last_rx_processed = buff_n;
>>                       buff_n = (buff_n + 1) & MAX_RX_DESC_MASK;
>> -                     /*And note which RX descriptor we last did */
>> -                     if (likely(last_rx_processed != -1))
>> -                             ksp->next_rx_desc_read =
>> -                                     (last_rx_processed + 1) &
>> -                                     MAX_RX_DESC_MASK;
>>       }
>> +
>> +     /*And note which RX descriptor we last did */
>> +     if (likely(last_rx_processed != -1))
>> +             ksp->next_rx_desc_read =
>> +                     (last_rx_processed + 1) & MAX_RX_DESC_MASK;
>> +
>
> Very strange to see all these computations...
>
> At this point, why not use
>
> if (likely(last_rx_processed != -1))
>        ksp->next_rx_desc_read = buff_n;
>
> or even get rid of last_rx_processed completely and do :
>
>
> diff --git a/drivers/net/arm/ks8695net.c b/drivers/net/arm/ks8695net.c
> index a1d4188..ac6ab04 100644
> --- a/drivers/net/arm/ks8695net.c
> +++ b/drivers/net/arm/ks8695net.c
> @@ -461,7 +461,6 @@ static int ks8695_rx(struct ks8695_priv *ksp, int budget)
>        int buff_n;
>        u32 flags;
>        int pktlen;
> -       int last_rx_processed = -1;
>        int received = 0;
>
>        buff_n = ksp->next_rx_desc_read;
> @@ -533,17 +532,9 @@ rx_failure:
>                        ksp->rx_ring[buff_n].status = cpu_to_le32(RDES_OWN);
>  rx_finished:
>                        received++;
> -                       /* And note this as processed so we can start
> -                        * from here next time
> -                        */
> -                       last_rx_processed = buff_n;
>                        buff_n = (buff_n + 1) & MAX_RX_DESC_MASK;
> -                       /*And note which RX descriptor we last did */
> -                       if (likely(last_rx_processed != -1))
> -                               ksp->next_rx_desc_read =
> -                                       (last_rx_processed + 1) &
> -                                       MAX_RX_DESC_MASK;
>        }
> +       ksp->next_rx_desc_read = buff_n;
>        /* And refill the buffers */
>        ks8695_refill_rxbuffers(ksp);

Thank you for reviewing. Great idea. This simplifies the driver a
little. I tested your patch and everything is working as before. I
also added your signed-off-by and made some beatifications.

Yegor