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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZImh4NunKEpay3zu@corigine.com>
Date: Wed, 14 Jun 2023 13:17:52 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Dave Ertman <david.m.ertman@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, daniel.machon@...rochip.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware
 changes for LAG

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);

...