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: <20170524142543.GD26577@lunn.ch>
Date:   Wed, 24 May 2017 16:25:43 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Bogdan Purcareata <bogdan.purcareata@....com>
Cc:     "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver

> Yes, I can do that. However, I don't see a "phy" folder under Documentation/devicetree/bindings/net/.
> Should I change the location to Documentation/devicetree/bindings/net/cortina.txt instead?

Ah, sorry, my error. Yes, Documentation/devicetree/bindings/net/cortina.txt.

> > I think there needs to be some explanation here. What exactly are you
> > using to indicate link up? What does CORTINA_GPIO_GPIO_INTS mean?
> 
> I can only assume it's the location of an interrupt status register. The Cortina PHYs go mostly undocumented, I've found an implementation in an old thread [1], that also did the job of programming the Cortina PHY microcode. I understand that microcode programming is now part of the bootloader.
> 
> The register seems to provide the link status properly, and based on that the phydev is initialized with the only (currently) supported configuration, 10Gbps full duplex.
> 
> I can change the register name to something more meaningful, though - e.g. CORTINA_LINK_STATUS.

No, leave the name as is. The MIPS driver you gave a reference to
seems to be using sensible names. My guess is, the boot loader is
configuring the PHY to generate an interrupt on link up via one of its
GPIO lines. And this code is reading that GPIO status. This is all
very fragile, you are making a lot of assumptions.

Have you tested pulling the cable out? And plugging it back in again?
There is a chance this interrupt is "link change", not "link up".

What i do like in that mips code is the probe function verifies the
product ID.

+       /*
+        * CS4312 keeps its ID values in non-standard registers, make
+        * sure we are talking to what we think we are.
+        */
+       id_lsb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_LSB);
+       if (id_lsb < 0) {
+               ret = id_lsb;
+               goto err;
+       }
+
+       id_msb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_MSB);
+       if (id_msb < 0) {
+               ret = id_msb;
+               goto err;
+       }
+
+       if (id_lsb != 0x23E5 || id_msb != 0x1002) {
+               ret = -ENODEV;
+               goto err;
+       }

You should do that as well.

      Andrew

Powered by blists - more mailing lists