Thu, Mar 28, 2019 at 08:51:34PM CET, and...@lunn.ch wrote:
>On Thu, Mar 28, 2019 at 05:59:50PM +0000, Petr Machata wrote:
>
>Hi Petr
>
>> - PHY driver: dev->phydev->drv->get_link_down_reason
>> Certain PHY drivers might want to have a custom handling for some
>> PHY-specific insight. Something like:
>>
>> modified include/linux/phy.h
>> @@ -636,4 +636,7 @@ struct phy_driver {
>> struct ethtool_tunable *tuna,
>> const void *data);
>> int (*set_loopback)(struct phy_device *dev, bool enable);
>> + int (*get_link_down_reason)(struct phy_device *dev,
>> + enum link_down_reason_major *major,
>> + u32 *minor);
>> };
>>
>> Return -ENODATA to indicate there's nothing to report.
>>
>> - Generic PHY
>> It would be possible to add a function that e.g. consults the PHY
>> state machine to figure out what went wrong. Phy as such may have to
>> be extended to keep track of e.g. why the state machine is PHY_HALTED,
>> or possibly other problems worthy of reporting.
>
>I would suggest changing the order of these two. Put the generic PHY
>first. If it sees the driver has a OP to get more specific detail, it
>would make the call into the driver. You don't violate any layering
>that way, and the locking is done correctly.
>
>The PHY being in state HALTED probably means there is a hardware error
>at the MDIO level. That does not happen very often at all.
>
>What you are more interested is state PHY_NOLINK, which indicates
>there is no link. Maybe because auto-neg didn't complete, if it was
>turned on, or if auto-neg is off and the link is forced, the peer is
>using something different, or is not there at all.
>
>> - phylink
>> I'm not sure how to generically plug in the phylink library, because
>> it is a private property of a MAC driver. There are currently only
>> three drivers using phylink (mvvp2, mvneta, DSA), but it looks as if
>> the intention is that phylink is used much more widely. So maybe it
>> would make sense to have an NDO like get_phylink and use that to call
>> into the phylink library for some generic handling.
>
>We need to consider adding a phylink pointer to the netdev structure
>soon, in the same way there is a phylib structure.
>
>> Speaking specifically, my patch set would only do the first step above,
>> because neither mlxsw nor mlx5 use the PHY subsystem.
>
>Shame. With just mlx, you will get a coverage of ~0. If you did the
>generic phylib work, you might get coverage of > 50% of the MAC
>drivers. It then becomes something useful and really used.
Yeah, that's what it is. But maybe someone would pick up the work and
do the phylib implementation too. Let's see.
>
> Andrew
