Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751569AbcDBX1g (ORCPT ); Sat, 2 Apr 2016 19:27:36 -0400 Received: from mail1.bemta8.messagelabs.com ([216.82.243.193]:16998 "EHLO mail1.bemta8.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750864AbcDBX1f convert rfc822-to-8bit (ORCPT ); Sat, 2 Apr 2016 19:27:35 -0400 X-Greylist: delayed 435 seconds by postgrey-1.27 at vger.kernel.org; Sat, 02 Apr 2016 19:27:35 EDT X-Env-Sender: Timothy.Sell@unisys.com X-Msg-Ref: server-15.tower-46.messagelabs.com!1459639216!20205237!1 X-Originating-IP: [192.61.61.104] X-StarScan-Received: X-StarScan-Version: 8.11; banners=-,-,- X-VirusChecked: Checked From: "Sell, Timothy C" To: Iban Rodriguez , "Kershner, David A" , Greg Kroah-Hartman , Benjamin Romer , Neil Horman CC: *S-Par-Maintainer , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" Subject: RE: Staging: unisys/verisonic: Correct double unlock Thread-Topic: Staging: unisys/verisonic: Correct double unlock Thread-Index: AQHRjQfC9PprpeeC3UqVF55KeH6iQp93TxRA Date: Sat, 2 Apr 2016 23:20:14 +0000 Message-ID: References: <1459619226-9786-1-git-send-email-iban.rodriguez@ono.com> In-Reply-To: <1459619226-9786-1-git-send-email-iban.rodriguez@ono.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [172.17.126.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1891 Lines: 48 > -----Original Message----- > From: Iban Rodriguez [mailto:iban.rodriguez@ono.com] > 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@driverdev.osuosl.org; linux- > kernel@vger.kernel.org; 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 > --- > 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. :-( > -- > 1.9.1