| 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: <20241021113857.2csr3dd55fxorbab@skbuf>
Date: Mon, 21 Oct 2024 14:38:57 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/3] net: dsa: vsc73xx: implement packet
reception via control interface
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@...il.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
>
Powered by blists - more mailing lists