Please revert 98b86296e6 as it breaks several ipq806x devices

[Hannu Nyman]

Christian Lamparter kirjoitti 28.12.2020 klo 16.59:
> On 28/12/2020 15:27, Adrian Schmutzler wrote:
>>> -----Original Message-----
>>> From: openwrt-devel [mailto:openwrt-devel-bounces at lists.openwrt.org]
>>> On Behalf Of Hannu Nyman
>>> Sent: Montag, 28. Dezember 2020 10:12
>>> To: OpenWrt Development List <openwrt-devel at lists.openwrt.org>
>>> Cc: Petr Štetiar <ynezz at true.cz>; Christian Lamparter
>>> <chunkeey at gmail.com>
>>> Subject: Please revert 98b86296e6 as it breaks several ipq806x devices
>>
>> https://github.com/openwrt/openwrt/commit/57e4cc8261ca6f0b32e4da6922a8f52ef82c4dc6 
>>
>>
>> This is meant as an emergency measure (thus no "Fixes"), and should be 
>> properly addressed at some point.
>>
>
> hmm, the R7800 must have some crazy bootargs-overrides when these
> impact sysupgrade as well. Let's hope there isn't an issue with
> the platform.sh + asrock.sh changes.
>
> @hannu: can you please post your R7800's /proc/device-tree/chosen/ files 
> (name + content)
> for reference?
>
> Cheers
> Christian


I did some further debugging, and noticed something.

In short: Possibly the definition of CONFIG_CMDLINE_OVERRIDE has been placed 
into a slightly wrong place in arch/arm/Kconfig.


I compared the kernel build log with the working code (without 
CONFIG_CMDLINE_OVERRIDE=y) to log with the faulty code, and there is only a 
small difference:

In the faulty version, there is a one-line warning:
   warning: override: CMDLINE_OVERRIDE changes choice state

context:
<code>
   HOSTCC  scripts/kconfig/symbol.o
   HOSTLD  scripts/kconfig/conf
scripts/kconfig/conf  --syncconfig Kconfig
net/sched/Kconfig:45: warning: menuconfig statement without prompt
.config:987:warning: override: CMDLINE_OVERRIDE changes choice state
   HOSTCC  scripts/dtc/dtc.o
</code>

So, the new option changes something else.

I looked at 
build_dir/target-arm_cortex-a15+neon-vfpv4_musl_eabi/linux-ipq806x_generic/linux-5.4.85/arch/arm/Kconfig 
and noticed that the new definition of CONFIG_CMDLINE_OVERRIDE has been 
placed into end of a multi-option choice block. Enabling this option may now 
change the selected value of the choice of ARM_ATAG_DTB_COMPAT_CMDLINE... 
options.


<code>
choice
         prompt "Kernel command line type" if ARM_ATAG_DTB_COMPAT
         default ARM_ATAG_DTB_COMPAT_CMDLINE_FROM_BOOTLOADER

config ARM_ATAG_DTB_COMPAT_CMDLINE_FROM_BOOTLOADER
         bool "Use bootloader kernel arguments if available"
         help
           Uses the command-line options passed by the boot loader instead of
           the device tree bootargs property. If the boot loader doesn't provide
           any, the device tree bootargs property will be used.

config ARM_ATAG_DTB_COMPAT_CMDLINE_EXTEND
         bool "Extend with bootloader kernel arguments"
         help
           The command-line arguments provided by the boot loader will be
           appended to the the device tree bootargs property.

config ARM_ATAG_DTB_COMPAT_CMDLINE_MANGLE
         bool "Append rootblock parsing bootloader's kernel arguments"
         help
           The command-line arguments provided by the boot loader will be
           appended to a new device tree property: bootloader-args.
           If there is a property "append-rootblock" in DT under /chosen
           and a root= option in bootloaders command line it will be parsed
           and added to DT bootargs with the form: <append-rootblock>XX.
           Only command line ATAG will be processed, the rest of the ATAGs
           sent by bootloader will be ignored.

config CMDLINE_OVERRIDE
     bool "Use alternative cmdline from device tree"
     help
       Some bootloaders may have uneditable bootargs. While CMDLINE_FORCE can
       be used, this is not a good option for kernels that are shared across
       devices. This setting enables using "chosen/cmdline-override" as the
       cmdline if it exists in the device tree.

endchoice

config CMDLINE
         string "Default kernel command string"
         default ""
         help
           On some architectures (EBSA110 and CATS), there is currently no way
</code>

Apparently the new selected option inside the ARM_ATAG_DTB_COMPAT_CMDLINE... 
choice block changes the selection of the existing options, causing bugs to 
devices depending on those selections. Asrock router apparently is not 
affected by that change, so it works ok.

Possibly the new definition should be after the "endchoice" line, so that 
ARM_ATAG_DTB_COMPAT_CMDLINE things do not change.

Or possibly it should be inside the next choice block about CMDLINE:

<code>
config CMDLINE
         string "Default kernel command string"
         default ""
         help
           On some architectures (EBSA110 and CATS), there is currently no way
           for the boot loader to pass arguments to the kernel. For these
           architectures, you should supply some command-line options at build
           time by entering them here. As a minimum, you should specify the
           memory size and the root device (e.g., mem=64M root=/dev/nfs).

choice
         prompt "Kernel command line type" if CMDLINE != ""
         default CMDLINE_FROM_BOOTLOADER
         depends on ATAGS

config CMDLINE_FROM_BOOTLOADER
         bool "Use bootloader kernel arguments if available"
         help
           Uses the command-line options passed by the boot loader. If
           the boot loader doesn't provide any, the default kernel command
           string provided in CMDLINE will be used.

config CMDLINE_EXTEND
         bool "Extend bootloader kernel arguments"
         help
           The command-line arguments provided by the boot loader will be
           appended to the default kernel command string.

config CMDLINE_FORCE
         bool "Always use the default kernel command string"
         help
           Always use the default kernel command string, even if the boot
           loader passes other arguments to the kernel.
           This is useful if you cannot or don't want to change the
           command-line options your boot loader passes to the kernel.
endchoice
</code>

Kernel & bootloader option specialists may take a closed look, but I think 
that this might well be the reason.