lkml.org 
[lkml]   [2016]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v4 09/29] staging: unisys: visorinput: remove unnecessary locking
Date
> -----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

\
 
 \ /
  Last update: 2016-06-09 23:01    [W:2.186 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site