netdev - Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

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, 29 Oct 2019 10:59:07 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Michael Walle <michael@...le.cc>, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 0/3] net: phy: initialize PHYs via device tree properties

On 10/29/19 10:48 AM, Michael Walle wrote:
> I was trying to configure the Atheros PHY for my board. There are fixups
> all over the place, for example to enable the 125MHz clock output in almost
> any i.MX architecture. Instead of adding another fixup in architecture
> specific code, try to provide a generic way to init the PHY registers.
> 
> This patch series tries to pick up the "broadcom,reg-init" and
> "marvell,reg-init" device tree properties idea and make it a more generic
> "reg-init" which is handled by phy_device instead of a particular phy
> driver.

These two examples are actually quite bad and were symptomatic of a few
things at the time:

- rush to get a specific feature/device supported without thinking about
the big picture
- lack of appropriate review on the Device Tree bindings

Fortunately, the last item is now not happening anymore.

The problem with letting that approach go through is that the Device
Tree can now hold a configuration policy which is passed through as-is
from DT to the PHY device, this is bad on so many different levels,
starting with abstraction.

If all you need is to enable a particular clock, introduce device
specific properties that describe the hardware, and make the necessary
change to the local driver that needs to act on those. You can always
define a more generic scope property if you see a recurring pattern.

So just to be clear on the current approach: NACK.

> 
> Michael Walle (3):
>   dt-bindings: net: phy: Add reg-init property
>   net: phy: export __phy_{read|write}_page
>   net: phy: Use device tree properties to initialize any PHYs
> 
>  .../devicetree/bindings/net/ethernet-phy.yaml | 31 ++++++
>  MAINTAINERS                                   |  1 +
>  drivers/net/phy/phy-core.c                    | 24 ++++-
>  drivers/net/phy/phy_device.c                  | 97 ++++++++++++++++++-
>  include/dt-bindings/net/phy.h                 | 18 ++++
>  include/linux/phy.h                           |  2 +
>  6 files changed, 170 insertions(+), 3 deletions(-)
>  create mode 100644 include/dt-bindings/net/phy.h
> 
> Cc: Andrew Lunn <andrew@...n.ch>
> Cc: Florian Fainelli <f.fainelli@...il.com>
> Cc: Heiner Kallweit <hkallweit1@...il.com>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Rob Herring <robh+dt@...nel.org>
> Cc: Mark Rutland <mark.rutland@....com>
> 


-- 
Florian