linux-kernel - Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

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: <5EF6B0D2-2B84-4C98-B799-88062E035EC1@goldelico.com>
Date:   Fri, 16 Jun 2023 22:21:06 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     list@...ndingux.net, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        Conor Dooley <conor+dt@...nel.org>, linux-mips@...r.kernel.org,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH 0/9] MIPS: CI20: Add WiFi / Bluetooth support

Hi Paul,

> 
>>>> Since this patch will actually make the various ACT8600 regulators work at their specified voltage, maybe the voltage on one of the updated regulators is wrong?
>>>> 
>>>> Maybe change the regulators one by one back to their old name, until you find the one that is problematic? That may give us more info.
>>> 
>>> That is what I also have thought about but have not yet done.
>>> Will try as soon as I find a time slot.
>> 
>> I have reverted the whole patch (had only a conflict in wifi_io / LDO6) and now I can boot.
>> 
>> But do not see a WiFi or Bluetooth interface.
>> 
>> So it looks as if the CI20 variants are indeed different. Which would also explain why we
>> originally came up with two different solutions to add WiFi.
>> 
>> Next I will try to bisect the individual changes...
> 
> It is this and not the regulator names:
> 
> diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
> index e2221d44e4269..391be48e6427a 100644
> --- a/arch/mips/boot/dts/ingenic/ci20.dts
> +++ b/arch/mips/boot/dts/ingenic/ci20.dts
> @@ -295,7 +295,6 @@ &i2c0 {
>        act8600: act8600@5a {
>                compatible = "active-semi,act8600";
>                reg = <0x5a>;
> -               status = "okay";
> 
>                regulators {
>                        vddcore: SUDCDC1 {
> 
> 
> Now I wonder how it works without status = "okay" for you but not for me.

I have not found a reason for this and it was difficult to repeat. Potentially a bisect
with failed boot and wrong setup of some voltages may have damaged the file system on my
SD card (see below). At least I had to fsck -f after running the bisect, but I did not
do it for every bisect step. Sometimes bisecting is difficult if hardware effects are
involved...

So I started with the series + a revert of the offending patch, added some more logging
to the kernel and printk() in the driver.

Results:

- driver is always loaded, so the status = "okay" was spurious and not the problem.

- Adding/Removing the regulator names also does not make a difference.

- But renaming the DT nodes (e.g. SUDCDC1 -> DCDC1) (with or without regulator_name) makes
boot hang with strange errors which indicate that the processor power supply is not stable.
Once a while it did even automatically reboot. In most cases there are some EXT4 errors
afterwards.

Example:

[    3.003096] EXT4-fs error (device mmcblk0p1): ext4_mb_generate_buddy:1100: group 81, block bitmap and bg descriptor inconsistent: 30994 vs 31229 free clusters
/sbin/init: error while loading shared libraries: /lib/mipsel-li
[    3.291901] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00007f00
[    3.305122] Rebooting in 10 seconds..

I have not found a reason but it appears that if the DT nodes do match the
struct regulator_desc list, it is different from having them not match.

At least the result of regulator_of_get_init_data() is NULL if there is no
match and then some other code path is followed in regulator_register().

So at the moment I think that matching DT node names with the act8600_regulators[] list
changes something in the chip initialization which has influence depending on hardware
variation. Maybe your board is simply more robust than mine to that.

Deeper analysis will reveal the issue and indicate a solution...

BR,
Nikolaus