Broken commit d433c74eecdce1e4952ef4e8c712a9289c0dfcc2

Pali Rohár pali at kernel.org
Thu Dec 22 18:13:06 CET 2022 

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.

More information about the U-Boot mailing list