| 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: <YfDGxIKzvGlZnltP@Ansuel-xps.localdomain>
Date: Wed, 26 Jan 2022 04:57:56 +0100
From: Ansuel Smith <ansuelsmth@...il.com>
To: Florian Fainelli <f.fainelli@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [RFC PATCH v7 15/16] net: da: qca8k: add support for larger
read/write size with mgmt Ethernet
On Tue, Jan 25, 2022 at 07:48:27PM -0800, Florian Fainelli wrote:
>
>
> On 1/22/2022 5:33 PM, Ansuel Smith wrote:
> > mgmt Ethernet packet can read/write up to 16byte at times. The len reg
> > is limited to 15 (0xf). The switch actually sends and accepts data in 4
> > different steps of len values.
> > Len steps:
> > - 0: nothing
> > - 1-4: first 4 byte
> > - 5-6: first 12 byte
> > - 7-15: all 16 byte
>
> This is really odd, it almost felt like the length was a byte enable
> bitmask, but it is not?
>
To me it seems like they match the size to the 3 different operation
that is
4: normal mdio / reg access
12: acl table to directly write and read it
16: offload table that is 16 byte long to directl write and read.
With this in mind, it does make sense why it bheave like that.
> >
> > In the allock skb function we check if the len is 16 and we fix it to a
> > len of 15.
>
> s/allock/alloc/
>
> > It the read/write function interest to extract the real asked
> > data. The tagger handler will always copy the fully 16byte with a READ
> > command. This is useful for some big regs like the fdb reg that are
> > more than 4byte of data. This permits to introduce a bulk function that
> > will send and request the entire entry in one go.
> > Write function is changed and it does now require to pass the pointer to
> > val to also handle array val.
> >
> > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > ---
> > drivers/net/dsa/qca8k.c | 56 ++++++++++++++++++++++++++++++-----------
> > 1 file changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 2a43fb9aeef2..0183ce2d5b74 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -219,7 +219,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > if (cmd == MDIO_READ) {
> > mgmt_hdr_data->data[0] = mgmt_ethhdr->mdio_data;
> > - /* Get the rest of the 12 byte of data */
> > + /* Get the rest of the 12 byte of data.
> > + * The read/write function will extract the requested data.
> > + */
> > if (len > QCA_HDR_MGMT_DATA1_LEN)
> > memcpy(mgmt_hdr_data->data + 1, skb->data,
> > QCA_HDR_MGMT_DATA2_LEN);
> > @@ -229,16 +231,30 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
> > }
> > static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > - int seq_num, int priority)
> > + int seq_num, int priority, int len)
>
> unsigned int len
>
> > {
> > struct mgmt_ethhdr *mgmt_ethhdr;
> > struct sk_buff *skb;
> > + int real_len;
>
> Likewise.
>
> > + u32 *data2;
> > u16 hdr;
> > skb = dev_alloc_skb(QCA_HDR_MGMT_PKG_LEN);
> > if (!skb)
> > return NULL;
> > + /* Max value for len reg is 15 (0xf) but the switch actually return 16 byte
> > + * Actually for some reason the steps are:
> > + * 0: nothing
> > + * 1-4: first 4 byte
> > + * 5-6: first 12 byte
> > + * 7-15: all 16 byte
> > + */
> > + if (len == 16)
> > + real_len = 15;
> > + else
> > + real_len = len;
> > +
> > skb_reset_mac_header(skb);
> > skb_set_network_header(skb, skb->len);
> > @@ -253,7 +269,7 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
> > mgmt_ethhdr->seq = FIELD_PREP(QCA_HDR_MGMT_SEQ_NUM, seq_num);
> > mgmt_ethhdr->command = FIELD_PREP(QCA_HDR_MGMT_ADDR, reg);
> > - mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, 4);
> > + mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_LENGTH, real_len);
> > mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CMD, cmd);
> > mgmt_ethhdr->command |= FIELD_PREP(QCA_HDR_MGMT_CHECK_CODE,
> > QCA_HDR_MGMT_CHECK_CODE_VAL);
> > @@ -263,19 +279,22 @@ static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *
> > mgmt_ethhdr->hdr = htons(hdr);
> > - skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> > + data2 = skb_put_zero(skb, QCA_HDR_MGMT_DATA2_LEN + QCA_HDR_MGMT_PADDING_LEN);
> > + if (cmd == MDIO_WRITE && len > QCA_HDR_MGMT_DATA1_LEN)
> > + memcpy(data2, val + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > return skb;
> > }
> > -static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> > {
> > struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > struct sk_buff *skb;
> > bool ack;
> > int ret;
> > - skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200,
> > + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> > if (!skb)
> > return -ENOMEM;
> > @@ -297,6 +316,9 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > *val = mgmt_hdr_data->data[0];
> > + if (len > QCA_HDR_MGMT_DATA1_LEN)
> > + memcpy(val + 1, mgmt_hdr_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > +
> > ack = mgmt_hdr_data->ack;
> > mutex_unlock(&mgmt_hdr_data->mutex);
> > @@ -310,14 +332,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > return 0;
> > }
> > -static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> > {
> > struct qca8k_mgmt_hdr_data *mgmt_hdr_data = &priv->mgmt_hdr_data;
> > struct sk_buff *skb;
> > bool ack;
> > int ret;
> > - skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val, 200,
> > + QCA8K_ETHERNET_MDIO_PRIORITY, len);
> > if (!skb)
> > return -ENOMEM;
> > @@ -357,14 +380,14 @@ qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 wri
> > u32 val = 0;
> > int ret;
> > - ret = qca8k_read_eth(priv, reg, &val);
> > + ret = qca8k_read_eth(priv, reg, &val, 4);
>
> sizeof(val) instead of 4.
>
> > if (ret)
> > return ret;
> > val &= ~mask;
> > val |= write_val;
> > - return qca8k_write_eth(priv, reg, val);
> > + return qca8k_write_eth(priv, reg, &val, 4);
>
> Likewise
>
> > }
> > static int
> > @@ -376,7 +399,7 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> > u16 r1, r2, page;
> > int ret;
> > - if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val))
> > + if (priv->mgmt_master && !qca8k_read_eth(priv, reg, val, 4))
>
> Likewise and everywhere below as well.
> --
> Florian
--
Ansuel
Powered by blists - more mailing lists