Received-SPF: pass (google.com: domain of linux-kernel+bounces-54431-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Shinas Rasheed <srasheed@marvell.com>
To: Jakub Kicinski <kuba@kernel.org>
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
Thread-Topic: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver
 framework and device initialization
Thread-Index: AQHaUnB8V/UVqVyNZ0ObKemnEj9BQrD0okIAgAHlTwCABe4pAIAAWMHg
Date: Tue, 6 Feb 2024 07:42:12 +0000
Message-ID: 
 <PH0PR18MB473460118D0FC68BE4B3BC45C7462@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>
 <20240205154448.1c5a5ad8@kernel.org>
In-Reply-To: <20240205154448.1c5a5ad8@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?rZvHMH5gy90d4GIwUwiKXZ+KSGdraCv1ynscad40XjuFl5o2uOFQIP8BW4cc?=
 =?us-ascii?Q?8b4YFd5m0spqJ1Zyz7Nrg8v/JhJojfMcvzWdE1KX8e1wdCBENF4JGWS9PG33?=
 =?us-ascii?Q?zUg0XbXfBuJ2wxC7ghp71N7xsbxcyWB5ctP1OV+sGW+LnuxiyuE1qGjrAXQD?=
 =?us-ascii?Q?IKKY5Tk5bomYYl/tcsK+MCf+SKepEH3XsA/oUqOeQ0lE+pqz1qmSVHvF5bPB?=
 =?us-ascii?Q?h+eszXBB39NZIlGXsP91yTqWymXUuxyS8dB/ULhuuHt+iX/mgrBgKiJjn9N+?=
 =?us-ascii?Q?L9Up0hWHHqKQMJFIyuwp8iZGrHl+1iZR80kaw8YSToIsghRzuUCcbjVS88CW?=
 =?us-ascii?Q?QoTKAtF0UTBgTahQSqRy8/1AqazHgAW/+btpg/gQJlSlZRyzOOuhks4+5baE?=
 =?us-ascii?Q?NTsS59jk/W9oKZ7e/O5hptjJZEVG61pLH+BHd1HTkanhGjk6zRKBMCq9YyJH?=
 =?us-ascii?Q?n23xHnCgX4bcYxjUOG1TZWTlC1bl5GjaTb0EAdO+anQ9LFsaV/DFZbGF8vpx?=
 =?us-ascii?Q?bu7dLIsEIHpxtQHcVeoNY6E96D90iIS6Q4PzLhEm6vBH5znZx9pFaA8DNv3/?=
 =?us-ascii?Q?KG+qr2MIsjFXHUION0vsS0qTjRFZlx190zi7ndUC1KXkvgMZLmnl1ddqDRYW?=
 =?us-ascii?Q?MHiuu/k+Plj6btRxDCh00pf7TktmTvGEuP0+QfyLROAzoJqEfxW+O2Vwv6DA?=
 =?us-ascii?Q?2EZ5qiCwp+B8wvS7PgQ25iJS60bOCtX36lSK42eoPYBcTeNUtY6pNr2C4OQp?=
 =?us-ascii?Q?YmuhZN5AiljlDQPJ8OAVWkxO2v3uhQTlnxMXj9hg5h5xzqIAwepkHEZQmacV?=
 =?us-ascii?Q?gNn/2kZ71yJ7G81VCy4Yn9F88KhJW3lYW0gl6iWtq4I9N35A7xITCcxYZ4fS?=
 =?us-ascii?Q?+oLPPaiW6dlDYJxqCPedyNOadYTrpqKdeNtTnr1eAPh4lc2s+vd+nCoQR2BI?=
 =?us-ascii?Q?2ITT21GcytygheyaB+5oAaV6adCUf/aX1H8/thjH1Fn8adnPjZqoO7SSa/tT?=
 =?us-ascii?Q?sxigkQPijtaWy3uHszkEWx7hsk3Nle9Zybt7pRIeOVyAe2g9gmdBZQLDeEQQ?=
 =?us-ascii?Q?VxUzOC2YZri1p9vp65n8Q/hRbncW4FYEvM9LFS/Lr5Tvf9JW4y1sC4qNXym+?=
 =?us-ascii?Q?GDw7+rtwEBoX+jwKSQeRIFcNtmpil+EDXb9p68iHbfqWJ1g6qPQjSQlg2Q8Z?=
 =?us-ascii?Q?iPyc1uT7rUdShAG/lPCcz6PZeQqrKQ6NIh+E3lLWJD/5M26EagmHGjkZpnFG?=
 =?us-ascii?Q?VQCO3Oqx/kGjKTlbR25Wg/l+zNu3VklBsUerxIgvJXpM7zGZmk3yBnG2SmyH?=
 =?us-ascii?Q?IDreNg7+z11dWMUkUN3JmUh7mDbmrUa6hrnvhtgM4Wr3grrh//y+3AEzJcf/?=
 =?us-ascii?Q?isDT32wWHPDq+EwqEQISwlEFmtHhooPdF0K8VGBeb/skaaG7MjTmHTKI+x06?=
 =?us-ascii?Q?TLNSl2je0cSZmMPhfIWNq8QAbo/4BHut3ehRJCqjodjzibwbqz1rrPjaYbEL?=
 =?us-ascii?Q?Pj6BFVTYNBzRIOyGxx1g+l91AUEV7bUO18uaEbwmJHyVCLeOQBdCGL7kXTGH?=
 =?us-ascii?Q?pvsD8smXniZIH2p026zYg7+Srx4juAxK2yyBZtBc?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR18MB4734.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c364ca1-e45d-46fe-e9cf-08dc26e721b6
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Feb 2024 07:42:12.4349
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: mWkgvlDr0bL8Vtaolnc3CyAU0IDz1wM5LAAxinDqXyP4CcnrqpxdwFb2PeizZx3mNqX2HFaE1vejx4Qe9UmQtA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR18MB4018
X-Proofpoint-ORIG-GUID: 9-15Dx6LyKbBCVZ93uKukLfvY0v6Tb0i
X-Proofpoint-GUID: 9-15Dx6LyKbBCVZ93uKukLfvY0v6Tb0i
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-06_01,2024-01-31_01,2023-05-22_02

Hi,

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, February 6, 2024 5:15 AM
> To: Shinas Rasheed <srasheed@marvell.com>
> Cc: netdev@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; mschmidt@redhat.com;
> pabeni@redhat.com; horms@kernel.org; wizhao@redhat.com;
> kheib@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
>=20
> > > > +static void octep_vf_tx_timeout(struct net_device *netdev, unsigne=
d int
> > > txqueue)
> > > > +{
> > > > +	struct octep_vf_device *oct =3D 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.
>=20
> But the device is still registered, so the timeout can happen after you
> cancel but before you unregister.

There is rtnl_lock inside octep_vf_tx_timeout_task (the work task function)=
, which can protect
from unregister_netdev, for such cases (code snippet for quick reference be=
low):

static void octep_vf_tx_timeout_task(struct work_struct *work)
{
        struct octep_vf_device *oct =3D container_of(work, struct octep_vf_=
device,
                                                tx_timeout_task);
        struct net_device *netdev =3D oct->netdev;

        rtnl_lock();
        if (netif_running(netdev)) {
                octep_vf_stop(netdev);
                octep_vf_open(netdev);
        }
        rtnl_unlock();
}

I hope this takes care of it? Please let me know if my thought process feel=
s wrong. Thanks!

> > > > +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 =3D
> > > create_singlethread_workqueue(OCTEP_VF_DRV_NAME);
> > >
> > > Is there a reason this wq has to be single threaded and different tha=
n
> > > 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.
>=20
> 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.

Sorry, my bad. The only task in this workqueue for VF driver is the tx time=
out currently as I understand.
So, yes we can switch to system workqueue for now, and maybe change if furt=
her on such a requirement emerges.=20
If the previous comment is okay by you, I'll put in this change
as well in the next patch and submit.

Thanks for the review!