Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Thu, 12 Oct 2023 19:32:20 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Armin Wolf <W_Armin@gmx.de>, Hans de Goede <hdegoede@redhat.com>
cc:     markgross@kernel.org, platform-driver-x86@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/6] platform/x86: wmi: Fix probe failure when failing
 to register WMI devices
In-Reply-To: <20231007233933.72121-5-W_Armin@gmx.de>
Message-ID: <6fc5eacc-15e7-8de4-a031-d57ae27568bd@linux.intel.com>
References: <20231007233933.72121-1-W_Armin@gmx.de> <20231007233933.72121-5-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:

> When a WMI device besides the first one somehow fails to register, retval
> is returned while still containing a negative error code. This causes the
> ACPI device failing to probe, leaving behind zombie WMI devices leading
> to various errors later.
> Fix this by handling the single error path separately and return 0 after
> trying to register all WMI devices. Also continue to register WMI devices
> even if some fail to allocate.

I think the usual approach would be to unroll all registerations done so 
far when an error occurs while registering n devices.

Do you Hans have something to add what would be the best course of action 
here?

-- 
 i.

> Fixes: 6ee50aaa9a20 ("platform/x86: wmi: Instantiate all devices before adding them")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/wmi.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index e3984801883a..ab24ea9ffc9a 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1338,8 +1338,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  	struct wmi_block *wblock;
>  	union acpi_object *obj;
>  	acpi_status status;
> -	int retval = 0;
>  	u32 i, total;
> +	int retval;
> 
>  	status = acpi_evaluate_object(device->handle, "_WDG", NULL, &out);
>  	if (ACPI_FAILURE(status))
> @@ -1350,8 +1350,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  		return -ENXIO;
> 
>  	if (obj->type != ACPI_TYPE_BUFFER) {
> -		retval = -ENXIO;
> -		goto out_free_pointer;
> +		kfree(obj);
> +		return -ENXIO;
>  	}
> 
>  	gblock = (const struct guid_block *)obj->buffer.pointer;
> @@ -1366,8 +1366,8 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
> 
>  		wblock = kzalloc(sizeof(*wblock), GFP_KERNEL);
>  		if (!wblock) {
> -			retval = -ENOMEM;
> -			break;
> +			dev_err(wmi_bus_dev, "Failed to allocate %pUL\n", &gblock[i].guid);
> +			continue;
>  		}
> 
>  		wblock->acpi_device = device;
> @@ -1398,9 +1398,9 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>  		}
>  	}
> 
> -out_free_pointer:
> -	kfree(out.pointer);
> -	return retval;
> +	kfree(obj);
> +
> +	return 0;
>  }
> 
>  /*
> --
> 2.39.2
>