Re: [PATCH 3/5] platform/x86: dell-smbios-wmi: Stop using WMI chardev

[Armin Wolf]

Am 08.12.23 um 14:41 schrieb Ilpo Järvinen:

> On Thu, 7 Dec 2023, Armin Wolf wrote:
>
> > The WMI chardev API will be removed in the near future.
> > Reimplement the necessary bits used by this driver so
> > that userspace software depending on it does no break.
> >
> > Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> > ---
> >   drivers/platform/x86/dell/dell-smbios-wmi.c | 163 ++++++++++++++------
> >   1 file changed, 117 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
> > index 931cc50136de..61f40f462eca 100644
> > --- a/drivers/platform/x86/dell/dell-smbios-wmi.c
> > +++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
> > @@ -32,7 +35,9 @@ struct wmi_smbios_priv {
> >   	struct list_head list;
> >   	struct wmi_device *wdev;
> >   	struct device *child;
> > -	u32 req_buf_size;
> > +	u64 req_buf_size;
> > +	u32 hotfix;
> > +	struct miscdevice char_dev;
> >   };
> >   static LIST_HEAD(wmi_list);
>
> >   static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
> >   {
> > -	struct wmi_driver *wdriver =
> > -		container_of(wdev->dev.driver, struct wmi_driver, driver);
> >   	struct wmi_smbios_priv *priv;
> > -	u32 hotfix;
> > +	u32 buffer_size;
> >   	int count;
> >   	int ret;
> >
> > @@ -162,39 +225,44 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
> >   	if (!priv)
> >   		return -ENOMEM;
> >
> > +	priv->wdev = wdev;
> > +	dev_set_drvdata(&wdev->dev, priv);
> > +
> >   	/* WMI buffer size will be either 4k or 32k depending on machine */
> > -	if (!dell_wmi_get_size(&priv->req_buf_size))
> > +	if (!dell_wmi_get_size(&buffer_size))
> >   		return -EPROBE_DEFER;
> >
> > +	priv->req_buf_size = buffer_size;
> > +
> >   	/* some SMBIOS calls fail unless BIOS contains hotfix */
> > -	if (!dell_wmi_get_hotfix(&hotfix))
> > +	if (!dell_wmi_get_hotfix(&priv->hotfix))
> >   		return -EPROBE_DEFER;
> > -	if (!hotfix) {
> > +
> > +	if (!priv->hotfix)
> >   		dev_warn(&wdev->dev,
> >   			"WMI SMBIOS userspace interface not supported(%u), try upgrading to a newer BIOS\n",
> > -			hotfix);
> > -		wdriver->filter_callback = NULL;
> > -	}
> > +			priv->hotfix);
> >
> >   	/* add in the length object we will use internally with ioctl */
> >   	priv->req_buf_size += sizeof(u64);
> > -	ret = set_required_buffer_size(wdev, priv->req_buf_size);
> > -	if (ret)
> > -		return ret;
> >
> >   	count = get_order(priv->req_buf_size);
> >   	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
> >   	if (!priv->buf)
> >   		return -ENOMEM;
> >
> > +	if (priv->hotfix) {
> > +		ret = dell_smbios_wmi_register_chardev(priv);
> > +		if (ret)
> > +			goto fail_chardev;
> > +	}
> > +
> >   	/* ID is used by dell-smbios to set priority of drivers */
> >   	wdev->dev.id = 1;
> >   	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
> >   	if (ret)
> >   		goto fail_register;
> >
> > -	priv->wdev = wdev;
> > -	dev_set_drvdata(&wdev->dev, priv);
> >   	mutex_lock(&list_mutex);
> >   	list_add_tail(&priv->list, &wmi_list);
> >   	mutex_unlock(&list_mutex);
> > @@ -202,6 +270,9 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev, const void *context)
> >   	return 0;
> >
> >   fail_register:
> > +	if (priv->hotfix)
> > +               dell_smbios_wmi_unregister_chardev(priv);
> I don't understand how hotfix -> priv->hotfix is related to this patch nor
> why it's necessary?
>
> Or did you mean to use it also in dell_smbios_wmi_remove() but forgot to
> add the if (priv->hotfix) there?

I indeed forgot to add the "if (priv->hotfix)" here, good catch.

> In any case, it would be better to put that conversion into own patch
> before this one.

I could also drop the priv->hotfix related changes and instead modify the driver
to use devres (devm_get_free_pages() for example). This would also simplify the
error handling code.

I will send a v2 soon containing the necessary patches.

> > @@ -211,6 +282,7 @@ static void dell_smbios_wmi_remove(struct wmi_device *wdev)
> >         struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
> >         int count;
> >
> > +      dell_smbios_wmi_unregister_chardev(priv);
> >         mutex_lock(&call_mutex);
> >         mutex_lock(&list_mutex);
> >         list_del(&priv->list);