Re: [net-next PATCH v7 2/4] net: phy: extend PHY package API to support multiple global address

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Dec 14, 2023 at 05:05:39PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 14, 2023 at 01:10:24PM +0100, Christian Marangi wrote:
> > @@ -1998,46 +1999,54 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
> >  		       struct kernel_hwtstamp_config *config,
> >  		       struct netlink_ext_ack *extack);
> >  
> > -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;
> > +	u8 addr = shared->base_addr + addr_offset;
> >  
> > -	if (!shared)
> > +	if (addr >= PHY_MAX_ADDR)
> >  		return -EIO;
> 
> I did notice that you're using u8 in patch 1 as well - and while it's
> fine in patch 1 (because we validate the range of the value we will
> assign to that variable) that is not the case here.
> 
> Yes, shared->base_addr is a u8, but addr_offset is an unsigned int,
> and this is implicitly cast-down to a u8 in the calculation of addr,
> chopping off the bits above bit 7.
> 
> How about this approach:
> 
> static int phy_package_address(struct phy_device *phydev,
> 			       unsigned int addr_offset)
> {
> 	struct phy_package_shared *shared = phydev->shared;
> 	unsigned int addr = shared->addr + addr_offset;
> 
> 	/* detect wrap */
> 	if (addr < addr_offset)
> 		return -EIO;
> 
> 	/* detect invalid address */
> 	if (addr >= PHY_ADDR_MAX)
> 		return -EIO;
> 
> 	/* we know that addr will be in the range 0..31 and thus the
> 	 * implicit cast to a signed int is not a problem.
> 	 */
> 	return addr;
> }
> 
> and then these functions all become:
> 
> 	int addr = phy_package_address(phydev, addr_offset);
> 
> 	if (addr < 0)
> 		return addr;
> 
> I'll give you that this is belt and braces, but it avoids problems
> should a negative errno value be passed in as addr_offset (which will
> be cast to a very large positive integer.)

I also feel an helper is needed (since as you pointed out in the mmd
function we would have duplicated logic)

What I don't like is the wrap check.

But I wonder... Isn't it easier to have 

unsigned int addr = shared->base_addr + addr_offset;

and check if >= PHY_MAC_ADDR?

Everything is unsigned (so no negative case) and wrap is not possible as
nothing is downcasted.

After the check value is O.K. and can be trated as an int in
mdiobus_read (as we are sure it's in the limits and positive)

If this is correct, and the thing is a simple condition I think the
helper is not needed (or should we use it anyway for consistency in each
function?)

> 
> Andrew, any opinions on how far this should be taken?
> 

-- 
	Ansuel