Re: [PATCH] Revert "common/memsize.c: Fix get_effective_memsize() to check for overflow"

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

On Fri, Jan 06, 2023 at 04:14:08PM -0500, Tom Rini wrote:
> On Fri, Jan 06, 2023 at 09:22:56PM +0100, Pali Rohár wrote:
> > On Friday 06 January 2023 12:25:24 Tom Rini wrote:
> > > On Fri, Jan 06, 2023 at 05:45:43PM +0100, Pali Rohár wrote:
> > > > On Friday 06 January 2023 10:51:43 Tom Rini wrote:
> > > > > This reverts commit 777aaaa706bcfe08c284aed06886db7d482af3f8.
> > > > > 
> > > > > The changes to this generic function, which is intended to help with
> > > > > 32bit platforms with large amounts of memory has unintended side effects
> > > > > (which in turn lead to boot failures) on other platforms which were
> > > > > previously functional.
> > > > 
> > > > As mentioned previously, unfortunately this revert breaks 32-bit u-boot
> > > > on 36-bit mpc85xx boards with 32-bit e500v2 cores and 4GB DDR module.
> > > > 
> > > > Which platforms currently have broken u-boot without this revert? The
> > > > only one which was reported is stm32mp but for it there different
> > > > workaround patch waiting in the queue.
> > > 
> > > Are you able to test on one of these PowerPC platforms currently?  As
> > > the stm32 problem shows, not everything is getting tested frequently
> > > enough, so how many other cases are lurking out there.  And, I think
> > > overall issue is that the overflow check-and-change you introduce here
> > > should just be in the CONFIG_MAX_MEM_MAPPED==true case.  As that's the
> > > case you're dealing with, yes?
> > 
> > I was planning to do big retest again after all powerpc patches are
> > reviewed and merged...
> 
> Yes, but can you test one of them now, to see if my idea works?
> 
> > Anyway, if the issue here is with ram_size and its reduction was needed
> > for mpc85xx (at the time of introduction of that patch), what about
> > putting mpc85xx ifdef around ram_size reduction? For arm boards it would
> > have same behavior as revert of that commit and for mpc85xx it would be
> > no change.
> > 
> > I agree that this code needs to be revisited, together with ram_top
> > issue and also code which fills DDR banks. Because really mapped memory
> > for u-boot and real size of DDR are two different things here.
> 
> The issue here is that we see two now (given Fabio's reminder about
> another thread I had forgotten) of unintended consequences, on 32bit
> platforms trying to normally have 2GB of memory, which does not require
> special treatment.

What I'm leaning towards right now even, is that since it's hard to test
the non-36bit platforms that do set CONFIG_MAX_MEM_MAPPED, to see if
their behavior also changed here, the 36bit platforms should just be
overriding get_effective_memsize.

-- 
Tom

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