| Message ID | 20190925145750.200592-82-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | x86: Add initial support for apollolake | expand |
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. 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. > /* Crossed the boundary */ > cache = NULL; > debug("%s: no available entries found\n", __func__); > -- Regards, Bin
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. 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
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
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) { /* Crossed the boundary */ cache = NULL; debug("%s: no available entries found\n", __func__);
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(-)