On 12/08/2016 08:27 AM, Johan Hovold wrote:
> On Tue, Dec 06, 2016 at 08:54:43PM -0800, Florian Fainelli wrote:
>> Commit 3e3aaf649416 ("phy: fix mdiobus module safety") fixed the way we
>> dealt with MDIO bus module reference count, but sort of introduced a
>> regression in that, if an Ethernet driver registers its own MDIO bus
>> driver, as is common, we will end up with the Ethernet driver's
>> module->refnct set to 1, thus preventing this driver from any removal.
>>
>> Fix this by comparing the network device's device driver owner against
>> the MDIO bus driver owner, and only if they are different, increment the
>> MDIO bus module refcount.
>>
>> Fixes: 3e3aaf649416 ("phy: fix mdiobus module safety")
>> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
>> ---
>> Russell,
>>
>> I verified this against the ethoc driver primarily (on a TS7300 board)
>> and bcmgenet.
>>
>> Thanks!
>>
>> drivers/net/phy/phy_device.c | 16 +++++++++++++---
>> 1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 1a4bf8acad78..c4ceb082e970 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -857,11 +857,17 @@ EXPORT_SYMBOL(phy_attached_print);
>> 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;
>
> Is this really safe? A driver does not need to set a parent device, and
> in that case you get a NULL-deref here (I tried using cpsw).
Humm, cpsw does call SET_NETDEV_DEV() which should take care of that, is
the call made too late? Do you have an example oops?
I don't mind safeguarding this with a check against dev->dev.parent, but
I would like to fix the drivers where relevant too, since
SET_NETDEV_DEV() should really be called, otherwise a number of things
just don't work
>
>> struct mii_bus *bus = phydev->mdio.bus;
>> struct device *d = &phydev->mdio.dev;
>> int err;
>>
>> - if (!try_module_get(bus->owner)) {
>> + /* For Ethernet device drivers that register their own MDIO bus, we
>> + * will have bus->owner match ndev_mod, so we do not want to increment
>
> You also wanted s/ndev_mod/ndev_owner/ here.
Meh, it's merged now, but thanks, I will fix this once we find out the
proper solution for cpsw.
--
Florian
