netdev - Re: [PATCH net-next 1/4] net: dsa: allow switch drivers to override default slave PHY addresses

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] [day] [month] [year] [list]

Message-ID: <f7b9de1b0caa4b27ccfe23c969ff586ec7ecefea.camel@ew.tq-group.com>
Date:   Mon, 20 Apr 2020 11:16:46 +0200
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        andrew@...n.ch, vivien.didelot@...il.com, davem@...emloft.net,
        kuba@...nel.org
Subject: Re: [PATCH net-next 1/4] net: dsa: allow switch drivers to override
 default slave PHY addresses

On Tue, 2020-03-31 at 11:09 +0200, Matthias Schiffer wrote:
> On Mon, 2020-03-30 at 20:04 -0700, Florian Fainelli wrote:
> > 
> > On 3/30/2020 6:53 AM, Matthias Schiffer wrote:
> > > Avoid having to define a PHY for every physical port when PHY
> > > addresses
> > > are fixed, but port index != PHY address.
> > > 
> > > Signed-off-by: Matthias Schiffer <
> > > matthias.schiffer@...tq-group.com
> > > > 
> > 
> > You could do this much more elegantly by doing this with Device
> > Tree
> > and
> > specifying the built-in PHYs to be hanging off the switch's
> > internal
> > MDIO bus and specifying the port to PHY address mapping, you would
> > only
> > patch #4 then.
> 
> This does work indeed, but it seems we have different ideas on
> elegance.
> 
> I'm not happy about the fact that an implementor needs to study the
> switch manual in great detail to find out about things like the PHY
> address offsets when the driver could just to the right thing by
> default. Requiring this only for some switch configurations, while
> others work fine with the defaults, doesn't make this any less
> confusing (I'd even argue that it would be better if there weren't
> any
> default PHY and IRQ mappings for the switch ports, but I also
> understand that this can't easily be removed at this point...)
> 
> In particular when PHY IRQ support is desired (not implemented on the
> PHY driver side for this switch yet; not sure if my current project
> will require it), indices are easy to get wrong - which might not be
> noticed as long as there is no PHY driver with IRQ support for the
> port
> PHYs, potentially breaking existing Device Trees with future kernel
> updates. For this reason, I think at least patch #2 should be
> considered even if #1 and #3 are rejected.
> 
> Kind regards,
> Matthias

net-next is open again, and I'd like to come to a conclusion regarding
this patch series before I resend it (or parts of it).

It is my impression that a detailed configuration in the Device Tree is
most useful when the driver that it configures is very generic, so
different values are useful in practice.

The mv88e6xxx driver is not generic in this sense: It already lists
every single piece of supported hardware, often using completely
different code for different chips - which is already not configurable
in the Device Tree (and being able to wouldn't make much sense IMO).

Having the additional pieces of sane defaults in the driver that are
introduced by the patches 1..3 of this series avoids mistakes in the DT
configuration, for settings that never need to be modified for a given
switch model supported by mv88e6xxx.

Kind regards,
Matthias