netdev - Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON EdgeRouter Lite

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: <20190324201734.GC7782@lunn.ch>
Date:   Sun, 24 Mar 2019 21:17:34 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Aaro Koskinen <aaro.koskinen@....fi>
Cc:     Vinod Koul <vkoul@...nel.org>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-mips@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [BISECTED, REGRESSION] Broken networking on MIPS/OCTEON
 EdgeRouter Lite

On Fri, Mar 22, 2019 at 11:41:20PM +0200, Aaro Koskinen wrote:
> Hi,
> 
> On Fri, Mar 22, 2019 at 10:25:57PM +0100, Andrew Lunn wrote:
> > > The OCTEON HW code knows only about RGMII. And looking at
> > > octeon ethernet staging driver it does phy connect always with
> > > PHY_INTERFACE_MODE_GMII. I did some experimentation, and it seems that
> > > with PHY_INTERFACE_MODE_RGMII_RXID it starts to work.. In the DT we have
> > > for ethernet for this board:
> > > 
> > > 	rx-delay = <0>;
> > > 	tx-delay = <0x10>;
> > 
> > These are not PHY properties. 
> > 
> > Looking at the code, it looks like these control delays the MAC
> > inserts. I don't see a binding document for these properties, so i've
> > no idea what 0x10 means. Before this driver moves out of staging,
> > these values should be changed to be in ns.
> 
> Documentation/devicetree/bindings/net/cavium-pip.txt

Hi Aaro

Ah, sorry, missed that.

- rx-delay: Delay value for RGMII receive clock. Optional. Disabled if 0.
  Value range is 1-31, and mapping to the actual delay varies depending on HW.

- tx-delay: Delay value for RGMII transmit clock. Optional. Disabled if 0.
  Value range is 1-31, and mapping to the actual delay varies depending on HW.

I'm surprised this made it passed review. We try to avoid having DT
poke magic values into registers. That is what this appears to be,
since it is unclear what the value actually means.

It is also good to state what happens if the property is not
present. It often means it defaults to zero. But this implementation
just leaves the value alone. So to be on the safe side, the DT blob
should probably have these properties, so the behaviour is well
defined.

	 Andrew