From: "Kani, Toshimitsu" <toshi.kani@hpe.com>
To: "bp@alien8.de" <bp@alien8.de>, "rjw@rjwysocki.net" <rjw@rjwysocki.net>
CC: "lenb@kernel.org" <lenb@kernel.org>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "tony.luck@intel.com" <tony.luck@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>
Subject: Re: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()
Thread-Topic: [PATCH v3 1/5] ACPI / blacklist: add acpi_match_platform_list()
Thread-Index: AQHTGFwjnywryIINFUyGdWm/Gs0MS6KOsEQAgABU+oA=
Date: Mon, 21 Aug 2017 16:41:38 +0000
Message-ID: <1503333107.2042.163.camel@hpe.com>
References: <20170818194644.14538-1-toshi.kani@hpe.com>
         <20170818194644.14538-2-toshi.kani@hpe.com>
         <20170821112657.hrtjoeagxhc67rrr@pd.tnic>
In-Reply-To: <20170821112657.hrtjoeagxhc67rrr@pd.tnic>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <06D03BAC07DD274198ACFE6A20494DBB@NAMPRD84.PROD.OUTLOOK.COM>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Aug 2017 16:41:38.4731
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0140
X-OriginatorOrg: hpe.com
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v7LGgTKr010139
Content-Length: 3646
Lines: 117

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