* [PATCH 0/2] Account for uncorrectable failures in probing
@ 2017-11-03 16:27 Mario Limonciello
2017-11-03 16:27 ` [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated Mario Limonciello
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Mario Limonciello @ 2017-11-03 16:27 UTC (permalink / raw)
To: dvhart, Andy Shevchenko
Cc: LKML, platform-driver-x86, pali.rohar, Mario Limonciello
Pali raised some concerns around corner case scenarios that probing
may fail on dell-wmi-descriptor causing dell-wmi and dell-smbios-wmi
to be stuck in infinite deferred probing loops.
This patch series accounts for that corner case.
Changes since original submission:
- Add as second patch that catches potential NULL pointer
- Avoid potential race condition between driver init and calling
check for if validation successful
Mario Limonciello (2):
platform/x86: dell-wmi-descriptor: check if memory was allocated
platform/x86: dell-*wmi*: Relay failed initial probe to dependent
drivers
drivers/platform/x86/dell-smbios-wmi.c | 4 ++++
drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
drivers/platform/x86/dell-wmi-descriptor.h | 7 +++++++
drivers/platform/x86/dell-wmi.c | 5 +++++
4 files changed, 32 insertions(+)
--
2.14.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated 2017-11-03 16:27 [PATCH 0/2] Account for uncorrectable failures in probing Mario Limonciello @ 2017-11-03 16:27 ` Mario Limonciello 2017-11-09 12:07 ` Pali Rohár 2017-11-03 16:27 ` [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Mario Limonciello 2017-11-07 16:17 ` [PATCH 0/2] Account for uncorrectable failures in probing Mario.Limonciello 2 siblings, 1 reply; 12+ messages in thread From: Mario Limonciello @ 2017-11-03 16:27 UTC (permalink / raw) To: dvhart, Andy Shevchenko Cc: LKML, platform-driver-x86, pali.rohar, Mario Limonciello devm_kzalloc will return NULL pointer if no memory was allocated. This should be checked. This problem also existed when the driver was dell-wmi.c. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- drivers/platform/x86/dell-wmi-descriptor.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c index 3204c408e261..28ef5f37cfbf 100644 --- a/drivers/platform/x86/dell-wmi-descriptor.c +++ b/drivers/platform/x86/dell-wmi-descriptor.c @@ -121,6 +121,11 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev) priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv), GFP_KERNEL); + if (!priv) { + ret = -ENOMEM; + goto out; + } + priv->interface_version = buffer[2]; priv->size = buffer[3]; ret = 0; -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated 2017-11-03 16:27 ` [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated Mario Limonciello @ 2017-11-09 12:07 ` Pali Rohár 0 siblings, 0 replies; 12+ messages in thread From: Pali Rohár @ 2017-11-09 12:07 UTC (permalink / raw) To: Mario Limonciello; +Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86 On Friday 03 November 2017 11:27:21 Mario Limonciello wrote: > devm_kzalloc will return NULL pointer if no memory was allocated. > This should be checked. This problem also existed when the driver > was dell-wmi.c. > > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> Reviewed-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/platform/x86/dell-wmi-descriptor.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c > index 3204c408e261..28ef5f37cfbf 100644 > --- a/drivers/platform/x86/dell-wmi-descriptor.c > +++ b/drivers/platform/x86/dell-wmi-descriptor.c > @@ -121,6 +121,11 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev) > priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv), > GFP_KERNEL); > > + if (!priv) { > + ret = -ENOMEM; > + goto out; > + } > + > priv->interface_version = buffer[2]; > priv->size = buffer[3]; > ret = 0; -- Pali Rohár pali.rohar@gmail.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-03 16:27 [PATCH 0/2] Account for uncorrectable failures in probing Mario Limonciello 2017-11-03 16:27 ` [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated Mario Limonciello @ 2017-11-03 16:27 ` Mario Limonciello 2017-11-04 0:53 ` Darren Hart 2017-11-09 16:02 ` Pali Rohár 2017-11-07 16:17 ` [PATCH 0/2] Account for uncorrectable failures in probing Mario.Limonciello 2 siblings, 2 replies; 12+ messages in thread From: Mario Limonciello @ 2017-11-03 16:27 UTC (permalink / raw) To: dvhart, Andy Shevchenko Cc: LKML, platform-driver-x86, pali.rohar, Mario Limonciello 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. Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> --- 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; + ret = dell_wmi_get_descriptor_valid(); + if (ret) + return ret; + priv = devm_kzalloc( &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL); if (!priv) -- 2.14.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-03 16:27 ` [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Mario Limonciello @ 2017-11-04 0:53 ` Darren Hart 2017-11-04 3:25 ` Mario.Limonciello 2017-11-09 16:02 ` Pali Rohár 1 sibling, 1 reply; 12+ messages in thread From: Darren Hart @ 2017-11-04 0:53 UTC (permalink / raw) To: Mario Limonciello; +Cc: Andy Shevchenko, LKML, platform-driver-x86, pali.rohar On Fri, Nov 03, 2017 at 11:27:22AM -0500, 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 Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a dependent driver is loaded. It isn't clear to me in which scenario we encounter this problem ?? > should still be verified to return success values otherwise deferred > probing be used. The part after "otherwise" is breaking my English parser... Should this read: "Userspace can unbind the driver, so all methods used from the driver should still be verified to return successful values, falling back to deferred probing in case of failure." ?? > 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: This should trigger a checkpatch error, but doesn't. Huh. For everything but "net", comment blocks should start with /* and not following text. /* * First line. * Second line. */ A nit, and I can clean up if no changes are deemed necessary here. > + * -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run > + * 0: valid descriptor, successfully probed > + * < 0: invalid descriptor, don't probe dependent devices > + */ -- Darren Hart VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-04 0:53 ` Darren Hart @ 2017-11-04 3:25 ` Mario.Limonciello 0 siblings, 0 replies; 12+ messages in thread From: Mario.Limonciello @ 2017-11-04 3:25 UTC (permalink / raw) To: dvhart; +Cc: andy.shevchenko, linux-kernel, platform-driver-x86, pali.rohar > -----Original Message----- > From: Darren Hart [mailto:dvhart@infradead.org] > Sent: Friday, November 3, 2017 7:53 PM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; LKML <linux- > kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > pali.rohar@gmail.com > Subject: Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to > dependent drivers > > On Fri, Nov 03, 2017 at 11:27:22AM -0500, 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 > > Userspace shouldn't be able to remove the dell-wmi-descriptor driver if a > dependent driver is loaded. It isn't clear to me in which scenario we encounter > this problem ?? Userspace can however unbind a bound GUID. When this happens getting the interface version and size will both fail. > > > > should still be verified to return success values otherwise deferred > > probing be used. > > The part after "otherwise" is breaking my English parser... > > Should this read: "Userspace can unbind the driver, so all methods used from the > driver should still be verified to return successful values, falling back to > deferred probing in case of failure." ?? Yeah that's clearer and correct. > > > 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: > > This should trigger a checkpatch error, but doesn't. Huh. For everything but > "net", comment blocks should start with /* and not following text. > OK. > /* > * First line. > * Second line. > */ > > A nit, and I can clean up if no changes are deemed necessary here. > > > + * -EPROBE_DEFER: probing for dell-wmi-descriptor not yet run > > + * 0: valid descriptor, successfully probed > > + * < 0: invalid descriptor, don't probe dependent devices > > + */ > > -- > Darren Hart > VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-03 16:27 ` [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Mario Limonciello 2017-11-04 0:53 ` Darren Hart @ 2017-11-09 16:02 ` Pali Rohár 2017-11-09 16:13 ` Mario.Limonciello 1 sibling, 1 reply; 12+ messages in thread From: Pali Rohár @ 2017-11-09 16:02 UTC (permalink / raw) To: Mario Limonciello; +Cc: dvhart, Andy Shevchenko, LKML, platform-driver-x86 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 <mario.limonciello@dell.com> > --- > 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. > + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-09 16:02 ` Pali Rohár @ 2017-11-09 16:13 ` Mario.Limonciello 2017-11-09 17:28 ` Pali Rohár 0 siblings, 1 reply; 12+ messages in thread From: Mario.Limonciello @ 2017-11-09 16:13 UTC (permalink / raw) To: pali.rohar; +Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86 > -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Thursday, November 9, 2017 10:03 AM > To: Limonciello, Mario <Mario_Limonciello@Dell.com> > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > LKML <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 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 <mario.limonciello@dell.com> > > --- > > 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. > > + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-09 16:13 ` Mario.Limonciello @ 2017-11-09 17:28 ` Pali Rohár 2017-11-09 17:34 ` Mario.Limonciello 0 siblings, 1 reply; 12+ messages in thread From: Pali Rohár @ 2017-11-09 17:28 UTC (permalink / raw) To: Mario.Limonciello Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86 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 <Mario_Limonciello@Dell.com> > > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > > LKML <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 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 <mario.limonciello@dell.com> > > > --- > > > 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. > > > + 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-09 17:28 ` Pali Rohár @ 2017-11-09 17:34 ` Mario.Limonciello 2017-11-09 17:52 ` Darren Hart 0 siblings, 1 reply; 12+ messages in thread From: Mario.Limonciello @ 2017-11-09 17:34 UTC (permalink / raw) To: pali.rohar; +Cc: dvhart, andy.shevchenko, linux-kernel, platform-driver-x86 > -----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 <Mario_Limonciello@Dell.com> > 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 <Mario_Limonciello@Dell.com> > > > Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>; > > > LKML <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 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 <mario.limonciello@dell.com> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers 2017-11-09 17:34 ` Mario.Limonciello @ 2017-11-09 17:52 ` Darren Hart 0 siblings, 0 replies; 12+ messages in thread From: Darren Hart @ 2017-11-09 17:52 UTC (permalink / raw) To: Mario.Limonciello Cc: pali.rohar, andy.shevchenko, linux-kernel, platform-driver-x86 On Thu, Nov 09, 2017 at 05:34:39PM +0000, Mario.Limonciello@dell.com wrote: > > > > > 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. This is a worthwhile improvement. -- Darren Hart VMware Open Source Technology Center ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 0/2] Account for uncorrectable failures in probing 2017-11-03 16:27 [PATCH 0/2] Account for uncorrectable failures in probing Mario Limonciello 2017-11-03 16:27 ` [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated Mario Limonciello 2017-11-03 16:27 ` [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Mario Limonciello @ 2017-11-07 16:17 ` Mario.Limonciello 2 siblings, 0 replies; 12+ messages in thread From: Mario.Limonciello @ 2017-11-07 16:17 UTC (permalink / raw) To: pali.rohar; +Cc: linux-kernel, platform-driver-x86, dvhart, andy.shevchenko > -----Original Message----- > From: Limonciello, Mario > Sent: Friday, November 3, 2017 11:27 AM > To: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org; > pali.rohar@gmail.com; Limonciello, Mario <Mario_Limonciello@Dell.com> > Subject: [PATCH 0/2] Account for uncorrectable failures in probing > > Pali raised some concerns around corner case scenarios that probing > may fail on dell-wmi-descriptor causing dell-wmi and dell-smbios-wmi > to be stuck in infinite deferred probing loops. > > This patch series accounts for that corner case. > > Changes since original submission: > - Add as second patch that catches potential NULL pointer > - Avoid potential race condition between driver init and calling > check for if validation successful > Mario Limonciello (2): > platform/x86: dell-wmi-descriptor: check if memory was allocated > platform/x86: dell-*wmi*: Relay failed initial probe to dependent > drivers > > drivers/platform/x86/dell-smbios-wmi.c | 4 ++++ > drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++ > drivers/platform/x86/dell-wmi-descriptor.h | 7 +++++++ > drivers/platform/x86/dell-wmi.c | 5 +++++ > 4 files changed, 32 insertions(+) > > -- > 2.14.1 Pali, Did you have any thoughts on these two patches? I believe they should account for the concerns you raised regarding the corner cases with probing in my series. The series is queued up in the platform-drivers-x86 testing branch and they are intended to be on top of that. Thanks, ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-11-09 17:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-03 16:27 [PATCH 0/2] Account for uncorrectable failures in probing Mario Limonciello 2017-11-03 16:27 ` [PATCH 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated Mario Limonciello 2017-11-09 12:07 ` Pali Rohár 2017-11-03 16:27 ` [PATCH 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers Mario Limonciello 2017-11-04 0:53 ` Darren Hart 2017-11-04 3:25 ` Mario.Limonciello 2017-11-09 16:02 ` Pali Rohár 2017-11-09 16:13 ` Mario.Limonciello 2017-11-09 17:28 ` Pali Rohár 2017-11-09 17:34 ` Mario.Limonciello 2017-11-09 17:52 ` Darren Hart 2017-11-07 16:17 ` [PATCH 0/2] Account for uncorrectable failures in probing Mario.Limonciello
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).