netdev - Re: [PATCH net-next 3/4] net: stmmac: Add glue driver for Motorcomm YT6801 ethernet controller

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]

Message-ID: <aPJMsNKwBYyrr-W-@pie>
Date: Fri, 17 Oct 2025 14:03:28 +0000
From: Yao Zi <ziyao@...root.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Frank <Frank.Sae@...or-comm.com>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	"Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
	Vladimir Oltean <vladimir.oltean@....com>,
	Choong Yong Liang <yong.liang.choong@...ux.intel.com>,
	Chen-Yu Tsai <wens@...e.org>, Jisheng Zhang <jszhang@...nel.org>,
	Furong Xu <0x1207@...il.com>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: stmmac: Add glue driver for Motorcomm
 YT6801 ethernet controller

On Thu, Oct 16, 2025 at 10:11:02PM +0200, Andrew Lunn wrote:
> > +static int motorcomm_efuse_read_byte(struct dwmac_motorcomm_priv *priv,
> > +				     u8 offset, u8 *byte)
> > +{
> > +	u32 reg;
> > +	int ret;
> > +
> > +	writel(FIELD_PREP(EFUSE_OP_MODE, EFUSE_OP_ROW_READ)	|
> > +	       FIELD_PREP(EFUSE_OP_ADDR, offset)		|
> > +	       EFUSE_OP_START, priv->base + EFUSE_OP_CTRL_0);
> > +
> > +	ret = readl_poll_timeout(priv->base + EFUSE_OP_CTRL_1,
> > +				 reg, reg & EFUSE_OP_DONE, 2000,
> > +				 EFUSE_READ_TIMEOUT_US);
> > +
> > +	reg = readl(priv->base + EFUSE_OP_CTRL_1);
> 
> Do you actually need this read? The documentation says:
> 
>  * Returns: 0 on success and -ETIMEDOUT upon a timeout. In either
>  * case, the last read value at @addr is stored in @val.

Oops, the extra call to readl() is indeed unnecessary. Will remove it in
next version.

> > +	*byte = FIELD_GET(EFUSE_OP_RD_DATA, reg);
> > +
> > +	return ret;
> > +}
> 
> > +static void motorcomm_reset_phy(struct dwmac_motorcomm_priv *priv)
> > +{
> > +	u32 reg = readl(priv->base + EPHY_CTRL);
> > +
> > +	reg &= ~EPHY_RESET;
> > +	writel(reg, priv->base + EPHY_CTRL);
> > +
> > +	reg |= EPHY_RESET;
> > +	writel(reg, priv->base + EPHY_CTRL);
> > +}
> 
> How does this differ to the PHY doing its own reset via BMCR?

It's named as EPHY_RESET according to the vendor driver, but with my
testing, it seems to reset at least the internal MDIO bus as well: with
this reset asserted (which is the default state after power on or
resumption from D3hot), mdiobus_scan() considers each possible MDIO
address corresponds to a PHY, while no one could be connected
successfully.

> We need to be careful of lifetimes here. It would be better if the PHY
> controlled its own reset. We don't want phylib to configure the PHY
> and then the MAC driver reset it etc.

Though it's still unclear the exact effect of the bit on the PHY since
there's no public documentation, it's essential to deassert it in MAC
code before registering and scanning the MDIO bus, or we could even not
probe the PHY correctly.

For the motorcomm_reset_phy() performed in probe function, it happens
before the registration of MDIO bus, and the PHY isn't probed yet, thus
I think it should be okay.

> > +static int motorcomm_resume(struct device *dev, void *bsp_priv)
> > +{
> > +	struct dwmac_motorcomm_priv *priv = bsp_priv;
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int ret;
> > +
> > +	pci_restore_state(pdev);
> > +	pci_set_power_state(pdev, PCI_D0);
> > +
> > +	ret = pcim_enable_device(pdev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pci_set_master(pdev);
> > +
> > +	motorcomm_reset_phy(priv);
> 
> Does the PHY support WoL? You probably should not be touching it if it
> can wake the system.

Yes, it supports WoL, though the functionality isn't implemented in this
series.

As I mentioned before, it's necesasry to at least deassert EPHY_RESET
after resuming from D3hot state, or phy_check_link_status() will always
fail with -EBUSY for the PHY and it cannot be correctly resumed.

Do you think it's acceptable to only deassert the EPHY_RESET without
asserting it manually in the resume hook? With this scheme, even though
EPHY_RESET does affect some PHY functionalities, we're not making the
situation worse, since it's already asserted automatically by hardware
after resuming from D3hot.

> > +		return NULL;
> > +
> > +	plat->mdio_bus_data = devm_kzalloc(dev, sizeof(*plat->mdio_bus_data),
> > +					   GFP_KERNEL);
> > +	if (!plat->mdio_bus_data)
> > +		return NULL;
> 
> Is this required? If you look at other glue drivers which allocate
> such a structure, they set members in it:
> 
> dwmac-intel.c:	plat->mdio_bus_data->needs_reset = true;
> dwmac-loongson.c:		plat->mdio_bus_data->needs_reset = true;
> dwmac-tegra.c:	plat->mdio_bus_data->needs_reset = true;
> stmmac_pci.c:	plat->mdio_bus_data->needs_reset = true;
> stmmac_platform.c:		plat->mdio_bus_data->needs_reset = true;
> 
> You don't set anything.

Yes, it's required, since stmmac_mdio.c won't register a MDIO bus if
plat_stmmacenet_data.mdio_bus_data is NULL.

> 
> 	Andrew

Thanks,
Yao Zi