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

[Gavin Shan]

Hi James,

On 7/22/20 6:42 AM, James Morse wrote:
> 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.
> 

Yep, the commit log isn't obvious. I will improve it in v2. I also
think it'd better to split the patch in v2.

drivers/firmware/sdei: Avoid nested statements in sdei_init()
drivers/firmware/sdei: Unregister driver on error in sdei_init()

There are 3 issues to be resolved and will be addressed in v2
commit log explicitly:

(1) The nest statements in sdei_init() can be avoided by bailing
     on error returned from platform_driver_register() or absent
     ACPI SDEI table.
(2) The platform driver isn't unregistered on failing to create
     the platform device. It means we're not restored to original
     state with this error. It's only true when ACPI is enabled,
     meaning we don't have the issue for DT case.
(3) The errno isn't updated accordingly on failing to create the
     platform device.

> 
>> 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
> 

Yeah, I was thinking @acpi_disable isn't defined when CONFIG_ACPI
is off. Thanks for the linker :)

> 
>>   	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.
> 

Correct, I will split this patch into two patches in v2. The
issue will be fixed there.

> 
>> -	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.
> 

Yes. I think the platform driver needs to be unregistered on
error to create the platform device when ACPI is enabled. With
it, we're restored to original state: no platform driver and
device.

For DT case, we needn't worry about the platform device because
it's not managed by the driver (arm_sdei.c).

> 
>> +		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.
> 

Sure, I will improve the commit log in v2 :)

Thanks,
Gavin