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
| ||
|
Message-ID: <b26664c9-7df9-f2dc-ca49-3e5abd3dab70@huawei.com> Date: Wed, 10 May 2023 19:43:36 +0800 From: Yunsheng Lin <linyunsheng@...wei.com> To: Mengyuan Lou <mengyuanlou@...-swift.com>, <netdev@...r.kernel.org> CC: <jiawenwu@...stnetic.com> Subject: Re: [PATCH net-next v4 2/7] net: wangxun: libwx add rx offload functions On 2023/5/10 17:38, Mengyuan Lou wrote: ... > +/** > + * wx_rx_checksum - indicate in skb if hw indicated a good cksum > + * @ring: structure containing ring specific data > + * @rx_desc: current Rx descriptor being processed > + * @skb: skb currently being received and modified > + **/ > +static void wx_rx_checksum(struct wx_ring *ring, > + union wx_rx_desc *rx_desc, > + struct sk_buff *skb) > +{ > + struct wx_dec_ptype dptype = wx_decode_ptype(WX_RXD_PKTTYPE(rx_desc)); > + > + skb->ip_summed = CHECKSUM_NONE; > + skb_checksum_none_assert(skb); It does not make much to check skb->ip_summed when it is just set one line above. Also the "skb->ip_summed = CHECKSUM_NONE" seems unnecessary, as alloc/build_skb() all have the below to make sure skb->ip_summed is zero: memset(skb, 0, offsetofstruct sk_buff, tail)) > + /* Rx csum disabled */ > + if (!(ring->netdev->features & NETIF_F_RXCSUM)) > + return; > + > + /* if IPv4 header checksum error */ > + if ((wx_test_staterr(rx_desc, WX_RXD_STAT_IPCS) && > + wx_test_staterr(rx_desc, WX_RXD_ERR_IPE)) || > + (wx_test_staterr(rx_desc, WX_RXD_STAT_OUTERIPCS) && > + wx_test_staterr(rx_desc, WX_RXD_ERR_OUTERIPER))) { > + ring->rx_stats.csum_err++; > + return; > + } > + > + /* L4 checksum offload flag must set for the below code to work */ > + if (!wx_test_staterr(rx_desc, WX_RXD_STAT_L4CS)) > + return; > + > + /*likely incorrect csum if IPv6 Dest Header found */ What does "likely incorrect" mean here? If it is incorrect, does ring->rx_stats.csum_err need incrementing? > + if (dptype.prot != WX_DEC_PTYPE_PROT_SCTP && WX_RXD_IPV6EX(rx_desc)) > + return; > + > + /* if L4 checksum error */ > + if (wx_test_staterr(rx_desc, WX_RXD_ERR_TCPE)) { > + ring->rx_stats.csum_err++; > + return; > + } > + > + /* If there is an outer header present that might contain a checksum > + * we need to bump the checksum level by 1 to reflect the fact that > + * we are indicating we validated the inner checksum. > + */ > + if (dptype.etype >= WX_DEC_PTYPE_ETYPE_IG) { > + skb->csum_level = 1; > + skb->encapsulation = 1; > + } > + > + /* It must be a TCP or UDP or SCTP packet with a valid checksum */ > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + ring->rx_stats.csum_good_cnt++; > +} > + > +/** > + * wx_process_skb_fields - Populate skb header fields from Rx descriptor > + * @rx_ring: rx descriptor ring packet is being transacted on > + * @rx_desc: pointer to the EOP Rx descriptor > + * @skb: pointer to current skb being populated > + * > + * This function checks the ring, descriptor, and packet information in > + * order to populate the hash, checksum, VLAN, timestamp, protocol, and For now VLAN, timestamp are not populated yet. > + * other fields within the skb. > + **/ > +static void wx_process_skb_fields(struct wx_ring *rx_ring, > + union wx_rx_desc *rx_desc, > + struct sk_buff *skb) > +{ > + wx_rx_hash(rx_ring, rx_desc, skb); > + wx_rx_checksum(rx_ring, rx_desc, skb); > + skb_record_rx_queue(skb, rx_ring->queue_index); > + skb->protocol = eth_type_trans(skb, rx_ring->netdev); > +} > + > /** > * wx_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf > * @q_vector: structure containing interrupt and ring information > @@ -491,8 +586,8 @@ static int wx_clean_rx_irq(struct wx_q_vector *q_vector, > /* probably a little skewed due to removing CRC */ > total_rx_bytes += skb->len; > > - skb_record_rx_queue(skb, rx_ring->queue_index); > - skb->protocol = eth_type_trans(skb, rx_ring->netdev); > + /* populate checksum, timestamp, VLAN, and protocol */ > + wx_process_skb_fields(rx_ring, rx_desc, skb); > napi_gro_receive(&q_vector->napi, skb); > > /* update budget accounting */ > diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h > index 70f5fd168e40..69a9ed7bc2df 100644 > --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h > +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h > @@ -321,8 +321,31 @@ ... > + > +static inline struct wx_dec_ptype wx_decode_ptype(const u8 ptype) If the above is only used in one .c file, maybe it does not need to be in the .h file? > +{ > + return wx_ptype_lookup[ptype]; > +} > + > /* Host Interface Command Structures */ > struct wx_hic_hdr { > u8 cmd; > @@ -624,6 +853,11 @@ struct wx_queue_stats { > u64 bytes; > }; > > +struct wx_rx_queue_stats { > + u64 csum_good_cnt; > + u64 csum_err; > +}; > + > /* iterator for handling rings in ring container */ > #define wx_for_each_ring(posm, headm) \ > for (posm = (headm).ring; posm; posm = posm->next) > @@ -665,6 +899,9 @@ struct wx_ring { > > struct wx_queue_stats stats; > struct u64_stats_sync syncp; > + union { > + struct wx_rx_queue_stats rx_stats; > + }; > } ____cacheline_internodealigned_in_smp; > > struct wx_q_vector { > @@ -680,6 +917,7 @@ struct wx_q_vector { > struct napi_struct napi; > struct rcu_head rcu; /* to avoid race with update stats on free */ > > + bool netpoll_rx; Unused? > char name[IFNAMSIZ + 17]; > > /* for dynamic allocation of rings associated with this q_vector */ >
Powered by blists - more mailing lists