| Message ID | 20180301180220.11333-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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>
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; +} +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
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(+)