netdev - Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

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, 13 Sep 2016 13:11:56 -0400
From:   Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:     Andrew Lunn <andrew@...n.ch>, John Crispin <john@...ozen.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        qsdk-review@....qualcomm.com
Subject: Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

Hi Andrew,

Andrew Lunn <andrew@...n.ch> writes:

>> ok, i will simply substract 1 from the phy_addr inside the mdio
>> callbacks. this would make the code more readable and make the DT
>> binding compliant with the ePAPR spec.
>
> It does however need well commenting. It is setting a trap for anybody
> who puts an external PHY on port 6. If they access that PHY via these
> functions, the address is off by one.
>
> This is the first silicon vendor who made their MDIO addresses for
> PHYs illogical. So i'm thinking we maybe should add a new function to
> dsa_switch_ops.
>
> 	/* Return the MDIO address for the PHY for this port. */
>         int     (*phy_port_map(struct dsa_switch *ds, int port);
>
> This should return the MDIO address for integrated PHYs only, or
> -ENODEV if the port does not have an integrated PHY. For an external
> PHY, a phy-handle should be used. This phy_port_map() is used in
> dsa_slave_phy_setup(). But dsa_slave_phy_setup() is already too
> complex, so it needs doing with care.

Note that some switch drivers *have to* register their slave MDIO bus
themselves (e.g. bcm_sf2). This becomes confusing with the DSA
phy_{read,write} ops.

Since the former alternative is prefered, we may want to remove the
latter soon from DSA. If this phy_port_map is needed for that case, it'd
be preferable not to add it.

Thanks,

        Vivien