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  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:   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

Powered by blists - more mailing lists