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

[Sell, Timothy C]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
> Sent: Thursday, June 09, 2016 3:56 PM
> To: Sell, Timothy C
> Cc: corbet@xxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; Arfvidson, Erik; hofrat@xxxxxxxxx;
> dzickus@xxxxxxxxxx; jes.sorensen@xxxxxxxxxx; Curtin, Alexander Paul;
> janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx;
> prarit@xxxxxxxxxx; Binder, David Anthony; nhorman@xxxxxxxxxx;
> dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *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@xxxxxxxxxxxxx]
> > >
> > > 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