| Message ID | 20171031233146.GA5787@fury (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Tuesday 31 October 2017 16:31:46 Darren Hart wrote: > On Mon, Oct 30, 2017 at 01:32:57PM +0000, Mario.Limonciello@dell.com wrote: > > > > > > > -----Original Message----- > > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > > Sent: Monday, October 30, 2017 6:47 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; Andy > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > <gnomes@lxorguk.ukuu.org.uk> > > > Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI > > > descriptor into it's own driver > > > > > > On Friday 20 October 2017 12:40:20 Mario Limonciello wrote: > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c > > > b/drivers/platform/x86/dell-wmi-descriptor.c > > > > new file mode 100644 > > > > index 000000000000..3204c408e261 > > > > --- /dev/null > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c > > > > > > This dell-wmi-descriptor.c looks good now! > > > > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h > > > b/drivers/platform/x86/dell-wmi-descriptor.h > > > > new file mode 100644 > > > > index 000000000000..5f7b69c2c83a > > > > --- /dev/null > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.h > > > > @@ -721,7 +652,9 @@ static int dell_wmi_events_set_enabled(bool enable) > > > > static int dell_wmi_probe(struct wmi_device *wdev) > > > > { > > > > struct dell_wmi_priv *priv; > > > > - int err; > > > > + > > > > + if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID)) > > > > + return -ENODEV; > > > > > > > > priv = devm_kzalloc( > > > > &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL); > > > > @@ -729,9 +662,8 @@ static int dell_wmi_probe(struct wmi_device *wdev) > > > > return -ENOMEM; > > > > dev_set_drvdata(&wdev->dev, priv); > > > > > > > > - err = dell_wmi_check_descriptor_buffer(wdev); > > > > - if (err) > > > > - return err; > > > > + if (!dell_wmi_get_interface_version(&priv->interface_version)) > > > > + return -EPROBE_DEFER; > > > > > > But here is still a problem. You added check that > > > DELL_WMI_DESCRIPTOR_GUID exists in APCI table, but it does not mean that > > > probe method of dell-wmi-descriptor cannot fail. > > > > > > With PROBE_DEFER, dell_wmi_probe function would be called later again > > > and again, even when probing dell-wmi-descriptor failed and so dell-wmi > > > in this case cannot work. > > > > > > > Yes it's possible that probe method can fail, but it depends on the reason for > > failure if it will fail again later. For example if not enough memory, it may work > > later. Or maybe user manually unbound from GUID, should continue to try until > > it's bound again. > > > > So in short, I believe this is the correct behavior to adopt. > > In this case, I believe the synchronous request_module("dell-wmi-descriptor") > would provide the desired result. The exit status doesn't even need to be > checked. If that is successful, and the interface_version still returns false, > then it can be considered an error and we can exit. If it fails, the > interface_version will return false, and it is also an error. > > This can be easily added as a single patch on top of this series: > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > index dcfa5de..964ca54 100644 > --- a/drivers/platform/x86/dell-wmi.c > +++ b/drivers/platform/x86/dell-wmi.c > @@ -665,8 +665,9 @@ static int dell_wmi_probe(struct wmi_device *wdev) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, priv); > > + request_module("dell-wmi-descriptor"); > if (!dell_wmi_get_interface_version(&priv->interface_version)) > - return -EPROBE_DEFER; > + return -ENODEV; > > return dell_wmi_input_setup(wdev); > } > > Pali, I believe this addresses your concern? I'm not sure what happen if both drivers are statically linked into vmlinuz and dell-wmi probe method would be called before dell-wmi-descriptor method. What is doing request_module when requested module is statically linked into vmlinuz?
> -----Original Message----- > From: Pali Rohár [mailto:pali.rohar@gmail.com] > Sent: Wednesday, November 1, 2017 3:19 AM > To: Darren Hart <dvhart@infradead.org> > Cc: Limonciello, Mario <Mario_Limonciello@Dell.com>; > andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver- > x86@vger.kernel.org; luto@kernel.org; quasisec@google.com; rjw@rjwysocki.net; > mjg59@google.com; hch@lst.de; greg@kroah.com; gnomes@lxorguk.ukuu.org.uk > Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI > descriptor into it's own driver > > On Tuesday 31 October 2017 16:31:46 Darren Hart wrote: > > On Mon, Oct 30, 2017 at 01:32:57PM +0000, Mario.Limonciello@dell.com wrote: > > > > > > > > > > -----Original Message----- > > > > From: Pali Rohár [mailto:pali.rohar@gmail.com] > > > > Sent: Monday, October 30, 2017 6:47 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; > Andy > > > > Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net; > > > > mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>; Alan Cox > > > > <gnomes@lxorguk.ukuu.org.uk> > > > > Subject: Re: [PATCH v11 05/15] platform/x86: dell-wmi-descriptor: split WMI > > > > descriptor into it's own driver > > > > > > > > On Friday 20 October 2017 12:40:20 Mario Limonciello wrote: > > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.c > > > > b/drivers/platform/x86/dell-wmi-descriptor.c > > > > > new file mode 100644 > > > > > index 000000000000..3204c408e261 > > > > > --- /dev/null > > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.c > > > > > > > > This dell-wmi-descriptor.c looks good now! > > > > > > > > > diff --git a/drivers/platform/x86/dell-wmi-descriptor.h > > > > b/drivers/platform/x86/dell-wmi-descriptor.h > > > > > new file mode 100644 > > > > > index 000000000000..5f7b69c2c83a > > > > > --- /dev/null > > > > > +++ b/drivers/platform/x86/dell-wmi-descriptor.h > > > > > @@ -721,7 +652,9 @@ static int dell_wmi_events_set_enabled(bool > enable) > > > > > static int dell_wmi_probe(struct wmi_device *wdev) > > > > > { > > > > > struct dell_wmi_priv *priv; > > > > > - int err; > > > > > + > > > > > + if (!wmi_has_guid(DELL_WMI_DESCRIPTOR_GUID)) > > > > > + return -ENODEV; > > > > > > > > > > priv = devm_kzalloc( > > > > > &wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL); > > > > > @@ -729,9 +662,8 @@ static int dell_wmi_probe(struct wmi_device > *wdev) > > > > > return -ENOMEM; > > > > > dev_set_drvdata(&wdev->dev, priv); > > > > > > > > > > - err = dell_wmi_check_descriptor_buffer(wdev); > > > > > - if (err) > > > > > - return err; > > > > > + if (!dell_wmi_get_interface_version(&priv->interface_version)) > > > > > + return -EPROBE_DEFER; > > > > > > > > But here is still a problem. You added check that > > > > DELL_WMI_DESCRIPTOR_GUID exists in APCI table, but it does not mean that > > > > probe method of dell-wmi-descriptor cannot fail. > > > > > > > > With PROBE_DEFER, dell_wmi_probe function would be called later again > > > > and again, even when probing dell-wmi-descriptor failed and so dell-wmi > > > > in this case cannot work. > > > > > > > > > > Yes it's possible that probe method can fail, but it depends on the reason for > > > failure if it will fail again later. For example if not enough memory, it may work > > > later. Or maybe user manually unbound from GUID, should continue to try until > > > it's bound again. > > > > > > So in short, I believe this is the correct behavior to adopt. > > > > In this case, I believe the synchronous request_module("dell-wmi-descriptor") > > would provide the desired result. The exit status doesn't even need to be > > checked. If that is successful, and the interface_version still returns false, > > then it can be considered an error and we can exit. If it fails, the > > interface_version will return false, and it is also an error. > > > > This can be easily added as a single patch on top of this series: > > > > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c > > index dcfa5de..964ca54 100644 > > --- a/drivers/platform/x86/dell-wmi.c > > +++ b/drivers/platform/x86/dell-wmi.c > > @@ -665,8 +665,9 @@ static int dell_wmi_probe(struct wmi_device *wdev) > > return -ENOMEM; > > dev_set_drvdata(&wdev->dev, priv); > > > > + request_module("dell-wmi-descriptor"); > > if (!dell_wmi_get_interface_version(&priv->interface_version)) > > - return -EPROBE_DEFER; > > + return -ENODEV; > > > > return dell_wmi_input_setup(wdev); > > } > > > > Pali, I believe this addresses your concern? This proposed approach would actually introduce a problem of when a device is unbound on the dell-wmi-descriptor then you bind dell-wmi now it's possible to get into a state of probe routine failed on dell-wmi and can't be fixed without unbind dell-wmi, bind dell-wmi-descriptor and then rebind dell-wmi. With deferred probing solution I had in place that above problem can't happen because probing would just re-run. > > I'm not sure what happen if both drivers are statically linked into > vmlinuz and dell-wmi probe method would be called before > dell-wmi-descriptor method. That's why this is better to do using deferred probing I think still. > > What is doing request_module when requested module is statically linked > into vmlinuz? > As I can tell request_module actually calls out to userspace to run modprobe on the requested module. Something that may help with Pali's concern is to in the probe routine for dell-wmi-descritptor set something in a global variable for an error code and have a function to look that up. In the error scenario for a non memory allocation error set that value. Something like this: /* valid = 0 means probe hasn't run, valid = 1 means successful probe, * valid < 0 means failed probe due to bad device */ static int dell_wmi_descriptor_valid(void) { int ret; mutex_lock(&list_mutex); if (valid == 0) ret = -EPROBE_DEFER; else if ret = valid; mutex_unlock(&list_mutex) return ret; } Then dell-wmi and dell-smbios-wmi can call that before dell_wmi_get_interface_version. Does that satisfy Pali's concern? I may follow up with this particular item in a later patch so as to not derail current efforts unless Darren or Andy would like this sorted ASAP or I get through testing it quickly.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index dcfa5de..964ca54 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -665,8 +665,9 @@ static int dell_wmi_probe(struct wmi_device *wdev) return -ENOMEM; dev_set_drvdata(&wdev->dev, priv); + request_module("dell-wmi-descriptor"); if (!dell_wmi_get_interface_version(&priv->interface_version)) - return -EPROBE_DEFER; + return -ENODEV; return dell_wmi_input_setup(wdev); }