[PATCH v5 05/11] spl: Convert mmc to spl_load

[Xavier Drudis Ferran]

El Tue, Sep 05, 2023 at 11:02:44AM -0400, Sean Anderson deia:
> On 9/4/23 08:59, Xavier Drudis Ferran wrote:
> > El Sun, Sep 03, 2023 at 08:17:26AM +0000, Jonas Karlman deia:
> > 
> >> > Fundamentally, we can't really deal with unaligned images without a
> >> > bounce-buffer. The method used by SPL_LOAD_FIT_IMAGE_BUFFER_SIZE will
> >> > continue working, since we call into the FIT routines to load the image.
> > 
> > Yes
> > 
> >> > I would like to defer bounce buffering for other images until someone
> >> > actually needs it.
> >> >
> > 
> > Fine.
> > 
> >> > Note that in the FIT case, you can also do `mkimage -EB`, at least if
> >> > you aren't using FIT_LOAD_FULL.
> >> 
> >> With the following two commits introduced in v2023.04-rc1, the alignment
> >> issue for Rockchip has been fixed and all external data is now accessed
> >> block aligned.
> >> 
> >> 9b2fd2d22852 ("binman: Add support for align argument to mkimage tool")
> >> 5ad03fc77dfa ("rockchip: Align FIT image data to SD/MMC block length")
> >> 
> >> Regards,
> >> Jonas
> >>
> > 
> > Well, yes, thanks.
> > 
> > I was carrying Jerome's patch still thinking it was needed for me, but
> > I just tried without and it works too, in mmc. In spi I didn't try but
> > it should be even easier (bl_len=1).
> > 
> > For me it's still odd to write outside intended memory. Would a warning
> > in case legacy image loading writes before load_addr be acceptable ?
> > Just in case someone was using the memory there for something else.
> 
> We could add something, but it would have to be behind SHOW_ERRORS. This
> series is already having a very tough time reducing bloat without adding
> any (other) new features.
>

When I looked at it I thought the check and warning was only needed at
the end of spl_load, for legacy images. For fit it was already
aligned by calculating offset as a integer multiple of sectors.
I might misremember now.

In cases where the buffer is reserved it could just include the extra
bytes at the start before the load address, as I tried to suggest (v5
02/11), and then no warning is needed. Which is silly because the
spl_get_load_buffer doesn't do much at all. But someday it might do
more...

Your changes moves the extra bytes from the end to the start, and I
find it more unexpected than just having extra bytes at the end for an
image that might not have such a fixed length. I'd expect people to
leave room at the end when planning memory, thinking a future image
might be bigger, but not thinking to leave room at the start. But I'm
not all that sure...

If not a check and warning at least note something in documentation or
comments somewhere...

And I think you are really adding new features, in that even if the
formats and methods stay the same, there'll be more combinations
available after your change. So I'd be tolerant to a small code size
increase in exchange for the new flexibility. But I can understand
some cases can't afford the luxury, maybe.

Whatever you decide, looking forward to test a little your v6 if I can.