Re: [PATCH v1 1/7] RISC-V: introduce ARCH_FOO kconfig aliases for SOC_FOO symbols

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3101 bytes --]

Hi Geert...

On Tue, Jan 10, 2023 at 10:14:51PM +0100, Geert Uytterhoeven wrote:
> On Mon, Nov 21, 2022 at 11:18 PM Conor Dooley <conor@kernel.org> wrote:
> > From: Conor Dooley <conor.dooley@microchip.com>
> >
> > To facilitate a transfer from SOC_FOO to ARCH_FOO, over a release cycle,
> > introduce some aliases so that drivers etc that use the SOC_FOO symbols
> > can be converted.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks for your patch, which is now commit fc43211939bb6874
> ("RISC-V: kconfig.socs: convert usage of SOC_CANAAN to
> ARCH_CANAAN") in riscv/for-next

I see this and I immediately know it is going to be bad news!

> > --- a/arch/riscv/Kconfig.socs
> > +++ b/arch/riscv/Kconfig.socs
> 
> > @@ -73,6 +91,9 @@ config SOC_CANAAN_K210_DTB_BUILTIN
> >           This option should be selected if no bootloader is being used.
> >           If unsure, say Y.
> >
> > +config ARCH_CANAAN_K210_DTB_SOURCE
> > +       def_bool SOC_CANAAN_K210_DTB_SOURCE
> 
> This is not correct, as SOC_CANAAN_K210_DTB_SOURCE below is
> not a bool, but a string.
> 
> > +
> >  config SOC_CANAAN_K210_DTB_SOURCE
> >         string "Source file for the Canaan Kendryte K210 builtin DTB"
> >         depends on SOC_CANAAN
> 
> Hence
> 
>     obj-$(CONFIG_ARCH_CANAAN_K210_DTB_BUILTIN) += $(addsuffix .dtb.o,
> $(CONFIG_ARCH_CANAAN_K210_DTB_SOURCE))
> 
> will do the wrong thing later, and I get a non-bootable system (no output)
> on my MAiX-BiT.
> 
> Unfortunately there is no def_string, so I don't think we can fix this
> in a backwards-compatible way, and have to replace all
> SOC_CANAAN_K210_DTB_SOURCE by ARCH_CANAAN_K210_DTB_SOURCE,
> and urging users to update their .config manually.

That sucks. I'm not sure how I missed this - I had tested originally on
my k210, but evidently there was something wrong with how I had done it.
I must have not tested a subsequent revision on the k210. Mea cupla.

As it wasn't my intention to inflict this change without time for the
symbols to appear in configs, my immediate feeling is that this part of
the change should be reverted.

Of course, we could so something like:
diff --git a/arch/riscv/Kconfig.socs b/arch/riscv/Kconfig.socs
index 34a54e5310a1..d36a5f39f13a 100644
--- a/arch/riscv/Kconfig.socs
+++ b/arch/riscv/Kconfig.socs
@@ -79,7 +79,8 @@ config SOC_CANAAN_K210_DTB_BUILTIN
          If unsure, say Y.
 
 config ARCH_CANAAN_K210_DTB_SOURCE
-       def_bool SOC_CANAAN_K210_DTB_SOURCE
+       string
+       default SOC_CANAAN_K210_DTB_SOURCE
 
 config SOC_CANAAN_K210_DTB_SOURCE
        string "Source file for the Canaan Kendryte K210 builtin DTB"

But I am not sure of how that interacts with the various methods of
updating ones config.
From, admittedly limited, testing it does get updated if
SOC_CANAAN_K210_DTB_SOURCE is changed using menuconfig. Similarly, if
one alters that symbol and does olddefconfig it also gets updated.

I'm sure I am overlooking something here though, what is it?

Thanks & apologies,
Conor.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv