Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking

From	Neil Horman <nhorman@redhat.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking
Date	2016-06-01 16:20 +0200
Message-ID	<rFfa2-11j-19@gated-at.bofh.it> (permalink)
References	<rF44V-2oj-3@gated-at.bofh.it> <rF4eB-2rF-7@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> From: Tim Sell <Timothy.Sell@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@unisys.com>
> Signed-off-by: David Kershner <david.kershner@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.

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

Thread

[PATCH v2 00/27] Fixed issues raised by  tglx, then move visorbus to drivers/virt David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock a mutex David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    Re: [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock  a mutex Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
  [PATCH v2 27/27] drivers: Add visorbus to the drivers directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 17/27] staging: unisys: visorbus: vbusdeviceinfo function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 12/27] staging: unisys: visorbus: remove periodic_work.h/.c David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 06/27] staging: unisys: visorbus: modify format string to match argument David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 16/27] staging: unisys: Remove reference to unused STANDALONE_CLIENT David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 05/27] staging: unisys: visorbus: remove unused struct David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 07/27] staging: unisys: visornic: Correct comment spelling mistake David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 24/27] staging: unisys: Move vbushelper.h to visorbus directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
      RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-03 06:40 +0200
    Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 16:20 +0200
      RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-01 17:10 +0200
        Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 20:50 +0200
          RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-02 07:10 +0200
            Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-02 14:50 +0200
  [PATCH v2 15/27] staging: unisys: visorbus: Remove unused functions David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel  function descriptions more kerneldoc-like Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
  [PATCH v2 23/27] staging: unisys: visorbus: Add kerneldoc-style comments for visorbus API David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 04/27] staging: unisys: visorbus: remove unused module parameters David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 18/27] staging: unisys: visorbus: make function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 14/27] staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL statements David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 02/27] staging: unisys: visorchipset change -1 return value David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 02/27] staging: unisys: visorchipset change -1 return  value Neil Horman <nhorman@redhat.com> - 2016-06-01 15:20 +0200
  [PATCH v2 08/27] staging: unisys: include: Remove thread-related enum members David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 22/27] staging: unisys: visorbus: Move visorbus-unique functions to private header David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1  return values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:40 +0200
  [PATCH v2 21/27] staging: unisys: visorbus: make visorchipset function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 01/27] staging: unisys: visorbus change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return  values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:30 +0200
  [PATCH v2 19/27] staging: unisys: visorbus: make visorbus_private.h function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 11/27] staging: unisys: visorbus: use kernel timer instead of workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 09/27] staging: unisys: visorbus: removed unused periodic_test_workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200

csiph-web