Re: [PATCH net-next v14 07/13] rtase: Implement a function to receive packets

Paolo Abeni
2023-12-12T10:10:15Z

On Fri, 2023-12-08 at 17:47 +0800, Justin Lai wrote:
> Implement rx_handler to read the information of the rx descriptor,
> thereby checking the packet accordingly and storing the packet
> in the socket buffer to complete the reception of the packet.
>
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> ---
> .../net/ethernet/realtek/rtase/rtase_main.c | 148 ++++++++++++++++++
> 1 file changed, 148 insertions(+)
>
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index eee792ea4760..83a119389110 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -451,6 +451,154 @@ static void rtase_rx_ring_clear(struct rtase_ring *ring)
> }
> }
>
> +static int rtase_fragmented_frame(u32 status)
> +{
> + return (status & (RX_FIRST_FRAG | RX_LAST_FRAG)) !=
> + (RX_FIRST_FRAG | RX_LAST_FRAG);
> +}
> +
> +static void rtase_rx_csum(const struct rtase_private *tp, struct sk_buff *skb,
> + const union rx_desc *desc)
> +{
> + u32 opts2 = le32_to_cpu(desc->desc_status.opts2);
> +
> + /* rx csum offload */
> + if (((opts2 & RX_V4F) && !(opts2 & RX_IPF)) || (opts2 & RX_V6F)) {
> + if (((opts2 & RX_TCPT) && !(opts2 & RX_TCPF)) ||
> + ((opts2 & RX_UDPT) && !(opts2 & RX_UDPF))) {
> + skb->ip_summed = CHECKSUM_UNNECESSARY;
> + } else {
> + skb->ip_summed = CHECKSUM_NONE;
> + }
> + } else {
> + skb->ip_summed = CHECKSUM_NONE;
> + }
> +}
> +
> +static void rtase_rx_vlan_skb(union rx_desc *desc, struct sk_buff *skb)
> +{
> + u32 opts2 = le32_to_cpu(desc->desc_status.opts2);
> +
> + if (!(opts2 & RX_VLAN_TAG))
> + return;
> +
> + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & VLAN_TAG_MASK));
> +}
> +
> +static void rtase_rx_skb(const struct rtase_ring *ring, struct sk_buff *skb)
> +{
> + struct rtase_int_vector *ivec = ring->ivec;
> +
> + napi_gro_receive(&ivec->napi, skb);
> +}
> +
> +static int rx_handler(struct rtase_ring *ring, int budget)
> +{
> + const struct rtase_private *tp = ring->ivec->tp;
> + u32 pkt_size, cur_rx, delta, entry, status;
> + union rx_desc *desc_base = ring->desc;
> + struct net_device *dev = tp->dev;
> + struct sk_buff *skb;
> + union rx_desc *desc;
> + int workdone = 0;
> +
> + if (!ring->desc)
> + return workdone;

Why is the above test required? How can be ring->desc NULL here?

> +
> + cur_rx = ring->cur_idx;
> + entry = cur_rx % NUM_DESC;
> + desc = &desc_base[entry];
> +
> + do {
> + /* make sure discriptor has been updated */
> + rmb();
> + status = le32_to_cpu(desc->desc_status.opts1);
> +
> + if (status & DESC_OWN)
> + break;
> +
> + if (unlikely(status & RX_RES)) {
> + if (net_ratelimit())
> + netdev_warn(dev, "Rx ERROR. status = %08x\n",
> + status);
> +
> + dev->stats.rx_errors++;
> +
> + if (status & (RX_RWT | RX_RUNT))
> + dev->stats.rx_length_errors++;

The device has a single RX queue, right? Otherwise this kind of stats
accounting is going to be costly.

Cheers,

Paolo