Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440182

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

Show all headers | View raw

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@gmail.com>
>>>>> 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@gmail.com>
>>>>>
>>>>> 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.

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 ?

If everybody agree with this, I may send a patch that implement this idea.


Philippe

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings Philippe Reynes <tremyfr@gmail.com> - 2016-07-03 17:40 +0200
  Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings David Miller <davem@davemloft.net> - 2016-07-05 01:10 +0200
    Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-05 06:40 +0200
      Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings Philippe Reynes <tremyfr@gmail.com> - 2016-07-05 23:10 +0200
        Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-05 23:20 +0200
          Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Ben Hutchings <ben@decadent.org.uk> - 2016-07-05 23:50 +0200
            Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings Philippe Reynes <tremyfr@gmail.com> - 2016-07-11 01:00 +0200
              Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-11 04:40 +0200
        Re: [PATCH 2/2] net: ethernet: bcmgenet: use  phy_ethtool_{get|set}_link_ksettings Florian Fainelli <f.fainelli@gmail.com> - 2016-07-08 03:30 +0200

csiph-web