Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <61f08471.1c69fb81.a3d6.4d94@mx.google.com>
Date:   Wed, 26 Jan 2022 00:14:55 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [RFC PATCH v7 12/16] net: dsa: qca8k: add support for phy
 read/write with mgmt Ethernet
References: <20220123013337.20945-1-ansuelsmth@gmail.com>
 <20220123013337.20945-13-ansuelsmth@gmail.com>
 <20220125150355.5ywi4fe3puxaphq3@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220125150355.5ywi4fe3puxaphq3@skbuf>
Precedence: bulk

On Tue, Jan 25, 2022 at 05:03:55PM +0200, Vladimir Oltean wrote:
> On Sun, Jan 23, 2022 at 02:33:33AM +0100, Ansuel Smith wrote:
> > Use mgmt Ethernet also for phy read/write if availabale. Use a different
> > seq number to make sure we receive the correct packet.
> 
> At some point, you'll have to do something about those sequence numbers.
> Hardcoding 200 and 400 isn't going to get you very far, it's prone to
> errors. How about dealing with it now? If treating them as actual
> sequence numbers isn't useful because you can't have multiple packets in
> flight due to reordering concerns, at least create a macro for each
> sequence number used by the driver for packet identification.
>

Is documenting define and adding some inline function acceptable? That
should make the separation more clear and also prepare for a future
implementation. The way I see an use for the seq number is something
like a global workqueue that would handle all this stuff and be the one
that handle the seq number.
I mean another way would be just use a counter that will overflow and
remove all this garbage with hardcoded seq number.
(think will follow this path and just implement a correct seq number...)

