linux-kernel - 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: <81124574-48ab-4297-9a74-bba7df68b973@lunn.ch>
Date: Fri, 17 Oct 2025 16:56:23 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Yao Zi <ziyao@...root.org>
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

> > > +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.

Since it resets more than the PHY, it probably should have a different
name, and maybe a comment describing what is actually resets.

And maybe rather than asserting and then deasserting reset, maybe just
deassert the reset? That makes it less dangerous in terms of
lifetimes.

> > > +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.

When WoL is implemented, what state will the MAC and the PHY be in? Is
it possible to put the MAC into a deeper suspend state than the PHY,
since the MAC is probably not needed? The PHY obviously needs to keep
working, so it cannot be put into a reset state. So resume should not
need to take it out of reset. Also, i _think_ the phylib core will
assume a PHY used for WoL is kept alive and configured, so it will not
reconfigure it on resume.

So it seems like this code will need some changes when WoL is
implemented. So leave it as is for the moment.

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

Why? Maybe zoom out, look at the big picture for this driver, and
figure out if that is the correct behaviour for stmmac_mdio to
implement. Is it possible to synthesizer this IP without MDIO?

I was also wondering about all the other parameters you set. Why have
i not seen any other glue driver with similar code? What makes this
glue driver different?

	   Andrew