diff mbox series

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

Message ID 20200730014531.310465-6-gshan@redhat.com (mailing list archive)
State New, archived
Headers show
Series Refactor SDEI client driver | expand

Commit Message

Gavin Shan July 30, 2020, 1:45 a.m. UTC 
The platform driver needs to be unregistered on error to create the
platform device, so that the state is restored to original one.

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

Signed-off-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/firmware/arm_sdei.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

James Morse Sept. 18, 2020, 4:12 p.m. UTC | #1 
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@arm.com>


Thanks,

James
Gavin Shan Sept. 20, 2020, 1:10 a.m. UTC | #2 
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@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
diff mbox series

Patch

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