Re: [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init()

From: James Morse <james.morse@arm.com>
To: Gavin Shan <gshan@redhat.com>
Cc: mark.rutland@arm.com, catalin.marinas@arm.com, will@kernel.org,
	shan.gavin@gmail.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 04/14] drivers/firmware/sdei: Rework sdei_init()
Date: Tue, 21 Jul 2020 21:42:34 +0100	[thread overview]
Message-ID: <d69c3727-c032-521b-c32e-59abe491fc63@arm.com> (raw)
In-Reply-To: <20200706054732.99387-5-gshan@redhat.com>

Hi Gavin,

On 06/07/2020 06:47, Gavin Shan wrote:
> This reworks sdei_init()
> 
>    * The function follows the steps in sequence: check ACPI existence,
>      register platform device, register platform driver.
>    * The corresponding error numbers are returned in failing paths.
>    * The platform device is deleted if the driver can't be registered.

What is your motivation for the change?

The commit message should describe the problem you are solving, and why you are doing it
this way.

> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
> index 35a319e7e1e6..7e7b26b1f91b 100644
> --- a/drivers/firmware/arm_sdei.c
> +++ b/drivers/firmware/arm_sdei.c
> @@ -1058,7 +1058,7 @@ static bool __init sdei_present_acpi(void)
>  	acpi_status status;
>  	struct acpi_table_header *sdei_table_header;
>  
> -	if (acpi_disabled)
> +	if (!IS_ENABLED(CONFIG_ACPI) || acpi_disabled)
>  		return false;
>  

This was already covered. From include/linux/acpi.h:
| #else	/* !CONFIG_ACPI */
|
| #define acpi_disabled 1

>  	status = acpi_get_table(ACPI_SIG_SDEI, 0, &sdei_table_header);
> @@ -1077,19 +1077,27 @@ static bool __init sdei_present_acpi(void)
>  
>  static int __init sdei_init(void)
>  {
> -	int ret = platform_driver_register(&sdei_driver);
> +	struct platform_device *pdev;
> +	int ret;
>  
> -	if (!ret && sdei_present_acpi()) {
> -		struct platform_device *pdev;
> +	if (!sdei_present_acpi())
> +		return -EPERM;
>  
> -		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));

> +	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));
> +		return -ENOMEM;
>  	}

This will break DT platforms. Not everything in the world is ACPI.

> -	return ret;
> +	ret = platform_driver_register(&sdei_driver);
> +	if (ret) {
> +		platform_device_del(pdev);

The platform device is not unregistered on APCI platforms because it does not get
unregistered on DT platforms either. When using DT its created by the OF core code when it
probes the '/firmware' node of the DT.

> +		return ret;
> +	}
> +
> +	return 0;
>  }

Without a motivation in the commit message, I can't work out if there is something here,
or its just churn.

Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel