linux-kernel - RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 May 2017 14:34:16 +0000
From:   Bogdan Purcareata <bogdan.purcareata@....com>
To:     Andrew Lunn <andrew@...n.ch>
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

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@...n.ch]
> Sent: Wednesday, May 24, 2017 5:26 PM
> To: Bogdan Purcareata <bogdan.purcareata@....com>
> Cc: f.fainelli@...il.com; netdev@...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".

No, I have not. I had it planned but it somehow slipped. Will test.

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

Okay, I will include this check in a probe function in v3.

Thanks a lot!
Bogdan