linux-kernel - Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support multiple global address

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: <cc37984c-13b1-4116-99f8-1a65546c477a@lunn.ch>
Date:   Sun, 26 Nov 2023 19:04:26 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Florian Fainelli <florian.fainelli@...adcom.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Vladimir Oltean <olteanv@...il.com>,
        David Epping <david.epping@...singlinkelectronics.com>,
        Harini Katakam <harini.katakam@....com>,
        "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 1/3] net: phy: extend PHY package API to support
 multiple global address

> @@ -1648,20 +1648,27 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g);
>  /**
>   * phy_package_join - join a common PHY group
>   * @phydev: target phy_device struct
> - * @addr: cookie and PHY address for global register access
> + * @base_addr: cookie and base PHY address of PHY package for offset
> + *   calculation of global register access
>   * @priv_size: if non-zero allocate this amount of bytes for private data
>   *
>   * This joins a PHY group and provides a shared storage for all phydevs in
>   * this group. This is intended to be used for packages which contain
>   * more than one PHY, for example a quad PHY transceiver.
>   *
> - * The addr parameter serves as a cookie which has to have the same value
> - * for all members of one group and as a PHY address to access generic
> - * registers of a PHY package. Usually, one of the PHY addresses of the
> - * different PHYs in the package provides access to these global registers.
> + * The addr parameter serves as cookie which has to have the same values

addr has been renamed base_addr.

> + * for all members of one group and as the base PHY address of the PHY package
> + * for offset calculation to access generic registers of a PHY package.
> + * Usually, one of the PHY addresses of the different PHYs in the package
> + * provides access to these global registers.
>   * The address which is given here, will be used in the phy_package_read()
> - * and phy_package_write() convenience functions. If your PHY doesn't have
> - * global registers you can just pick any of the PHY addresses.
> + * and phy_package_write() convenience functions as base and added to the
> + * passed offset in those functions. If your PHY doesn't have global registers
> + * you can just pick any of the PHY addresses.


I would not add this last sentence. We want a clearly defined meaning
of base_addr. Its the lowest address in the package. It does not
matter if its not used, it should still be the lowest address in the
package.

> + * In some special PHY package, multiple PHY are used for global init of

I don't see why they are special.

> -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> +static inline int phy_package_read(struct phy_device *phydev,
> +				   unsigned int addr_offset, u32 regnum)
>  {
>  	struct phy_package_shared *shared = phydev->shared;
> +	int addr;
>  
> -	if (!shared)
> +	if (!shared || shared->base_addr + addr_offset > PHY_MAX_ADDR)
>  		return -EIO;
>  
> -	return mdiobus_read(phydev->mdio.bus, shared->addr, regnum);
> +	addr = shared->base_addr + addr_offset;
> +	return mdiobus_read(phydev->mdio.bus, addr, regnum);

This might be a little bit more readable:

static inline int phy_package_read(struct phy_device *phydev,
				   unsigned int addr_offset, u32 regnum)
{
	struct phy_package_shared *shared = phydev->shared;
	int addr = shared->base_addr + addr_offset;

	if (!shared)
	if (!shared || addr > PHY_MAX_ADDR)
		return -EIO;

	return mdiobus_read(phydev->mdio.bus, addr, regnum);
}


    Andrew

---
pw-bot: cr