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

[Gavin Shan]

Hi James,

On 9/19/20 2:12 AM, James Morse wrote:
> 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?
> 

It's all about symmetry because the platform driver has the owner,
but platform device doesn't have it. It means it's fine to keep
dangling device, but it's not reasonable to keep dangling driver.
However, this patch isn't a big deal though.

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

Thanks. It's fixed to "accordingly in this case" in next revision.

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

Sure. I will have something like below in the change log:

---

The SDEI platform device is created from device-tree node or ACPI
(SDEI) table. For the later case, the platform device is created
explicitly by this module. It'd better to unregister the driver on
failure to create the device to keep the symmetry. The driver, owned
by this module, isn't needed if the device isn't existing.

Besides, the errno (@ret) should be updated accordingly in this
case.

Cheers,
Gavin