Re: [PATCH net-next 2/3] net: dsa: vsc73xx: implement packet reception via control interface

[Simon Horman]

On Sun, Oct 20, 2024 at 10:54:51PM +0200, Pawel Dembicki wrote:
> Some types of packets can be forwarded only to and from the PI/SI
> interface. For more information, see Chapter 2.7.1 (CPU Forwarding) in
> the datasheet.
> 
> This patch implements the routines required for link-local reception.
> This kind of traffic can't be transferred through the RGMII interface in
> vsc73xx.
> 
> The packet receiver poller uses a kthread worker, which checks if a packet
> has arrived in the CPU buffer. If the header is valid, the packet is
> transferred to the correct DSA conduit interface.
> 
> Signed-off-by: Pawel Dembicki <paweldembicki@xxxxxxxxx>

Hi Pawel,

This is not a full review, but I noticed a problem that I wanted to bring
to your attention. Please wait a day or so for others to provide a proper
review before posting a v2.

Thanks!

> ---
>  drivers/net/dsa/vitesse-vsc73xx-core.c | 174 +++++++++++++++++++++++++
>  drivers/net/dsa/vitesse-vsc73xx.h      |   4 +
>  2 files changed, 178 insertions(+)
> 
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c

...

> @@ -373,6 +415,7 @@
>  #define VSC73XX_POLL_SLEEP_US		1000
>  #define VSC73XX_MDIO_POLL_SLEEP_US	5
>  #define VSC73XX_POLL_TIMEOUT_US		10000
> +#define VSC73XX_RCV_POLL_INTERVAL	100
>  
>  #define VSC73XX_IFH_MAGIC		0x52
>  #define VSC73XX_IFH_SIZE		8
> @@ -834,6 +877,115 @@ static void vsc73xx_deferred_xmit(struct kthread_work *work)
>  	kfree(xmit_work);
>  }
>  
> +static void vsc73xx_polled_rcv(struct kthread_work *work)
> +{
> +	struct vsc73xx *vsc = container_of(work, struct vsc73xx, dwork.work);
> +	u16 ptr = VSC73XX_CAPT_FRAME_DATA;
> +	struct dsa_switch *ds = vsc->ds;
> +	int ret, buf_len, len, part;
> +	struct vsc73xx_ifh ifh;
> +	struct net_device *dev;
> +	struct dsa_port *dp;
> +	struct sk_buff *skb;
> +	u32 val, *buf;
> +	u16 count;
> +
> +	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_SYSTEM, 0, VSC73XX_CAPCTRL, &val);
> +	if (ret)
> +		goto queue;
> +
> +	if (!(val & VSC73XX_CAPCTRL_QUEUE0_READY))
> +		/* No frame to read */
> +		goto queue;
> +
> +	/* Initialise reading */
> +	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_CAPTURE, VSC73XX_BLOCK_CAPT_Q0,
> +			   VSC73XX_CAPT_CAPREADP, &val);
> +	if (ret)
> +		goto queue;
> +
> +	/* Get internal frame header */
> +	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_CAPTURE,
> +			   VSC73XX_BLOCK_CAPT_FRAME0, ptr++, &ifh.datah);
> +	if (ret)
> +		goto queue;
> +
> +	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_CAPTURE,
> +			   VSC73XX_BLOCK_CAPT_FRAME0, ptr++, &ifh.datal);
> +	if (ret)
> +		goto queue;
> +
> +	if (ifh.magic != VSC73XX_IFH_MAGIC) {
> +		/* Something goes wrong with buffer. Reset capture block */
> +		vsc73xx_write(vsc, VSC73XX_BLOCK_CAPTURE,
> +			      VSC73XX_BLOCK_CAPT_RST, VSC73XX_CAPT_CAPRST, 1);
> +		goto queue;
> +	}
> +
> +	if (!dsa_is_user_port(ds, ifh.port))
> +		goto release_frame;
> +
> +	dp = dsa_to_port(ds, ifh.port);
> +	dev = dp->user;
> +	if (!dev)
> +		goto release_frame;
> +
> +	count = (ifh.frame_length + 7 + VSC73XX_IFH_SIZE - ETH_FCS_LEN) >> 2;
> +
> +	skb = netdev_alloc_skb(dev, len);

len does not appear to be initialised here.

Flagged by W=1 builds.

> +	if (unlikely(!skb)) {
> +		netdev_err(dev, "Unable to allocate sk_buff\n");
> +		goto release_frame;
> +	}
> +
> +	buf_len = ifh.frame_length - ETH_FCS_LEN;
> +	buf = (u32 *)skb_put(skb, buf_len);
> +	len = 0;
> +	part = 0;
> +
> +	while (ptr < count) {
> +		ret = vsc73xx_read(vsc, VSC73XX_BLOCK_CAPTURE,
> +				   VSC73XX_BLOCK_CAPT_FRAME0 + part, ptr++,
> +				   buf + len);
> +		if (ret)
> +			goto free_skb;
> +		len++;
> +		if (ptr > VSC73XX_CAPT_FRAME_DATA_MAX &&
> +		    count != VSC73XX_CAPT_FRAME_DATA_MAX) {
> +			ptr = VSC73XX_CAPT_FRAME_DATA;
> +			part++;
> +			count -= VSC73XX_CAPT_FRAME_DATA_MAX;
> +		}
> +	}
> +
> +	/* Get FCS */
> +	ret = vsc73xx_read(vsc, VSC73XX_BLOCK_CAPTURE,
> +			   VSC73XX_BLOCK_CAPT_FRAME0, ptr++, &val);
> +	if (ret)
> +		goto free_skb;
> +
> +	/* Everything we see on an interface that is in the HW bridge
> +	 * has already been forwarded.
> +	 */
> +	if (dp->bridge)
> +		skb->offload_fwd_mark = 1;
> +
> +	skb->protocol = eth_type_trans(skb, dev);
> +
> +	netif_rx(skb);
> +	goto release_frame;
> +
> +free_skb:
> +	kfree_skb(skb);
> +release_frame:
> +	/* Release the frame from internal buffer */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_CAPTURE, VSC73XX_BLOCK_CAPT_Q0,
> +		      VSC73XX_CAPT_CAPREADP, 0);
> +queue:
> +	kthread_queue_delayed_work(vsc->rcv_worker, &vsc->dwork,
> +				   msecs_to_jiffies(VSC73XX_RCV_POLL_INTERVAL));
> +}

...

-- 
pw-bot: changes-requested