| Message ID | 20190523011958.14944-3-ioana.ciornei@nxp.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Decoupling PHYLINK from struct net_device | expand |
On 5/22/2019 6:20 PM, Ioana Ciornei wrote: > A prerequisite for PHYLIB to work in the absence of a struct net_device > is to not access pointers to it. > > Changes are needed in the following areas: > > - Printing: In some places netdev_err was replaced with phydev_err. > > - Incrementing reference count to the parent MDIO bus driver: If there > is no net device, then the reference count should definitely be > incremented since there is no chance that it was an Ethernet driver > who registered the MDIO bus. > > - Sysfs links are not created in case there is no attached_dev. > > - No netif_carrier_off is done if there is no attached_dev. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
On Thu, May 23, 2019 at 01:20:38AM +0000, Ioana Ciornei wrote: > A prerequisite for PHYLIB to work in the absence of a struct net_device > is to not access pointers to it. > > Changes are needed in the following areas: > > - Printing: In some places netdev_err was replaced with phydev_err. > > - Incrementing reference count to the parent MDIO bus driver: If there > is no net device, then the reference count should definitely be > incremented since there is no chance that it was an Ethernet driver > who registered the MDIO bus. > > - Sysfs links are not created in case there is no attached_dev. > > - No netif_carrier_off is done if there is no attached_dev. Hi Ioana Looking at the functions changed here, they seem to be related to phy_attach(), phy_connect(), and phy_detach() etc. Is the intention you can call these functions and pass a NULL pointer for the net_device? Andrew
> Subject: Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a > netdev > > On Thu, May 23, 2019 at 01:20:38AM +0000, Ioana Ciornei wrote: > > A prerequisite for PHYLIB to work in the absence of a struct > > net_device is to not access pointers to it. > > > > Changes are needed in the following areas: > > > > - Printing: In some places netdev_err was replaced with phydev_err. > > > > - Incrementing reference count to the parent MDIO bus driver: If there > > is no net device, then the reference count should definitely be > > incremented since there is no chance that it was an Ethernet driver > > who registered the MDIO bus. > > > > - Sysfs links are not created in case there is no attached_dev. > > > > - No netif_carrier_off is done if there is no attached_dev. > > Hi Ioana > > Looking at the functions changed here, they seem to be related to phy_attach(), > phy_connect(), and phy_detach() etc. Is the intention you can call these > functions and pass a NULL pointer for the net_device? > > Andrew Hi Andrew, Yes, the intention is exactly to pass a NULL pointer for the net_device from PHYLINK. The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add phylink_create_raw". -- Ioana
> > Hi Ioana > > > > Looking at the functions changed here, they seem to be related to phy_attach(), > > phy_connect(), and phy_detach() etc. Is the intention you can call these > > functions and pass a NULL pointer for the net_device? > > > > Andrew > > Hi Andrew, > > Yes, the intention is exactly to pass a NULL pointer for the net_device from PHYLINK. > The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add phylink_create_raw". Hi Ioana I think in general, we don't want MAC drivers doing this. We should enforce that the general APIs get a netdev. PHYLINK uses phy_attach_direct() which is the lowest level of these attach() and connect() calls. And there is only one MAC driver using phy_attach_direct(). So please add checks for the netdev and return -EINVAL in these higher level callers to phy_attach_direct(). Thanks Andrew
> Subject: Re: [RFC PATCH net-next 2/9] net: phy: Guard against the presence of a > netdev > > > > Hi Ioana > > > > > > Looking at the functions changed here, they seem to be related to > > > phy_attach(), phy_connect(), and phy_detach() etc. Is the intention > > > you can call these functions and pass a NULL pointer for the net_device? > > > > > > Andrew > > > > Hi Andrew, > > > > Yes, the intention is exactly to pass a NULL pointer for the net_device from > PHYLINK. > > The changes that do this are in "[RFC,net-next,5/9] net: phylink: Add > phylink_create_raw". > > Hi Ioana > > I think in general, we don't want MAC drivers doing this. > Agreed. > We should enforce that the general APIs get a netdev. PHYLINK uses > phy_attach_direct() which is the lowest level of these attach() and > connect() calls. And there is only one MAC driver using phy_attach_direct(). So > please add checks for the netdev and return -EINVAL in these higher level callers > to phy_attach_direct(). > Will add the checks in v2. -- Ioana
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 5cb01b9db7b5..25cc7c33f8dd 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1138,6 +1138,9 @@ static void phy_sysfs_create_links(struct phy_device *phydev) struct net_device *dev = phydev->attached_dev; int err; + if (!dev) + return; + err = sysfs_create_link(&phydev->mdio.dev.kobj, &dev->dev.kobj, "attached_dev"); if (err) @@ -1176,9 +1179,9 @@ static void phy_sysfs_create_links(struct phy_device *phydev) int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, u32 flags, phy_interface_t interface) { - struct module *ndev_owner = dev->dev.parent->driver->owner; struct mii_bus *bus = phydev->mdio.bus; struct device *d = &phydev->mdio.dev; + struct module *ndev_owner = NULL; bool using_genphy = false; int err; @@ -1187,8 +1190,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, * our own module->refcnt here, otherwise we would not be able to * unload later on. */ + if (dev) + ndev_owner = dev->dev.parent->driver->owner; if (ndev_owner != bus->owner && !try_module_get(bus->owner)) { - dev_err(&dev->dev, "failed to get the bus module\n"); + phydev_err(phydev, "failed to get the bus module\n"); return -EIO; } @@ -1207,7 +1212,7 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, } if (!try_module_get(d->driver->owner)) { - dev_err(&dev->dev, "failed to get the device driver module\n"); + phydev_err(phydev, "failed to get the device driver module\n"); err = -EIO; goto error_put_device; } @@ -1228,8 +1233,10 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, } phydev->phy_link_change = phy_link_change; - phydev->attached_dev = dev; - dev->phydev = phydev; + if (dev) { + phydev->attached_dev = dev; + dev->phydev = phydev; + } /* Some Ethernet drivers try to connect to a PHY device before * calling register_netdevice() -> netdev_register_kobject() and @@ -1252,7 +1259,8 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev, /* Initial carrier state is off as the phy is about to be * (re)initialized. */ - netif_carrier_off(phydev->attached_dev); + if (dev) + netif_carrier_off(phydev->attached_dev); /* Do initial configuration here, now that * we have certain key parameters @@ -1358,16 +1366,19 @@ EXPORT_SYMBOL_GPL(phy_driver_is_genphy_10g); void phy_detach(struct phy_device *phydev) { struct net_device *dev = phydev->attached_dev; - struct module *ndev_owner = dev->dev.parent->driver->owner; + struct module *ndev_owner = NULL; struct mii_bus *bus; if (phydev->sysfs_links) { - sysfs_remove_link(&dev->dev.kobj, "phydev"); + if (dev) + sysfs_remove_link(&dev->dev.kobj, "phydev"); sysfs_remove_link(&phydev->mdio.dev.kobj, "attached_dev"); } phy_suspend(phydev); - phydev->attached_dev->phydev = NULL; - phydev->attached_dev = NULL; + if (dev) { + phydev->attached_dev->phydev = NULL; + phydev->attached_dev = NULL; + } phydev->phylink = NULL; phy_led_triggers_unregister(phydev); @@ -1390,6 +1401,8 @@ void phy_detach(struct phy_device *phydev) bus = phydev->mdio.bus; put_device(&phydev->mdio.dev); + if (dev) + ndev_owner = dev->dev.parent->driver->owner; if (ndev_owner != bus->owner) module_put(bus->owner);