On Thu, 4 May 2006 19:21:12 -0500 Andy Fleming <afleming at freescale.com> wrote:
> What happened to this patch? It doesn't seem to have been applied to > any trees. Well, I'm gonna give it a little review now, since I have > some time. > Under final review/updates, gonna to push uptodated shortly... > On Apr 3, 2006, at 10:26, Vitaly Bordug wrote: > > > > > This makes it possible for HW PHY-less boards to utilize PAL goodies. > > Generic routines to connect to fixed PHY are provided, as well as > > ability > > to specify software callback that fills up link, speed, etc. > > information > > into PHY descriptor (the latter feature not tested so far). > > > > Signed-off-by: Vitaly Bordug <vbordug at ru.mvista.com> > > --- > > [snip] > > > +/ > > *--------------------------------------------------------------------- > > -------- > > + * This func is used to create all the necessary stuff, bind > > + * the fixed phy driver and register all it on the mdio_bus_type. > > + * speed is either 10 or 100, duplex is boolean. > > + * number is used to create multiple fixed PHYs, so that several > > devices can > > + * utilize them simultaneously. > > + > > *--------------------------------------------------------------------- > > --------*/ > > +static int fixed_mdio_register_device(int number, int speed, int > > duplex) > > +{ > > + struct mii_bus *new_bus; > > + struct fixed_info *fixed; > > + struct phy_device *phydev; > > + int err = 0; > > + > > + struct device* dev = kzalloc(sizeof(struct device), GFP_KERNEL); > > + > > + if (NULL == dev) > > + return -EINVAL; > > + > > + new_bus = kzalloc(sizeof(struct mii_bus), GFP_KERNEL); > > + > > + if (NULL == new_bus) > > + return -ENOMEM; > > You don't free dev, here > > > + > > + fixed = kzalloc(sizeof(struct fixed_info), GFP_KERNEL); > > + > > + if (NULL == fixed) { > > + kfree(new_bus); > > + return -ENOMEM; > > + } > > And dev > > > > + > > + fixed->regs = kzalloc(MII_REGS_NUM*sizeof(int), GFP_KERNEL); > > You don't check for failure for regs's allocation. > As to upper notes, OK. > [snip] > > > + /* create phy_device and register it on the mdio bus */ > > + phydev = phy_device_create(new_bus, 0, 0); > > + > > + /* > > + Put the phydev pointer into the fixed pack so that bus read/ > > write code could be able > > + to access for instance attached netdev. Well it doesn't have to > > do so, only in case > > + of utilizing user-specified link-update... > > + */ > > + fixed->phydev = phydev; > > + > > + if(NULL == phydev) { > > + err = -ENOMEM; > > + goto bus_register_fail; > > + } > > You're going to need to change this, because phydev isn't guaranteed > to be NULL in the event of a failure to allocate. it will be ERR_PTR > (-ENOMEM). I know you changed this in phy_device_create(), but I > have more on that later. You should do: > > if (IS_ERR(phydev)) { > err = PTR_ERR(-ENOMEM); > goto bus_register_fail; > } > Assuming IS_ERR will shoot on NULL too, the code is not quite right(see below) :) But I agree this check is odd - will fix. > [snip] > > > + > > + return 0; > > + > > +bus_register_fail: > > + kfree(new_bus); > > You need to free regs and dev, too > > ok > > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > > index 459443b..c87f89e 100644 > > --- a/drivers/net/phy/mdio_bus.c > > +++ b/drivers/net/phy/mdio_bus.c > > @@ -66,7 +66,7 @@ int mdiobus_register(struct mii_bus *bus > > phydev = get_phy_device(bus, i); > > > > if (IS_ERR(phydev)) > > - return PTR_ERR(phydev); > > + continue; > > > No. Why'd you change that? Now mdiobus_register doesn't return an > error if memory runs out. Here's how the system works: > get_phy_device() can return one of three things: > > 1) A pointer to a newly allocated phy_device > 2) a NULL pointer, indicating that there is no PHY at that address > (indicated by the bus returning all Fs) > 3) an error (due to bus read failure, or to memory allocation > failure, as indicated by PTR_ERR(phydev) > > This change has several issues: > 1) due to the change below, IS_ERR(phydev) is never true > 2) If you continue, mdiobus_register() will not inform its caller > that it failed. > I am not really stick to this change, but it simply does not work otherwise. I want the whole bus to be scanned, and the code scans until first fail, and returns error when there's no phy. Hereby, having phy's on 0 and 3 I end up with only 0 registered on bus. So maybe check for NULL and continue, check for err and return... Will inquire and fix - no big deal. > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/ > > phy_device.c > > index 7da0e3d..0dffecf 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -46,6 +46,35 @@ static struct phy_driver genphy_driver; > > extern int mdio_bus_init(void); > > extern void mdio_bus_exit(void); > > > > +struct phy_device* phy_device_create(struct mii_bus *bus, int > > addr, int phy_id) > > +{ > > + struct phy_device *dev; > > + /* We allocate the device, and initialize the > > + * default values */ > > + dev = kcalloc(1, sizeof(*dev), GFP_KERNEL); > > + > > + if (NULL == dev) > > + return NULL; > > Here's the other change which breaks get_phy_device(). Now it > doesn't return an error when it fails to allocate memory, it returns > NULL. Which mdiobus_register doesn't interpret as an error (because > it isn't. Not every PHY address has a device on it). > OK, this part definitely needs a bit attention and a rework. So, phy_device_create should return PTR_ERR if it fail to allocate memory, and we need to keep get_phy_device() return as it was, right? > > + > > + dev->speed = 0; > > + dev->duplex = -1; > > + dev->pause = dev->asym_pause = 0; > > + dev->link = 1; > > + > > + dev->autoneg = AUTONEG_ENABLE; > > + > > + dev->addr = addr; > > + dev->phy_id = phy_id; > > + dev->bus = bus; > > + > > + dev->state = PHY_DOWN; > > + > > + spin_lock_init(&dev->lock); > > + > > + return dev; > > +} > > +EXPORT_SYMBOL(phy_device_create); > > Also, as a side note, I'm not completely convinced you need to go > through this degree of effort to circumvent the PHY Layer's normal > operation. I think it should be possible to make it simpler. With > the right implementation, it should even be possible to do really > "clever" things, like allow users to change the PHY settings with > ethtool. However, this code exists and works (I'm assuming), and > that's good enough for now. I'll be glad to have this capability > next time someone asks me to boot linux on a simulator. > I made it as it is not that complex as it seemed at the first sight, and may be a proving ground to some incoming PAL feature. Also, if there is fixed PHY, it often does not mean it is really "fixed" - it may be just far too weird to suite into any known form, but still able to control the link etc. So, the main aim is to do: if PAL doesn't know PHY, use fixed phy. If you do not want it fixed and wanna to control the link - ok, just implement the link update callback, pass it to emulated PHY, and here we go... -- Sincerely, Vitaly