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