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

Ansuel Smith
2021-12-12T17:41:15Z

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