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

Sell, Timothy C Timothy.Sell at unisys.com
Fri Jun 3 04:34:08 UTC 2016


> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx at linutronix.de]
> Sent: Wednesday, June 01, 2016 2:41 AM
> To: Kershner, David A
> Cc: corbet at lwn.net; mingo at redhat.com; hpa at zytor.com;
> gregkh at linuxfoundation.org; Arfvidson, Erik; Sell, Timothy C;
> hofrat at osadl.org; dzickus at redhat.com; jes.sorensen at redhat.com; Curtin,
> Alexander Paul; janani.rvchndrn at gmail.com;
> sudipm.mukherjee at gmail.com; prarit at redhat.com; Binder, David Anthony;
> nhorman at redhat.com; dan.j.williams at intel.com; linux-
> kernel at vger.kernel.org; linux-doc at vger.kernel.org; driverdev-
> devel at linuxdriverproject.org; *S-Par-Maintainer
> Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> unnecessary locking
> 
> On Tue, 31 May 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);
> 
> Why is this a rw_semaphore? It's only ever taken with down_write() and it's
> always the same context. Should be a mutex, right?
> 

Correct.  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?

> While at it, please convert the notifier_lock to a mutex as well.

Thanks.  Since this is visorbus-specific, we DO plan to address this in v3 of
this patchset, which will most-likely just be REMOVING notifier_lock altogether.

Tim Sell

> 
> Thanks,
> 
> 	tglx


More information about the devel mailing list