linux-kernel - RE: [PATCH net-next 3/3] net: phy: Enable C45 PHYs with vendor specific address space

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:   Wed, 18 Apr 2018 09:38:47 +0000
From:   Vicenţiu Galanopulo 
        <vicentiu.galanopulo@....com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "robh@...nel.org" <robh@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "marcel@...tmann.org" <marcel@...tmann.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
CC:     Alexandru Marginean <alexandru.marginean@....com>,
        Madalin-cristian Bucur <madalin.bucur@....com>
Subject: RE: [PATCH net-next 3/3] net: phy: Enable C45 PHYs with vendor
 specific address space

> > Having dev-addr stored in devices_addrs, in get_phy_c45_ids(), when
> > probing the identifiers, dev-addr can be extracted from devices_addrs
> > and probed if devices_addrs[current_identifier] is not 0.
> 
> I must clearly be missing something, but why are you introducing all these
> conditionals instead of updating the existing code to be able to operate against
> an arbitrary dev-addr value, and then just making sure the first thing you do is
> fetch that property from Device Tree? There is no way someone is going to be
> testing with your specific use case in the future (except yourselves) so unless you
> make supporting an arbitrary "dev-addr" value become part of how the code
> works, this is going to be breaking badly.
>

Hi Florian,

My intention was to have this patch as "plugin" and modify the existing kernel API little to none. I was thinking that with a #ifdef, ideally,  all changes could be part of a CONFIG kernel option.
Updating the existing code, instead of the conditionals, might run into just that, and the change could propagate across multiple modules. This is from my first RFC patch, review by Andrew:
        of_mdiobus_register(), when it loops over the children, looks for 
        the new property. If found, it passed dev-id to of_mdiobus_register_phy().
        That passes it to get_phy_device(). I think get_phy_device() can 
        then set the ID in c45_ids, before passing it to get_phy_id().
        get_phy_c45_ids() will first look at devices in package and can add 
        further devices to c45_ids. It will then probe both those found, and 
        the static
        one you added.
                                                                                              Andrew

        [Vicenţiu Galanopulo]
        Just to make sure I understand. Do you want me to change the signature 
        of all of_mdiobus_register_phy(), get_phy_device(), get_phy_id() and
        get_phy_c45_ids() and include the dev_addr parameter obtained from the 
        device tree?  (a propagation of this parameter across all functions 
        all the way to
        get_phy_c45_devs_in_pkg?) This will impact xgbe-mdio.c, fixed_phy.c 
        because get_phy_device() is used in these files. 

 The "catch" is to transport the dev-addr value from of_mdio.c (location of the  loop of the PHY device tree node which reads all PHY node properties) to phy_device.c (this is where you can get the PHY ID).
My understanding from Andrew's comment is that the key here is the c45_ids, and that these could be filled in of_mdio.c, first, with the IDs from dev-addr (he called them "static" as they are queried directly by using the value of dev-addr) and afterwards, in phy_device.c (following the lookup loop - in a "dynamic" way).
There's nothing more to this patch than some functionality from phy_device.c ported to of_mdio.c, to enable the extraction of the PHY IDs. 
I guess the code redundancy could be reduced (between of_mdio.c and phy_device.c) and maybe you or Andrew could comment on this if you would like to go with this patch approach.

Not sure I understand your comment about the specific use case and the breaking badly part.  
Right now I'm able to test because I have access to a PHY with dev-addr = 0x1e. 
But the whole mechanism in this patch starts to work the moment you set <dev-addr> in the device tree. If you don't set that, nothing happens. If you set it to a bogus value, no PHY ID will be found at that address. Besides that, the PHY ID extraction code is the same as what is currently working in phy_device.c. 
80-90% of the patch is based on what already exists in phy_device.c and of_mdio.c. Where is the breaking badly part supposed to happen? 

> And please, can you keep me copied for next submissions?
Yes, the "to" list was pretty long and I somehow missed you. Sorry.

Vicentiu