netdev - Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy translation

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: <aea81dab-0f1c-b479-1ca5-b913ae93503d@gmail.com>
Date:   Wed, 6 Feb 2019 14:32:56 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Christian Lamparter <chunkeey@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH v1] net: dsa: qca8k: implement DT-based ports <-> phy
 translation

On 2/6/19 2:29 PM, Florian Fainelli wrote:
> On 2/6/19 1:57 PM, Christian Lamparter wrote:
>> On Tuesday, February 5, 2019 11:29:36 PM CET Florian Fainelli wrote:
>>> On 2/5/19 2:12 PM, Christian Lamparter wrote:
>>>> On Tuesday, February 5, 2019 10:29:34 PM CET Andrew Lunn wrote:
>>>>>> For now, I added the DT binding update to the patch as well.
>>>>>> But if this is indeed the way to go, it'll get a separate patch.
>>>>>
>>>>> Hi Christian 
>>>>>
>>>>> You need to be careful with the DT binding. You need to keep backwards
>>>>> compatible with it. An old DT blob needs to keep working. I don't
>>>>> think this is true with this change.
>>>>
>>>> Do you mean because of the 
>>>>
>>>> -               switch0@0 {
>>>> +               switch@10 {
>>>>                         compatible = "qca,qca8337";
>>>>                         #address-cells = <1>;
>>>>                         #size-cells = <0>;
>>>>  
>>>> -                       reg = <0>;
>>>> +                       reg = <0x10>;
>>>>
>>>> change?
>>>>
>>>> or because I removed the phy-handles?>
>>>> The reg = <0x10>; will be necessary regardless. Because this
>>>> is really a bug in the existing binding example and if it is
>>>> copied it will prevent the qca8k driver from loading. 
>>>> This is due to a resource conflict, because there will be 
>>>> already a "phy_port1: phy@0" registered at reg = <0>;
>>>> So this never worked would have worked.
>>>
>>> That part is fine, it is the removal of the phy-handle properties that
>>> is possibly a problem, but in hindsight, I do not believe it will be a
>>> compatibility issue. Lack of "phy-handle" property within the core DSA
>>> layer means: utilize the switch's internal MDIO bus (ds->slave_mii_bus)
>>> instance, which you are not removing, you are just changing how the PHYs
>>> map to port numbers.
>>>
>> Ok, thanks. 
>>
>> I think I'm almost ready for v2. I have fully addressed the compatibility
>> issue by forking off the qca8k_switch_ops depending on whenever a phy-handle
>> property on one of the ports was found or not. If there was no phy-handle the
>> driver adds the slave-bus accessors to the ops which tells DSA to allocate
>> the slave bus and allows the phys can be enumerated. If the phy-handles are
>> found the driver will not have the accessors and DSA will not setup a
>> redundant/fake bus and this prevents the second/double/duplicated discovery
>> and enumeration of the same PHYs again.
> 
> The logic you have sounds a little too broad since it stops as soon as
> one port is found with a 'phy-handle' property and assumes that the
> parent MDIO bus from which qca8k itself is a child device, is the MDIO
> bus to be used. There are possibly 3 cases:
> 
> 1) All ports using internal/build-in PHYs. In that case, you can either
> not specify a 'phy-handle' property and DSA assumes that they are part
> of the switch's internal MDIO bus. You can also specify a 'phy-handle'
> property that references the internal MDIO bus, although then we also
> expect qca8k to register its internal MDIO bus (ala mv88e6xxx)
> 
> 2) Some ports using internal PHYs, some using external PHYs. Similar
> situation again, ports may, or may not specify a 'phy-handle' property,
> so without a 'phy-handle' property that means the port connects to an
> internal PHY, with a 'phy-handle' it could connect to either internal
> PHY or external PHY
> 
> 3) All ports using external PHYs, in that case, we must have a
> 'phy-handle' for each port to specify where and how they connect to
> their external PHYs.
> 
> With respect to your patch, what I would do is register QCA8k's internal
> MDIO bus as a proper mdio bus and use ds->slave_mii_bus as a storage for
> that bus, such that tell the DSA layer: look, here is the internal MDIO
> bus, would you ever find a port that needs to use a PHY in there.
> 
> Then you can still scan each enabled port device, and for each of them,
> populate ds->phys_mii_mask, thus telling DSA exacly which ports are
> using an internal PHY because that would be the ports that do not have a
> 'phy-handle' property. Ports that have a 'phy-handle' property.

Forgot to finish my sentence here. This should read:

Ports that have a 'phy-handle' property will either point to the QCA8k's
internal MDIO bus or an external one, but that will be transparently be
handled during PHY device creation.

Thanks!
-- 
Florian