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