Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417899 > unrolled thread

[PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking David Kershner <david.kershner@unisys.com> - 2016-06-08 23:30 +0200
    Re: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary  locking Thomas Gleixner <tglx@linutronix.de> - 2016-06-09 11:10 +0200
      RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-09 14:10 +0200
        RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary  locking Thomas Gleixner <tglx@linutronix.de> - 2016-06-09 22:00 +0200
          RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-09 22:40 +0200
          RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-11 05:40 +0200

#1417899 — [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

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 d67cd763..f633985 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;
 	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;
 	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);
+
 	rc = 0;
 out_locked:
 	up_write(&devdata->lock_visor_dev);
-- 
1.9.1

[toc] | [next] | [standalone]

#1418186 — Re: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

On Wed, 8 Jun 2016, David Kershner wrote:
> +	/*
> +	 * 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);

I think I asked this before, but I might have missed the answer.

Why is this a rw_sempahore? It's never taken with down_read and looking at the
usage sites it's simply a mutex, right?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1418274 — RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Thursday, June 09, 2016 5:01 AM
> To: Kershner, David A
> Cc: corbet@lwn.net; mingo@redhat.com; hpa@zytor.com;
> gregkh@linuxfoundation.org; Arfvidson, Erik; Sell, Timothy C;
> hofrat@osadl.org; dzickus@redhat.com; jes.sorensen@redhat.com; Curtin,
> Alexander Paul; janani.rvchndrn@gmail.com;
> sudipm.mukherjee@gmail.com; prarit@redhat.com; Binder, David Anthony;
> nhorman@redhat.com; dan.j.williams@intel.com; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org; driverdev-
> devel@linuxdriverproject.org; *S-Par-Maintainer
> Subject: Re: [PATCH v4 09/29] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Wed, 8 Jun 2016, David Kershner wrote:
> > +	/*
> > +	 * 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);
> 
> I think I asked this before, but I might have missed the answer.
> 
> Why is this a rw_sempahore? It's never taken with down_read and looking
> at the
> usage sites it's simply a mutex, right?
> 
> Thanks,
> 
> 	tglx

(Yes, I attempted to address this question in a post I made
6/3/2016 4:30 UTC, which I have basically pasted below.)

You are correct that this should be a mutex.

We have a local patch that addresses this, but would like
to submit this via a follow-on patchset if possible.  I'll explain.

Rationale: our intent for this patchset was to focus on the visorbus
driver ONLY.  The only reason visorinput got involved in the first place
was due to the visorbus change that necessitated that we remove the locking
from visorinput_channel_interrupt(), due to that now being called from atomic
context.

If the semaphore --> mutex change would have been as simple as it sounds,
we would have had NO problem including it with the next version (v3) of this
patchset.  But unfortunately, this change uncovered a latent defect, which
necessitated yet another patch.  (I know... hard to believe that something
this simple would do that, but it did.)  Rather than further complicating this
patchset, we thought it would be better to address the visorinput issues via a
separate follow-on patchset.

Is that acceptable for you?

Tim Sell

[toc] | [prev] | [next] | [standalone]

#1418613 — RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

On Thu, 9 Jun 2016, Sell, Timothy C wrote:
> > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> >
> > I think I asked this before, but I might have missed the answer.
> > 
> > Why is this a rw_sempahore? It's never taken with down_read and looking
> > at the usage sites it's simply a mutex, right?
> 
> If the semaphore --> mutex change would have been as simple as it sounds,
> we would have had NO problem including it with the next version (v3) of this
> patchset.  But unfortunately, this change uncovered a latent defect, which
> necessitated yet another patch.  (I know... hard to believe that something
> this simple would do that, but it did.)  Rather than further complicating this
> patchset, we thought it would be better to address the visorinput issues via a
> separate follow-on patchset.

That makes me curious. What's the issue? Functional is the mutex the same
thing as the r/w semaphore when the latter is only taken down_write and locked
and released by the same thread, which is the case as far as I can tell.
 
> Is that acceptable for you?

Please fix it before moving the drivers out of staging.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1418632 — RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Thursday, June 09, 2016 3:56 PM
> To: Sell, Timothy C
> Cc: corbet@lwn.net; mingo@redhat.com; hpa@zytor.com;
> gregkh@linuxfoundation.org; Arfvidson, Erik; hofrat@osadl.org;
> dzickus@redhat.com; jes.sorensen@redhat.com; Curtin, Alexander Paul;
> janani.rvchndrn@gmail.com; sudipm.mukherjee@gmail.com;
> prarit@redhat.com; Binder, David Anthony; nhorman@redhat.com;
> dan.j.williams@intel.com; linux-kernel@vger.kernel.org; linux-
> doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer; Kershner, David A
> Subject: RE: [PATCH v4 09/29] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Thu, 9 Jun 2016, Sell, Timothy C wrote:
> > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > >
> > > I think I asked this before, but I might have missed the answer.
> > >
> > > Why is this a rw_sempahore? It's never taken with down_read and
> looking
> > > at the usage sites it's simply a mutex, right?
> >
> > If the semaphore --> mutex change would have been as simple as it
> sounds,
> > we would have had NO problem including it with the next version (v3) of
> this
> > patchset.  But unfortunately, this change uncovered a latent defect, which
> > necessitated yet another patch.  (I know... hard to believe that something
> > this simple would do that, but it did.)  Rather than further complicating
> this
> > patchset, we thought it would be better to address the visorinput issues
> via a
> > separate follow-on patchset.
> 
> That makes me curious. What's the issue? Functional is the mutex the same
> thing as the r/w semaphore when the latter is only taken down_write and
> locked
> and released by the same thread, which is the case as far as I can tell.
> 

The issue: using it uninitialized (<blush>).

A semaphore appears to let you get away with it, but a mutex does NOT.
We had to shuffle some things around to get this right.  If you're
interested in a preview, you can find a patch in github at
https://github.com/davidker/unisys/commit/039e6e517b4a17e2d135a9df85cc1e24a39c2670.
The second bullet in that commit comment describes the scenario
where we were attempting to access the lock in visorinput_open()
before we had actually initialized it:

	* we canNOT get into visorinput_open() until the device
	structure is totally  initialized, by delaying the
	input_register_device() until the end of device initialization

I.e., before this patch, we WERE getting into visorinput_open()
during the call to input_register_device() that was done before 
device initialization was complete, which was BEFORE we initialized
the semaphore.

There is a 2nd follow-on patch that actually does the simple
semaphore --> mutex conversion at
https://github.com/davidker/unisys/commit/6f57ed62ae206c23c58ce4a016b08e15639ce9af.

> > Is that acceptable for you?
> 
> Please fix it before moving the drivers out of staging.

Absolutely.  We will probably push that patchset (containing the
2 github patches referenced above) within the next few days,
even if this visorbus patchset hasn't moved.

Thanks.

Tim Sell

> 
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1419901 — RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking

> > > > From: Thomas Gleixner [mailto:tglx@linutronix.de]
> > > >
> > > > I think I asked this before, but I might have missed the answer.
> > > >
> > > > Why is this a rw_sempahore? It's never taken with down_read and
> > looking
> > > > at the usage sites it's simply a mutex, right?
> > >
> > > If the semaphore --> mutex change would have been as simple as it
> > sounds,
> > > we would have had NO problem including it with the next version (v3) of
> > this
> > > patchset.  But unfortunately, this change uncovered a latent defect,
> which
> > > necessitated yet another patch.  (I know... hard to believe that
> something
> > > this simple would do that, but it did.)  Rather than further complicating
> > this
> > > patchset, we thought it would be better to address the visorinput issues
> > via a
> > > separate follow-on patchset.
> >
> > That makes me curious. What's the issue? Functional is the mutex the
> same
> > thing as the r/w semaphore when the latter is only taken down_write and
> > locked
> > and released by the same thread, which is the case as far as I can tell.
> >
> 
> The issue: using it uninitialized (<blush>).
> 
> A semaphore appears to let you get away with it, but a mutex does NOT.
> We had to shuffle some things around to get this right.  If you're
> interested in a preview, you can find a patch in github at
> https://github.com/davidker/unisys/commit/039e6e517b4a17e2d135a9df85
> cc1e24a39c2670.
> The second bullet in that commit comment describes the scenario
> where we were attempting to access the lock in visorinput_open()
> before we had actually initialized it:
> 
> 	* we canNOT get into visorinput_open() until the device
> 	structure is totally  initialized, by delaying the
> 	input_register_device() until the end of device initialization
> 
> I.e., before this patch, we WERE getting into visorinput_open()
> during the call to input_register_device() that was done before
> device initialization was complete, which was BEFORE we initialized
> the semaphore.
> 
> There is a 2nd follow-on patch that actually does the simple
> semaphore --> mutex conversion at
> https://github.com/davidker/unisys/commit/6f57ed62ae206c23c58ce4a016
> b08e15639ce9af.
> 
> > > Is that acceptable for you?
> >
> > Please fix it before moving the drivers out of staging.
> 
> Absolutely.  We will probably push that patchset (containing the
> 2 github patches referenced above) within the next few days,
> even if this visorbus patchset hasn't moved.
> 

The 2 patches referenced above (which include the bug fix, and the change
from semaphore to mutex) are included as patches #27 and #28 in the
patchset just submitted by David Kershner:

	[PATCH RESEND 00/28] staging: unisys: fix visorbus & visorinput issues raised by tglx

Thanks.

Tim Sell

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web