Received-SPF: pass (google.com: domain of linux-kernel+bounces-54103-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 5 Feb 2024 15:44:48 -0800
From: Jakub Kicinski <kuba@kernel.org>
To: Shinas Rasheed <srasheed@marvell.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Haseeb Gani
 <hgani@marvell.com>, Vimlesh Kumar <vimleshk@marvell.com>, Sathesh B Edara
 <sedara@marvell.com>, "egallen@redhat.com" <egallen@redhat.com>,
 "mschmidt@redhat.com" <mschmidt@redhat.com>, "pabeni@redhat.com"
 <pabeni@redhat.com>, "horms@kernel.org" <horms@kernel.org>,
 "wizhao@redhat.com" <wizhao@redhat.com>, "kheib@redhat.com"
 <kheib@redhat.com>, "konguyen@redhat.com" <konguyen@redhat.com>, "David S.
 Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jonathan
 Corbet <corbet@lwn.net>, Veerasenareddy Burru <vburru@marvell.com>,
 Satananda Burla <sburla@marvell.com>, Shannon Nelson
 <shannon.nelson@amd.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, Joshua
 Hay <joshua.a.hay@intel.com>, Rahul Rameshbabu <rrameshbabu@nvidia.com>,
 Brett Creeley <brett.creeley@amd.com>, Andrew Lunn <andrew@lunn.ch>, Jacob
 Keller <jacob.e.keller@intel.com>
Subject: Re: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver
 framework and device initialization
Message-ID: <20240205154448.1c5a5ad8@kernel.org>
In-Reply-To: <PH0PR18MB47345E3ADCC35D0ECA763DBBC7412@PH0PR18MB4734.namprd18.prod.outlook.com>
References: <20240129050254.3047778-1-srasheed@marvell.com>
	<20240129050254.3047778-2-srasheed@marvell.com>
	<20240131161406.22a9e330@kernel.org>
	<PH0PR18MB47345E3ADCC35D0ECA763DBBC7412@PH0PR18MB4734.namprd18.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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.