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

[Greg KH <gregkh@linuxfoundation.org>]

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?

>  	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?

> +	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?

thanks,

greg k-h