Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1690378 > unrolled thread

Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-07-18 17:50 +0200
    Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface Borislav Petkov <bp@alien8.de> - 2017-07-18 18:50 +0200
      Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-07-18 19:30 +0200
        Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface Borislav Petkov <bp@alien8.de> - 2017-07-18 19:50 +0200
          Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-07-18 21:00 +0200
            Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface Borislav Petkov <bp@alien8.de> - 2017-07-18 21:40 +0200
              Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface "Kani, Toshimitsu" <toshi.kani@hpe.com> - 2017-07-18 22:20 +0200

#1690378 — Re: [PATCH 1/3] ACPI / blacklist: add acpi_match_oemlist() interface

On Tue, 2017-07-18 at 07:34 +0200, Borislav Petkov wrote:
> On Mon, Jul 17, 2017 at 03:59:10PM -0600, Toshi Kani wrote:
> > ACPI OEM ID / OEM Table ID / Revision can be used to identify
> > platform type based on ACPI firmware.  acpi_blacklisted(),
> > intel_pstate_platform_pwr_mgmt_exists() and some other funcs
> > have been using this type of check to detect a list of platforms
> > that require special handlings.
> > 
> > Move the platform type check in acpi_blacklisted() to a common
> > utility function, acpi_match_oemlist(), so that other drivers
> > do not have to implement their own.
> > 
> > There is no change in functionality.
 :
> >  /*
> >   * POLICY: If *anything* doesn't work, put it on the blacklist.
> >   *	   If they are critical errors, mark it critical, and
> > abort driver load.
> >   */
> > -static struct acpi_blacklist_item acpi_blacklist[] __initdata = {
> > +static struct acpi_oemlist acpi_blacklist[] __initdata = {
> 
> Why the arbitrary rename?

This patch defines 'struct acpi_oemlist' in "include/linux/acpi.h" as a
common structure, and replaces this specific 'struct acpi_blacklist'.

> If anything, you should shorten that
> 
> 	enum acpi_blacklist_predicates oem_revision_predicate;
> 
> unreadable insanity.

Agreed.  Will change to a shorter name like below. 

	enum acpi_oemlist_pred predicate;

+	i = acpi_match_oemlist(acpi_blacklist);
> > +	if (i >= 0) {
> > +		pr_err(PREFIX "Vendor \"%6.6s\" System \"%8.8s\" "
> > +		       "Revision 0x%x has a known ACPI BIOS
> > problem.\n",
> 
> Put that string on a single line for grepping. checkpatch catches
> that error, didn't you see it?

Will do.

Thanks!
-Toshi

[toc] | [next] | [standalone]

#1690440

On Tue, Jul 18, 2017 at 03:48:54PM +0000, Kani, Toshimitsu wrote:
> This patch defines 'struct acpi_oemlist' in "include/linux/acpi.h" as a

I see that.

> common structure, and replaces this specific 'struct acpi_blacklist'.

And what makes acpi_oemlist "common" and acpi_blacklist "specific"?

So let me save you some time - "oemlist" is more specific than
"blacklist" and I can imagine a blacklist item not always being
oem-specific.

What I'm hinting at is, don't change that name. acpi_blacklist is just
fine.

> Agreed.  Will change to a shorter name like below. 
> 
> 	enum acpi_oemlist_pred predicate;

	enum acpi_predicate pred;

is even better.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1690507

On Tue, 2017-07-18 at 18:43 +0200, Borislav Petkov wrote:
> On Tue, Jul 18, 2017 at 03:48:54PM +0000, Kani, Toshimitsu wrote:
> > This patch defines 'struct acpi_oemlist' in "include/linux/acpi.h"
> > as a
> 
> I see that.
> 
> > common structure, and replaces this specific 'struct
> > acpi_blacklist'.
> 
> And what makes acpi_oemlist "common" and acpi_blacklist "specific"?
> 
> So let me save you some time - "oemlist" is more specific than
> "blacklist" and I can imagine a blacklist item not always being
> oem-specific.
> 
> What I'm hinting at is, don't change that name. acpi_blacklist is
> just fine.

Well, a list does not need to be a black-list.  It can be a white-list
or anything that matters.  The caller defines the usage of a list.  So,
I tried to avoid putting any usage to the structure name.

> > Agreed.  Will change to a shorter name like below. 
> > 
> > 	enum acpi_oemlist_pred predicate;
> 
> 	enum acpi_predicate pred;
> 
> is even better.

Sounds good.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1690529

On Tue, Jul 18, 2017 at 05:24:50PM +0000, Kani, Toshimitsu wrote:
> Well, a list does not need to be a black-list.

But this one *is* a blacklist.

> So, I tried to avoid putting any usage to the structure name.

So OEM is a usage. The moment you need to use it for something else
besides an OEM, it is not an OEM list anymore - it is a generic
blacklist which blacklists OEMs too.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1690570

On Tue, 2017-07-18 at 19:42 +0200, Borislav Petkov wrote:
> On Tue, Jul 18, 2017 at 05:24:50PM +0000, Kani, Toshimitsu wrote:
> > Well, a list does not need to be a black-list.
> 
> But this one *is* a blacklist.

Right.  Hence, acpi_backlisted() still declares the list as
'acpi_blacklist[]'.

> > So, I tried to avoid putting any usage to the structure name.
> 
> So OEM is a usage. The moment you need to use it for something else
> besides an OEM, it is not an OEM list anymore - it is a generic
> blacklist which blacklists OEMs too.

The term "oem" represents data types of the structure, oem_id[],
oem_table_id[], and oem_revision, which are defined by the ACPI spec.  

ghes_edac uses this structure as a while-list, so the term backlist is
misleading.  intel_pstate also uses it to list the platforms that do
not need OS control.

Thanks,
-Toshi

[toc] | [prev] | [next] | [standalone]

#1690592

On Tue, Jul 18, 2017 at 06:49:51PM +0000, Kani, Toshimitsu wrote:
> ghes_edac uses this structure as a while-list, so the term backlist is
> misleading.

So this matching function gets both blacklists and whitelists. No wonder
it is confusing. Now I finally understand what you wanna do: you want to
call all those lists something agnostic as platform_list or so because
they contain exactly that: platforms - not OEMs.

And then you want to match *platforms*. *Not* OEMs.

*Now* I understand what you're trying to tell me.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

[toc] | [prev] | [next] | [standalone]

#1690627

On Tue, 2017-07-18 at 21:32 +0200, Borislav Petkov wrote:
> On Tue, Jul 18, 2017 at 06:49:51PM +0000, Kani, Toshimitsu wrote:
> > ghes_edac uses this structure as a while-list, so the term backlist
> > is misleading.
> 
> So this matching function gets both blacklists and whitelists. No
> wonder it is confusing. Now I finally understand what you wanna do:
> you want to call all those lists something agnostic as platform_list
> or so because they contain exactly that: platforms - not OEMs.

Right.

> And then you want to match *platforms*. *Not* OEMs.

True, there is some stretch to use OEMIDs for detecting platforms.  But
we do not have other standard interfaces better than this one.

> *Now* I understand what you're trying to tell me.

:-)

Thanks,
-Toshi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web