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

[Jakub Kicinski <kuba@kernel.org>]

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.