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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Wed, Dec 13, 2023 at 04:50:33PM +0100, Christian Marangi wrote:
> On Wed, Dec 13, 2023 at 03:45:24PM +0000, Russell King (Oracle) wrote:
> > On Wed, Dec 13, 2023 at 11:57:28AM +0100, Christian Marangi wrote:
> > > -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 = shared->base_addr + addr_offset;
> > >  
> > > -	if (!shared)
> > > +	if (addr >= PHY_MAX_ADDR)
> > >  		return -EIO;
> > 
> > If we're going to check the address, I think we should check it
> > properly, which means also checking whether it's become negative.
> > 
> > Alternatively, we could consider making "addr" and "base_addr"
> > unsigned types, since they should never be negative. However,
> > that probably should be done as a separate patch before this one.
> >
> 
> Maybe I'm confused but isn't already like that?
> On phy_package_join base_addr is already checked if it's negative (and
> rejected)
> 
> addr_offset is unsigned so it can't be negative.

True, but with base_addr being an int, addr_offset being unsigned int,
and addr being an int, if addr_offset is a very large number,
2-complement arithmetic will have the effect of treating it as a
negative number.

So, base_addr=0, addr_offset=~0 results in addr being -1.

If "addr" were unsigned int, and as we've already established,
"base_addr" can't be less than zero because of the checks already done
(thus it can also be unsigned int) then we'll end up with the checks
you're adding automatically doing the right thing, because...

base_addr=0, addr_offset=~0 results in addr being ~0 (a large
positive unsigned number).

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!