netdev - Re: [net-next RFC PATCH v4 12/15] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 12 Dec 2021 18:41:07 +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: [net-next RFC PATCH v4 12/15] net: dsa: qca8k: add support for
 mdio read/write in Ethernet packet

On Sat, Dec 11, 2021 at 08:12:51PM -0800, Florian Fainelli wrote:
> 
> 
> On 12/11/2021 11:57 AM, Ansuel Smith wrote:
> > Add qca8k side support for mdio read/write in Ethernet packet.
> > qca8k supports some specially crafted Ethernet packet that can be used
> > for mdio read/write instead of the legacy method uart/internal mdio.
> > This add support for the qca8k side to craft the packet and enqueue it.
> > Each port and the qca8k_priv have a special struct to put data in it.
> > The completion API is used to wait for the packet to be received back
> > with the requested data.
> > 
> > The various steps are:
> > 1. Craft the special packet with the qca hdr set to mdio read/write
> >     mode.
> > 2. Set the lock in the dedicated mdio struct.
> > 3. Reinit the completion.
> > 4. Enqueue the packet.
> > 5. Wait the packet to be received.
> > 6. Use the data set by the tagger to complete the mdio operation.
> > 
> > If the completion timeouts or the ack value is not true, the legacy
> > mdio way is used.
> > 
> > It has to be considered that in the initial setup mdio is still used and
> > mdio is still used until DSA is ready to accept and tag packet.
> > 
> > tag_proto_connect() is used to fill the required handler for the tagger
> > to correctly parse and elaborate the special Ethernet mdio packet.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > ---
> 
> [snip]
> 
> > +
> > +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > +					       int seq_num)
> > +{
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	struct sk_buff *skb;
> > +	u16 hdr;
> > +
> > +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
> 
> No out of memory condition handling?
>

Will add.

> > +
> > +	prefetchw(skb->data);
> 
> What's the point you are going to dirty the cache lines right below with
> your skb_push().
> 

Will drop.

> > +
> > +	skb_reset_mac_header(skb);
> > +	skb_set_network_header(skb, skb->len);
> > +
> > +	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
> > +
> > +	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> > +	hdr |= QCA_HDR_XMIT_FROM_CPU;
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> > +
> > +	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, seq_num);
> > +
> > +	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
> > +
> > +	if (cmd == MDIO_WRITE)
> > +		mdio_ethhdr->mdio_data = *val;
> > +
> > +	mdio_ethhdr->hdr = htons(hdr);
> > +
> > +	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
> > +	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
> > +
> > +	return skb;
> > +}
> > +
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200);
> > +	skb->dev = dsa_to_port(priv->ds, 0)->master;
> 
> Surely you should be checking that this is the CPU port and obtain it from
> DSA instead of hard coding it to 0.
> 

You are right as we can also have port6 as cpu port.

> > +
> > +	mutex_lock(&mdio_hdr_data->mutex);
> > +
> > +	reinit_completion(&mdio_hdr_data->rw_done);
> > +	mdio_hdr_data->seq = 200;
> > +	mdio_hdr_data->ack = false;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> 
> msec_to_jiffies(QCA8K_ETHERNET_TIMEOUT) at least?

Sorry got it work and misread that the conversion was implicit.

> -- 
> Florian

-- 
	Ansuel