netdev - Re: [PATCH] net: macb: reject unsupported rgmii delays

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]

Message-ID: <20200618081433.GA22636@laureti-dev>
Date:   Thu, 18 Jun 2020 10:14:33 +0200
From:   Helmut Grohne <helmut.grohne@...enta.de>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     Nicolas Ferre <nicolas.ferre@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] net: macb: reject unsupported rgmii delays

On Wed, Jun 17, 2020 at 02:08:09PM +0200, Russell King - ARM Linux admin wrote:
> With a fixed link, we could be in either a MAC-to-PHY or MAC-to-MAC
> setup; we just don't know.  However, we don't have is access to the
> PHY (if it exists) in the fixed link case to configure it for the
> delay.

Let me twist that a little: We may have access to the PHY, but we don't
always have access. When we do have access, we have a separate device
tree node with another fixed-link and another phy-mode. For fixed-links,
we specify the phy-mode for each end.

> In the MAC-to-MAC RGMII setup, where neither MAC can insert the
> necessary delay, the only way to have a RGMII conformant link is to
> have the PCB traces induce the necessary delay. That errs towards
> PHY_INTERFACE_MODE_RGMII for this case.

Yes.

> However, considering the MAC-to-PHY RGMII fixed link case, where the
> PHY may not be accessible, and may be configured with the necessary
> delay, should that case also use PHY_INTERFACE_MODE_RGMII - clearly
> that would be as wrong as using PHY_INTERFACE_MODE_RGMII_ID would
> be for the MAC-to-MAC RGMII with PCB-delays case.

If you take into account that the PHY has a separate node with phy-mode
being rgmii-id, it makes a lot more sense to use rgmii for the phy-mode
of the MAC. So I don't think it is that clear that doing so is wrong.

In an earlier discussion Florian Fainelli said:
https://patchwork.ozlabs.org/project/netdev/patch/20190410005700.31582-19-olteanv@gmail.com/#2149183
| fixed-link really should denote a MAC to MAC connection so if you have
| "rgmii-id" on one side, you would expect "rgmii" on the other side
| (unless PCB traces account for delays, grrr).

For these reasons, I still think that rgmii would be a useful
description for the fixed-link to PHY connection where the PHY adds the
delay.

> So, I think a MAC driver should not care about the specific RGMII
> mode being asked for in any case, and just accept them all.

I would like to agree to this. However, the implication is that when you
get your delays wrong, your driver silently ignores you and you never
notice your mistake until you see no traffic passing and wonder why.

In this case, I was faced with a PHY that would do rgmii-txid and I
configured that on the MAC. Unfortunately, macb_main.c didn't tell me
that it did rgmii-id instead.

> I also think that some of this ought to be put in the documentation
> as guidance for new implementations.

That seems to be the part where everyone agrees.

Given the state of the discussion, I'm wondering whether this could be
fixed at a more fundamental level in the device tree bindings.

A number of people (including you) pointed out that retroactively fixing
the meaning of phy modes does not work and causes pain instead. That
hints that the only way to fix this is adding new properties. How about
this?

rgmii-delay-type:
  description:
    Responsibility for adding the rgmii delay
  enum:
    # The remote PHY or MAC to this MAC is responsible for adding the
    # delay.
    - remote
    # The delay is added by neither MAC nor MAC, but using PCB traces
    # instead.
    - pcb
    # The MAC must add the delay.
    - local
rgmii-rx-delay:
  # Responsibility for RX delay. Delay specification in the phy-mode is
  # ignored when this is present.
  $ref: "#/properties/rgmii-delay-type"
rgmii-tx-delay:
  # Responsibility for TX delay. Delay specification in the phy-mode is
  # ignored when this is present.
  $ref: "#/properties/rgmii-delay-type"

The naming is up to discussion, but I think you get the idea. The core
properties of this proposal are:
 * It does not break existing device trees.
 * It completely resolves the present ambiguity.
The major downside is that you never know whether your driver supports
such delay specifications already.

Helmut