Re: [PATCH net-next 0/2] net: phy: add generic ndo_do_ioctl handler phy_do_ioctl

[Heiner Kallweit <hkallweit1@gmail.com>]

On 19.01.2020 18:51, Andrew Lunn wrote:
> Hi Heiner
> 
>> Almost all drivers have the running check. I found five that don't:
>>
>> *ag71xx, fec_mpc52xx*
>> They don't have the running check but should, because the PHY is
>> attached in ndo_open only.
> 
> So long an ndo_close() sets the phydev pointer to NULL, it should be
> safe. But do the drivers do this?
> 
Setting ndev->phydev to NULL is done by phy_detach(), typically
called from phy_disconnect(). Both drivers do this.
In mpc52xx_fec_close() there might be a small chance for a race
because the chip is stopped before calling phy_diconnect().
Stopping the chip may make the MDIO bus unaccessible.

Actually I was wondering why we need to check at all whether
net_device is running. Typically the PHY is connected / disconnected
in ndo_open / ndo_close, and if both are implemented sanely then
checking for ndev->phydev should be sufficient.

Also it seems we don't consider situations like runtime PM yet.
Then the MDIO bus may not be accessible, but ndev is running
and PHY is attached. Maybe we should add a check for ndev being
present? Because typically netif_device_detach() is called
when (runtime-)suspending.

>> *agere, faraday, rdc*
>> They don't have the running check and attach the PHY in probe.
>>
>> So yes, we could add a second helper w/o the running check, even if
>> it's just for three drivers. There may be more in the future.
>>
>>> Do you plan to convert any more MAC drivers?
>>>
>> Not yet ;) Question would be whether one patch would be sufficient
>> or whether we need one patch per driver that needs to be ACKed by
>> the respective maintainer.
> 
> For this sort of mechanical change, i would do one patch for all
> without running, and another with running. If any driver needs more
> than a mechanical change, then do a patch per driver, and get the
> maintainer involved.
> 
>     Andrew
> 

Heiner