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: <f1de6600-4de9-4914-95e6-8cdb3481e364@lunn.ch>
Date: Thu, 16 Oct 2025 22:11:02 +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 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.

> +	*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?

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.

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

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

	Andrew