[PATCH v2 0/2] Account for uncorrectable failures in probing

[PATCH v2 0/2] Account for uncorrectable failures in probing

[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 from v1 to v2:
- Add Reviewed by from Pali for patch 1
- Move wmi_has_guid check into dell-wmi-descriptor.c instead of
  dependent drivers.
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     |  5 +++--
 drivers/platform/x86/dell-wmi-descriptor.c | 21 +++++++++++++++++++++
 drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
 drivers/platform/x86/dell-wmi.c            |  6 ++++--
 4 files changed, 35 insertions(+), 5 deletions(-)

-- 
2.14.1

[PATCH v2 1/2] platform/x86: dell-wmi-descriptor: check if memory was allocated

[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>
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;
-- 
2.14.1

[PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[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:
-ENODEV: Descriptor GUID missing from WMI bus
-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     |  5 +++--
 drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
 drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
 drivers/platform/x86/dell-wmi.c            |  6 ++++--
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index 35c13815b24c..ca556803c8c9 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -148,8 +148,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 	int count;
 	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 wmi_smbios_priv),
 			    GFP_KERNEL);
diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
index 28ef5f37cfbf..4dfef1f53481 100644
--- a/drivers/platform/x86/dell-wmi-descriptor.c
+++ b/drivers/platform/x86/dell-wmi-descriptor.c
@@ -21,14 +21,26 @@
 #include <linux/wmi.h>
 #include "dell-wmi-descriptor.h"
 
+#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
+
 struct descriptor_priv {
 	struct list_head list;
 	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)
+{
+	if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
+		return -ENODEV;
+
+	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 +103,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 +115,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 +125,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..1e8cb96ffd78 100644
--- a/drivers/platform/x86/dell-wmi-descriptor.h
+++ b/drivers/platform/x86/dell-wmi-descriptor.h
@@ -13,7 +13,13 @@
 
 #include <linux/wmi.h>
 
-#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
+/* possible return values:
+ *  -ENODEV: Descriptor GUID missing from WMI bus
+ *  -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..39d2f4518483 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -655,9 +655,11 @@ 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);
-- 
2.14.1

Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[sathyanarayanan kuppuswamy]

Hi,


On 11/09/2017 09:49 AM, Mario Limonciello wrote:
> dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> finishing probe successfully to probe themselves.
if they are dependent, then why not control the device creation of 
dell-wmi and dell-smbios-wmi drivers in del-wmi-descriptor probe ?

> 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:
> -ENODEV: Descriptor GUID missing from WMI bus
> -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     |  5 +++--
>   drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
>   drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
>   drivers/platform/x86/dell-wmi.c            |  6 ++++--
>   4 files changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
> index 35c13815b24c..ca556803c8c9 100644
> --- a/drivers/platform/x86/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell-smbios-wmi.c
> @@ -148,8 +148,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>   	int count;
>   	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 wmi_smbios_priv),
>   			    GFP_KERNEL);
> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
> index 28ef5f37cfbf..4dfef1f53481 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> @@ -21,14 +21,26 @@
>   #include <linux/wmi.h>
>   #include "dell-wmi-descriptor.h"
>   
> +#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +
>   struct descriptor_priv {
>   	struct list_head list;
>   	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)
> +{
> +	if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> +		return -ENODEV;
> +
> +	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 +103,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 +115,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 +125,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..1e8cb96ffd78 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.h
> +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> @@ -13,7 +13,13 @@
>   
>   #include <linux/wmi.h>
>   
> -#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +/* possible return values:
> + *  -ENODEV: Descriptor GUID missing from WMI bus
> + *  -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..39d2f4518483 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,9 +655,11 @@ 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);

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

RE: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
> owner@vger.kernel.org] On Behalf Of sathyanarayanan kuppuswamy
> Sent: Thursday, November 9, 2017 12:51 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; 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
> Subject: Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to
> dependent drivers
> 
> Hi,
> 
> 
> On 11/09/2017 09:49 AM, Mario Limonciello wrote:
> > dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
> > finishing probe successfully to probe themselves.
> if they are dependent, then why not control the device creation of
> dell-wmi and dell-smbios-wmi drivers in del-wmi-descriptor probe ?

The dependency is just in a function call that dell-wmi-descriptor provides
information to the other drivers.

Each of these is a driver that binds to a GUID on the WMI bus.
They can each be built/configured independently of one another and also 
independently bound or unbound to a GUID on the WMI bus. 

Since these are bus drivers, the WMI bus will control creation of devices
when probe routines are run.

> 
> > 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:
> > -ENODEV: Descriptor GUID missing from WMI bus
> > -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     |  5 +++--
> >   drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
> >   drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
> >   drivers/platform/x86/dell-wmi.c            |  6 ++++--
> >   4 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-
> smbios-wmi.c
> > index 35c13815b24c..ca556803c8c9 100644
> > --- a/drivers/platform/x86/dell-smbios-wmi.c
> > +++ b/drivers/platform/x86/dell-smbios-wmi.c
> > @@ -148,8 +148,9 @@ static int dell_smbios_wmi_probe(struct wmi_device
> *wdev)
> >   	int count;
> >   	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 wmi_smbios_priv),
> >   			    GFP_KERNEL);
> > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
> b/drivers/platform/x86/dell-wmi-descriptor.c
> > index 28ef5f37cfbf..4dfef1f53481 100644
> > --- a/drivers/platform/x86/dell-wmi-descriptor.c
> > +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> > @@ -21,14 +21,26 @@
> >   #include <linux/wmi.h>
> >   #include "dell-wmi-descriptor.h"
> >
> > +#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
> B622A1EF5492"
> > +
> >   struct descriptor_priv {
> >   	struct list_head list;
> >   	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)
> > +{
> > +	if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> > +		return -ENODEV;
> > +
> > +	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 +103,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 +115,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 +125,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..1e8cb96ffd78 100644
> > --- a/drivers/platform/x86/dell-wmi-descriptor.h
> > +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> > @@ -13,7 +13,13 @@
> >
> >   #include <linux/wmi.h>
> >
> > -#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
> B622A1EF5492"
> > +/* possible return values:
> > + *  -ENODEV: Descriptor GUID missing from WMI bus
> > + *  -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..39d2f4518483 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -655,9 +655,11 @@ 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);
> 
> --
> Sathyanarayanan Kuppuswamy
> Linux kernel developer

Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[sathyanarayanan kuppuswamy]

Hi,


On 11/09/2017 11:00 AM, Mario.Limonciello@dell.com wrote:
>
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org [mailto:platform-driver-x86-
>> owner@vger.kernel.org] On Behalf Of sathyanarayanan kuppuswamy
>> Sent: Thursday, November 9, 2017 12:51 PM
>> To: Limonciello, Mario <Mario_Limonciello@Dell.com>; 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
>> Subject: Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to
>> dependent drivers
>>
>> Hi,
>>
>>
>> On 11/09/2017 09:49 AM, Mario Limonciello wrote:
>>> dell-wmi and dell-smbios-wmi are dependent upon dell-wmi-descriptor
>>> finishing probe successfully to probe themselves.
>> if they are dependent, then why not control the device creation of
>> dell-wmi and dell-smbios-wmi drivers in del-wmi-descriptor probe ?
> The dependency is just in a function call that dell-wmi-descriptor provides
> information to the other drivers.
Got it. Thanks for the clarification.
>
> Each of these is a driver that binds to a GUID on the WMI bus.
> They can each be built/configured independently of one another and also
> independently bound or unbound to a GUID on the WMI bus.
>
> Since these are bus drivers, the WMI bus will control creation of devices
> when probe routines are run.
>
>>> 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:
>>> -ENODEV: Descriptor GUID missing from WMI bus
>>> -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     |  5 +++--
>>>    drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
>>>    drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
>>>    drivers/platform/x86/dell-wmi.c            |  6 ++++--
>>>    4 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-
>> smbios-wmi.c
>>> index 35c13815b24c..ca556803c8c9 100644
>>> --- a/drivers/platform/x86/dell-smbios-wmi.c
>>> +++ b/drivers/platform/x86/dell-smbios-wmi.c
>>> @@ -148,8 +148,9 @@ static int dell_smbios_wmi_probe(struct wmi_device
>> *wdev)
>>>    	int count;
>>>    	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 wmi_smbios_priv),
>>>    			    GFP_KERNEL);
>>> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c
>> b/drivers/platform/x86/dell-wmi-descriptor.c
>>> index 28ef5f37cfbf..4dfef1f53481 100644
>>> --- a/drivers/platform/x86/dell-wmi-descriptor.c
>>> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
>>> @@ -21,14 +21,26 @@
>>>    #include <linux/wmi.h>
>>>    #include "dell-wmi-descriptor.h"
>>>
>>> +#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
>> B622A1EF5492"
>>> +
>>>    struct descriptor_priv {
>>>    	struct list_head list;
>>>    	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)
>>> +{
>>> +	if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
>>> +		return -ENODEV;
>>> +
>>> +	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 +103,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 +115,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 +125,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..1e8cb96ffd78 100644
>>> --- a/drivers/platform/x86/dell-wmi-descriptor.h
>>> +++ b/drivers/platform/x86/dell-wmi-descriptor.h
>>> @@ -13,7 +13,13 @@
>>>
>>>    #include <linux/wmi.h>
>>>
>>> -#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-
>> B622A1EF5492"
>>> +/* possible return values:
>>> + *  -ENODEV: Descriptor GUID missing from WMI bus
>>> + *  -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..39d2f4518483 100644
>>> --- a/drivers/platform/x86/dell-wmi.c
>>> +++ b/drivers/platform/x86/dell-wmi.c
>>> @@ -655,9 +655,11 @@ 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);
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux kernel developer

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer

Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Darren Hart]

On Thu, Nov 09, 2017 at 11:49:10AM -0600, 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:
> -ENODEV: Descriptor GUID missing from WMI bus
> -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.


This is a good improvement. Will give Pali a chance to offer his signature, and
will merge then.
-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2 2/2] platform/x86: dell-*wmi*: Relay failed initial probe to dependent drivers

[Pali Rohár]

On Thursday 09 November 2017 11:49:10 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:
> -ENODEV: Descriptor GUID missing from WMI bus
> -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     |  5 +++--
>  drivers/platform/x86/dell-wmi-descriptor.c | 16 ++++++++++++++++
>  drivers/platform/x86/dell-wmi-descriptor.h |  8 +++++++-
>  drivers/platform/x86/dell-wmi.c            |  6 ++++--
>  4 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
> index 35c13815b24c..ca556803c8c9 100644
> --- a/drivers/platform/x86/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell-smbios-wmi.c
> @@ -148,8 +148,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>  	int count;
>  	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 wmi_smbios_priv),
>  			    GFP_KERNEL);
> diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
> index 28ef5f37cfbf..4dfef1f53481 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell-wmi-descriptor.c
> @@ -21,14 +21,26 @@
>  #include <linux/wmi.h>
>  #include "dell-wmi-descriptor.h"
>  
> +#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +
>  struct descriptor_priv {
>  	struct list_head list;
>  	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)
> +{
> +	if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID))
> +		return -ENODEV;
> +
> +	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 +103,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 +115,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 +125,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..1e8cb96ffd78 100644
> --- a/drivers/platform/x86/dell-wmi-descriptor.h
> +++ b/drivers/platform/x86/dell-wmi-descriptor.h
> @@ -13,7 +13,13 @@
>  
>  #include <linux/wmi.h>
>  
> -#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
> +/* possible return values:
> + *  -ENODEV: Descriptor GUID missing from WMI bus
> + *  -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..39d2f4518483 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -655,9 +655,11 @@ 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);

Looks good,

Reviewed-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v2 0/2] Account for uncorrectable failures in probing

[Darren Hart]

On Thu, Nov 09, 2017 at 11:49:08AM -0600, Mario Limonciello wrote:
> 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.

Queued, thanks Mario.

-- 
Darren Hart
VMware Open Source Technology Center