From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753293AbcFBMqT (ORCPT ); Thu, 2 Jun 2016 08:46:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49850 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751464AbcFBMqR (ORCPT ); Thu, 2 Jun 2016 08:46:17 -0400 Date: Thu, 2 Jun 2016 08:46:09 -0400 From: Neil Horman To: "Sell, Timothy C" Cc: "Kershner, David A" , "corbet@lwn.net" , "tglx@linutronix.de" , "mingo@redhat.com" , "hpa@zytor.com" , "gregkh@linuxfoundation.org" , "Arfvidson, Erik" , "hofrat@osadl.org" , "dzickus@redhat.com" , "jes.sorensen@redhat.com" , "Curtin, Alexander Paul" , "janani.rvchndrn@gmail.com" , "sudipm.mukherjee@gmail.com" , "prarit@redhat.com" , "Binder, David Anthony" , "dan.j.williams@intel.com" , "linux-kernel@vger.kernel.org" , "linux-doc@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , *S-Par-Maintainer Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking Message-ID: <20160602124609.GA17646@hmsreliant.think-freely.org> References: <1464748013-31309-1-git-send-email-david.kershner@unisys.com> <1464748013-31309-11-git-send-email-david.kershner@unisys.com> <20160601141748.GD4578@hmsreliant.think-freely.org> <1c4e9b86ed344191a2be886d571ca7b8@US-EXCH13-2.na.uis.unisys.com> <20160601184251.GE4578@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.1 (2016-04-27) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 02 Jun 2016 12:46:15 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 02, 2016 at 05:02:11AM +0000, Sell, Timothy C wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@redhat.com] > > Sent: Wednesday, June 01, 2016 2:43 PM > > To: Sell, Timothy C > > Cc: Kershner, David A; corbet@lwn.net; tglx@linutronix.de; > > mingo@redhat.com; hpa@zytor.com; gregkh@linuxfoundation.org; > > Arfvidson, Erik; hofrat@osadl.org; dzickus@redhat.com; > > jes.sorensen@redhat.com; Curtin, Alexander Paul; > > janani.rvchndrn@gmail.com; sudipm.mukherjee@gmail.com; > > prarit@redhat.com; Binder, David Anthony; dan.j.williams@intel.com; > > linux-kernel@vger.kernel.org; linux-doc@vger.kernel.org; driverdev- > > devel@linuxdriverproject.org; *S-Par-Maintainer > > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove > > unnecessary locking > > > > On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote: > > > > -----Original Message----- > > > > From: Neil Horman [mailto:nhorman@redhat.com] > > > > Sent: Wednesday, June 01, 2016 10:18 AM > > > > To: Kershner, David A > > > > Cc: corbet@lwn.net; tglx@linutronix.de; mingo@redhat.com; > > > > hpa@zytor.com; gregkh@linuxfoundation.org; Arfvidson, Erik; Sell, > > Timothy > > > > C; hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com; > > > > Curtin, Alexander Paul; janani.rvchndrn@gmail.com; > > > > sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David > > Anthony; > > > > dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux- > > > > doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par- > > > > Maintainer > > > > Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove > > > > unnecessary locking > > > > > > > > On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote: > > > > > From: Tim Sell > > > > > > > > > > Locking in the _interrupt() function is NOT necessary so long as we > > ensure > > > > > that interrupts have been stopped whenever we need to pause or > > resume > > > > the > > > > > device, which we now do. > > > > > > > > > > While a device is paused, we ensure that interrupts stay disabled, i.e. > > > > > that the _interrupt() function will NOT be called, yet remember the > > > > desired > > > > > state in devdata->interrupts_enabled if open() or close() are called are > > > > > called while the device is paused. Then when the device is resumed, > > we > > > > > restore the actual state of interrupts (i.e., whether _interrupt() is going > > > > > to be called or not) to the desired state in devdata- > > >interrupts_enabled. > > > > > > > > > > Signed-off-by: Tim Sell > > > > > Signed-off-by: David Kershner > > > > > --- > > > > > drivers/staging/unisys/visorinput/visorinput.c | 57 > > > > +++++++++++++++++++++----- > > > > > 1 file changed, 47 insertions(+), 10 deletions(-) > > > > > > > > > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c > > > > b/drivers/staging/unisys/visorinput/visorinput.c > > > > > index 12a3570..9c00710 100644 > > > > > --- a/drivers/staging/unisys/visorinput/visorinput.c > > > > > +++ b/drivers/staging/unisys/visorinput/visorinput.c > > > > > @@ -66,6 +66,7 @@ struct visorinput_devdata { > > > > > struct rw_semaphore lock_visor_dev; /* lock for dev */ > > > > > struct input_dev *visorinput_dev; > > > > > bool paused; > > > > > + bool interrupts_enabled; > > > > > unsigned int keycode_table_bytes; /* size of following array */ > > > > > /* for keyboard devices: visorkbd_keycode[] + > > > > visorkbd_ext_keycode[] */ > > > > > unsigned char keycode_table[0]; > > > > > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev > > > > *visorinput_dev) > > > > > return -EINVAL; > > > > > } > > > > > dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__); > > > > > + > > > > > + /* > > > > > + * If we're not paused, really enable interrupts. > > > > > + * Regardless of whether we are paused, set a flag indicating > > > > > + * interrupts should be enabled so when we resume, interrupts > > > > > + * will really be enabled. > > > > > + */ > > > > > + down_write(&devdata->lock_visor_dev); > > > > > + devdata->interrupts_enabled = true; > > > > > + if (devdata->paused) > > > > > + goto out_unlock; > > > > Don't you want to wait until you actually enable interrupts here to set > > > > interrupts_enabled to true? Otherwise, if devdata->paused is true, you > > will > > > > be > > > > out of sync. > > > > > > No. That's the intent of this code, to remember what the > > > state of interrupts SHOULD be (via devdata->interrupts_enabled), at > > > a point in time when interrupts can NOT be enabled, e.g., when > > > the device is paused (devdata->paused). After the device is resumed, > > > the real interrupt state (visorbus_enable_channel_interrupts()) > > > will be synchronized with the remembered state. > > > > > > > Ok, I'll buy that, but it still looks rather racy to me. It appears to me that > > the code path in which the paused state is toggled > > (visorinput_pause|resume), is > > called from a path that originates in visorchipset, specifically in the work > > queue function controlvm_periodic_work. Given that, its entirely possible > > for > > the paused state of the virutal hardware to change while the device is being > > opened. That is to say devdata->paused can become true immediately after > > its > > checked in visorinput_open above, and so we can enable interrupts on > > hardware > > that is paused, which seems to be what this code is trying to avoid. > > > > You are absolutely correct about the 2 different threads of execution > where these functions can be called. > > But in this code, we hold devdata->lock_visor_dev in order to prevent > the scenario you describe. I.e., the code in all of the paths involved: > * never changes dev->paused or dev->interrupts_enabled without > holding devdata->lock_visor_dev > * never makes any decisions based on dev->paused or > dev->interrupts_enabled without holding devdata->lock_visor_dev > > > > > > > > > > visorbus_enable_channel_interrupts(devdata->dev); > > > > > + > > > > > +out_unlock: > > > > > + up_write(&devdata->lock_visor_dev); > > > > > return 0; > > > > > } > > > > > > > > > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev > > > > *visorinput_dev) > > > > > return; > > > > > } > > > > > dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__); > > > > > + > > > > > + /* > > > > > + * If we're not paused, really disable interrupts. > > > > > + * Regardless of whether we are paused, set a flag indicating > > > > > + * interrupts should be disabled so when we resume we will > > > > > + * not re-enable them. > > > > > + */ > > > > > + > > > > > + down_write(&devdata->lock_visor_dev); > > > > > + devdata->interrupts_enabled = false; > > > > > + if (devdata->paused) > > > > > + goto out_unlock; > > > > Ditto to my above comment > > > > > > Ditto my response above. > > > > > Same comment regarding racyness. > > > > > > > > > > > visorbus_disable_channel_interrupts(devdata->dev); > > > > > + > > > > > +out_unlock: > > > > > + up_write(&devdata->lock_visor_dev); > > > > > } > > > > > > > > > > /* > > > > > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev) > > > > > * in visorinput_channel_interrupt() > > > > > */ > > > > > > > > > > - down_write(&devdata->lock_visor_dev); > > > > > dev_set_drvdata(&dev->device, NULL); > > > > > unregister_client_input(devdata->visorinput_dev); > > > > > - up_write(&devdata->lock_visor_dev); > > > > > kfree(devdata); > > > > > } > > > > > > > > > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct > > visor_device > > > > *dev) > > > > > if (!devdata) > > > > > return; > > > > > > > > > > - down_write(&devdata->lock_visor_dev); > > > > > - if (devdata->paused) /* don't touch device/channel when paused */ > > > > > - goto out_locked; > > > > > - > > > > > visorinput_dev = devdata->visorinput_dev; > > > > > - if (!visorinput_dev) > > > > > - goto out_locked; > > > > > > > > > > while (visorchannel_signalremove(dev->visorchannel, 0, &r)) { > > > > > scancode = r.activity.arg1; > > > > > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct > > visor_device > > > > *dev) > > > > > break; > > > > > } > > > > > } > > > > > -out_locked: > > > > > - up_write(&devdata->lock_visor_dev); > > > > > } > > > > > > > > > > static int > > > > > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev, > > > > > rc = -EBUSY; > > > > > goto out_locked; > > > > > } > > > > > + if (devdata->interrupts_enabled) > > > > > + visorbus_disable_channel_interrupts(dev); > > > > > + > > > > > + /* > > > > > + * due to above, at this time no thread of execution will be > > > > > + * in visorinput_channel_interrupt() > > > > > + */ > > > > > + > > > > > devdata->paused = true; > > > > > complete_func(dev, 0); > > > > > rc = 0; > > > > > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev, > > > > > } > > > > > devdata->paused = false; > > > > > complete_func(dev, 0); > > > > > + > > > > > + /* > > > > > + * Re-establish calls to visorinput_channel_interrupt() if that is > > > > > + * the desired state that we've kept track of in interrupts_enabled > > > > > + * while the device was paused. > > > > > + */ > > > > > + if (devdata->interrupts_enabled) > > > > > + visorbus_enable_channel_interrupts(dev); > > > > > + > > > > > > > > Unless I'm mistaken, it seems that visorinput_pause and > > visorinput_open or > > > > close > > > > can be called in parallel on different cpus. As such the state of > > > > interrupts_enabled may change during the execution of this function, > > which > > > > would > > > > lead to interrupts not getting properly enabled. > > > > > > > > > > > > > You are correct that visorinput_pause and visorinput_open/close > > > can be called in parallel. However, as I alluded to in my comment > > > above, the intent of this code is to just restore the actual interrupt > > > state with the desired state (remembered in > > > devdata->interrupts_enabled). It's ok if interrupts don't get > > > enabled, because that would be our intent if there are no longer > > > any users of the device. (In this case visorinput_close() would have > > > been called and devdata->interrupts_enabled would have got set > > > false while the device was paused.) > > > > > > > > > Heres an illustration of my concern. Assume the visorinput device is > > currently > > paused, and someone has called open on it while at the same time > > resuming it > > > > CPU0 CPU1 > > visoinput_resume > > visorinput_open > > check ->interrupts_enabled (false) > > > > set interrupts_enabled=true > > check ->paused (true) > > set ->paused = true > > return 0 > > > > In the above scenario visorinput_open and visorinput_resume will both > > return > > without having enabled interrupts, rendering the device non-responsive. > > > > A simmmilar scenario can be seen on close/pause, in which interrupts are > > left > > enabled on a device that is paused. > > > > It seems you can't remove all level of serialization here (though you can > > remove > > some). I would recommend that, instead of keeping your own mutex, you > > instead > > augment visorinput_pause/resume, to extract the input_device structure > > from the > > driver private data and hold the input device mutex when > > pausing/resuming the > > device. That will ensure that neither the paused or interrupts_enabled > > state > > will change during the execution of visorinput_open/close > > > > Neil > > Nice illustration. That would usually be enough to drill something thru > my thick skull, but I'm still missing something in this case. ;-( > > I'm still missing how this scenario could happen given our usage of > devdata->lock_visor_dev. We hold that lock for the entire execution of > visorinput_open(), visorinput_close(), visorinput_pause(), and > visorinput_resume(), where we are dealing with the checks and state > transitions of devdata->paused, devdata->interrupts_enabled, and > the actual state of channel interrupts. So even if the circumstance > presented itself where we were ready to run thru 2 of those functions > for the same device on mutiple cpus at the same exact time, the > execution would be serialized due to devdata->lock_visor_dev. > Ok, there it is, that works then, thanks for the clarification Neil > Tim Sell >