[PATCH v4 05/15] drivers/firmware/sdei: Unregister driver on error in sdei_init()

[James Morse]

Hi Gavin,

On 30/07/2020 02:45, Gavin Shan wrote:
> The platform driver needs to be unregistered on error to create the
> platform device, so that the state is restored to original one.

Why is this important? Is it just symmetry? 'needs' causes me to look at this as a bug-fix.

DT systems leave a dangling platform device in this case. Is that a problem?
See commit 3aa0582fdb82 ("of: platform: populate /firmware/ node from
of_platform_default_populate_init()").

On DT systems, sdei_init() doesn't create the platform device, so it doesn't remove it
either. ACPI leaves it dangling because ... why should ACPI behave differently?


> Besides, the errno (@ret) should be updated acccording in that case.

(according)


> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index c9f2bedfb6b5..909f27abf8a7 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -1090,9 +1090,12 @@ static int __init sdei_init(void)
>  
>  	pdev = platform_device_register_simple(sdei_driver.driver.name,
>  					       0, NULL, 0);
> -	if (IS_ERR(pdev))
> -		pr_info("Failed to register ACPI:SDEI platform device %ld\n",
> -			PTR_ERR(pdev));
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		platform_driver_unregister(&sdei_driver);
> +		pr_info("Failed to register ACPI:SDEI platform device %d\n",
> +			ret);
> +	}
>  
>  	return ret;
>  }

If the argument here is about symmetry, sure. Please stuff sneak the word symmetry into
the commit message - I keep reading this as if its a bug. Its probably worth a comment in
the commit message that its only like this for ACPI. Previously the motivation was to keep
these things as similar as possible.

With that:
Reviewed-by: James Morse <james.morse at arm.com>


Thanks,

James