| Message ID | 20240930160845.8520-6-paul@pbarker.dev (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Extend GbEth checksum offload support to VLAN/IPv6 packets | expand |
On 9/30/24 19:08, Paul Barker wrote: > From: Paul Barker <paul.barker.ct@bp.renesas.com> > > The HW checksum value is used as a 16-bit flag, it is zero when the I think I prefer s/HW/hardware/ but there's no hard feelings... :-) > checksum has been validated and non-zero otherwise. Therefore we don't > need to treat this as an actual __wsum type or call csum_unfold(), we > can just use a u16 pointer. > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 1dd2152734b0..9350ca10ab22 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c [...] > @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb) > * The last 2 bytes are the protocol checksum status which will be zero > * if the checksum has been validated. > */ > - if (unlikely(skb->len < sizeof(__sum16) * 2)) > + csum_len = sizeof(*hw_csum) * 2; Could've been done by an initializer instead? > + if (unlikely(skb->len < csum_len)) > return; > > if (skb_is_nonlinear(skb)) { > - last_frag = &shinfo->frags[shinfo->nr_frags - 1]; > - hw_csum = skb_frag_address(last_frag) + > - skb_frag_size(last_frag); > - skb_frag_size_sub(last_frag, 2 * sizeof(__sum16)); > + skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1]; Could've been done in the previous patch... > + > + hw_csum = (u16 *)(skb_frag_address(last_frag) + > + skb_frag_size(last_frag)); > + skb_frag_size_sub(last_frag, csum_len); > } else { > - hw_csum = skb_tail_pointer(skb); > - skb_trim(skb, skb->len - 2 * sizeof(__sum16)); > + hw_csum = (u16 *)skb_tail_pointer(skb); > + skb_trim(skb, skb->len - csum_len); > } > > - hw_csum -= sizeof(__sum16); > - csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); > - > - if (!csum_proto) > + if (!*--hw_csum) Hm, you lost get_unaligned_le16() here. The checksum can be anywhere, unaligned too... [...] MBR, Sergey
On 9/30/24 22:11, Sergey Shtylyov wrote: [...] >> From: Paul Barker <paul.barker.ct@bp.renesas.com> >> >> The HW checksum value is used as a 16-bit flag, it is zero when the > > I think I prefer s/HW/hardware/ but there's no hard feelings... :-) > >> checksum has been validated and non-zero otherwise. Therefore we don't >> need to treat this as an actual __wsum type or call csum_unfold(), we >> can just use a u16 pointer. >> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > [...] >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 1dd2152734b0..9350ca10ab22 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c > [...] >> @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb) [...] >> + if (unlikely(skb->len < csum_len)) >> return; >> >> if (skb_is_nonlinear(skb)) { >> - last_frag = &shinfo->frags[shinfo->nr_frags - 1]; >> - hw_csum = skb_frag_address(last_frag) + >> - skb_frag_size(last_frag); >> - skb_frag_size_sub(last_frag, 2 * sizeof(__sum16)); >> + skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1]; > > Could've been done in the previous patch... Even fit better there, I think... >> + >> + hw_csum = (u16 *)(skb_frag_address(last_frag) + >> + skb_frag_size(last_frag)); >> + skb_frag_size_sub(last_frag, csum_len); >> } else { >> - hw_csum = skb_tail_pointer(skb); >> - skb_trim(skb, skb->len - 2 * sizeof(__sum16)); >> + hw_csum = (u16 *)skb_tail_pointer(skb); >> + skb_trim(skb, skb->len - csum_len); >> } >> >> - hw_csum -= sizeof(__sum16); >> - csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); >> - >> - if (!csum_proto) >> + if (!*--hw_csum) > > Hm, you lost get_unaligned_le16() here. The checksum can be anywhere, > unaligned too... No need to keep using get_unaligned_le16() itself but you should then switch to using get_unaligned(). [...] MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 1dd2152734b0..9350ca10ab22 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -749,12 +749,11 @@ static void ravb_get_tx_tstamp(struct net_device *ndev) static void ravb_rx_csum_gbeth(struct sk_buff *skb) { struct skb_shared_info *shinfo = skb_shinfo(skb); - __wsum csum_proto; - skb_frag_t *last_frag; - u8 *hw_csum; + size_t csum_len; + u16 *hw_csum; - /* The hardware checksum status is contained in sizeof(__sum16) * 2 = 4 - * bytes appended to packet data. + /* The hardware checksum status is contained in 4 bytes appended to + * packet data. * * For ipv4, the first 2 bytes are the ip header checksum status. We can * ignore this as it will always be re-checked in inet_gro_receive(). @@ -762,23 +761,22 @@ static void ravb_rx_csum_gbeth(struct sk_buff *skb) * The last 2 bytes are the protocol checksum status which will be zero * if the checksum has been validated. */ - if (unlikely(skb->len < sizeof(__sum16) * 2)) + csum_len = sizeof(*hw_csum) * 2; + if (unlikely(skb->len < csum_len)) return; if (skb_is_nonlinear(skb)) { - last_frag = &shinfo->frags[shinfo->nr_frags - 1]; - hw_csum = skb_frag_address(last_frag) + - skb_frag_size(last_frag); - skb_frag_size_sub(last_frag, 2 * sizeof(__sum16)); + skb_frag_t *last_frag = &shinfo->frags[shinfo->nr_frags - 1]; + + hw_csum = (u16 *)(skb_frag_address(last_frag) + + skb_frag_size(last_frag)); + skb_frag_size_sub(last_frag, csum_len); } else { - hw_csum = skb_tail_pointer(skb); - skb_trim(skb, skb->len - 2 * sizeof(__sum16)); + hw_csum = (u16 *)skb_tail_pointer(skb); + skb_trim(skb, skb->len - csum_len); } - hw_csum -= sizeof(__sum16); - csum_proto = csum_unfold((__force __sum16)get_unaligned_le16(hw_csum)); - - if (!csum_proto) + if (!*--hw_csum) skb->ip_summed = CHECKSUM_UNNECESSARY; }