linux-kernel - Re: [PATCH net-next v4 06/12] net: ethernet: oa_tc6: implement internal PHY initialization

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: <aae5f0be-7e1f-443e-831a-ab0b4df0b839@lunn.ch>
Date: Wed, 24 Apr 2024 01:48:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, saeedm@...dia.com,
	anthony.l.nguyen@...el.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, corbet@....net,
	linux-doc@...r.kernel.org, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	devicetree@...r.kernel.org, horatiu.vultur@...rochip.com,
	ruanjinjie@...wei.com, steen.hegelund@...rochip.com,
	vladimir.oltean@....com, UNGLinuxDriver@...rochip.com,
	Thorsten.Kummermehr@...rochip.com, Pier.Beruto@...emi.com,
	Selvamani.Rajagopal@...emi.com, Nicolas.Ferre@...rochip.com,
	benjamin.bigler@...nformulastudent.ch
Subject: Re: [PATCH net-next v4 06/12] net: ethernet: oa_tc6: implement
 internal PHY initialization

> +/* PHY Clause 22 and 29 registers base address and mask */
> +#define OA_TC6_PHY_STD_REG_ADDR_BASE		0xFF00
> +#define OA_TC6_PHY_STD_REG_ADDR_MASK		0x3F

Did you every find out why C29 is reference here? From the standard:

29. System considerations for multisegment 100BASE-T networks

NOTE—This clause relates to clauses that are not recommended for new
installations. This clause is not recommended for new
installations. Since March 2012, maintenance changes are no longer
being considered for this clause.

I don't think you should be referencing it in the code.

> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int addr, int regnum)
> +{
> +	struct oa_tc6 *tc6 = bus->priv;
> +	u32 regval;
> +	bool ret;
> +
> +	ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
> +				   (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
> +				   &regval);
> +	if (ret)
> +		return -ENODEV;

In general, you should not replace an error code from a lower level
function with a different error code. If there is a good reason to do
this, please add a comment.

> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
> index 534ca7d1b061..769a88254285 100644
> --- a/drivers/net/phy/microchip_t1s.c
> +++ b/drivers/net/phy/microchip_t1s.c
> @@ -268,6 +268,34 @@ static int lan86xx_read_status(struct phy_device *phydev)
>  	return 0;
>  }

Please put this into a new patch.

>  
> +/* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
> + * C45 registers space. If the PHY is discovered via C22 bus protocol it assumes
> + * it uses C22 protocol and always uses C22 registers indirect access to access
> + * C45 registers. This is because, we don't have a clean separation between
> + * C22/C45 register space and C22/C45 MDIO bus protocols. Resulting, PHY C45
> + * registers direct access can't be used which can save multiple SPI bus access.
> + * To support this feature, set .read_mmd/.write_mmd in the PHY driver to call
> + * .read_c45/.write_c45 in the OPEN Alliance framework
> + * drivers/net/ethernet/oa_tc6.c
> + */
> +static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
> +				u16 regnum)
> +{
> +	struct mii_bus *bus = phydev->mdio.bus;
> +	int addr = phydev->mdio.addr;
> +
> +	return bus->read_c45(bus, addr, devnum, regnum);

It is better to use __mdiobus_c45_read(). That will check you have the
lock held, won't jump through a null pointer if the bus does not
implement C45, does tracing, and increments the MDIO statistics.

	  Andrew