[PATCH] PCI: cpqphp: Fix error handling in cpqhpc_init()

[PATCH] PCI: cpqphp: Fix error handling in cpqhpc_init()

[Yuan Can]

The cpqhpc_init() returns the result of pci_register_driver() without
checking it, if pci_register_driver() failed, the debugfs created in
cpqhp_initialize_debugfs() is not removed, resulting the debugfs of
cpqhp can never be created later.
Fix by calling cpqhp_shutdown_debugfs() when pci_register_driver() failed.

Fixes: 9f3f4681291f ("[PATCH] PCI Hotplug: fix up the sysfs file in the compaq pci hotplug driver")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/pci/hotplug/cpqphp_core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index c94b40e64baf..c47981ef92ea 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -1389,6 +1389,8 @@ static int __init cpqhpc_init(void)
 	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
 	cpqhp_initialize_debugfs();
 	result = pci_register_driver(&cpqhpc_driver);
+	if (result)
+		cpqhp_shutdown_debugfs();
 	dbg("pci_register_driver = %d\n", result);
 	return result;
 }
-- 
2.17.1

Re: [PATCH] PCI: cpqphp: Fix error handling in cpqhpc_init()

[Bjorn Helgaas]

On Tue, Nov 22, 2022 at 10:13:46AM +0000, Yuan Can wrote:
> The cpqhpc_init() returns the result of pci_register_driver() without
> checking it, if pci_register_driver() failed, the debugfs created in
> cpqhp_initialize_debugfs() is not removed, resulting the debugfs of
> cpqhp can never be created later.
> Fix by calling cpqhp_shutdown_debugfs() when pci_register_driver() failed.

Add a blank line between paragraphs.

> Fixes: 9f3f4681291f ("[PATCH] PCI Hotplug: fix up the sysfs file in the compaq pci hotplug driver")
> Signed-off-by: Yuan Can <yuancan@huawei.com>
> ---
>  drivers/pci/hotplug/cpqphp_core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
> index c94b40e64baf..c47981ef92ea 100644
> --- a/drivers/pci/hotplug/cpqphp_core.c
> +++ b/drivers/pci/hotplug/cpqphp_core.c
> @@ -1389,6 +1389,8 @@ static int __init cpqhpc_init(void)
>  	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>  	cpqhp_initialize_debugfs();
>  	result = pci_register_driver(&cpqhpc_driver);
> +	if (result)
> +		cpqhp_shutdown_debugfs();

Is there some reason cpqhp_initialize_debugfs() needs to be called
before pci_register_driver()?

In other words, could we do this instead?

  result = pci_register_driver(&cpqhpc_driver);
  if (result)
    return result;

  cpqhp_initialize_debugfs();
  return 0;

I assume this was found by code inspection?  I've never heard of
anybody actually using this driver :)

>  	dbg("pci_register_driver = %d\n", result);
>  	return result;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH] PCI: cpqphp: Fix error handling in cpqhpc_init()

[Yuan Can]

在 2022/11/23 4:05, Bjorn Helgaas 写道:
> On Tue, Nov 22, 2022 at 10:13:46AM +0000, Yuan Can wrote:
>> The cpqhpc_init() returns the result of pci_register_driver() without
>> checking it, if pci_register_driver() failed, the debugfs created in
>> cpqhp_initialize_debugfs() is not removed, resulting the debugfs of
>> cpqhp can never be created later.
>> Fix by calling cpqhp_shutdown_debugfs() when pci_register_driver() failed.
> Add a blank line between paragraphs.
Ok.
>> Fixes: 9f3f4681291f ("[PATCH] PCI Hotplug: fix up the sysfs file in the compaq pci hotplug driver")
>> Signed-off-by: Yuan Can <yuancan@huawei.com>
>> ---
>>   drivers/pci/hotplug/cpqphp_core.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
>> index c94b40e64baf..c47981ef92ea 100644
>> --- a/drivers/pci/hotplug/cpqphp_core.c
>> +++ b/drivers/pci/hotplug/cpqphp_core.c
>> @@ -1389,6 +1389,8 @@ static int __init cpqhpc_init(void)
>>   	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
>>   	cpqhp_initialize_debugfs();
>>   	result = pci_register_driver(&cpqhpc_driver);
>> +	if (result)
>> +		cpqhp_shutdown_debugfs();
> Is there some reason cpqhp_initialize_debugfs() needs to be called
> before pci_register_driver()?
>
> In other words, could we do this instead?
>
>    result = pci_register_driver(&cpqhpc_driver);
>    if (result)
>      return result;
>
>    cpqhp_initialize_debugfs();
>    return 0;
Thanks for the suggestion! I do not see any reason that the order must 
be kept, will change to this style in the next version.
> I assume this was found by code inspection?  I've never heard of
> anybody actually using this driver :)
Yes, you are right :)
>>   	dbg("pci_register_driver = %d\n", result);
>>   	return result;
>>   }
>> -- 
>> 2.17.1
>>
-- 
Best regards,
Yuan Can

[PATCH] PCI: cpqphp: Fix error handling in cpqhpc_init()

[Yuan Can]

The cpqhpc_init() returns without checking the retval from
pci_register_driver().
If the pci_register_driver() failed, the module failed to install,
leaving the cpqhp debugfs not unregistered.

Fixes: 9f3f4681291f ("[PATCH] PCI Hotplug: fix up the sysfs file in the compaq pci hotplug driver")
Signed-off-by: Yuan Can <yuancan@huawei.com>
---
 drivers/pci/hotplug/cpqphp_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index 47a3ed16159a..933392fab8a3 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -1390,6 +1390,10 @@ static int __init cpqhpc_init(void)
 	cpqhp_initialize_debugfs();
 	result = pci_register_driver(&cpqhpc_driver);
 	dbg("pci_register_driver = %d\n", result);
+
+	if (result)
+		cpqhp_shutdown_debugfs();
+
 	return result;
 }
 
-- 
2.17.1