Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

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, , 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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

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, , 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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

On Mon, 2018-02-26 at 10:29 +0100, Rafael J. Wysocki wrote:
> On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
>  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+gtwpvirqrw...@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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

On Mon, 2018-02-26 at 10:29 +0100, Rafael J. Wysocki wrote:
> On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
>  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+gtwpvirqrw...@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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Jean Delvare]

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 
> ---
>  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, , 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, , 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__ */

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Jean Delvare]

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 
> ---
>  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, , 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, , 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__ */

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Rafael J. Wysocki]

On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
 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 
>> > ---
>> >  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, , 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+gtwpvirqrw...@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?

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Rafael J. Wysocki]

On Sun, Feb 25, 2018 at 2:23 PM, Andy Shevchenko
 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 
>> > ---
>> >  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, , 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+gtwpvirqrw...@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?

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

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 
> > ---
> >  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, , 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+gtwpvirqrw...@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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Andy Shevchenko]

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 
> > ---
> >  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, , 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+gtwpvirqrw...@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.

-- 
Andy Shevchenko 
Intel Finland Oy

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Bjorn Helgaas]

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 
> ---
>  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, , 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+gtwpvirqrw...@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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/4] dmi: Introduce dmi_get_bios_year() helper

[Bjorn Helgaas]

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 
> ---
>  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, , 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+gtwpvirqrw...@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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html