| Message ID | 6312b76d08d3b6b5562c773fce8e13d85036ddd5.1509725778.git.mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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 > + */
> -----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
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)
> -----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
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
> -----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
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.
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)
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(+)