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

[Nihar Chaithanya <niharchaithanya@gmail.com>]

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