Hi James,

On 9/19/20 2:12 AM, James Morse wrote:

On 30/07/2020 02:45, Gavin Shan wrote:

quoted

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.

quoted

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

(according)

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

quoted

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


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