Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

From: Kani, Toshimitsu
Date: Mon Aug 21 2017 - 12:42:28 EST


On Mon, 2017-08-21 at 13:27 +0200, Borislav Petkov wrote:
> On Fri, Aug 18, 2017 at 01:46:40PM -0600, Toshi Kani wrote:
> > ACPI OEM ID / OEM Table ID / Revision can be used to identify
> > a platform based on ACPI firmware info.ÂÂacpi_blacklisted(),
> > intel_pstate_platform_pwr_mgmt_exists(), and some other funcs,
> > have been using similar check to detect a list of platforms
> > that require special handlings.
> >
> > Move the platform check in acpi_blacklisted() to a new common
> > utility function, acpi_match_platform_list(), so that other
> > drivers do not have to implement their own version.
> >
> > There is no change in functionality.
:
> > +
> > + for (; plat->oem_id[0]; plat++, idx++) {
> > + if (ACPI_FAILURE(acpi_get_table_header(plat-
> > >table, 0, &hdr)))
> > + continue;
> > +
> > + if (strncmp(plat->oem_id, hdr.oem_id,
> > ACPI_OEM_ID_SIZE))
> > + continue;
> > +
> > + if (strncmp(plat->oem_table_id, hdr.oem_table_id,
> > + ÂÂÂÂACPI_OEM_TABLE_ID_SIZE))
>
> Let that stick out.

Putting to a single line leads to "line over 80 characters" warning
from checkpatch.pl. Would you still advice to do that?

> > + continue;
> > +
> > + if ((plat->pred == all_versions) ||
> > + ÂÂÂÂ(plat->pred == less_than_or_equal
> > + && hdr.oem_revision <= plat->oem_revision)
> > ||
> > + ÂÂÂÂ(plat->pred == greater_than_or_equal
> > + && hdr.oem_revision >= plat->oem_revision)
> > ||
> > + ÂÂÂÂ(plat->pred == equal
> > + && hdr.oem_revision == plat-
> > >oem_revision))
> > + return idx;
>
> Make that more readable:
>
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif ((plat->pred == all_versions) ||
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(plat->pred == less_than_or_equalÂÂÂÂ&&
> hdr.oem_revision <= plat->oem_revision) ||
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(plat->pred == greater_than_or_equal &&
> hdr.oem_revision >= plat->oem_revision) ||
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(plat->pred == equalÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&&
> hdr.oem_revision == plat->oem_revision))
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn idx;

Same here. These lead to checkpatch warnings.

> > + }
> > +
> > + return -ENODEV;
> > +}
> > +EXPORT_SYMBOL(acpi_match_platform_list);
> > diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> > index 27b4b66..a9b6dc2 100644
> > --- a/include/linux/acpi.h
> > +++ b/include/linux/acpi.h
> > @@ -556,6 +556,25 @@ extern acpi_status
> > acpi_pci_osc_control_set(acpi_handle handle,
> > Â#define ACPI_OST_SC_DRIVER_LOAD_FAILURE 0x81
> > Â#define ACPI_OST_SC_INSERT_NOT_SUPPORTED 0x82
> > Â
> > +enum acpi_predicate {
> > + all_versions,
> > + less_than_or_equal,
> > + equal,
> > + greater_than_or_equal,
> > +};
> > +
> > +/* Table must be terminted by a NULL entry */
> > +struct acpi_platform_list {
> > + char oem_id[ACPI_OEM_ID_SIZE];
>
> + 1
>
> > + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE];
>
> ÂÂÂ+ 1

strncmp() is fine without these, but it'd be prudent in case someone
decides to print these strings with printk(). Will do.

> > + u32 oem_revision;
> > + char *table;
> > + enum acpi_predicate pred;
> > + char *reason;
> > + u32 data;
>
> Ok, turning that into data from is_critical_error is a step in the
> right direction. Let's make it even better:
>
> u32 flags;
>
> and do
>
> #define ACPI_PLAT_IS_CRITICAL_ERROR BIT(0)
>
> so that future elements add new bits instead of wasting a whole u32
> as a boolean.

'data' here is private to the caller. So, I do not think we need to
define the bits. Shall I change the name to 'driver_data' to make it
more explicit?

Thanks,
-Toshi