lkml.org 
[lkml]   [2024]   [Feb]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver framework and device initialization
Date
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
>
> > > > +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.

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 below):

static void octep_vf_tx_timeout_task(struct work_struct *work)
{
struct octep_vf_device *oct = container_of(work, struct octep_vf_device,
tx_timeout_task);
struct net_device *netdev = 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 feels 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 =
> > > 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.

Sorry, my bad. The only task in this workqueue for VF driver is the tx timeout currently as I understand.
So, yes we can switch to system workqueue for now, and maybe change if further on such a requirement emerges.
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!

\
 
 \ /
  Last update: 2024-05-27 14:50    [W:0.097 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site