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.
> 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
> 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.
> 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
