linux-kernel - Re: [net-next PATCH v4 4/7] net: ravb: Refactor GbEth RX code path

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]

Message-ID: <908c525b-10e2-464f-ad66-df431d48ca03@bp.renesas.com>
Date: Wed, 29 May 2024 20:07:03 +0100
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Sergey Shtylyov <s.shtylyov@....ru>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Biju Das <biju.das.jz@...renesas.com>,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
 Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
 netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH v4 4/7] net: ravb: Refactor GbEth RX code path

On 29/05/2024 19:30, Sergey Shtylyov wrote:
> On 5/28/24 6:03 PM, Paul Barker wrote:
> 
>> We can reduce code duplication in ravb_rx_gbeth().
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@...renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index 7df7d2e93a3a..c9c5cc641589 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -817,47 +817,54 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>>  				stats->rx_missed_errors++;
>>  		} else {
>>  			die_dt = desc->die_dt & 0xF0;
>> +			skb = ravb_get_skb_gbeth(ndev, entry, desc);
>>  			switch (die_dt) {
> 
>    Why not do instead (as I've asked you alraedy):
> 
> 			case DT_FSTART:
> 				priv->rx_1st_skb = skb;
> 				fallthrough;

I've avoided that change to keep patch 7/7 simpler (as we have to move
the assignment of skb later in that patch). I can change this if you
want though.

> 
>>  			case DT_FSINGLE:
>> -				skb = ravb_get_skb_gbeth(ndev, entry, desc);
> 
> 
>> +			case DT_FSTART:
>> +				/* Start of packet:
>> +				 * Set initial data length.
>> +				 */
> 
>    Please consider turning that block comment into one-liner...

Ack.

> 
>>  				skb_put(skb, desc_len);
>> +
>> +				/* Save this SKB if the packet spans multiple
>> +				 * descriptors.
>> +				 */
> 
>    This one too...
>    (The current line length limit is 100 columns.)

Ack. I'll re-flow some other lines with a 100 col limit as well - I'm
immediately thinking of the skb_copy_to_linear_data_offset call below.

> 
>> +				if (die_dt == DT_FSTART)
>> +					priv->rx_1st_skb = skb;
> 
>    This needs to be done under *case* DT_FSTART above instead...

See above comment. We can do this under DT_FSTART in this patch if you
want, but this if condition will then come back in a revised patch 7/7.

> 
>> +				break;
>> +
>> +			case DT_FMID:
>> +			case DT_FEND:
>> +				/* Continuing a packet:
>> +				 * Move data into the saved SKB.
>> +				 */
>> +				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
>> +							       priv->rx_1st_skb->len,
>> +							       skb->data,
>> +							       desc_len);
>> +				skb_put(priv->rx_1st_skb, desc_len);
>> +				dev_kfree_skb(skb);
>> +
>> +				/* Set skb to point at the whole packet so that
> 
>    Please call it consistently, either SKB or skb (I prefer this one).

Ack.

> 
>> +				 * we only need one code path for finishing a
>> +				 * packet.
>> +				 */
>> +				skb = priv->rx_1st_skb;
>> +			}
>> +
>> +			switch (die_dt) {
>> +			case DT_FSINGLE:
>> +			case DT_FEND:
>> +				/* Finishing a packet:
>> +				 * Determine protocol & checksum, hand off to
>> +				 * NAPI and update our stats.
>> +				 */
>>  				skb->protocol = eth_type_trans(skb, ndev);
>>  				if (ndev->features & NETIF_F_RXCSUM)
>>  					ravb_rx_csum_gbeth(skb);
>> +				stats->rx_bytes += skb->len;
>>  				napi_gro_receive(&priv->napi[q], skb);
>>  				rx_packets++;
> 
>    Otherwise, this is very good patch! Sorry for letting in the duplcate
> code earlier! :-)
> 
> [...]
> 
> MBR, Sergey

Thanks for the review!

-- 
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)