On Mon, Oct 30, 2017 at 01:32:57PM +0000, Mario.Limonciello@xxxxxxxx wrote: > > > > -----Original Message----- > > From: Pali Rohár [mailto:pali.rohar@xxxxxxxxx] > > Sent: Monday, October 30, 2017 6:47 AM > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx> > > Cc: dvhart@xxxxxxxxxxxxx; Andy Shevchenko <andy.shevchenko@xxxxxxxxx>; > > LKML <linux-kernel@xxxxxxxxxxxxxxx>; platform-driver-x86@xxxxxxxxxxxxxxx; Andy > > Lutomirski <luto@xxxxxxxxxx>; quasisec@xxxxxxxxxx; rjw@xxxxxxxxxxxxx; > > mjg59@xxxxxxxxxx; hch@xxxxxx; Greg KH <greg@xxxxxxxxx>; Alan Cox > > <gnomes@xxxxxxxxxxxxxxxxxxx> > > 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? -- Darren Hart VMware Open Source Technology Center
