| Message ID | 20200706054732.99387-5-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor SDEI client driver | expand |
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
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
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; 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; } - return ret; + ret = platform_driver_register(&sdei_driver); + if (ret) { + platform_device_del(pdev); + return ret; + } + + return 0; } /*
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. Signed-off-by: Gavin Shan <gshan@redhat.com> --- drivers/firmware/arm_sdei.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)