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
