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

["Kani, Toshimitsu" <toshi.kani@hpe.com>]

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