Messages in this thread | | | From | "Sell, Timothy C" <> | Subject | RE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking | Date | Thu, 9 Jun 2016 20:30:29 +0000 |
| |
> -----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
| |