| 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: <20241021120735.xpriovox6tzof45l@skbuf>
Date: Mon, 21 Oct 2024 15:07:35 +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 1/3] net: dsa: vsc73xx: implement transmit via
control interface
On Sun, Oct 20, 2024 at 10:54:50PM +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 transmission.
> This kind of traffic can't be transferred through the RGMII interface in
> vsc73xx.
>
> It uses a method similar to the sja1005 driver, where the DSA tagger
> checks if the packet is link-local and uses a special deferred transmit
> route for that kind of packet.
>
> The vsc73xx uses an "Internal Frame Header" (IFH) in communication via the
> PI/SI interface. Every packet must be prefixed with an IFH. The hardware
> fixes the checksums, so there's no need to calculate the FCS in the
> driver.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> ---
> drivers/net/dsa/vitesse-vsc73xx-core.c | 172 +++++++++++++++++++++++++
> drivers/net/dsa/vitesse-vsc73xx.h | 1 +
> include/linux/dsa/vsc73xx.h | 20 +++
> net/dsa/tag_vsc73xx_8021q.c | 73 +++++++++++
> 4 files changed, 266 insertions(+)
> create mode 100644 include/linux/dsa/vsc73xx.h
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index f18aa321053d..21ab3f214490 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -73,6 +73,9 @@
> #define VSC73XX_CAT_PR_USR_PRIO 0x75
> #define VSC73XX_CAT_VLAN_MISC 0x79
> #define VSC73XX_CAT_PORT_VLAN 0x7a
> +#define VSC73XX_CPUTXDAT 0xc0
> +#define VSC73XX_MISCFIFO 0xc4
> +#define VSC73XX_MISCSTAT 0xc8
> #define VSC73XX_Q_MISC_CONF 0xdf
>
> /* MAC_CFG register bits */
> @@ -166,6 +169,14 @@
> #define VSC73XX_CAT_PORT_VLAN_VLAN_USR_PRIO GENMASK(14, 12)
> #define VSC73XX_CAT_PORT_VLAN_VLAN_VID GENMASK(11, 0)
>
> +/* MISCFIFO Miscellaneous Control Register */
> +#define VSC73XX_MISCFIFO_REWIND_CPU_TX BIT(1)
> +#define VSC73XX_MISCFIFO_CPU_TX BIT(0)
> +
> +/* MISCSTAT Miscellaneous Status */
> +#define VSC73XX_MISCSTAT_CPU_TX_DATA_PENDING BIT(8)
> +#define VSC73XX_MISCSTAT_CPU_TX_DATA_OVERFLOW BIT(7)
> +
> /* Frame analyzer block 2 registers */
> #define VSC73XX_STORMLIMIT 0x02
> #define VSC73XX_ADVLEARN 0x03
> @@ -363,6 +374,9 @@
> #define VSC73XX_MDIO_POLL_SLEEP_US 5
> #define VSC73XX_POLL_TIMEOUT_US 10000
>
> +#define VSC73XX_IFH_MAGIC 0x52
> +#define VSC73XX_IFH_SIZE 8
> +
> struct vsc73xx_counter {
> u8 counter;
> const char *name;
> @@ -375,6 +389,31 @@ struct vsc73xx_fdb {
> bool valid;
> };
>
> +/* Internal frame header structure */
> +struct vsc73xx_ifh {
> + union {
> + u32 datah;
> + struct {
> + u32 wt:1, /* Frame was tagged but tag has removed from frame */
> + : 1,
> + frame_length:14, /* Frame Length including CRC */
> + : 11,
> + port:5; /* SRC port of switch */
Please indent the struct field members.
> + };
> + };
> + union {
> + u32 datal;
Is the union with datah/datal actually useful in any way? Just a comment
about high word/low word should suffice?
Is there any field that crosses the word boundary? Or is the IFH nicely
arranged?
Does CPU endianness affect the correct bit layout?
> + struct {
> + u32 vid:16, /* VLAN ID */
> + : 3,
> + magic:9, /* IFH magic field */
> + lpa:1, /* SMAC is subject of learning */
> + : 1,
> + priority:2; /* Switch categorizer assigned priority */
> + };
> + };
> +};
__packed
> +
> /* Counters are named according to the MIB standards where applicable.
> * Some counters are custom, non-standard. The standard counters are
> * named in accordance with RFC2819, RFC2021 and IEEE Std 802.3-2002 Annex
> @@ -683,6 +722,133 @@ static int vsc73xx_phy_write(struct dsa_switch *ds, int phy, int regnum,
> return 0;
> }
>
> +static int vsc73xx_tx_fifo_busy_check(struct vsc73xx *vsc, int port)
> +{
> + int ret, err;
> + u32 val;
> +
> + ret = read_poll_timeout(vsc73xx_read, err,
> + err < 0 ||
> + !(val & VSC73XX_MISCSTAT_CPU_TX_DATA_PENDING),
> + VSC73XX_POLL_SLEEP_US,
> + VSC73XX_POLL_TIMEOUT_US, false, vsc,
> + VSC73XX_BLOCK_MAC, port, VSC73XX_MISCSTAT,
> + &val);
> + if (ret)
> + return ret;
> + return err;
> +}
> +
> +static int
> +vsc73xx_write_tx_fifo(struct vsc73xx *vsc, int port, u32 data0, u32 data1)
> +{
> + vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CPUTXDAT, data0);
> + vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CPUTXDAT, data1);
> +
> + return vsc73xx_tx_fifo_busy_check(vsc, port);
> +}
> +
> +static int
> +vsc73xx_inject_frame(struct vsc73xx *vsc, int port, struct sk_buff *skb)
> +{
> + struct vsc73xx_ifh *ifh;
> + u32 length, i, count;
> + u32 *buf;
> + int ret;
> +
> + if (skb->len + VSC73XX_IFH_SIZE < 64)
> + length = 64;
> + else
> + length = skb->len + VSC73XX_IFH_SIZE;
length = min_t(u32, 64, skb->len + VSC73XX_IFH_SIZE)?
Also, what does 64 represent? ETH_ZLEN + ?
> +
> + count = DIV_ROUND_UP(length, 8);
> + buf = kzalloc(count * 8, GFP_KERNEL);
this can return NULL
> + memset(buf, 0, sizeof(buf));
> +
> + ifh = (struct vsc73xx_ifh *)buf;
> + ifh->frame_length = skb->len;
> + ifh->magic = VSC73XX_IFH_MAGIC;
> +
> + skb_copy_and_csum_dev(skb, (u8 *)(buf + 2));
Do you really _have_ to allocate dynamically a buffer and copy the skb
to it? Can't you write_tx_fifo() based on a pointer from skb->data, and
allocate the IFH as a separate on-stack structure?
For the checksum calculation, you could add the same logic as ocelot_defer_xmit():
if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
return NULL;
> +
> + for (i = 0; i < count; i++) {
> + ret = vsc73xx_write_tx_fifo(vsc, port, buf[2 * i],
> + buf[2 * i + 1]);
> + if (ret) {
> + /* Clear buffer after error */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_MISCFIFO,
> + VSC73XX_MISCFIFO_REWIND_CPU_TX,
> + VSC73XX_MISCFIFO_REWIND_CPU_TX);
> + goto err;
> + }
> + }
> +
> + vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MISCFIFO,
> + VSC73XX_MISCFIFO_CPU_TX);
> +
> + skb_tx_timestamp(skb);
When is the packet transmission actually started? At VSC73XX_MISCFIFO_CPU_TX
time? skb_tx_timestamp() should be done prior to that. PHY TX
timestamping is also hooked into this call, and will be completely
broken if it is racing with the packet transmission.
> +
> + skb->dev->stats.tx_packets++;
> + skb->dev->stats.tx_bytes += skb->len;
> +err:
> + kfree(buf);
> + return ret;
> +}
> +
> +#define work_to_xmit_work(w) \
> + container_of((w), struct vsc73xx_deferred_xmit_work, work)
> +
> +static void vsc73xx_deferred_xmit(struct kthread_work *work)
> +{
> + struct vsc73xx_deferred_xmit_work *xmit_work = work_to_xmit_work(work);
> + struct dsa_switch *ds = xmit_work->dp->ds;
> + struct sk_buff *skb = xmit_work->skb;
> + int port = xmit_work->dp->index;
> + struct vsc73xx *vsc = ds->priv;
> + int ret;
> +
> + if (vsc73xx_tx_fifo_busy_check(vsc, port)) {
> + dev_err(vsc->dev, "port %d failed to inject skb\n",
> + port);
> +
> + /* Clear buffer after error */
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_MISCFIFO,
> + VSC73XX_MISCFIFO_REWIND_CPU_TX,
> + VSC73XX_MISCFIFO_REWIND_CPU_TX);
> +
> + kfree_skb(skb);
> + return;
> + }
> +
> + ret = vsc73xx_inject_frame(vsc, port, skb);
> +
extraneous blank line
> + if (ret) {
> + dev_err(vsc->dev, "port %d failed to inject skb\n",
dev_err_ratelimited(... %pe, ERR_PTR(ret))?
> + port);
> + return;
> + }
Is this hardware procedure completely reentrant (can it simultaneously
inject packets towards multiple ports) or should there be a spinlock
serializing the access?
> +
> + consume_skb(skb);
> + kfree(xmit_work);
> +}
Powered by blists - more mailing lists