RE: [PATCH v9 05/17] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver

[Mario.Limonciello]

> -----Original Message-----
> From: Pali RohÃr [mailto:pali.rohar@xxxxxxxxx]
> Sent: Tuesday, October 17, 2017 1:59 PM
> 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 v9 05/17] platform/x86: dell-wmi-descriptor: split WMI
> descriptor into it's own driver
> 
> On Tuesday 17 October 2017 13:21:49 Mario Limonciello wrote:
> > +struct descriptor_priv {
> > +	struct list_head list;
> > +	u32 interface_version;
> > +	u32 size;
> > +};
> > +static LIST_HEAD(wmi_list);
> > +
> > +bool dell_wmi_get_interface_version(u32 *version)
> > +{
> > +	struct descriptor_priv *priv;
> > +
> > +	priv = list_first_entry_or_null(&wmi_list,
> > +					struct descriptor_priv,
> > +					list);
> > +	if (!priv)
> > +		return false;
> > +	*version = priv->interface_version;
> 
> There is a race condition. dell_wmi_descriptor_remove can be called
> between list_first_entry_or_null and dereferencing priv pointer.
> 

OK I'll add a mutex for this.

> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_wmi_get_interface_version);
> > +
> > +bool dell_wmi_get_size(u32 *size)
> > +{
> > +	struct descriptor_priv *priv;
> > +
> > +	priv = list_first_entry_or_null(&wmi_list,
> > +					struct descriptor_priv,
> > +					list);
> > +	if (!priv)
> > +		return false;
> > +	*size = priv->size;
> 
> And same there.

OK I'll add a mutex for this.

> 
> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(dell_wmi_get_size);
> 
> ...
> 
> > @@ -733,9 +659,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;
> 
> This could lead to another problem, when Dell decide to change WMI API
> and would not provide descriptor WMI GUID anymore, but still provide
> even WMI GUID.

This is getting into territory of "undefined new stuff" how can the kernel
know what to do with hardware that doesn't yet exist?
Anyway, its fine to think about.

> 
> Basically it is needed to distinguish between states:
> 
> 1) probe function of dell-wmi was called before probe function of
>    dell-wmi-descriptor device initialization
> 
> 2) probe function of dell-wmi was called, but there is no device
>    instance of dell-wmi-descriptor
> 
> 3) there is a device instance of dell-wmi-descriptor, but device is not
>    registered to dell-wmi-descriptor driver, e.g. because userspace
>    decided to forbid such thing, or because probing of
>    dell-wmi-descriptor device failed
> 
> 4) probe function of dell-wmi was called after probe function of
>    dell-wmi-descriptor successfully
> 
> I do not know how to handle such situation other drivers or how to do it
> correctly. I just wanted to show the fact that binding device <-->
> driver can fail in linux kernel (for more reasons) and in some cases
> repeating it does not make sense...
> 
> Maybe other developers would comment this part?

So I think the simple answer to this is to  use wmi_has_guid to determine
if the GUID that we're depending on exists on the bus.
If it doesn't exist abort the driver during probing.

If the device exists on the bus then addressing each state: 
<1> is solved by deferred probing
<2> won't happen
<3> dell-wmi should continue to wait in deferred probing in case 
userspace decides to bind later or probing succeeded later.
<4> is the good situation