netdev - Re: [PATCH v5 net-next 5/9] enetc: Make MDIO accessors more generic and export to include/linux/fsl

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]

Date:   Tue, 7 Jan 2020 11:22:26 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Xiaoliang Yang <xiaoliang.yang_1@....com>,
        "Y.b. Lu" <yangbo.lu@....com>, netdev <netdev@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH v5 net-next 5/9] enetc: Make MDIO accessors more generic
 and export to include/linux/fsl

On 1/7/20 1:46 AM, Vladimir Oltean wrote:
> 
> So we don't typically have the external MDIO controller be part of the
> memory map of an Ethernet port, it has its own region in the SoC. But
> this is not an external MDIO controller, and it's a completely
> separate hardware instance with its own memory region not shared with
> anybody else.

OK, I do not think I managed to express myself correctly, by external
MDIO controller I meant a controller that is used to interface with
off-chip (and sometimes on-chip, too) MDIO devices. That is an on-chip
MDIO controller technically, pardon me for not expressing myself more
clearly here.

> 
> And I think your need for registering the slave MDIO bus in the
> Starfighter 2 switch driver, on top of the same memory region as the
> GENET's MDIO bus, is just to work around the fact that the switch
> pseudo-PHY MDIO address is fixed at 30 on some chips, and you need to
> intercept those transactions by wrapping the mii_bus->read and
> mii_bus->write methods of the master, otherwise none of this would
> have been needed.

You are conflating two things that are not related to the point being
discussed. The SF2 switch used on 7445 and 7278 has some wrapping around
the Roboswitch original IP. The roboswitch has an internal MDIO
controller which is managed through the register space within the
switch. We added a MDIO controller to interface with both our internal
Gigabit PHYs as well as external MDIO devices would such thing exist.
The binding show the MDIO node as part of "switch_top" but not being
part of "core":
Documentation/devicetree/bindings/net/brcm,bcm7445-switch-v4.0.txt.

The same IP used as the MDIO controller (those two 32-bit words with CFG
and CMD) are the same as those that are used on GENET, hence the desire
to re-use the same piece of driver.

> If I understand correctly, couldn't you have just overridden the
> mii_bus->read and mii_bus->write ops of the master MDIO bus, without
> registering another one?

Yes maybe, I do not think this is particularly relevant to this
discussion though.

> 
>>>
>>>> Your commit message does not provide a justification for why this
>>>> abstraction (mii_bus) was not suitable or considered here. Do you think
>>>> that could be changed?
>>>>
>>>
>>> I'm sorry, was the mii_bus abstraction really not considered here?
>>> Based on the stuff exported in this patch, an mii_bus is exactly what
>>> I'm registering in 9/9, no?
>>
>> I meat in the commit message, there is no justification why this was not
>> considered or used, by asking you ended up providing one, that is
>> typically what one would expect to find to explain why something was/was
>> not considered. It's fine, the code is merge, I won't object or require
>> you to use a mii_bus abstraction.
> 
> So I answered your question? If so, it was by mistake, because I still
> don't exactly understand your point (although I would like to).
> The difference is that we don't register a new platform device for the
> mii_bus (we use mii_bus->dev = ocelot->dev), and that it's not in
> drivers/net/phy/mdio-fsl-enetc.c, although it could be, but I don't
> see a clear benefit.

To go back to the original point, I suppose the answer is no: we did not
consider using a mii_bus abstraction and in hindsight, we do not see the
point but it was interesting you brought that up. Case closed from my
side, thank you.
-- 
Florian