netdev - Re: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

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] [day] [month] [year] [list]

Date:   Mon, 22 Jun 2020 19:45:27 +0530
From:   Calvin Johnson <calvin.johnson@....nxp.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     "Madalin Bucur (OSS)" <madalin.bucur@....nxp.com>,
        Jeremy Linton <jeremy.linton@....com>, Jon <jon@...id-run.com>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "linux.cj@...il.com" <linux.cj@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe
 c45 devices

On Mon, Jun 22, 2020 at 02:36:19PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Jun 22, 2020 at 06:46:52PM +0530, Calvin Johnson wrote:
> > On Mon, Jun 22, 2020 at 09:29:17AM +0000, Madalin Bucur (OSS) wrote:
> > > > -----Original Message-----
> > > > From: Calvin Johnson (OSS) <calvin.johnson@....nxp.com>
> > > > Sent: Monday, June 22, 2020 11:19 AM
> > > > To: Jeremy Linton <jeremy.linton@....com>; Russell King - ARM Linux admin
> > > > <linux@...linux.org.uk>; Jon <jon@...id-run.com>; Cristi Sovaiala
> > > > <cristian.sovaiala@....com>; Ioana Ciornei <ioana.ciornei@....com>; Andrew
> > > > Lunn <andrew@...n.ch>; Andy Shevchenko <andy.shevchenko@...il.com>;
> > > > Florian Fainelli <f.fainelli@...il.com>; Madalin Bucur (OSS)
> > > > <madalin.bucur@....nxp.com>
> > > > Cc: linux.cj@...il.com; netdev@...r.kernel.org; Calvin Johnson (OSS)
> > > > <calvin.johnson@....nxp.com>
> > > > Subject: [net-next PATCH v2 1/3] net: phy: Allow mdio buses to auto-probe
> > > > c45 devices
> > > > 
> > > > From: Jeremy Linton <jeremy.linton@....com>
> > > > 
> > > > The mdiobus_scan logic is currently hardcoded to only
> > > > work with c22 devices. This works fairly well in most
> > > > cases, but its possible that a c45 device doesn't respond
> > > > despite being a standard phy. If the parent hardware
> > > > is capable, it makes sense to scan for c22 devices before
> > > > falling back to c45.
> > > > 
> > > > As we want this to reflect the capabilities of the STA,
> > > > lets add a field to the mii_bus structure to represent
> > > > the capability. That way devices can opt into the extended
> > > > scanning. Existing users should continue to default to c22
> > > > only scanning as long as they are zero'ing the structure
> > > > before use.
> > > 
> > > How is this default working for existing users, the code below does not seem
> > > to do anything for a zeroed struct, as there is no default in the switch?
> > 
> > Looking further into this, I think MDIOBUS_C22 = 0, was correct. Prior to
> > this patch, get_phy_device() was executed for C22 in this path. I'll discuss
> > with Russell and Andrew on this and get back.
> 
> It is not correct for the reasons I stated when I made the comment.
> When you introduce "probe_capabilities", every MDIO bus will have
> that field as zero.
> 
> In your original patch, that means the bus only supports clause 22.
> However, we have buses today that _that_ is factually incorrect.
> Therefore, introducing probe_capabilities with zero meaning MDIOBUS_C22
> is wrong.  It means we can _never_ assume that bus->probe_capabilities
> means the bus does not support Clause 45.
> 
> Now, as per your patch below, that is better.  It means we're able to
> identify those drivers that have not declared which bus access methods
> are supported, while we can positively identify those which have.
> 
> All that's needed is for your switch() statement to maintain today's
> behaviour where no declared probe_capabilities means that the bus
> should be probed for clause 22 PHYs.
> 
> This means we can later introduce the ability to prevent clause 45
> probing for PHYs that declare themselves as explicitly only supporting
> clause 22 if we need to without having been backed into a corner, and
> left wondering whether the lack of probe_capabilities is because someone
> decided "it's zero, so doesn't need to be initialised" and didn't bother
> explicitly stating .probe_capabilities = MDIOBUS_C22.

Got it. I'll add the MDIOBUS_NO_CAP case also.

Thanks
Calvin