On Thu, Jun 02, 2016 at 05:02:11AM +0000, Sell, Timothy C wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > > Sent: Wednesday, June 01, 2016 2:43 PM > > To: Sell, Timothy C > > Cc: Kershner, David A; corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; > > mingo@xxxxxxxxxx; hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; > > Arfvidson, Erik; hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; > > jes.sorensen@xxxxxxxxxx; Curtin, Alexander Paul; > > janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx; > > prarit@xxxxxxxxxx; Binder, David Anthony; dan.j.williams@xxxxxxxxx; > > linux-kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; driverdev- > > devel@xxxxxxxxxxxxxxxxxxxxxx; *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@xxxxxxxxxx] > > > > Sent: Wednesday, June 01, 2016 10:18 AM > > > > To: Kershner, David A > > > > Cc: corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; > > > > hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Arfvidson, Erik; Sell, > > Timothy > > > > C; hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; jes.sorensen@xxxxxxxxxx; > > > > Curtin, Alexander Paul; janani.rvchndrn@xxxxxxxxx; > > > > sudipm.mukherjee@xxxxxxxxx; prarit@xxxxxxxxxx; Binder, David > > Anthony; > > > > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > > > > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *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 <Timothy.Sell@xxxxxxxxxx> > > > > > > > > > > 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 <Timothy.Sell@xxxxxxxxxx> > > > > > Signed-off-by: David Kershner <david.kershner@xxxxxxxxxx> > > > > > --- > > > > > 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 > > <handle random smi> check ->interrupts_enabled (false) > > <return from smi> <handle random smi> > > set interrupts_enabled=true > > check ->paused (true) <return from smi> > > 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 > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
