Re: [PATCH] firmware: google: update vpd_decode from upstream

[Guenter Roeck <linux@roeck-us.net>]

On Fri, Aug 02, 2019 at 03:27:54PM -0700, Stephen Boyd wrote:
> Quoting Hung-Te Lin (2019-08-02 01:20:31)
> > The VPD implementation from Chromium Vital Product Data project has been
> > updated so vpd_decode be easily shared by kernel, firmware and the user
> > space utility programs. Also improved value range checks to prevent
> > kernel crash due to bad VPD data.
> 
> Please add a Fixes: tag here to fix the commit that introduces the
> problem. It would also be nice to get a description of the problem that
> this patch is fixing. For example, explaining why the types change from
> signed to unsigned.
> 
> > 
> > Signed-off-by: Hung-Te Lin <hungte@chromium.org>
> > ---
> >  drivers/firmware/google/vpd.c        | 38 +++++++++------
> >  drivers/firmware/google/vpd_decode.c | 69 +++++++++++++++-------------
> >  drivers/firmware/google/vpd_decode.h | 17 ++++---
> >  3 files changed, 71 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> > index 0739f3b70347..ecf217a7db39 100644
> > --- a/drivers/firmware/google/vpd.c
> > +++ b/drivers/firmware/google/vpd.c
> > @@ -73,7 +73,7 @@ static ssize_t vpd_attrib_read(struct file *filp, struct kobject *kobp,
> >   * exporting them as sysfs attributes. These keys present in old firmwares are
> >   * ignored.
> >   *
> > - * Returns VPD_OK for a valid key name, VPD_FAIL otherwise.
> > + * Returns VPD_DECODE_OK for a valid key name, VPD_DECODE_FAIL otherwise.
> 
> Maybe we should convert these things to use linux conventions instead of
> VPD error codes?
> 
> >   *
> >   * @key: The key name to check
> >   * @key_len: key name length
> > @@ -86,14 +86,14 @@ static int vpd_section_check_key_name(const u8 *key, s32 key_len)
> >                 c = *key++;
> >  
> >                 if (!isalnum(c) && c != '_')
> > -                       return VPD_FAIL;
> > +                       return VPD_DECODE_FAIL;
> >         }
> >  
> > -       return VPD_OK;
> > +       return VPD_DECODE_OK;
> 
> Can you split this rename out into it's own patch. That way we can
> confirm that there are no changes due to the rename of the enum.
> 
> >  }
> >  
> > -static int vpd_section_attrib_add(const u8 *key, s32 key_len,
> > -                                 const u8 *value, s32 value_len,
> > +static int vpd_section_attrib_add(const u8 *key, u32 key_len,
> > +                                 const u8 *value, u32 value_len,
> >                                   void *arg)
> >  {
> >         int ret;
> > @@ -246,7 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
> >  
> >  static int vpd_sections_init(phys_addr_t physaddr)
> >  {
> > -       struct vpd_cbmem *temp;
> > +       struct vpd_cbmem __iomem *temp;

The change to __iomem should also be a separate patch.

Guenter