Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Date:   Mon, 9 Oct 2023 16:19:41 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Armin Wolf <W_Armin@gmx.de>
cc:     Hans de Goede <hdegoede@redhat.com>, markgross@kernel.org,
        platform-driver-x86@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/6] platform/x86: wmi: Decouple probe deferring from
 wmi_block_list
In-Reply-To: <20231007233933.72121-3-W_Armin@gmx.de>
Message-ID: <9e3c21ec-9b57-5ee-fc1d-8911b0a93cc@linux.intel.com>
References: <20231007233933.72121-1-W_Armin@gmx.de> <20231007233933.72121-3-W_Armin@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sun, 8 Oct 2023, Armin Wolf wrote:

> Many aggregate WMI drivers do not use -EPROBE_DEFER when they
> cannot find a WMI device during probe, instead they require
> all WMI devices associated with an platform device to become
> available at once. This is currently achieved by adding those
> WMI devices to the wmi_block_list before they are registered,
> which is then used by the deprecated GUID-based functions to
> search for WMI devices.
> Replace this approach with a device link which defers probing

To make these more readable, you should break the paragraphs with an empty 
line, not just using a newline.

-- 
 i.

> of the WMI device until the associated platform device has finished
> probing (and has registered all WMI devices). New aggregate WMI
> drivers should not rely on this behaviour.


> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 39 +++++++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index a78ddd83cda0..1dbef16acdeb 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1221,6 +1221,26 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>  	return 0;
>  }
> 
> +static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
> +{
> +	struct device_link *link;
> +
> +	/*
> +	 * Many aggregate WMI drivers do not use -EPROBE_DEFER when they
> +	 * are unable to find a WMI device during probe, instead they require
> +	 * all WMI devices associated with an platform device to become available
> +	 * at once. This device link thus prevents WMI drivers from probing until
> +	 * the associated platform device has finished probing (and has registered
> +	 * all discovered WMI devices).
> +	 */
> +
> +	link = device_link_add(&wdev->dev, &pdev->dev, DL_FLAG_AUTOREMOVE_SUPPLIER);
> +	if (!link)
> +		return -EINVAL;
> +
> +	return device_add(&wdev->dev);
> +}
> +
>  static void wmi_free_devices(struct acpi_device *device)
>  {
>  	struct wmi_block *wblock, *next;
> @@ -1263,11 +1283,12 @@ static bool guid_already_parsed_for_legacy(struct acpi_device *device, const gui
>  /*
>   * Parse the _WDG method for the GUID data blocks
>   */
> -static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
> +static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  {
> +	struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
>  	struct acpi_buffer out = {ACPI_ALLOCATE_BUFFER, NULL};
>  	const struct guid_block *gblock;
> -	struct wmi_block *wblock, *next;
> +	struct wmi_block *wblock;
>  	union acpi_object *obj;
>  	acpi_status status;
>  	int retval = 0;
> @@ -1317,22 +1338,14 @@ static int parse_wdg(struct device *wmi_bus_dev, struct acpi_device *device)
>  			wblock->handler = wmi_notify_debug;
>  			wmi_method_enable(wblock, true);
>  		}
> -	}
> 
> -	/*
> -	 * Now that all of the devices are created, add them to the
> -	 * device tree and probe subdrivers.
> -	 */
> -	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
> -		if (wblock->acpi_device != device)
> -			continue;
> -
> -		retval = device_add(&wblock->dev.dev);
> +		retval = wmi_add_device(pdev, &wblock->dev);
>  		if (retval) {
>  			dev_err(wmi_bus_dev, "failed to register %pUL\n",
>  				&wblock->gblock.guid);
>  			if (debug_event)
>  				wmi_method_enable(wblock, false);
> +
>  			list_del(&wblock->list);
>  			put_device(&wblock->dev.dev);
>  		}
> @@ -1487,7 +1500,7 @@ static int acpi_wmi_probe(struct platform_device *device)
>  	}
>  	dev_set_drvdata(&device->dev, wmi_bus_dev);
> 
> -	error = parse_wdg(wmi_bus_dev, acpi_device);
> +	error = parse_wdg(wmi_bus_dev, device);
>  	if (error) {
>  		pr_err("Failed to parse WDG method\n");
>  		goto err_remove_busdev;
> --
> 2.39.2
>