Re: [net-next RFC PATCH 1/6] net: phy: add support for defining multiple PHY IDs in PHY driver

[Russell King (Oracle)]

On Sun, Feb 18, 2024 at 09:44:03PM +0100, Christian Marangi wrote:
> On Sun, Feb 18, 2024 at 08:34:16PM +0000, Russell King (Oracle) wrote:
> > On Sun, Feb 18, 2024 at 09:27:22PM +0100, Christian Marangi wrote:
> > > On Sun, Feb 18, 2024 at 09:10:30PM +0100, Andrew Lunn wrote:
> > > > > > > +	phy_dev_id = (struct mdio_device_id *)&phydev->dev_id;
> > > > > > 
> > > > > > Why this cast? Try to write code that doesn't need casts.
> > > > > > 
> > > > > 
> > > > > This cast is needed to keep the dev_id const in the phy_device struct so
> > > > > that other are warned to not modify it and should only be handled by
> > > > > phy_probe since it's the one that fills it.
> > > > > 
> > > > > Alternative is to drop const and drop the warning.
> > > > 
> > > > Can you propagate the const. Make phy_dev_id point to a const?
> > > >
> > > 
> > > Mhh not following, I tried changing to const struct mdio_device_id *phy_dev_id
> > > but that results in memcpy complain (dest is void * not const) and
> > > writing in read-only for the single PHY part (the else part)
> > > 
> > > An alternative might be to make dev_id a pointer in struct phy_device
> > > and dynamically allocate a mdio_device_id for the case of single PHY
> > > (else case). That effectively remove the need of this cast but I would
> > > love to skip checking for -ENOMEM (this is why i made that local)
> > > 
> > > If it's OK to dynamically allocate for the else case then I will make
> > > this change. I just tested this implementation and works correctly with
> > > not warning.
> > 
> > Why do we need memcpy() etc - as I demonstrated in my proposal, it's
> > not necessary if we introduce a mdio_device_id within struct phy_driver
> > and we can just store a const pointer to the mdio_device_id that
> > matched. That was very much an intentional decision in my proposal to
> > make the code simple.
> >
> 
> With the allocated mdio_devic_id it would result in this snipped
> 
> 	const struct mdio_device_id *driver_dev_id;
> 	struct mdio_device_id *dev_id;
> 	int err = 0;
> 
> 	phydev->drv = phydrv;
> 	/* Fill the mdio_device_id for the PHY istance.
> 	 * If PHY driver provide an array of PHYs, search the right one,
> 	 * in the other case fill it with the phy_driver data.
> 	 */
> 	if (phy_driver_match(phydrv, phydev, &driver_dev_id) && driver_dev_id) {
> 		/* If defined, overwrite the PHY driver dev name with a
> 		 * more specific one from the matching dev_id.
> 		 */
> 		phydev->dev_id = driver_dev_id;
> 		if (driver_dev_id->name)
> 			drv->name = driver_dev_id->name;
> 	} else {
> 		dev_id = kzalloc(sizeof(*phydev->dev_id), GFP_KERNEL);
> 		if (!dev_id) {
> 			err = -ENOMEM;
> 			goto out;
> 		}
> 		dev_id->phy_id = phydrv->phy_id;
> 		dev_id->phy_id_mask = phydrv->phy_id_mask;
> 		dev_id->name = phydrv->name;
> 		phydev->dev_id = dev_id;
> 	}
> 
> Is it ok? (in phy.h the thing is const struct mdio_device_id *ids)
> I don't really like modifying phy_driver too much.

The thing that I don't like about this is that we need to free this
allocation (and know that we need to free it) which adds more
complexity and more possibilities for things leaking.

The advantage to putting it in the phy_driver structure is that its
lifetime is inherently tied to the driver structure and we don't
have the issue of having to free it - nor do we need to have separate
allocations for each PHY device.

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