Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings

[Florian Fainelli]

Le 10/07/2016 15:50, Philippe Reynes a Ãcrit :
> Hi all,
> 
> 
> On 05/07/16 23:40, Ben Hutchings wrote:
>> On Tue, 2016-07-05 at 14:15 -0700, Florian Fainelli wrote:
>>> On 07/05/2016 02:07 PM, Philippe Reynes wrote:
>>>> Hi Florian,
>>>>
>>>> On 05/07/16 06:30, Florian Fainelli wrote:
>>>>> Le 04/07/2016 16:03, David Miller a Ãcrit :
>>>>>> From: Philippe Reynes<tremyfr@xxxxxxxxx>
>>>>>> Date: Sun,  3 Jul 2016 17:33:57 +0200
>>>>>>
>>>>>>> There are two generics functions
>>>>>>> phy_ethtool_{get|set}_link_ksettings,
>>>>>>> so we can use them instead of defining the same code in the driver.
>>>>>>>
>>>>>>> Signed-off-by: Philippe Reynes<tremyfr@xxxxxxxxx>
>>>>>>
>>>>>> Applied.
>>>>>>
>>>>>
>>>>> The transformation is not equivalent, we lost the checks on
>>>>> netif_running() in the process, and those are here for a reason, if
>>>>> the
>>>>> interface is down and therefore clock gated, MDIO accesses to the PHY
>>>>> will simply fail outright and cause bus errors.
>>>>
>>>> Oh, I see, I've missed this. Sorry for this mistake.
>>>> We should revert this path.
>>>
>>> Well, maybe better than that, actually put the check in the generic
>>> functions, because if the link is down, aka netif_running() returns
>>> false, link parameters cannot be reliably queried and they are invalid.
> 
> In my understanding, if the link is down, the pointer phydev in the
> struct net_device is NULL. So generic functions
> phy_ethtool_{get|set}_link_ksetting
> should return -ENODEV.

This pointer is NULL only when phy_detach() is called typically by
phy_disconnect(), but remains and needs to be valid reference throughout
the entire time the network interface is open regardless of the link state.

One of the reasons why some drivers are checking for the phydev pointer
to be NULL is because you can have one process open the network
interface, attaching to the PHY at some point in that ndo_open, and
another one calling in ethtool_get_settings for instance and this second
process can see a transient state where the PHY device is not yet
attached (aka NULL).

> 
> If I'm wrong, and it everybody agree, I'll send a patch that add a check
> on netif_running.
> 
>> Either the hardware or the driver needs to remember:
>>
>> - Is auto-negotiation enabled
>> - If so, which modes are advertised
>> - If not, which mode is forced
>>
>> And it should still be possible to get or set that information when the
>> interface is down.
> 
> It could be possible to save the set_link_ksettings request if the link
> is down, and apply it when the link become up.
> It also could be possible to save the last state of the link before it
> goes to down, and return it to a get_link_ksettings when the link is down.
> But what happen if the link was never up before the first
> get_link_kettings ?

Humm, sure, I would take a few examples first and see how consistent
they are with allowing these operations while the link is down and move
from there.

Thanks!
-- 
Florian