[PATCH] x86/platform/intel/iosf_mbi: Fix error handling in iosf_mbi_init()

[PATCH] x86/platform/intel/iosf_mbi: Fix error handling in iosf_mbi_init()

[Yuan Can]

A problem about modprobe iosf_mbi failed is triggered with the following
log given:

 debugfs: Directory 'iosf_sb' with parent '/' already present!

The reason is that iosf_mbi_init() returns pci_register_driver()
directly without checking its return value, if pci_register_driver()
failed, it returns without removing debugfs, resulting the debugfs of
iosf_sb can never be created later.

 iosf_mbi_init()
   iosf_mbi_dbg_init() # create debugfs
   pci_register_driver()
     driver_register()
       bus_add_driver()
         priv = kzalloc(...) # OOM happened
   # return without remove debugfs and destroy workqueue

Fix by remove debugfs and iosf_mbi_pm_qos when pci_register_driver()
returns error.

Fixes: 8dc12f933c9d ("x86/iosf: Add debugfs support")
Fixes: e09db3d241f8 ("x86: baytrail/cherrytrail: Rework and move P-Unit PMIC bus semaphore code")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 arch/x86/platform/intel/iosf_mbi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index fdd49d70b437..98632ac3db2f 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -545,11 +545,19 @@ static struct pci_driver iosf_mbi_pci_driver = {
 
 static int __init iosf_mbi_init(void)
 {
+	int ret;
+
 	iosf_debugfs_init();
 
 	cpu_latency_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
 
-	return pci_register_driver(&iosf_mbi_pci_driver);
+	ret = pci_register_driver(&iosf_mbi_pci_driver);
+	if (ret) {
+		cpu_latency_qos_remove_request(&iosf_mbi_pm_qos);
+		iosf_debugfs_remove();
+	}
+
+	return ret;
 }
 
 static void __exit iosf_mbi_exit(void)
-- 
2.17.1

Re: [PATCH] x86/platform/intel/iosf_mbi: Fix error handling in iosf_mbi_init()

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 9:38 AM Yuan Can <yuancan@huawei.com> wrote:
>
> A problem about modprobe iosf_mbi failed is triggered with the following
> log given:
>
>  debugfs: Directory 'iosf_sb' with parent '/' already present!
>
> The reason is that iosf_mbi_init() returns pci_register_driver()
> directly without checking its return value, if pci_register_driver()
> failed, it returns without removing debugfs, resulting the debugfs of
> iosf_sb can never be created later.
>
>  iosf_mbi_init()
>    iosf_mbi_dbg_init() # create debugfs
>    pci_register_driver()
>      driver_register()
>        bus_add_driver()
>          priv = kzalloc(...) # OOM happened
>    # return without remove debugfs and destroy workqueue
>
> Fix by remove debugfs and iosf_mbi_pm_qos when pci_register_driver()

removing

> returns error.

>  static int __init iosf_mbi_init(void)
>  {
> +       int ret;
> +
>         iosf_debugfs_init();
>
>         cpu_latency_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
>
> -       return pci_register_driver(&iosf_mbi_pci_driver);
> +       ret = pci_register_driver(&iosf_mbi_pci_driver);
> +       if (ret) {
> +               cpu_latency_qos_remove_request(&iosf_mbi_pm_qos);
> +               iosf_debugfs_remove();
> +       }
> +
> +       return ret;
>  }

Can we rewrite it as

  if (ret)
    goto err_remove;

  return 0;

err_remove:
  ...
  return ret;

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] x86/platform/intel/iosf_mbi: Fix error handling in iosf_mbi_init()

[Yuan Can]

在 2022/11/15 16:26, Andy Shevchenko 写道:
> On Tue, Nov 15, 2022 at 9:38 AM Yuan Can <yuancan@huawei.com> wrote:
>> A problem about modprobe iosf_mbi failed is triggered with the following
>> log given:
>>
>>   debugfs: Directory 'iosf_sb' with parent '/' already present!
>>
>> The reason is that iosf_mbi_init() returns pci_register_driver()
>> directly without checking its return value, if pci_register_driver()
>> failed, it returns without removing debugfs, resulting the debugfs of
>> iosf_sb can never be created later.
>>
>>   iosf_mbi_init()
>>     iosf_mbi_dbg_init() # create debugfs
>>     pci_register_driver()
>>       driver_register()
>>         bus_add_driver()
>>           priv = kzalloc(...) # OOM happened
>>     # return without remove debugfs and destroy workqueue
>>
>> Fix by remove debugfs and iosf_mbi_pm_qos when pci_register_driver()
> removing
>
>> returns error.
>>   static int __init iosf_mbi_init(void)
>>   {
>> +       int ret;
>> +
>>          iosf_debugfs_init();
>>
>>          cpu_latency_qos_add_request(&iosf_mbi_pm_qos, PM_QOS_DEFAULT_VALUE);
>>
>> -       return pci_register_driver(&iosf_mbi_pci_driver);
>> +       ret = pci_register_driver(&iosf_mbi_pci_driver);
>> +       if (ret) {
>> +               cpu_latency_qos_remove_request(&iosf_mbi_pm_qos);
>> +               iosf_debugfs_remove();
>> +       }
>> +
>> +       return ret;
>>   }
> Can we rewrite it as
>
>    if (ret)
>      goto err_remove;
>
>    return 0;
>
> err_remove:
>    ...
>    return ret;
>
> ?
Thanks for these suggestions! will be changed in the next version.

-- 
Best regards,
Yuan Can