netdev - Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions for XGMAC2

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:   Fri, 3 Aug 2018 12:06:05 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH v3 net-next 5/9] net: stmmac: Add MDIO related functions
 for XGMAC2

On 08/03/2018 08:50 AM, Jose Abreu wrote:
> Add the MDIO related funcionalities for the new IP block XGMAC2.
> 
> Signed-off-by: Jose Abreu <joabreu@...opsys.com>
> Cc: David S. Miller <davem@...emloft.net>
> Cc: Joao Pinto <jpinto@...opsys.com>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@...com>
> Cc: Alexandre Torgue <alexandre.torgue@...com>
> Cc: Andrew Lunn <andrew@...n.ch>
> ---

> +satic int stmmac_xgmac2_c22_format(struct stmmac_priv *priv, int phyaddr,
> +				    int phyreg, u32 *hw_addr)
> +{
> +	unsigned int mii_data = priv->hw->mii.data;
> +	u32 tmp;
> +
> +	/* HW does not support C22 addr >= 4 */
> +	if (phyaddr >= 4)
> +		return -ENODEV;

It would be nice if this could be moved at probe time so you don't have
to wait until you connect to the PHY, read its PHY OUI and find out it
has a MDIO address >= 4. Not a blocker, but something that could be
improved further on.

In premise you could even scan the MDIO bus' device tree node, and find
that out ahead of time.

> +	/* Wait until any existing MII operation is complete */
> +	if (readl_poll_timeout(priv->ioaddr + mii_data, tmp,
> +			       !(tmp & MII_XGMAC_BUSY), 100, 10000))
> +		return -EBUSY;
> +
> +	/* Set port as Clause 22 */
> +	tmp = readl(priv->ioaddr + XGMAC_MDIO_C22P);
> +	tmp |= BIT(phyaddr);

Since the registers are being Read/Modify/Write here, don't you need to
clear the previous address bits as well?

You probably did not encounter any problems in your testing if you had
only one PHY on the MDIO bus, but this is not something that is
necessarily true, e.g: if you have an Ethernet switch, several MDIO bus
addresses are going to be responding.

Your MDIO bus implementation must be able to support one transaction
with one PHY address and the next transaction with another PHY address ,
etc...

That is something that should be easy to fix and be resubmitted as part
of v4.
-- 
Florian