netdev - Re: [net-next] dsa: slave: support phy devices on external MII bus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8994adaa-6223-20f1-9221-49e9b644d03f@prevas.dk>
Date:   Wed, 18 Oct 2017 19:30:59 +0200
From:   Martin Hundebøll <mnhu@...vas.dk>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [net-next] dsa: slave: support phy devices on external MII bus



On 2017-10-18 18:51, Florian Fainelli wrote:
> On 10/18/2017 09:21 AM, Andrew Lunn wrote:
>> Hi Martin
>>
>> Sorry for starting a new thread. I deleted the patchset from my mailbox.
>>
>> Florian said:
>>
>>> The logic goes like this:
>>>
>>> - try to connect to the PHY via phy-handle
>>> - if we have a PHY we are connecting via phy-handle but we need to
>>>    divert MDIO reads/writes connect using its address on the diverted
>>>    bus
>>> - connect using a fixed PHY
>>> - finally try using the DSA slave MII bus which would connect to the switch internal PHYs
>>
>> This is not quite correct. Looking at the code:
>>
>>          phy_dn = of_parse_phandle(port_dn, "phy-handle", 0);
>>   ...
>>
>>          if (phy_dn) {
>>                  int phy_id = of_mdio_parse_addr(&slave_dev->dev, phy_dn);
>>
>>                  /* If this PHY address is part of phys_mii_mask, which means
>>                   * that we need to divert reads and writes to/from it, then we
>>                   * want to bind this device using the slave MII bus created by
>>                   * DSA to make that happen.
>>                   */
>>                  if (!phy_is_fixed && phy_id >= 0 &&
>>                      (ds->phys_mii_mask & (1 << phy_id))) {
>>                          ret = dsa_slave_phy_connect(p, slave_dev, phy_id);
>>                          if (ret) {
>>                                  netdev_err(slave_dev, "failed to connect to phy%d: %d\n", phy_id, ret);
>>                                  of_node_put(phy_dn);
>>                                  return ret;
>>                          }
>>                  } else {
>>                          p->phy = of_phy_connect(slave_dev, phy_dn,
>>
>> The first point really is:
>>
>> - try to connect to the PHY via phy-handle, if the phy_id is not
>> valid, or if the phy_id does not map to a phy that the switch says
>> does not exist.
>>
>> In your case, all these points are true, so it uses
>> dsa_slave_phy_connect().  But we actually want it to use
>> of_phy_connect(), which will use the correct bus.
>>
>> For some Marvell chips, you cannot actually go on ds->phys_mii_mask.
>> These devices can have in built PHYs and SERDES interfaces which can
>> be assigned to ports. These SERDES interfaces could have external PHYs
>> connected to them, and be on the external MDIO bus. So
>> ds->phys_mii_mask indicates there is a PHY, but the phy-handle points
>> to a different phy.
>>
>> So i think this code block needs to change. If we have a phy-handle,
>> use it.  i.e. what Florian _thinks_ it should be doing. If not, then
>> use dsa_slave_phy_connect().
> 
> I see what you mean now, the logic above gets defeated because it does
> not concern itself with the MDIO controller parent of the PHY node
> pointed to by phy-handle. So if like Martin you have two MDIO busses,
> but both happen to have MDIO addresses that are valid for both busses,
> the logic above gets defeated and we wrongly try to attach to the switch
> internal MDIO bus under ds->slave_mii_bus.
> 
> The easiest fix would certainly to lookup the parent MDIO bus and do
> that only if ds->slave_mii_bus->of_node and the parent of the node
> pointed to 'phy-handle' match.
> 
> Does that work for you?
> 

As Andrew implies, I think we should rewrite the entire block to make it 
more intuitive.

Are these the cases that should be handled?

0) Fixed link
Register using of_phy_register_fixed_link().

1) No phy-handle
Use dsa_slave_phy_connect() to connect on internal MDIO bus with phy 
address from index/port-reg property.

2) Valid phy handle
Use of_phy_connect() to connect using parent MDIO bus handle.


I am most certainly missing some corner cases here, so please educate me!

// Martin