From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751601AbdI3Tsn (ORCPT ); Sat, 30 Sep 2017 15:48:43 -0400 Received: from esa2.dell-outbound.iphmx.com ([68.232.149.220]:4766 "EHLO esa2.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbdI3Tsl (ORCPT ); Sat, 30 Sep 2017 15:48:41 -0400 From: X-LoopCount0: from 10.166.132.190 X-IronPort-AV: E=Sophos;i="5.42,459,1500958800"; d="scan'208";a="67793470" X-DLP: DLP_GlobalPCIDSS To: CC: , , , , , Subject: RE: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check Thread-Topic: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check Thread-Index: AQHTOYuIcRms8r8ZDk2OMR9SmpnBv6LN1ksw Date: Sat, 30 Sep 2017 19:48:39 +0000 Message-ID: References: <20170930012912.GB13307@fury> In-Reply-To: <20170930012912.GB13307@fury> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.143.18.86] Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v8UJmnZC012777 > -----Original Message----- > From: Darren Hart [mailto:dvhart@infradead.org] > Sent: Friday, September 29, 2017 8:29 PM > To: Limonciello, Mario > Cc: Andy Shevchenko ; LKML kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; Andy Lutomirski > ; quasisec@google.com; pali.rohar@gmail.com > Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI > descriptor check > > On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote: > > The Dell WMI descriptor check is used as an indication that WMI > > calls are safe to run both when used with the notification > > ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair. > > > > As some code in dell-wmi-smbios is already a prerequisite for > > dell-wmi, move the code for performing the descriptor check into > > dell-wmi-smbios and let both drivers use it from there. > > > > Signed-off-by: Mario Limonciello > > --- > ... > > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell- > smbios.c > ... > > > > +/* > > + * Descriptor buffer is 128 byte long and contains: > ... > > + if (obj->buffer.length != 128) { > > + dev_err(&wdev->dev, > > + "Dell descriptor buffer has invalid length (%d)\n", > > + obj->buffer.length); > > This seems odd. We call it an error (not a warning) if != 128, but we only abort > and return an error if it's < 16. > > If it's an error, we should return an error code, if anything above 16 is > acceptable but 128 is preferred, the above should be a warning at best. (this > scenario seems unlikely). Hopefully the original author can speak up to the intentions here. I would feel that it should have errored out if it wasn't expected length too. > > > + if (obj->buffer.length < 16) { > > + ret = -EINVAL; > > + goto out; > > + } > > + } > > + desc_buffer = (u32 *)obj->buffer.pointer; > > + > > + if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720) > > This seems like it should be an || ? > > (I see this is fixed in a later patch) > > I would have suggested fixing it, then moving it - just from a pure ease of > review perspective. Perhaps not in the comment above that this is a verbatim > move, some fixes are coming in subsequent patches. This is worth noting for a > future develop who may be choosing what to backport. > I'll re-order things so that the fixes come before the move. > > static int dell_smbios_wmi_probe(struct wmi_device *wdev) > > { > > + int ret; > > + u32 interface_version; > > Declarations in order of decreasing line length please. > > > @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device > *wdev) > > if (!buffer) > > return -ENOMEM; > > bufferlen = sizeof(struct wmi_calling_interface_buffer); > > + > > Stray whitespace, should have been in the previous patch I guess? > > > -- > Darren Hart > VMware Open Source Technology Center