> -----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. > > > 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. > > > 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.) Tim Sell > > rc = 0; > > out_locked: > > up_write(&devdata->lock_visor_dev); > > -- > > 1.9.1 > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
