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