From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bin Meng <bmeng.cn@gmail.com>
Date: Sat, 12 Oct 2019 12:44:48 +0800
Subject: [U-Boot] [PATCH 081/126] x86: Correct mrccache
	find_next_mrc_cache() calculation
In-Reply-To: <CAPnjgZ0tN5pgpOVUX6KYJW1NyL=BFhP34n7fUBA-aKs+M_Xmwg@mail.gmail.com>
References: <20190925145750.200592-1-sjg@chromium.org>
 <20190925145750.200592-82-sjg@chromium.org>
 <CAEUhbmVAh--q+KEfTUTJdpGxyDJLRWXQJBvazW5CDGcrgo-Nuw@mail.gmail.com>
 <CAPnjgZ0tN5pgpOVUX6KYJW1NyL=BFhP34n7fUBA-aKs+M_Xmwg@mail.gmail.com>
Message-ID: <CAEUhbmUqCrpfJOw4GJJKrdw=KxQBdksmpZx+RRikz=JFkyPYMg@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On Sat, Oct 12, 2019 at 11:38 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Bin,
>
> On Thu, 10 Oct 2019 at 00:23, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > Hi Simon,
> >
> > On Wed, Sep 25, 2019 at 10:59 PM Simon Glass <sjg@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@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.

Yep, that would be better.

>
> Perhaps rename the function to find_next_mrc_cache_pos()?

Sounds good.

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

Regards,
Bin