Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware changes for LAG

[Simon Horman <simon.horman@corigine.com>]

On Fri, Jun 09, 2023 at 02:16:18PM -0700, Dave Ertman wrote:

...

Hi Dave,

some minor feedback from my side.

> @@ -5576,10 +5579,18 @@ static int __init ice_module_init(void)
>  		return -ENOMEM;
>  	}
>  
> +	ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0);
> +	if (!ice_lag_wq) {
> +		pr_err("Failed to create LAG workqueue\n");

Is the allocation failure already logged by core code?
If so, perhaps this is unnecessary?

> +		destroy_workqueue(ice_wq);
> +		return -ENOMEM;
> +	}
> +
>  	status = pci_register_driver(&ice_driver);
>  	if (status) {
>  		pr_err("failed to register PCI driver, err %d\n", status);
>  		destroy_workqueue(ice_wq);
> +		destroy_workqueue(ice_lag_wq);
>  	}
>  
>  	return status;

As there are now a few things (more than zero) to unwind I think it would
be best to use the Kernel's idiomatic goto-based approach.

(Completely untested!)

	ice_lag_wq = alloc_ordered_workqueue("ice_lag_wq", 0);
	if (!ice_lag_wq) {
		pr_err("Failed to create LAG workqueue\n");
		status = -ENOMEM;
		goto err_destroy_ice_wq;
	}

	status = pci_register_driver(&ice_driver);
	if (status) {
		pr_err("failed to register PCI driver, err %d\n", status);
		goto err_destroy_lag_wq;
	}

	return status;

err_destroy_lag_wq:
	destroy_workqueue(ice_lag_wq);
err_destroy_ice_wq:
	destroy_workqueue(ice_wq);
	return status


> @@ -5596,6 +5607,7 @@ static void __exit ice_module_exit(void)
>  {
>  	pci_unregister_driver(&ice_driver);
>  	destroy_workqueue(ice_wq);
> +	destroy_workqueue(ice_lag_wq);
>  	pr_info("module unloaded\n");
>  }
>  module_exit(ice_module_exit);

...