> > On any error, we fallback to the legacy mdio read/write.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 191 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |   1 +
> >  2 files changed, 192 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index f51a6d8993ff..e7bc0770bae9 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -848,6 +848,166 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
> >  		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
> >  }
> >  
> > +static int
> > +qca8k_phy_eth_busy_wait(struct qca8k_mgmt_hdr_data *phy_hdr_data,
> > +			struct sk_buff *read_skb, u32 *val)
> > +{
> > +	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> > +	bool ack;
> > +	int ret;
> > +
> > +	reinit_completion(&phy_hdr_data->rw_done);
> > +	phy_hdr_data->seq = 400;
> > +	phy_hdr_data->ack = false;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > +					  QCA8K_ETHERNET_TIMEOUT);
> > +
> > +	ack = phy_hdr_data->ack;
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	*val = phy_hdr_data->data[0];
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
> > +		      int regnum, u16 data)
> > +{
> > +	struct sk_buff *write_skb, *clear_skb, *read_skb;
> > +	struct qca8k_mgmt_hdr_data *phy_hdr_data;
> > +	const struct net_device *mgmt_master;
> > +	u32 write_val, clear_val = 0, val;
> > +	int seq_num = 400;
> > +	int ret, ret1;
> > +	bool ack;
> > +
> > +	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
> > +		return -EINVAL;
> > +
> > +	phy_hdr_data = &priv->mgmt_hdr_data;
> > +
> > +	write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
> > +		    QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
> > +		    QCA8K_MDIO_MASTER_REG_ADDR(regnum);
> > +
> > +	if (read) {
> > +		write_val |= QCA8K_MDIO_MASTER_READ;
> > +	} else {
> > +		write_val |= QCA8K_MDIO_MASTER_WRITE;
> > +		write_val |= QCA8K_MDIO_MASTER_DATA(data);
> > +	}
> > +
> > +	/* Prealloc all the needed skb before the lock */
> > +	write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> > +					    &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > +	if (!write_skb)
> > +		return -ENOMEM;
> > +
> > +	clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
> > +					    &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > +	if (!write_skb)
> 
> Clean up the resources!!! (not just here but everywhere)
> 

Sorry!!! I'm checking all the skb to free and wow this is a mess...
Hope when I will send v8 you won't have a stoke checking this mess ahahahah

> > +		return -ENOMEM;
> > +
> > +	read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
> > +					   &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
> > +	if (!write_skb)
> > +		return -ENOMEM;
> > +
> > +	/* Actually start the request:
> > +	 * 1. Send mdio master packet
> > +	 * 2. Busy Wait for mdio master command
> > +	 * 3. Get the data if we are reading
> > +	 * 4. Reset the mdio master (even with error)
> > +	 */
> > +	mutex_lock(&phy_hdr_data->mutex);
> 
> Shouldn't qca8k_master_change() also take phy_hdr_data->mutex?
> 

Is actually the normal mgmg_hdr_data. 

phy_hdr_data = &priv->mgmt_hdr_data;

Should I remove this and use mgmt_hdr_data directly to remove any
confusion? 

> > +
> > +	/* Recheck mgmt_master under lock to make sure it's operational */
> > +	mgmt_master = priv->mgmt_master;
> > +	if (!mgmt_master)
> > +		return -EINVAL;
> > +
> > +	read_skb->dev = (struct net_device *)mgmt_master;
> > +	clear_skb->dev = (struct net_device *)mgmt_master;
> > +	write_skb->dev = (struct net_device *)mgmt_master;
> 
> If you need the master to be a non-const pointer, just make the DSA
> method give you a non-const pointer.
> 
> > +
> > +	reinit_completion(&phy_hdr_data->rw_done);
> > +	phy_hdr_data->ack = false;
> > +	phy_hdr_data->seq = seq_num;
> > +
> > +	dev_queue_xmit(write_skb);
> > +
> > +	ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > +					  QCA8K_ETHERNET_TIMEOUT);
> > +
> > +	ack = phy_hdr_data->ack;
> > +
> > +	if (ret <= 0) {
> > +		ret = -ETIMEDOUT;
> > +		goto exit;
> > +	}
> > +
> > +	if (!ack) {
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
> > +				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
> > +				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
> > +				phy_hdr_data, read_skb, &val);
> > +
> > +	if (ret < 0 && ret1 < 0) {
> > +		ret = ret1;
> > +		goto exit;
> > +	}
> > +
> > +	if (read) {
> > +		reinit_completion(&phy_hdr_data->rw_done);
> > +		phy_hdr_data->ack = false;
> > +
> > +		dev_queue_xmit(read_skb);
> > +
> > +		ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > +						  QCA8K_ETHERNET_TIMEOUT);
> > +
> > +		ack = phy_hdr_data->ack;
> > +
> > +		if (ret <= 0) {
> > +			ret = -ETIMEDOUT;
> > +			goto exit;
> > +		}
> > +
> > +		if (!ack) {
> > +			ret = -EINVAL;
> > +			goto exit;
> > +		}
> > +
> > +		ret = phy_hdr_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> > +	}
> > +
> > +exit:
> > +	reinit_completion(&phy_hdr_data->rw_done);
> > +	phy_hdr_data->ack = false;
> > +
> > +	dev_queue_xmit(clear_skb);
> > +
> > +	wait_for_completion_timeout(&phy_hdr_data->rw_done,
> > +				    QCA8K_ETHERNET_TIMEOUT);
> > +
> > +	mutex_unlock(&phy_hdr_data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> >  static u32
> >  qca8k_port_to_phy(int port)
> >  {
> > @@ -970,6 +1130,14 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
> >  {
> >  	struct qca8k_priv *priv = slave_bus->priv;
> >  	struct mii_bus *bus = priv->bus;
> > +	int ret;
> > +
> > +	/* Use mdio Ethernet when available, fallback to legacy one on error */
> > +	if (priv->mgmt_master) {
> > +		ret = qca8k_phy_eth_command(priv, false, phy, regnum, data);
> > +		if (!ret)
> > +			return 0;
> > +	}
> >  
> >  	return qca8k_mdio_write(bus, phy, regnum, data);
> >  }
> > @@ -979,6 +1147,14 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
> >  {
> >  	struct qca8k_priv *priv = slave_bus->priv;
> >  	struct mii_bus *bus = priv->bus;
> > +	int ret;
> > +
> > +	/* Use mdio Ethernet when available, fallback to legacy one on error */
> > +	if (priv->mgmt_master) {
> > +		ret = qca8k_phy_eth_command(priv, true, phy, regnum, 0);
> > +		if (ret >= 0)
> > +			return ret;
> > +	}
> >  
> >  	return qca8k_mdio_read(bus, phy, regnum);
> >  }
> > @@ -987,6 +1163,7 @@ static int
> >  qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> >  {
> >  	struct qca8k_priv *priv = ds->priv;
> > +	int ret;
> >  
> >  	/* Check if the legacy mapping should be used and the
> >  	 * port is not correctly mapped to the right PHY in the
> > @@ -995,6 +1172,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
> >  	if (priv->legacy_phy_port_mapping)
> >  		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> >  
> > +	/* Use mdio Ethernet when available, fallback to legacy one on error */
> > +	if (priv->mgmt_master) {
> > +		ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> > +		if (!ret)
> > +			return ret;
> > +	}
> > +
> >  	return qca8k_mdio_write(priv->bus, port, regnum, data);
> >  }
> >  
> > @@ -1011,6 +1195,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
> >  	if (priv->legacy_phy_port_mapping)
> >  		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
> >  
> > +	/* Use mdio Ethernet when available, fallback to legacy one on error */
> > +	if (priv->mgmt_master) {
> > +		ret = qca8k_phy_eth_command(priv, true, port, regnum, 0);
> > +		if (ret >= 0)
> > +			return ret;
> > +	}
> > +
> >  	ret = qca8k_mdio_read(priv->bus, port, regnum);
> >  
> >  	if (ret < 0)
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index dc1365542aa3..952217db2047 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -14,6 +14,7 @@
> >  #include <linux/dsa/tag_qca.h>
> >  
> >  #define QCA8K_ETHERNET_MDIO_PRIORITY			7
> > +#define QCA8K_ETHERNET_PHY_PRIORITY			6
> >  #define QCA8K_ETHERNET_TIMEOUT				100
> >  
> >  #define QCA8K_NUM_PORTS					7
> > -- 
> > 2.33.1
> > 

-- 
	Ansuel