[v2,1/4] firmware: dmi_scan: Introduce the dmi_get_bios_year() helper

The pattern to only extract the year portion of date is used in
several places and more users may come.

By using this helper they may create slightly cleaner code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/firmware/dmi_scan.c | 11 +++++++++++
 include/linux/dmi.h         |  2 ++
 2 files changed, 13 insertions(+)

On Thu, Mar 01, 2018 at 08:02:17PM +0200, Andy Shevchenko wrote:
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
>  }
>  EXPORT_SYMBOL(dmi_get_date);
>  
> +int dmi_get_bios_year(void)
> +{
> +	bool exists;
> +	int year;
> +
> +	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
> +
> +	return exists ? year : -ENODATA;
> +}
> +EXPORT_SYMBOL(dmi_get_bios_year);

It would be good if kerneldoc was added to this function.  One thing
to mention is that direct usage of the function in a conditional only
works reliably when asserting an exact or minimum BIOS date.  It doesn't
work reliably when asserting a maximum BIOS date unless the return
value is explicitly checked for -ENODATA.  (Fortunately that use case
seems to be rare, but still worth mentioning IMHO.)


> +static inline int dmi_get_bios_year(void) { return -ENXIO; }

Shouldn't this be -ENODATA as well for consistency?  Otherwise one would
have to check for -ENODATA *and* -ENXIO.

Thanks,

Lukas

On Fri, 2018-03-02 at 15:05 +0100, Lukas Wunner wrote:
> On Thu, Mar 01, 2018 at 08:02:17PM +0200, Andy Shevchenko wrote:
> > --- a/drivers/firmware/dmi_scan.c
> > +++ b/drivers/firmware/dmi_scan.c
> > @@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int
> > *monthp, int *dayp)
> >  }
> >  EXPORT_SYMBOL(dmi_get_date);
> >  
> > +int dmi_get_bios_year(void)
> > +{
> > +	bool exists;
> > +	int year;
> > +
> > +	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
> > +
> > +	return exists ? year : -ENODATA;
> > +}
> > +EXPORT_SYMBOL(dmi_get_bios_year);
> 
> It would be good if kerneldoc was added to this function.

Perhaps.

>   One thing
> to mention is that direct usage of the function in a conditional only
> works reliably when asserting an exact or minimum BIOS date.  It
> doesn't
> work reliably when asserting a maximum BIOS date unless the return
> value is explicitly checked for -ENODATA.

Just < 0.

>   (Fortunately that use case
> seems to be rare, but still worth mentioning IMHO.)
> 
> 
> > +static inline int dmi_get_bios_year(void) { return -ENXIO; }
> 
> Shouldn't this be -ENODATA as well for consistency?  Otherwise one
> would
> have to check for -ENODATA *and* -ENXIO.

I was thinking about this, though checking for negative errors is a
pattern in kernel. If user would like to distinguish what really
happened, then they may to check for explicit code.

ENXIO is chosen to be consistent with other calls when !DMI.
ENODATA on the other hand is not related to access to the data, but to
the contents of it. So, I would like to keep them different.

Hi Andy,

On Thu,  1 Mar 2018 20:02:17 +0200, Andy Shevchenko wrote:
> The pattern to only extract the year portion of date is used in
> several places and more users may come.
> 
> By using this helper they may create slightly cleaner code.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/firmware/dmi_scan.c | 11 +++++++++++
>  include/linux/dmi.h         |  2 ++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 6ce299926196..616ec17cc802 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -1015,6 +1015,17 @@ bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
>  }
>  EXPORT_SYMBOL(dmi_get_date);
>  
> +int dmi_get_bios_year(void)
> +{
> +	bool exists;
> +	int year;
> +
> +	exists = dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL);
> +
> +	return exists ? year : -ENODATA;

If the DMI field exists but year can't be parsed, "exists" is true but
"year" is 0. From the caller's perspective, it is the same as if the
field did not exist (year is not available in both cases), but the
function returns 0 in one case and -ENODATA in the other. Don't you
think that:

	return year ? year : -ENODATA;

would serve the caller better?

> +}
> +EXPORT_SYMBOL(dmi_get_bios_year);
> +
>  /**
>   *	dmi_walk - Walk the DMI table and get called back for every record
>   *	@decode: Callback function
> diff --git a/include/linux/dmi.h b/include/linux/dmi.h
> index 46e151172d95..6a86d8db16d9 100644
> --- a/include/linux/dmi.h
> +++ b/include/linux/dmi.h
> @@ -106,6 +106,7 @@ extern void dmi_scan_machine(void);
>  extern void dmi_memdev_walk(void);
>  extern void dmi_set_dump_stack_arch_desc(void);
>  extern bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp);
> +extern int dmi_get_bios_year(void);
>  extern int dmi_name_in_vendors(const char *str);
>  extern int dmi_name_in_serial(const char *str);
>  extern int dmi_available;
> @@ -133,6 +134,7 @@ static inline bool dmi_get_date(int field, int *yearp, int *monthp, int *dayp)
>  		*dayp = 0;
>  	return false;
>  }
> +static inline int dmi_get_bios_year(void) { return -ENXIO; }
>  static inline int dmi_name_in_vendors(const char *s) { return 0; }
>  static inline int dmi_name_in_serial(const char *s) { return 0; }
>  #define dmi_available 0

Looks good otherwise.

Reviewed-by: Jean Delvare <jdelvare@suse.de>