From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 004FF1EB49; Mon, 5 Feb 2024 23:44:50 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707176691; cv=none; b=l8yXM1Oxbn3uiu89cQ/b3f2AYmtc4nUH659sCCXT5JXyy6wZZIU6jlkrH0Yaro7KyvPUVMs4wKSd64znZOZ6b4eQ0HV53cv++osy6/rClfWDFDoYlt9XHxCvSmBD15YWSHaWwId8Pi9uhDMQ1htQDLkpPiDz7b/5BfibLkRVpYE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1707176691; c=relaxed/simple; bh=SxLaOpN/aBErJ9kXh6d9dU6KlKpKu99pWHS/STEMU8M=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=IbDxozMIvorEf5/rKabDZZeP72C8ZhJacJ1qCs+3o0sLSEUqAAF9L/2KJo5ByZ6LmzIkZ8qblmggt63xAWvDh0W+OKfXzRc9EC3VElWGQCkA9Lth4FeJP6TVXTFHN9HFtaGhmDCXSclPHH9U177vqmT6UdhdS4BJ3s27MJGnvOU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=j5CaDjy/; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="j5CaDjy/" Received: by smtp.kernel.org (Postfix) with ESMTPSA id A1413C433F1; Mon, 5 Feb 2024 23:44:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1707176690; bh=SxLaOpN/aBErJ9kXh6d9dU6KlKpKu99pWHS/STEMU8M=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=j5CaDjy/VjxbeQbdgNT8rBul3sjRJv85BNfhU7NBA3ouFRrZ9V9uxMt7N8/1Fxd18 meUCBNTQblbFgDEQJe+UT9h1L8dqhim7LslBtyeknQzuzRhdhMQ9JaFjmoAO8zepYs nJZb2IxMHfZXpZ579O++i4hrpSVSRqECbx0e1IRiCKjkWv3IsDOgkgWkp5HhFoPZYl DiZjuRNBA2X/EI09LTQgegSqHCwSWb4bY2xNNdpL9KG/A8CefmjnXwYw7TCbixR+8j OdXgjcfz6VUfd6A0VCwlCLIJpAct5EjHtn0jkuf8N6d1XaR2rdhtcy7fbsEEsuLRkt M0tiRWjOa/Yqg== Date: Mon, 5 Feb 2024 15:44:48 -0800 From: Jakub Kicinski To: Shinas Rasheed Cc: "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Haseeb Gani , Vimlesh Kumar , Sathesh B Edara , "egallen@redhat.com" , "mschmidt@redhat.com" , "pabeni@redhat.com" , "horms@kernel.org" , "wizhao@redhat.com" , "kheib@redhat.com" , "konguyen@redhat.com" , "David S. Miller" , Eric Dumazet , Jonathan Corbet , Veerasenareddy Burru , Satananda Burla , Shannon Nelson , Tony Nguyen , Joshua Hay , Rahul Rameshbabu , Brett Creeley , Andrew Lunn , Jacob Keller 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: References: <20240129050254.3047778-1-srasheed@marvell.com> <20240129050254.3047778-2-srasheed@marvell.com> <20240131161406.22a9e330@kernel.org> Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: 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.