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
| ||
|
Message-ID: <65638967.5d0a0220.28475.43b3@mx.google.com> Date: Sun, 26 Nov 2023 19:07:32 +0100 From: Christian Marangi <ansuelsmth@...il.com> To: Andrew Lunn <andrew@...n.ch> 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 On Sun, Nov 26, 2023 at 07:04:26PM +0100, Andrew Lunn wrote: > > @@ -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; Isn't this problematic if shared is NULL? I can add 2 if condition and set addr in between only after shared not NULL check is done? > > if (!shared) > if (!shared || addr > PHY_MAX_ADDR) > return -EIO; > > return mdiobus_read(phydev->mdio.bus, addr, regnum); > } > > -- Ansuel
Powered by blists - more mailing lists