netdev - Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

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] [day] [month] [year] [list]

Message-ID: <Ym/Rr6BN7b/Y6mqu@lunn.ch>
Date:   Mon, 2 May 2022 14:42:23 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Oleksij Rempel <o.rempel@...gutronix.de>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, kernel@...gutronix.de,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v1 1/1] net: phy: dp83td510: Add support for the
 DP83TD510 Ethernet PHY

> +static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
> +			     u16 regnum)
> +{
> +	/* Write the desired MMD Devad */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
> +
> +	/* Write the desired MMD register address */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
> +
> +	/* Select the Function : DATA with no post increment */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
> +			devad | MII_MMD_CTRL_NOINCR);
> +}

Please make the version in phy-core.c global scope.

A better explanation of what is going on here would be good.

> +	/* This PHY supports only C22 MDIO opcodes. We can use only indirect
> +	 * access.
> +	 */
> +	mmd_phy_indirect(bus, phy_addr, devad, regnum);

This comment suggests it is because it cannot do C45. But the core
should handle this, it would use indirect access. However, you have
hijacked phydev->drv->read_mmd to allow you to translate standard
registers to vendor registers. This bypasses the cores fallback to
indirect access.

> +static struct phy_driver dp83td510_driver[] = {
> +{
> +	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
> +	.name		= "TI DP83TD510E",
> +
> +	.config_aneg	= genphy_c45_config_aneg,
> +	.read_status	= genphy_c45_read_status,
> +	.get_features	= dp83td510_get_features,
> +	.config_intr	= dp83td510_config_intr,
> +	.handle_interrupt = dp83td510_handle_interrupt,
> +
> +	.suspend	= genphy_suspend,
> +	.resume		= genphy_resume,
> +	.read_mmd	= dp83td510_read_mmd,
> +	.write_mmd	= dp83td510_write_mmd,

Given how far this PHY is away from standards, you might get a smaller
simpler driver if you ignore genphy all together, write your own
config_aneg and read_status, and don't mess with .read_mmd and
write_mmd.

	Andrew