Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Message-ID: <18552cff6fe32d4c21b4751cd6be4ff4757c63e8.camel@redhat.com>
Subject: Re: [PATCH net-next v14 07/13] rtase: Implement a function to
 receive packets
From:   Paolo Abeni <pabeni@redhat.com>
To:     Justin Lai <justinlai0215@realtek.com>, kuba@kernel.org
Cc:     davem@davemloft.net, edumazet@google.com,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        andrew@lunn.ch, pkshih@realtek.com, larry.chiu@realtek.com
Date:   Tue, 12 Dec 2023 11:04:52 +0100
In-Reply-To: <20231208094733.1671296-8-justinlai0215@realtek.com>
References: <20231208094733.1671296-1-justinlai0215@realtek.com>
         <20231208094733.1671296-8-justinlai0215@realtek.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

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.
>=20
> Signed-off-by: Justin Lai <justinlai0215@realtek.com>
> ---
>  .../net/ethernet/realtek/rtase/rtase_main.c   | 148 ++++++++++++++++++
>  1 file changed, 148 insertions(+)
>=20
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/ne=
t/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)
>  	}
>  }
> =20
> +static int rtase_fragmented_frame(u32 status)
> +{
> +	return (status & (RX_FIRST_FRAG | RX_LAST_FRAG)) !=3D
> +		(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 =3D 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 =3D CHECKSUM_UNNECESSARY;
> +		} else {
> +			skb->ip_summed =3D CHECKSUM_NONE;
> +		}
> +	} else {
> +		skb->ip_summed =3D CHECKSUM_NONE;
> +	}
> +}
> +
> +static void rtase_rx_vlan_skb(union rx_desc *desc, struct sk_buff *skb)
> +{
> +	u32 opts2 =3D 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 =3D ring->ivec;
> +
> +	napi_gro_receive(&ivec->napi, skb);
> +}
> +
> +static int rx_handler(struct rtase_ring *ring, int budget)
> +{
> +	const struct rtase_private *tp =3D ring->ivec->tp;
> +	u32 pkt_size, cur_rx, delta, entry, status;
> +	union rx_desc *desc_base =3D ring->desc;
> +	struct net_device *dev =3D tp->dev;
> +	struct sk_buff *skb;
> +	union rx_desc *desc;
> +	int workdone =3D 0;
> +
> +	if (!ring->desc)
> +		return workdone;

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

> +
> +	cur_rx =3D ring->cur_idx;
> +	entry =3D cur_rx % NUM_DESC;
> +	desc =3D &desc_base[entry];
> +
> +	do {
> +		/* make sure discriptor has been updated */
> +		rmb();
> +		status =3D 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 =3D %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