From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755048AbdKIReq (ORCPT ); Thu, 9 Nov 2017 12:34:46 -0500 Received: from esa6.dell-outbound.iphmx.com ([68.232.149.229]:5374 "EHLO esa6.dell-outbound.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754463AbdKIRem (ORCPT ); Thu, 9 Nov 2017 12:34:42 -0500 IronPort-PHdr: =?us-ascii?q?9a23=3AMIQ8FxV5ku7mOhRu5vYMNv9aE2/V8LGtZVwlr6E/?= =?us-ascii?q?grcLSJyIuqrYbByCt8tkgFKBZ4jH8fUM07OQ6PGwHzRYqb+681k6OKRWUBEEjc?= =?us-ascii?q?hE1ycBO+WiTXPBEfjxciYhF95DXlI2t1uyMExSBdqsLwaK+i764jEdAAjwOhRo?= =?us-ascii?q?LerpBIHSk9631+ev8JHPfglEnjSwbLdxIRmssQndqtQdjJd/JKo21hbHuGZDdf?= =?us-ascii?q?5MxWNvK1KTnhL86dm18ZV+7SleuO8v+tBZX6nicKs2UbJXDDI9M2Ao/8LrrgXM?= =?us-ascii?q?TRGO5nQHTGoblAdDDhXf4xH7WpfxtTb6tvZ41SKHM8D6Uaw4VDK/5KptVRTmij?= =?us-ascii?q?oINyQh/W/XlMJ+kb5brhyiqRxxwYHUYZ2aOvVxca7GYdMVXmhBUtpNWyBdAI6x?= =?us-ascii?q?aZYEAeobPeZfqonwv1wDoxykCgm2BePvzSVEiHn33a0/1OQhFx3J3A0+ENIKtH?= =?us-ascii?q?TUq874O7oMXuCxyKnE1ynMb/RT2Trk7oXDbxMvoemUUL9xcsfd01cjGg3bglmK?= =?us-ascii?q?tIDoPzyY2v4JvmWf9+ZsSP6jh3Q6pwx+rDWj3Nogh4rTio8Wy13I7Th1zYk7KN?= =?us-ascii?q?GiVUJ2YN+pHIFTuiyaLYd6XN8uTmButS0n0LMJo4S7czIPyJk/wh7fbOGIfJaQ?= =?us-ascii?q?7xL4UeaRPS94hHV4eLKjnxqy8Vavyun7VsSszllKtTBKn9jWun8QyRPT7syHRu?= =?us-ascii?q?J6/ke8xTaAzAfT6vxCIU8pi6bXMZ8hwqYwlpoWvkXPBDP5mELzjKOOd0Uk/Pan?= =?us-ascii?q?6/j/b7jnpZKQLZF4hw/gPqg0h8CyAes1PhIKUmWf4ei80afs/Uz9QLVElP02la?= =?us-ascii?q?zZvYjdK8sBvK65AghV3pwl5Ra+Cjem19IYkmUGLF1bfBKHi4/pNkrTL//mCfe/?= =?us-ascii?q?h06gnytsx/DDJrHhGInCLmDfkLf9erZw80pcyAs1zdBC6JNYE7IBL+zpWk/3qt?= =?us-ascii?q?PYCgQ0MxK7w+n5EtVxzIAeVnyVAq+fLqzStUWE5uU1I+mDfIUVoiryK+A55/7y?= =?us-ascii?q?in80gUcdfa2z0psLZnC4Ge5mI0CAbXXxmNcBEHkKsRQkTODzh1yPUj9eam2sX6?= =?us-ascii?q?Iz+D47EpiqDYTdSYC3hryOwiO7EodRZmBcBVDfWUvvItGcUvMNLjiVIsZ7ujMB?= =?us-ascii?q?XLmlDYQm0Ef9mhX9zu8zC+PO+ypekZPm095+5uDXkRYa+TFwC4KW1GTbHDI8pX?= =?us-ascii?q?8BWzJjhPM3mkd60FrWiaU=3D?= X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2E0AQCCkARah2Oa6ERcGQEBAQEBAQEBA?= =?us-ascii?q?QEBAQcBAQEBAYQIficHg3aZRoF8gX+GV414ghEKhTsCGoQdQRYBAQEBAQEBAQE?= =?us-ascii?q?BAhABAQEKCwkIKC+COCKCRAEBAQQjBA1FDAQCAQgRBAEBAQICIwMCAgIfJQEIC?= =?us-ascii?q?AIEDgUIE4lwAxWpYYFtOodCDYNIAQEBAQEBAQMBAQEBAQEBAQEBAR2BD4Ihgge?= =?us-ascii?q?DPYMqgmuCMxAhAoJbgmMFiiKJLY4PPZAFhHCTRI0iiQCBOSYCgiF6g0KCXBAMG?= =?us-ascii?q?YFOd4lILIEFgREBAQE?= X-IPAS-Result: =?us-ascii?q?A2E0AQCCkARah2Oa6ERcGQEBAQEBAQEBAQEBAQcBAQEBAYQ?= =?us-ascii?q?IficHg3aZRoF8gX+GV414ghEKhTsCGoQdQRYBAQEBAQEBAQEBAhABAQEKCwkIK?= =?us-ascii?q?C+COCKCRAEBAQQjBA1FDAQCAQgRBAEBAQICIwMCAgIfJQEICAIEDgUIE4lwAxW?= =?us-ascii?q?pYYFtOodCDYNIAQEBAQEBAQMBAQEBAQEBAQEBAR2BD4IhggeDPYMqgmuCMxAhA?= =?us-ascii?q?oJbgmMFiiKJLY4PPZAFhHCTRI0iiQCBOSYCgiF6g0KCXBAMGYFOd4lILIEFgRE?= =?us-ascii?q?BAQE?= From: X-LoopCount0: from 10.166.132.169 X-IronPort-AV: E=Sophos;i="5.44,370,1505797200"; d="scan'208";a="1176224774" X-DLP: DLP_GlobalPCIDSS To: CC: , , , Subject: RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Thread-Topic: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Thread-Index: AQHTWXRzZxN2cgUwok2mrgpP5WsL66MMN2gwgAB6mgD//5y0gA== Date: Thu, 9 Nov 2017 17:34:39 +0000 Message-ID: <2ee7e355941f4ef5a244f152d4953753@ausx13mpc120.AMER.DELL.COM> References: <6312b76d08d3b6b5562c773fce8e13d85036ddd5.1509725778.git.mario.limonciello@dell.com> <20171109160249.jjiros7c4z6f76wr@pali> <8746e7178fb84a1b9309a6b228af604f@ausx13mpc120.AMER.DELL.COM> <20171109172834.wi3udtnuxkqolbzm@pali> In-Reply-To: <20171109172834.wi3udtnuxkqolbzm@pali> 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="utf-8" 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 base64 to 8bit by nfs id vA9HYo74020514 > -----Original Message----- > From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86- > owner@vger.kernel.org] On Behalf Of Pali Rohár > Sent: Thursday, November 9, 2017 11:29 AM > To: Limonciello, Mario > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux- > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org > Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to > dependent drivers > > On Thursday 09 November 2017 16:13:52 Mario.Limonciello@dell.com wrote: > > > -----Original Message----- > > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > > Sent: Thursday, November 9, 2017 10:03 AM > > > To: Limonciello, Mario > > > Cc: dvhart@infradead.org; Andy Shevchenko ; > > > LKML ; platform-driver-x86@vger.kernel.org > > > Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to > > > dependent drivers > > > > > > On Friday 03 November 2017 11:27:22 Mario Limonciello wrote: > > > > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor > > > > finishing probe successfully to probe themselves. > > > > > > > > Currently if dell-wmi-descriptor fails probing in a non-recoverable way > > > > (such as invalid header) dell-wmi and dell-smbios-wmi will continue to > > > > try to redo probing due to deferred probing. > > > > > > > > To solve this have the dependent drivers query the dell-wmi-descriptor > > > > driver whether the descriptor has been determined valid. The possible > > > > results are: > > > > -EPROBE_DEFER: Descriptor not yet probed, dependent driver should wait > > > > and use deferred probing > > > > < 0: Descriptor probed, invalid. Dependent driver should return an > > > > error. > > > > 0: Successful descriptor probe, dependent driver can continue > > > > > > > > Successful descriptor probe still doesn't mean that the descriptor driver > > > > is necessarily bound at the time of initialization of dependent driver. > > > > Userspace can unbind the driver, so all methods used from driver > > > > should still be verified to return success values otherwise deferred > > > > probing be used. > > > > > > Darren, Andy, any comments on this patch? > > > > > > I think now it should work also those corner, but legit cases. > > > > > > > Signed-off-by: Mario Limonciello > > > > --- > > > > drivers/platform/x86/dell-smbios-wmi.c | 4 ++++ > > > > drivers/platform/x86/dell-wmi-descriptor.c | 11 +++++++++++ > > > > drivers/platform/x86/dell-wmi-descriptor.h | 7 +++++++ > > > > drivers/platform/x86/dell-wmi.c | 5 +++++ > > > > 4 files changed, 27 insertions(+) > > > > > > > > diff --git a/drivers/platform/x86/dell-smbios-wmi.c > b/drivers/platform/x86/dell- > > > smbios-wmi.c > > > > index 35c13815b24c..3fa53fa174b2 100644 > > > > --- a/drivers/platform/x86/dell-smbios-wmi.c > > > > +++ b/drivers/platform/x86/dell-smbios-wmi.c > > > > @@ -151,6 +151,10 @@ static int dell_smbios_wmi_probe(struct > wmi_device > > > *wdev) > > > > if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID)) > > > > return -ENODEV; > > > > > > > > + ret = dell_wmi_get_descriptor_valid(); > > > > + if (ret) > > > > + return ret; > > > > + > > > > priv = devm_kzalloc(&wdev->dev, sizeof(struct wmi_smbios_priv), > > > > GFP_KERNEL); > > > > if (!priv) > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c > > > b/drivers/platform/x86/dell-wmi-descriptor.c > > > > index 28ef5f37cfbf..e7f4c3a7bfc4 100644 > > > > --- a/drivers/platform/x86/dell-wmi-descriptor.c > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c > > > > @@ -26,9 +26,16 @@ struct descriptor_priv { > > > > u32 interface_version; > > > > u32 size; > > > > }; > > > > +static int descriptor_valid = -EPROBE_DEFER; > > > > static LIST_HEAD(wmi_list); > > > > static DEFINE_MUTEX(list_mutex); > > > > > > > > +int dell_wmi_get_descriptor_valid(void) > > > > +{ > > > > + return descriptor_valid; > > > > +} > > > > +EXPORT_SYMBOL_GPL(dell_wmi_get_descriptor_valid); > > > > + > > > > bool dell_wmi_get_interface_version(u32 *version) > > > > { > > > > struct descriptor_priv *priv; > > > > @@ -91,6 +98,7 @@ static int dell_wmi_descriptor_probe(struct wmi_device > > > *wdev) > > > > if (obj->type != ACPI_TYPE_BUFFER) { > > > > dev_err(&wdev->dev, "Dell descriptor has wrong type\n"); > > > > ret = -EINVAL; > > > > + descriptor_valid = ret; > > > > goto out; > > > > } > > > > > > > > @@ -102,6 +110,7 @@ static int dell_wmi_descriptor_probe(struct > wmi_device > > > *wdev) > > > > "Dell descriptor buffer has unexpected length (%d)\n", > > > > obj->buffer.length); > > > > ret = -EINVAL; > > > > + descriptor_valid = ret; > > > > goto out; > > > > } > > > > > > > > @@ -111,8 +120,10 @@ static int dell_wmi_descriptor_probe(struct > wmi_device > > > *wdev) > > > > dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature > > > (%8ph)\n", > > > > buffer); > > > > ret = -EINVAL; > > > > + descriptor_valid = ret; > > > > goto out; > > > > } > > > > + descriptor_valid = 0; > > > > > > > > if (buffer[2] != 0 && buffer[2] != 1) > > > > dev_warn(&wdev->dev, "Dell descriptor buffer has unknown > > > version (%lu)\n", > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h > > > b/drivers/platform/x86/dell-wmi-descriptor.h > > > > index 5f7b69c2c83a..776cddd5e135 100644 > > > > --- a/drivers/platform/x86/dell-wmi-descriptor.h > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.h > > > > @@ -15,6 +15,13 @@ > > > > > > > > #define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012- > > > B622A1EF5492" > > > > > > > > +/* possible return values: > > > > + * -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run > > > > + * 0: valid descriptor, successfully probed > > > > + * < 0: invalid descriptor, don't probe dependent devices > > > > + */ > > > > +int dell_wmi_get_descriptor_valid(void); > > > > + > > > > bool dell_wmi_get_interface_version(u32 *version); > > > > bool dell_wmi_get_size(u32 *size); > > > > > > > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell- > wmi.c > > > > index 54321080a30d..bb7c1e681792 100644 > > > > --- a/drivers/platform/x86/dell-wmi.c > > > > +++ b/drivers/platform/x86/dell-wmi.c > > > > @@ -655,10 +655,15 @@ static int dell_wmi_events_set_enabled(bool > enable) > > > > static int dell_wmi_probe(struct wmi_device *wdev) > > > > { > > > > struct dell_wmi_priv *priv; > > > > + int ret; > > > > > > > > if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID)) > > > > return -ENODEV; > > > > > > Just one suggestion, is above check still needed in dell-wmi.c code? > > > I think that now it should be job of dell_wmi_get_descriptor_valid() > > > function to fully validate if dell wmi descriptor driver is able to > > > provide all needed information for dell-wmi driver. > > > > > > > Yes, I believe it's still needed because if the GUID isn't on the bus the probe > > routine for dell-wmi-descriptor won't run and dell_wmi_get_descriptor_valid() > > will continually return -EPROBE_DEFER. > > > > That's the exact problem this patch exists for (preventing infinite deferred > > probing). > > > > Perhaps a "cleaner" solution is to have the init routine of dell-wmi-descriptor > > do this wmi_has_guid check and set the descriptor_valid variable to -ENODEV > > so that "dependent" drivers don't need to. > > I understand. But I mean, if function dell_wmi_get_descriptor_valid() > should not do that check for DELL_WMI_DESCRIPTOR_GUID itself. > > Because every driver which would use dell-wmi-descriptor needs to > do that check. I'll move the check to dell_wmi_descriptor_valid(). That actually does remove the need for even including the GUID #define in a header file. All use will be contained now directly in dell-wmi-descriptor.c. > > > > > + ret = dell_wmi_get_descriptor_valid(); > > > > + if (ret) > > > > + return ret; > > > > + > > > > priv = devm_kzalloc( > > > > &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL); > > > > if (!priv) > > > > > > -- > > > Pali Rohár > > > pali.rohar@gmail.com > > -- > Pali Rohár > pali.rohar@gmail.com