[U-Boot] [PATCH 081/126] x86: Correct mrccache find_next_mrc_cache() calculation

[Simon Glass]

Hi Bin,

On Thu, 10 Oct 2019 at 00:23, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Sep 25, 2019 at 10:59 PM Simon Glass <sjg at chromium.org> wrote:
> >
> > This should take account of the end of the new cache record since a record
> > cannot extend beyond the end of the flash region. This problem was not
> > seen before due to the alignment of the relatively small amount of MRC
> > data.
> >
> > But with apollolake the MRC data is about 45KB, even if most of it is
> > zeroes.
> >
> > Fix this bug and update the parameter name to be less confusing.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> >  arch/x86/lib/mrccache.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/lib/mrccache.c b/arch/x86/lib/mrccache.c
> > index 33bb52039bd..e286bdf1b30 100644
> > --- a/arch/x86/lib/mrccache.c
> > +++ b/arch/x86/lib/mrccache.c
> > @@ -86,15 +86,16 @@ struct mrc_data_container *mrccache_find_current(struct mrc_region *entry)
> >   * @return next cache entry if found, NULL if we got to the end
> >   */
> >  static struct mrc_data_container *find_next_mrc_cache(struct mrc_region *entry,
> > -               struct mrc_data_container *cache)
> > +               struct mrc_data_container *prev)
> >  {
> > +       struct mrc_data_container *cache;
> >         ulong base_addr, end_addr;
> >
> >         base_addr = entry->base + entry->offset;
> >         end_addr = base_addr + entry->length;
> >
> > -       cache = next_mrc_block(cache);
> > -       if ((ulong)cache >= end_addr) {
> > +       cache = next_mrc_block(prev);
> > +       if ((ulong)cache + mrc_block_size(prev->data_size) > end_addr) {
>
> This does not look good to me. Why adding the "next" cache position to
> "prev" cache size? It should add the "next" cache size.

Yes, although here we assume they are the same. Should I add a comment?

Alternatively I could pass in the size that the caller wants for the new item?

>
> I agree there is an issue in missing check of boundary, but the check
> should not happen here, but mrccache_update(), before writing to SPI
> flash.

OK, so passing in the size would be best, I suspect.

Perhaps rename the function to find_next_mrc_cache_pos()?


>
> >                 /* Crossed the boundary */
> >                 cache = NULL;
> >                 debug("%s: no available entries found\n", __func__);
> > --
>

Regards,
SImon