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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 3 Feb 2024 05:35:21 +0000
From: Shinas Rasheed <srasheed@...vell.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Haseeb Gani
	<hgani@...vell.com>, Vimlesh Kumar <vimleshk@...vell.com>,
        Sathesh B Edara
	<sedara@...vell.com>,
        "egallen@...hat.com" <egallen@...hat.com>,
        "mschmidt@...hat.com" <mschmidt@...hat.com>,
        "pabeni@...hat.com"
	<pabeni@...hat.com>,
        "horms@...nel.org" <horms@...nel.org>,
        "wizhao@...hat.com" <wizhao@...hat.com>,
        "kheib@...hat.com"
	<kheib@...hat.com>,
        "konguyen@...hat.com" <konguyen@...hat.com>,
        "David S.
 Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>, Jonathan
 Corbet <corbet@....net>,
        Veerasenareddy Burru <vburru@...vell.com>,
        Satananda
 Burla <sburla@...vell.com>,
        Shannon Nelson <shannon.nelson@....com>,
        Tony
 Nguyen <anthony.l.nguyen@...el.com>,
        Joshua Hay <joshua.a.hay@...el.com>,
        Rahul Rameshbabu <rrameshbabu@...dia.com>,
        Brett Creeley
	<brett.creeley@....com>, Andrew Lunn <andrew@...n.ch>,
        Jacob Keller
	<jacob.e.keller@...el.com>
Subject: RE: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver
 framework and device initialization

Hi Jakub,

> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Thursday, February 1, 2024 5:44 AM
> To: Shinas Rasheed <srasheed@...vell.com>
> Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; Haseeb Gani
> <hgani@...vell.com>; Vimlesh Kumar <vimleshk@...vell.com>; Sathesh B
> Edara <sedara@...vell.com>; egallen@...hat.com; mschmidt@...hat.com;
> pabeni@...hat.com; horms@...nel.org; wizhao@...hat.com;
> kheib@...hat.com; konguyen@...hat.com; David S. Miller
> <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Jonathan
> Corbet <corbet@....net>; Veerasenareddy Burru <vburru@...vell.com>;
> Satananda Burla <sburla@...vell.com>; Shannon Nelson
> <shannon.nelson@....com>; Tony Nguyen <anthony.l.nguyen@...el.com>;
> Joshua Hay <joshua.a.hay@...el.com>; Rahul Rameshbabu
> <rrameshbabu@...dia.com>; Brett Creeley <brett.creeley@....com>; Andrew
> Lunn <andrew@...n.ch>; Jacob Keller <jacob.e.keller@...el.com>
> Subject: [EXT] Re: [PATCH net-next v5 1/8] octeon_ep_vf: Add driver framework
> and device initialization
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Sun, 28 Jan 2024 21:02:47 -0800 Shinas Rasheed wrote:
> 
> > +	netif_carrier_off(netdev);
> > +	netif_tx_disable(netdev);
> 
> 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]. 

> > +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.

> > +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.

Let me know if there are other changes to be made (I ack the other ones you have made), so I can gather up everything and submit another patch. Thanks for your review!