netdev - Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities() to get

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: <16511bd0d6421549b2968a9419e971923c0d0146.camel@analog.com>
Date:   Fri, 9 Aug 2019 12:00:17 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
Subject: Re: [PATCH v2 02/15] net: phy: adin: hook genphy_read_abilities()
 to get_features

On Thu, 2019-08-08 at 21:32 +0200, Heiner Kallweit wrote:
> [External]
> 
> On 08.08.2019 17:24, Andrew Lunn wrote:
> > On Thu, Aug 08, 2019 at 03:30:13PM +0300, Alexandru Ardelean wrote:
> > > The ADIN PHYs can operate with Clause 45, however they are not typical for
> > > how phylib considers Clause 45 PHYs.
> > > 
> > > If the `features` field & the `get_features` hook are unspecified, and the
> > > device wants to operate via Clause 45, it would also try to read features
> > > via the `genphy_c45_pma_read_abilities()`, which will try to read PMA regs
> > > that are unsupported.
> > > 
> > > Hooking the `genphy_read_abilities()` function to the `get_features` hook
> > > will ensure that this does not happen and the PHY features are read
> > > correctly regardless of Clause 22 or Clause 45 operation.
> > 
> > I think we need to stop and think about a PHY which supports both C22
> > and C45.
> > 
> > How does bus enumeration work? Is it discovered twice?  I've always
> > considered phydev->is_c45 means everything is c45, not that some
> > registers can be accessed via c45. But the driver is mixing c22 and
> > c45. Does the driver actually require c45? Are some features which are
> > only accessibly via C45? What does C45 actually bring us for this
> > device?
> > 

Hmm, I can't answer [all] these questions.

These PHYs seem to be a bit different from the rest that I looked at in drivers/net/phy with regard to C45 & C22.
And C45 seems to be more/closer related to 10G PHYs [from what I can tell].

The ADIN PHYs can operate only in C22.
The only thing that is needed [and a bit special] is EEE, which [for C22] requires the translation layer to convert C45
reg addresses to internal C22 equivalent.

> genphy_c45_pma_read_abilities() is only called if phydev->is_c45 is set.
> And this flag means that the PHY complies with Clause 45 incl. all the
> standard devices like PMA. In the case here only some vendor-specific
> registers can be accessed via Clause 45 and therefore is_c45 shouldn't
> bet set. As a consequence this patch isn't needed.

ack, will drop patch

> 
> >      Andrew
> > 
> Heiner
>