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 for Android: free password hash cracker in your pocket

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

Date: Wed, 14 Jun 2023 16:56:53 +0000
From: "Ertman, David M" <david.m.ertman@...el.com>
To: Simon Horman <simon.horman@...igine.com>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"daniel.machon@...rochip.com" <daniel.machon@...rochip.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH iwl-next v4 02/10] ice: Add driver support for firmware
 changes for LAG

> -----Original Message-----
> From: Simon Horman <simon.horman@...igine.com>
> Sent: Wednesday, June 14, 2023 4:18 AM
> To: Ertman, David M <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?

I do not see any messaging from the core, so I should probably leave this here
unless you can point out something I missed 😊

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

Sounds reasonable - will switch to a goto based approach.

Changes to come in v5.

DaveE

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