[PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking
Neil Horman
nhorman at redhat.com
Wed Jun 1 18:42:51 UTC 2016
On Wed, Jun 01, 2016 at 03:09:13PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman at redhat.com]
> > Sent: Wednesday, June 01, 2016 10:18 AM
> > To: Kershner, David A
> > Cc: corbet at lwn.net; tglx at linutronix.de; mingo at redhat.com;
> > hpa at zytor.com; gregkh at linuxfoundation.org; Arfvidson, Erik; Sell, Timothy
> > C; hofrat at osadl.org; dzickus at redhat.com; jes.sorensen at redhat.com;
> > Curtin, Alexander Paul; janani.rvchndrn at gmail.com;
> > sudipm.mukherjee at gmail.com; prarit at redhat.com; Binder, David Anthony;
> > dan.j.williams at intel.com; linux-kernel at vger.kernel.org; linux-
> > doc at vger.kernel.org; driverdev-devel at 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 <Timothy.Sell at unisys.com>
> > >
> > > 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 at unisys.com>
> > > Signed-off-by: David Kershner <david.kershner at unisys.com>
> > > ---
> > > 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
More information about the devel
mailing list