linux-kernel - Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1503333107.2042.163.camel@hpe.com>
Date:   Mon, 21 Aug 2017 16:41:38 +0000
From:   "Kani, Toshimitsu" <toshi.kani@....com>
To:     "bp@...en8.de" <bp@...en8.de>,
        "rjw@...ysocki.net" <rjw@...ysocki.net>
CC:     "lenb@...nel.org" <lenb@...nel.org>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>
Subject: Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()

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