netdev - Re: [PATCH] net: fec: Defer probe if other FEC has deferred MDIO

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: <60f22dab4c51ee7e1a62d91c64e55205c18b9265.camel@siemens.com>
Date:   Thu, 9 Feb 2023 10:10:58 +0000
From:   "Sverdlin, Alexander" <alexander.sverdlin@...mens.com>
To:     "wei.fang@....com" <wei.fang@....com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "xiaoning.wang@....com" <xiaoning.wang@....com>,
        "shenwei.wang@....com" <shenwei.wang@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-imx@....com" <linux-imx@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] net: fec: Defer probe if other FEC has deferred MDIO

Hello Wei,

On Thu, 2023-02-09 at 09:17 +0000, Wei Fang wrote:
> > > > -       if ((fep->quirks & FEC_QUIRK_SINGLE_MDIO) && fep-
> > > > >dev_id >
> > > > 0) {
> > > > +       if (fep->quirks & FEC_QUIRK_SINGLE_MDIO) {
> > > >                 /* fec1 uses fec0 mii_bus */
> > > >                 if (mii_cnt && fec0_mii_bus) {
> > > >                         fep->mii_bus = fec0_mii_bus;
> > > >                         mii_cnt++;
> > > >                         return 0;
> > > >                 }
> > > > -               return -ENOENT;
> > > 
> > > Could you not add an else clause here? return -EPROBE_DEFFER?
> > > 
> > > Basically, if fec0 has not probed, deffer the probing of fec1?
> > 
> > we do have a configuration with i.MX8 where we have only fec2
> > enabled (and
> > has mdio node).
> > I'm not sure if it was thought of by fec driver developers (it
> > makes a lot of
> > non-obvious assumtions), but that's how it works now.
> > 
> 
> Hi Alexander,
> 
> This issue seems that the fec2 (without mdio subnode) registers
> mii_bus first, then
> the fec1 (has mdio subnode) use the fec2's mii_bus when fec1 probes
> again, finally
> both fec1 and fec2 can not connect to PHY. Am I right?

yes, this is exactly what happens (except that we have more than one
PHY in this mdio node, which is then not registered/parsed).

> If so, I think this issue can't be reproduced on the upstream tree,
> because the quirks of
> i.MX8 on the upstream tree do not set the FEC_QUIRK_SINGLE_MDIO bit.
> So, the fec1
> will registers a mii_bus in your case rather than using the fec2's
> mii_bus. I'm a bit curious
> that have you tried to reproduce this issue base on the upstream
> tree?

You are right, there is unfortunately no i.MX8 support in the upstream
tree, so it's not possible to reproduce anything. Just wanted to
discuss the probe concept of this driver, which is rather fragile
with all there static local variables, probe call counters and relying
on the probe order. All of this falls together like a house of cards
if something gets deferred.

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com