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

[Paul Barker]

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@xxxxxxxxxxxxxx>
> [...]
> 
>> 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

Attachment: OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature