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

[Vladimir Oltean <olteanv@gmail.com>]

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@gmail.com>
> ---

Is there no way to use an IRQ for packet reception from the PI/SI interface?
And if not, is there no way to use any workaround to do event-based reception?
Like for example how felix has the quirk_no_xtr_irq mechanism through which
it replicates all traffic that would go to the PI/SI interface to also go
over Ethernet, and use the reception of the packet as the trigger for something
being available to read from PI/SI?

> +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);

Log that?

> +		goto queue;
> +	}
> +
> +	if (!dsa_is_user_port(ds, ifh.port))
> +		goto release_frame;

First do the dsa_to_port(), and then convert this to dsa_port_is_user().

> +
> +	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;

What's "(.. + 7) >> 2" doing? Some sort of DIV_ROUND_UP(..., 4)? But why 7?
Please don't be afraid to use the arithmetic macros that make the code
more readable. You can confirm with "make drivers/net/dsa/vitesse-vsc73xx-core.lst"
that they should end up generating code that is just as efficient as the
"optimized" bit shift.

> +
> +	skb = netdev_alloc_skb(dev, len);
> +	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:

Log errors with dev_err_ratelimited() maybe?

> +	kthread_queue_delayed_work(vsc->rcv_worker, &vsc->dwork,
> +				   msecs_to_jiffies(VSC73XX_RCV_POLL_INTERVAL));
> +}
> +
>  static int
>  vsc73xx_connect_tag_protocol(struct dsa_switch *ds, enum dsa_tag_protocol proto)
>  {
> @@ -1111,14 +1263,36 @@ static int vsc73xx_setup(struct dsa_switch *ds)
>  	ret = dsa_tag_8021q_register(ds, htons(ETH_P_8021Q));
>  	rtnl_unlock();
>  
> +	/* Reset capture block */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_CAPTURE, VSC73XX_BLOCK_CAPT_RST,
> +		      VSC73XX_CAPT_CAPRST, 1);
> +
> +	/* Capture BPDU frames */
> +	vsc73xx_write(vsc, VSC73XX_BLOCK_ANALYZER, 0, VSC73XX_CAPENAB,
> +		      VSC73XX_CAPENAB_BPDU);
> +
> +	vsc->rcv_worker = kthread_create_worker(0, "vsc73xx_rcv");
> +	if (IS_ERR(vsc->rcv_worker))
> +		return PTR_ERR(vsc->rcv_worker);

There's teardown work to do on error here.

> +
> +	kthread_init_delayed_work(&vsc->dwork, vsc73xx_polled_rcv);
> +
> +	kthread_queue_delayed_work(vsc->rcv_worker, &vsc->dwork,
> +				   msecs_to_jiffies(VSC73XX_RCV_POLL_INTERVAL));
> +
>  	return ret;

This "return ret" is the error code of dsa_tag_8021q_register(). The new
code block is very badly placed.

>  }
>  
>  static void vsc73xx_teardown(struct dsa_switch *ds)
>  {
> +	struct vsc73xx *vsc = ds->priv;
> +
>  	rtnl_lock();
>  	dsa_tag_8021q_unregister(ds);
>  	rtnl_unlock();
> +
> +	kthread_cancel_delayed_work_sync(&vsc->dwork);
> +	kthread_destroy_worker(vsc->rcv_worker);

This needs to be the reverse process of vsc73xx_setup().

>  }
>  
>  static void vsc73xx_init_port(struct vsc73xx *vsc, int port)
> diff --git a/drivers/net/dsa/vitesse-vsc73xx.h b/drivers/net/dsa/vitesse-vsc73xx.h
> index bf55a20f07f3..5dd458793741 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx.h
> +++ b/drivers/net/dsa/vitesse-vsc73xx.h
> @@ -47,6 +47,8 @@ struct vsc73xx_portinfo {
>   *	every vlan configured in port vlan operation. It doesn't cover tag_8021q
>   *	vlans.
>   * @fdb_lock: Mutex protects fdb access
> + * @rcv_worker: Kthread worker struct for packet reciver poller
> + * @dwork: Work struct for scheduling work to the packet reciver poller
>   */
>  struct vsc73xx {
>  	struct device			*dev;
> @@ -60,6 +62,8 @@ struct vsc73xx {
>  	struct vsc73xx_portinfo		portinfo[VSC73XX_MAX_NUM_PORTS];
>  	struct list_head		vlans;
>  	struct mutex			fdb_lock;
> +	struct kthread_worker		*rcv_worker;
> +	struct kthread_delayed_work	dwork;
>  };
>  
>  /**
> -- 
> 2.34.1
>