| Message ID | 20180222125923.57385-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote: > It's used in several places and more users may come. > By using this helper they may create a slightly cleaner code. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/dmi.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/linux/dmi.h b/include/linux/dmi.h > index 46e151172d95..241c27008c70 100644 > --- a/include/linux/dmi.h > +++ b/include/linux/dmi.h > @@ -147,4 +147,11 @@ static inline const struct dmi_system_id * > > #endif > > +static inline int dmi_get_bios_year(void) > +{ > + int year; > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); > + return year; > +} I don't really care personally, and I assume this series will go via a non-PCI tree, but making this inline looks similar to this, which wasn't well-received: https://lkml.kernel.org/r/CA+55aFypU331cQy-6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com > #endif /* __DMI_H__ */ > -- > 2.16.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote: > On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote: > > It's used in several places and more users may come. > > By using this helper they may create a slightly cleaner code. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > include/linux/dmi.h | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/include/linux/dmi.h b/include/linux/dmi.h > > index 46e151172d95..241c27008c70 100644 > > --- a/include/linux/dmi.h > > +++ b/include/linux/dmi.h > > @@ -147,4 +147,11 @@ static inline const struct dmi_system_id * > > > > #endif > > > > +static inline int dmi_get_bios_year(void) > > +{ > > + int year; > > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); > > + return year; > > +} > > I don't really care personally, and I assume this series will go via a > non-PCI tree, but making this inline looks similar to this, which > wasn't well-received: > > https://lkml.kernel.org/r/CA+55aFypU331cQy- > 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com "Yes, that header file is already full of random inline functions, but they are generally wrapper functions that don't really do anything, ..." I think the function above is exactly from the "wrapper that doesn't really do anything" category.
On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote: >> On Thu, Feb 22, 2018 at 02:59:20PM +0200, Andy Shevchenko wrote: >> > It's used in several places and more users may come. >> > By using this helper they may create a slightly cleaner code. >> > >> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> > --- >> > include/linux/dmi.h | 7 +++++++ >> > 1 file changed, 7 insertions(+) >> > >> > diff --git a/include/linux/dmi.h b/include/linux/dmi.h >> > index 46e151172d95..241c27008c70 100644 >> > --- a/include/linux/dmi.h >> > +++ b/include/linux/dmi.h >> > @@ -147,4 +147,11 @@ static inline const struct dmi_system_id * >> > >> > #endif >> > >> > +static inline int dmi_get_bios_year(void) >> > +{ >> > + int year; >> > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); >> > + return year; >> > +} >> >> I don't really care personally, and I assume this series will go via a >> non-PCI tree, but making this inline looks similar to this, which >> wasn't well-received: >> >> https://lkml.kernel.org/r/CA+55aFypU331cQy- >> 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com > > "Yes, that header file is already full of random inline functions, but > they are generally wrapper functions that don't really do anything, ..." > > I think the function above is exactly from the "wrapper that doesn't > really do anything" category. Yes, but honestly does it need to be inline even so? Why don't you simply put the wrapper into dmi_scan.c and export it?
Hi Andy, On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote: > It's used in several places and more users may come. > By using this helper they may create a slightly cleaner code. In general I'm not a big fan of arbitrary API shortcuts. I won't object if you think it's a good one to have, however I'm a bit concerned about the silent handling of the "error case", which could cause unexpected default decisions as seen in patch 2/4 of this series. The fact that dmi_get_bios_year() returns 0 if there is no DMI BIOS date provided should at least be documented, to limit the risk. I also wonder if dmi_get_date() itself couldn't be optimized a bit if it turns out that most callers are only interested in the year. Currently it will parse the whole string even if the caller isn't interested in the month and day. The fact that dmi_get_date() returns true even if it couldn't parse the date string at all is also strange, although unrelated with your current work. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/dmi.h | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/include/linux/dmi.h b/include/linux/dmi.h > index 46e151172d95..241c27008c70 100644 > --- a/include/linux/dmi.h > +++ b/include/linux/dmi.h > @@ -147,4 +147,11 @@ static inline const struct dmi_system_id * > > #endif > > +static inline int dmi_get_bios_year(void) > +{ > + int year; > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); > + return year; > +} checkpatch complains: WARNING: Missing a blank line after declarations #61: FILE: include/linux/dmi.h:153: + int year; + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); And I would tend to agree. Just because it is an inline function in a header file doesn't mean we don't stick to the usual coding style policy. > + > #endif /* __DMI_H__ */
On Mon, 2018-02-26 at 10:29 +0100, Rafael J. Wysocki wrote: > On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, 2018-02-23 at 15:35 -0600, Bjorn Helgaas wrote: > > > but making this inline looks similar to this, which > > > wasn't well-received: > > > > > > https://lkml.kernel.org/r/CA+55aFypU331cQy- > > > 6ZJ6szF=2KVLqcbwCUGd+gTwPViRqRWN+g@mail.gmail.com > > > > "Yes, that header file is already full of random inline functions, > > but > > they are generally wrapper functions that don't really do anything, > > ..." > > > > I think the function above is exactly from the "wrapper that doesn't > > really do anything" category. > > Yes, but honestly does it need to be inline even so? > > Why don't you simply put the wrapper into dmi_scan.c and export it? Taking into consideration that there are few comments to be addressed and series had been applied, I will send a fix up series where I move this to be a normal function.
On Tue, 2018-02-27 at 10:14 +0100, Jean Delvare wrote: > Hi Andy, > > On Thu, 22 Feb 2018 14:59:20 +0200, Andy Shevchenko wrote: > > It's used in several places and more users may come. > > By using this helper they may create a slightly cleaner code. > > In general I'm not a big fan of arbitrary API shortcuts. I won't > object > if you think it's a good one to have, however I'm a bit concerned > about > the silent handling of the "error case", which could cause unexpected > default decisions as seen in patch 2/4 of this series. Yes, we better to return -ERRNO in such case and allow callers to behave correspondingly. > The fact that > dmi_get_bios_year() returns 0 if there is no DMI BIOS date provided > should at least be documented, to limit the risk. > > I also wonder if dmi_get_date() itself couldn't be optimized a bit if > it turns out that most callers are only interested in the year. > Currently it will parse the whole string even if the caller isn't > interested in the month and day. > > The fact that dmi_get_date() returns true even if it couldn't parse > the > date string at all is also strange, although unrelated with your > current work. So, what I consider is to - move inline function to be regular one - optimize it with current dmi_get_date() - return error code when year is not parsable - consider current use cases where we do compare for less than a given year Does it sound a correct approach? > > checkpatch complains: > > WARNING: Missing a blank line after declarations > #61: FILE: include/linux/dmi.h:153: > + int year; > + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); > > And I would tend to agree. Just because it is an inline function in a > header file doesn't mean we don't stick to the usual coding style > policy. Ingo fixed that.
diff --git a/include/linux/dmi.h b/include/linux/dmi.h index 46e151172d95..241c27008c70 100644 --- a/include/linux/dmi.h +++ b/include/linux/dmi.h @@ -147,4 +147,11 @@ static inline const struct dmi_system_id * #endif +static inline int dmi_get_bios_year(void) +{ + int year; + dmi_get_date(DMI_BIOS_DATE, &year, NULL, NULL); + return year; +} + #endif /* __DMI_H__ */
It's used in several places and more users may come. By using this helper they may create a slightly cleaner code. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- include/linux/dmi.h | 7 +++++++ 1 file changed, 7 insertions(+)