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

[PATCH 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 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

[PATCH 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>
---
 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 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:
-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

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

[Darren Hart]

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

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

[Mario.Limonciello]

> -----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

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

[Mario.Limonciello]

> -----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,

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

[Pali Rohár]

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

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

[Pali Rohár]

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

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

[Mario.Limonciello]

> -----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

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

[Pali Rohár]

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

RE: [PATCH 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 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

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

[Darren Hart]

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