> -----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. Tim Sell _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
