On Tue, Apr 05, 2016 at 03:49:57PM +0000, Sell, Timothy C wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > > Sent: Tuesday, April 05, 2016 10:58 AM > > To: Sell, Timothy C > > Cc: Iban Rodriguez; Kershner, David A; Greg Kroah-Hartman; Benjamin > > Romer; *S-Par-Maintainer; devel@xxxxxxxxxxxxxxxxxxxx; linux- > > kernel@xxxxxxxxxxxxxxx > > Subject: Re: Staging: unisys/verisonic: Correct double unlock > > > > On Mon, Apr 04, 2016 at 09:40:13PM +0000, Sell, Timothy C wrote: > > > > -----Original Message----- > > > > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > > > > Sent: Monday, April 04, 2016 10:35 AM > > > > To: Sell, Timothy C > > > > Cc: Iban Rodriguez; Kershner, David A; Greg Kroah-Hartman; Benjamin > > > > Romer; *S-Par-Maintainer; devel@xxxxxxxxxxxxxxxxxxxx; linux- > > > > kernel@xxxxxxxxxxxxxxx > > > > Subject: Re: Staging: unisys/verisonic: Correct double unlock > > > > > > > > On Sat, Apr 02, 2016 at 11:20:14PM +0000, Sell, Timothy C wrote: > > > > > > -----Original Message----- > > > > > > From: Iban Rodriguez [mailto:iban.rodriguez@xxxxxxx] > > > > > > Sent: Saturday, April 02, 2016 1:47 PM > > > > > > To: Kershner, David A; Greg Kroah-Hartman; Benjamin Romer; Sell, > > > > Timothy > > > > > > C; Neil Horman > > > > > > Cc: *S-Par-Maintainer; devel@xxxxxxxxxxxxxxxxxxxx; linux- > > > > > > kernel@xxxxxxxxxxxxxxx; Iban Rodriguez > > > > > > Subject: Staging: unisys/verisonic: Correct double unlock > > > > > > > > > > > > 'priv_lock' is unlocked twice. The first one is removed and > > > > > > the function 'visornic_serverdown_complete' is now called with > > > > > > 'priv_lock' locked because 'devdata' is modified inside. > > > > > > > > > > > > Signed-off-by: Iban Rodriguez <iban.rodriguez@xxxxxxx> > > > > > > --- > > > > > > drivers/staging/unisys/visornic/visornic_main.c | 1 - > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/staging/unisys/visornic/visornic_main.c > > > > > > b/drivers/staging/unisys/visornic/visornic_main.c > > > > > > index be0d057346c3..af03f2938fe9 100644 > > > > > > --- a/drivers/staging/unisys/visornic/visornic_main.c > > > > > > +++ b/drivers/staging/unisys/visornic/visornic_main.c > > > > > > @@ -368,7 +368,6 @@ visornic_serverdown(struct visornic_devdata > > > > > > *devdata, > > > > > > } > > > > > > devdata->server_change_state = true; > > > > > > devdata->server_down_complete_func = complete_func; > > > > > > - spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > > > > visornic_serverdown_complete(devdata); > > > > > > } else if (devdata->server_change_state) { > > > > > > dev_dbg(&devdata->dev->device, "%s changing state\n", > > > > > > > > > > I agree there is a bug here involving priv_lock being unlocked > > > > > twice, but this patch isn't the appropriate fix. Reason is, we can NOT > > > > > call visornic_serverdown_complete() while holding a spinlock > > > > > (which is what this patch would cause to occur) because > > > > > visornic_serverdown_complete() might block when it calls > > > > > rtnl_lock() in this code sequence (rtnl_lock() grabs a mutex): > > > > > > > > > > rtnl_lock(); > > > > > dev_close(netdev); > > > > > rtnl_unlock(); > > > > > > > > > > Blocking with a spinlock held is always a bad idea. :-( > > > > > > > > > > > > > You should just get rid of the priv_lock entirely, its not needed. > > > > > > > > priv_lock is used the following functions: > > > > > > > > visornic_serverdown - only called at the end of a tx_timeout reset > > > > operation, so > > > > you are sure that the rx and tx paths are quiesced (i.e. no data access > > > > happening) > > > > > > > > visornic_disable_with_timeout - move the netif_stop_queue operation > > to > > > > the top > > > > of this function and you will be guaranteed no concurrent access in the tx > > > > path > > > > > > > > visornic_enable_with_timeout - same as above, make sure that > > > > netif_start_queue > > > > and napi_enable are at the end of the function and you are guarantted > > no > > > > concurrent access. > > > > > > > > visornic_xmit - The queue lock in the netdev_start_xmit routine > > guarantees > > > > you > > > > single access here from multiple transmits. > > > > > > > > visornic_xmit_timeout - only called on a tx timeout, when you are > > > > guaranteed not > > > > to have concurrent transmit occuing, by definition. > > > > > > > > visornic_rx - the only tests made here are to devdata members that are > > > > altered > > > > in service_resp_queue, and the visornic_rx is only called from > > > > service_resp_queue, so you are guaranteed a stable data structure, as > > there > > > > is > > > > only ever one context in service_resp_queue as its called from the napi > > poll > > > > routine > > > > > > > > service_resp_queue - Same as above, for any given queue, > > > > service_resp_queue only > > > > has one context exectuing at once. > > > > > > > > host_side_disappeared - only called from visornic_remove, when > > implies > > > > that all > > > > associated devices are closed already, guaranteeing single access. > > > > > > > > visornic_remove > > > > visornic_resume - Both of these function only get called when all > > network > > > > interfaces are quiesced. > > > > > > > > just remove the lock and make the minor changes needed to guarantee > > > > isolated > > > > access. It makes the code cleaner and faster > > > > > > > > Neil > > > > > > Neil, > > > > > > Although I would also love to get rid of this lock, I think we still > > > need it, and will attempt to explain. > > > > > > There's a thread of execution present in visornic that doesn't exist > > > in traditional network drivers, which involves the visornic_pause() and > > > visornic_resume() functions registered during: > > > > > > visorbus_register_visor_driver(&visornic_driver) > > > > > > visornic_pause() and visornic_resume() are called on a thread managed > > > by visorbus, in response to messages received from our hypervisor > > > back-end. > > > > > Ok, but you still can get away without the lock. The other lock points are all > > in the tx/rx paths, so insted of holding the lock, stop the transmit queues > > with > > netif_tx_stop_all_queues, and pause the napi instance with napi_disable. > > That > > allows you to guarantee that the tx and rx paths have no execute going on, > > and > > you can complete the serverdown path safely. > > > > > Note that visornic_pause() calls visornic_serverdown(), which is one of > > > the users of priv_lock. (I.e., visornic_serverdown() is called from > > > other places besides the end of a tx_timeout reset operation, which is > > > what you called out in your explanation above). We need priv_lock to > > > do a false --> true transition of devdata->server_change_state in the > > > pause/resume path, so we can prevent this transition from occurring > > > during critical sections in the normal networking path. > > > > > > The comment present on priv_lock's declaration: > > > > > > spinlock_t priv_lock; /* spinlock to access devdata structures */ > > > > > > is indeed inadequate to the point of being misleading. > > > > > > visornic_serverdown() in its present form is hard-to-follow, in > > > addition to having the double-unlock bug. I would prefer if it were > > > corrected and rewritten to look like this (where the main-path falls > > > thru down the left side of the screen): > > > > > Right, but the point of the lock is still to protect the devdata structure, and > > there are ways to do so (in my previous email and point above), without > > needing > > a lock. You just have to ensure mutual exclusion > > > > Neil > > You convinced me; I like the idea. > > Would it be acceptable to fix the immediate double-unlock bug now, and > address the lock-removal along with the other features we have queued > up for after the driver gets out of staging (like interrupt support)? > Thats fine with me, I just personally worry about the "later becomming never" potential. But yeah, if you think you'll get to it, I'm fine with fixing the immediate problem > The lock removal will touch many of our code paths dealing with > back-end hypervisor recovery scenarios, which is historically very > time-consuming to debug and test. Plus it seems like this could be > classified as a 'feature'. > > Thanks Neil. > Thanks! Neil > Tim > > > > > static int > > > visornic_serverdown(struct visornic_devdata *devdata, > > > visorbus_state_complete_func complete_func) > > > { > > > unsigned long flags; > > > int err; > > > > > > spin_lock_irqsave(&devdata->priv_lock, flags); > > > if (devdata->server_change_state) { > > > dev_dbg(&devdata->dev->device, "%s changing state\n", > > > __func__); > > > err = -EINVAL; > > > goto err_unlock; > > > } > > > if (devdata->server_down) { > > > dev_dbg(&devdata->dev->device, "%s already down\n", > > > __func__); > > > err = -EINVAL; > > > goto err_unlock; > > > } > > > if (devdata->going_away) { > > > dev_dbg(&devdata->dev->device, > > > "%s aborting because device removal pending\n", > > > __func__); > > > err = -ENODEV; > > > goto err_unlock; > > > } > > > devdata->server_change_state = true; > > > devdata->server_down_complete_func = complete_func; > > > spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > > > > visornic_serverdown_complete(devdata); > > > return 0; > > > > > > err_unlock: > > > spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > return err; > > > } > > > > > > Tim > > > > > > > > > > > > > -- > > > > > > 1.9.1 > > > > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
