linux-kernel - Re: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver framework and device initialization

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]

Date: Mon, 5 Feb 2024 15:44:48 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Shinas Rasheed <srasheed@...vell.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Haseeb Gani
 <hgani@...vell.com>, Vimlesh Kumar <vimleshk@...vell.com>, Sathesh B Edara
 <sedara@...vell.com>, "egallen@...hat.com" <egallen@...hat.com>,
 "mschmidt@...hat.com" <mschmidt@...hat.com>, "pabeni@...hat.com"
 <pabeni@...hat.com>, "horms@...nel.org" <horms@...nel.org>,
 "wizhao@...hat.com" <wizhao@...hat.com>, "kheib@...hat.com"
 <kheib@...hat.com>, "konguyen@...hat.com" <konguyen@...hat.com>, "David S.
 Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jonathan
 Corbet <corbet@....net>, Veerasenareddy Burru <vburru@...vell.com>,
 Satananda Burla <sburla@...vell.com>, Shannon Nelson
 <shannon.nelson@....com>, Tony Nguyen <anthony.l.nguyen@...el.com>, Joshua
 Hay <joshua.a.hay@...el.com>, Rahul Rameshbabu <rrameshbabu@...dia.com>,
 Brett Creeley <brett.creeley@....com>, Andrew Lunn <andrew@...n.ch>, Jacob
 Keller <jacob.e.keller@...el.com>
Subject: Re: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver
 framework and device initialization

On Sat, 3 Feb 2024 05:35:21 +0000 Shinas Rasheed wrote:
> > You haven't masked any IRQ or disabled NAPI. What prevents the queues
> > from getting restarted right after this call?  
> 
> The napi functionality (along with disabling it when stopping), is introduced (and used) in the patch after this one [2/8]. Also we disable interrupts in the 
> disable_interrupt hook which is also called in the next patch [2/8]. 

You gotta make the patches reviewable :(

> > > +static void octep_vf_tx_timeout(struct net_device *netdev, unsigned int  
> > txqueue)  
> > > +{
> > > +	struct octep_vf_device *oct = netdev_priv(netdev);
> > > +
> > > +	queue_work(octep_vf_wq, &oct->tx_timeout_task);
> > > +}  
> > 
> > I don't see you canceling this work. What if someone unregistered
> > the device before it runs? You gotta netdev_hold() a reference.  
> 
> We do cancel_work_sync in octep_vf_remove function.

But the device is still registered, so the timeout can happen after you
cancel but before you unregister.

> > > +static int __init octep_vf_init_module(void)
> > > +{
> > > +	int ret;
> > > +
> > > +	pr_info("%s: Loading %s ...\n", OCTEP_VF_DRV_NAME OCTEP_VF_DRV_STRING);  
> > > +
> > > +	/* work queue for all deferred tasks */
> > > +	octep_vf_wq =  
> > create_singlethread_workqueue(OCTEP_VF_DRV_NAME);
> > 
> > Is there a reason this wq has to be single threaded and different than
> > system queue? All you schedule on it in this series is the reset task.  
> 
> We also schedule the control mailbox task on this workqueue. The
> workqueue was created with the intention that there could be other
> driver specific tasks to add in the future. It has been single
> threaded for now, but we might optimize implementation in the future,
> although for now as far as to service our control plane this has been
> enough.

I haven't spotted the mailbox task in this series, if it's not here,
let's switch to system wq, and only add your own when needed.