Broken commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2

Thu Dec 22 18:36:08 CET 2022

Hi Pali,

On Thu, 22 Dec 2022 at 10:13, Pali Rohár <pali at kernel.org> wrote:
>
> On Thursday 22 December 2022 09:29:27 Tom Rini wrote:
> > On Thu, Dec 22, 2022 at 08:49:47AM +0100, Pali Rohár wrote:
> > > On Wednesday 21 December 2022 21:54:15 Tom Rini wrote:
> > > > On Fri, Dec 16, 2022 at 10:56:39PM +0100, Pali Rohár wrote:
> > > > > On Friday 05 August 2022 16:21:24 Pali Rohár wrote:
> > > > > > Broken is also commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2. Seems
> > > > > > that all kconfig migration changes done after that commit are broken.
> > > > > >
> > > > > > I really do not have energy to investigate what and how was broken due
> > > > > > to incorrect kconfig migration.
> > > > > >
> > > > > >
> > > > > > I did simple test. Applied following change:
> > > > > >
> > > > > > diff --git a/include/configs/p1_p2_rdb_pc.h b/include/configs/p1_p2_rdb_pc.h
> > > > > > index a6523753d5ca..489f24df0ab1 100644
> > > > > > --- a/include/configs/p1_p2_rdb_pc.h
> > > > > > +++ b/include/configs/p1_p2_rdb_pc.h
> > > > > > @@ -624,3 +624,7 @@ __stringify(__PCIE_RST_CMD)"\0"
> > > > > >  "bootm $norbootaddr - $norfdtaddr"
> > > > > >
> > > > > >  #endif /* __CONFIG_H */
> > > > > > +
> > > > > > +#ifdef CONFIG_SDCARD
> > > > > > +#error
> > > > > > +#endif
> > > > > >
> > > > > > And then called:
> > > > > >
> > > > > > make CROSS_COMPILE=powerpc-linux-gnuspe- P2020RDB-PC_defconfig u-boot.bin
> > > > > >
> > > > > > And it failed, even when this defconfig file is not SD card builds.
> > > > >
> > > > > Tom, that commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2 is yours.
> > > > > Could you please look at it? Because it is a regressions which made P1
> > > > > and P2 broken. Based on the past experience it really does not make
> > > > > sense to wait for somebody who promised to do something as same
> > > > > situation is just repeating.
> > > > >
> > > > > Above diff is a simple check to verify if code conversion is correct or
> > > > > not. If _before_ conversion CONFIG_SDCARD was not defined then also
> > > > > _after_ conversion this macro must not be defined. Right?
> > > >
> > > > I dug through all of this again. I thought I understood what the right
> > > > answer was again for a moment, but I don't. You, however, understand
> > > > what platforms don't use PBL and what they use instead. I understand
> > > > half of the fix, which is to change:
> > > > choice
> > > >         prompt "Freescale PBL load location"
> > > >         depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB \
> > > >                 || TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB) \
> > > >                 && !CMD_NAND)
> > > >
> > > > To, I think:
> > > > choice
> > > >         prompt "Freescale PBL load location"
> > > >         depends on RAMBOOT_PBL
> > > >
> > > > Then introduce some new, not "SDCARD" symbol, for the P1/P2 platforms
> > > > that don't use PBL but instead the FSL_PREPBL_ESDHC_BOOT_SECTOR logic
> > > > you introduced before.
> > >
> > > P2020RDB-PC_defconfig is for booting from FLASH NOR, not from SD card.
> > > CONFIG_SDCARD for P1/P2 must be defined when booting from SD card.
> > >
> > > Before commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2 everything worked
> > > fine and CONFIG_SDCARD was not defined for P2020RDB-PC_defconfig. After
> > > commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2, symbol CONFIG_SDCARD is
> > > defined.
> > >
> > > You can check it by adding into config.h:
> > >
> > > +#ifdef CONFIG_SDCARD
> > > +#error
> > > +#endif
> > >
> > > > I say I almost thought I had it because I thought this would work:
> > > > diff --git a/arch/powerpc/cpu/mpc85xx/Kconfig b/arch/powerpc/cpu/mpc85xx/Kconfig
> > > > index 24d3f1f20c25..59740b173b11 100644
> > > > --- a/arch/powerpc/cpu/mpc85xx/Kconfig
> > > > +++ b/arch/powerpc/cpu/mpc85xx/Kconfig
> > > > @@ -15,7 +15,7 @@ config CMD_ERRATA
> > > >  config FSL_PREPBL_ESDHC_BOOT_SECTOR
> > > >   bool "Generate QorIQ pre-PBL eSDHC boot sector"
> > > >   depends on MPC85xx
> > > > - depends on SDCARD
> > > > + depends on TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB
> > >
> > > No, original code was OK. As is written in description
> > > FSL_PREPBL_ESDHC_BOOT_SECTOR is for writing bootsector to SD card. And
> > > it is optional as there is other way how to generate it, as described in
> > > some doc/ file.
> > >
> > > But if you choose to compile u-boot for P2020RDB-PC_defconfig (NOR
> > > FLASH) then there is no SD card booting and hence
> > > FSL_PREPBL_ESDHC_BOOT_SECTOR for P2020RDB-PC_defconfig must never be
> > > generated. So FSL_PREPBL_ESDHC_BOOT_SECTOR must depends on SDCARD.
> > >
> > > >   help
> > > >     With this option final image would have prepended QorIQ pre-PBL eSDHC
> > > >     boot sector suitable for SD card images. This boot sector instruct
> > > > diff --git a/boot/Kconfig b/boot/Kconfig
> > > > index 4a001bcee851..d1c9c5f25067 100644
> > > > --- a/boot/Kconfig
> > > > +++ b/boot/Kconfig
> > > > @@ -725,8 +725,7 @@ config RAMBOOT_PBL
> > > >
> > > >  choice
> > > >   prompt "Freescale PBL load location"
> > > > - depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB \
> > > > -         || TARGET_P1020RDB_PC || TARGET_P1020RDB_PD || TARGET_P2020RDB) \
> > > > + depends on RAMBOOT_PBL || ((TARGET_P1010RDB_PA || TARGET_P1010RDB_PB) \
> > > >           && !CMD_NAND)
> > > >
> > > >  config SDCARD
> > > >
> > > > But no one enables CONFIG_FSL_PREPBL_ESDHC_BOOT_SECTOR.
> > >
> > > It is optional _user_ symbol. During compilation of sd card version of
> > > u-boot, user can enable it.
> > >
> > > For turris 1.x board there is waiting patch on the list which uses it.
> > > No review yet?
> > >
> > > > But maybe that
> > > > just needs to be enabled on the platforms in question, and then the
> > > > first dependency change above is just dropping the SDCARD line? I really
> > > > don't know, and I'd be equally happy to just remove all of the P1*/P2*
> > > > boards if they don't boot and no one cares to fix them.
> > > >
> > > > --
> > > > Tom
> > >
> > > Just fix the conversion process. The rule is simple: if config.h did
> > > not have enabled CONFIG_SDCARD then after conversion config.h must not
> > > have it enabled.
> > >
> > > Compare git checkout d433c74eecdce1e4952ef4e8c712a9289c0dfcc2~1
> > > and git checkout d433c74eecdce1e4952ef4e8c712a9289c0dfcc2
> > >
> > > Because it worked fine before commit
> > > d433c74eecdce1e4952ef4e8c712a9289c0dfcc2.
> >
> > Thanks for explaining. Since you clearly understand what it should do,
> > and I do not, please submit something to implement the correct questions
> > in Kconfig. There was some overloading of a symbol before which I didn't
> > understand, and still don't exactly. Otherwise, since I believe you're
> > saying the Turris platform is fine, once Marek merges it (you will
> > likely need to rebase on next, next week, once I finish merging all of
> > the CONFIG migration stuff), I'll just delete the P1/P2 platforms
> > sometime in the next release if no one actually cares to fix them.
> >
> > --
> > Tom
>
> In lot of projects it is common that developer who introduced broken
> commit, is responsible to either fix it or revert it. I really do not
> have a time and power to fix every one broken commit behind every one
> developer. I sent lot of other fixes and in this case I at least wrote
> what is broken. Moreover it my was not my decision to use Kconfig for
> this stuff, which is unsuitable tool here. So do not take me wrong but I
> do not have motivation to fight with another tool for things for which
> it was not designed and try to hunt bugs in it.
>
> It should have been pretty simple migration from tool A to tool B as it
> does not introduce any new feature nor code change. It should produce
> same u-boot.bin binary before and after change. And also it is not
> needed to fully understand what which option means. And if binaries are
> not same then conversion was wrong. No need for HW, just compiling and
> unit testing.
>
> I have rebased turris 1.x patches at least 3 times. This is not enough??
> Why should I rebase it again? Note that it depends on P1/P2 because
> turris 1.x is based on P2.
>
> Also I have sent tons of patches and fixes for P1/P2 platforms and the
> only thing which you are going to do is to delete this platform?
>
> You should have wrote this statement year ago and I would never take
> care of P1/P2 and fixing it. This is absolutely rude decision to show
> active developers: go away, we do not care about you, your time and your
> contributions.

I don't think it needs to be deleted, from my reading of what Tom said.

One thing to bear in mind is that the Kconfig migration has been going
on for 6 years and Tom has actually managed to complete it. It will be
a big win for the project. One thing that has made it so hard is that
some board maintainers have moved on and don't take notice of
migrations.

I know from bitter experience that converting to Kconfig and keeping
everything the same is extremely tricky. Buildman has a -K option to
show CONFIG differences largely for this reason. But in this case, we
need to move things to CFG_ also...so...it is not easy.

Regards,
Simon