netdev - Re: mt7530: dsa_switch_parse_of() fails, causes probe code to run twice

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: <5e10f823-88f1-053a-d691-6bc900bd85a6@arinc9.com>
Date:   Sat, 15 Apr 2023 03:28:55 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Daniel Golle <daniel@...rotopia.org>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        Frank Wunderlich <frank-w@...lic-files.de>,
        netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com
Subject: Re: mt7530: dsa_switch_parse_of() fails, causes probe code to run
 twice

On 15.04.2023 02:53, Daniel Golle wrote:
> On Sat, Apr 15, 2023 at 02:23:16AM +0300, Arınç ÜNAL wrote:
>> On 15.04.2023 01:48, Daniel Golle wrote:
>>> On Sat, Apr 15, 2023 at 01:41:07AM +0300, Arınç ÜNAL wrote:
>>>> Hey there,
>>>>
>>>> I've been working on the MT7530 DSA subdriver. While doing some tests, I
>>>> realised mt7530_probe() runs twice. I moved enabling the regulators from
>>>> mt7530_setup() to mt7530_probe(). Enabling the regulators there ends up
>>>> with exception warnings on the first time. It works fine when
>>>> mt7530_probe() is run again.
>>>>
>>>> This should not be an expected behaviour, right? Any ideas how we can make
>>>> it work the first time?
>>>
>>> Can you share the patch or work-in-progress tree which will allow me
>>> to reproduce this problem?
>>
>> I tested this on vanilla 6.3-rc6. There's just the diff below that is
>> applied. I encountered it on the standalone MT7530 on my Bananapi BPI-R2. I
>> haven't tried it on MCM MT7530 on MT7621 SoC yet.
>>
>>>
>>> It can of course be that regulator driver has not yet been loaded on
>>> the first run and -EPROBE_DEFER is returned in that case. Knowing the
>>> value of 'err' variable below would hence be valuable information.
>>
>> Regardless of enabling the regulator on either mt7530_probe() or
>> mt7530_setup(), dsa_switch_parse_of() always fails.
> 
> So dsa_switch_parse_of() can return -EPROBE_DEFER if the ethernet
> driver responsible for the CPU port has not yet been loaded.
> 
> See net/dsa/dsa.c (inside function dsa_port_parse_of):
> [...]
> 1232)                master = of_find_net_device_by_node(ethernet);
> 1233)                of_node_put(ethernet);
> 1234)                if (!master)
> 1235)                        return -EPROBE_DEFER;
> [...]
> 
> Hence it would be important to include the value of 'err' in your
> debugging printf output, as -EPROBE_DEFER can be an expected and
> implicitely intended reality and nothing is wrong then.

Thanks Daniel. I can't do more tests soon but this is probably what's 
going on as the logs already indicate that the MediaTek ethernet driver 
was yet to load.

As acknowledged, since running the MT7530 DSA subdriver from scratch is 
expected if the ethernet driver is not loaded yet, there's not really a 
problem. Though the switch is reset twice in a short amount of time. I 
don't think that's very great.

The driver initialisation seems serialised (at least for the drivers 
built into the kernel) as I tried sleeping for 5 seconds on 
mt7530_probe() but no other driver was loaded in the meantime so I got 
the same behaviour.

The regulator code will cause a long and nasty exception the first time. 
Though there's nothing wrong as it does what it's supposed to do on the 
second run. I'm not sure if that's negligible.

Could we at least somehow make the MT7530 DSA subdriver wait until the 
regulator driver is loaded?

Arınç