Re: [PATCH v5 11/15] staging: gpib: ines: Handle gpib_register_driver() errors

Nihar Chaithanya
2024-12-29T10:46:17Z

On 29/12/24 15:58, Greg KH wrote:
> On Sat, Dec 28, 2024 at 11:37:59AM +0530, Nihar Chaithanya wrote:
>> The function ines_pcmcia_init_module() can be replaced by calling
>> pcmcia_register_driver() directly. The error value from this
>> function is not returned and the previous registering functions
>> are not unregistered.
>>
>> The function gpib_register_driver() can fail and similar to
>> pcmcia_register_driver() function failing, the previous registering
>> functions are not unregistered.
>>
>> Replace cb_pcmcia_init_module() with pcmcia_register_driver().
>> Unregister the gpib and pci register functions if the subsequent
>> gpib or pcmcia register functions fail and return the error value.
>>
>> Signed-off-by: Nihar Chaithanya <niharchaithanya@gmail.com>
>> ---
>> drivers/staging/gpib/ines/ines_gpib.c | 81 ++++++++++++++++++++-------
>> 1 file changed, 60 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/staging/gpib/ines/ines_gpib.c b/drivers/staging/gpib/ines/ines_gpib.c
>> index 9d8387c3bf01..73df4cd16926 100644
>> --- a/drivers/staging/gpib/ines/ines_gpib.c
>> +++ b/drivers/staging/gpib/ines/ines_gpib.c
>> @@ -1227,12 +1227,6 @@ static struct pcmcia_driver ines_gpib_cs_driver = {
>> .resume = ines_gpib_resume,
>> };
>>
>> -int ines_pcmcia_init_module(void)
>> -{
>> - pcmcia_register_driver(&ines_gpib_cs_driver);
>> - return 0;
>> -}
>> -
>> void ines_pcmcia_cleanup_module(void)
>> {
>> DEBUG(0, "ines_cs: unloading\n");
>> @@ -1420,28 +1414,73 @@ void ines_pcmcia_detach(gpib_board_t *board)
>>
>> static int __init ines_init_module(void)
>> {
>> - int err = 0;
>> + int ret;
>>
>> - err = pci_register_driver(&ines_pci_driver);
>> - if (err) {
>> - pr_err("ines_gpib: pci_driver_register failed!\n");
>> - return err;
>> + ret = pci_register_driver(&ines_pci_driver);
>> + if (ret) {
>> + pr_err("ines_gpib: pci_driver_register failed: error = %d\n", ret);
>> + return ret;
>> }
>>
>> - gpib_register_driver(&ines_pci_interface, THIS_MODULE);
>> - gpib_register_driver(&ines_pci_unaccel_interface, THIS_MODULE);
>> - gpib_register_driver(&ines_pci_accel_interface, THIS_MODULE);
>> - gpib_register_driver(&ines_isa_interface, THIS_MODULE);
>> + ret = gpib_register_driver(&ines_pci_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pci;
>> +
>> + ret = gpib_register_driver(&ines_pci_unaccel_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pci_unaccel;
>> +
>> + ret = gpib_register_driver(&ines_pci_accel_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pci_accel;
>> +
>> + ret = gpib_register_driver(&ines_isa_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_isa;
>> +
>> #ifdef GPIB_PCMCIA
>> - gpib_register_driver(&ines_pcmcia_interface, THIS_MODULE);
>> - gpib_register_driver(&ines_pcmcia_unaccel_interface, THIS_MODULE);
>> - gpib_register_driver(&ines_pcmcia_accel_interface, THIS_MODULE);
>> - err += ines_pcmcia_init_module();
>> + ret = gpib_register_driver(&ines_pcmcia_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pcmcia;
>> +
>> + ret = gpib_register_driver(&ines_pcmcia_unaccel_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pcmcia_unaccel;
>> +
>> + ret = gpib_register_driver(&ines_pcmcia_accel_interface, THIS_MODULE);
>> + if (ret)
>> + goto err_pcmcia_accel;
>> +
>> + ret = pcmcia_register_driver(&ines_gpib_cs_driver);
>> + if (ret)
>> + goto err_pcmcia_driver;
>> #endif
>> - if (err)
>> - return -1;
>>
>> + pr_info("ines_gpib: module init is complete\n");
> Why did you add this debugging line here that was not here at all?
>

Hi Greg, I thought to keep all the *_init_module consistent in gpib, so
added

this. If they were not present earlier I won't add them in the next version.

>> return 0;
>> +
>> +#ifdef GPIB_PCMCIA
>> +err_pcmcia_driver:
>> + pr_err("ines_gpib: pcmcia_register_driver failed: error = %d\n", ret);
> Why just this error message for only this one failure?

This failure is when pcmcia_register_driver fails, it could give multiple

error values.

>
>> + gpib_unregister_driver(&ines_pcmcia_accel_interface);
>> +err_pcmcia_accel:
>> + gpib_unregister_driver(&ines_pcmcia_unaccel_interface);
>> +err_pcmcia_unaccel:
>> + gpib_unregister_driver(&ines_pcmcia_interface);
>> +err_pcmcia:
>> +#endif
>> + gpib_unregister_driver(&ines_isa_interface);
>> +err_isa:
>> + gpib_unregister_driver(&ines_pci_accel_interface);
>> +err_pci_accel:
>> + gpib_unregister_driver(&ines_pci_unaccel_interface);
>> +err_pci_unaccel:
>> + gpib_unregister_driver(&ines_pci_interface);
>> +err_pci:
>> + pci_unregister_driver(&ines_pci_driver);
>> +
>> + pr_err("ines_gpib: gpib_register_driver failed!\n");
> Why not provide the error number here too?

When gpib_register_driver fails it would always be ENOMEM error value,

so should I mention this number?

>
> thanks,
>
> greg k-h

Thanks,

Nihar