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

Groups > linux.kernel > #1412783

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

Show all headers | View raw

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, June 01, 2016 2:41 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 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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 00/27] Fixed issues raised by  tglx, then move visorbus to drivers/virt David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock a mutex David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
    Re: [PATCH v2 13/27] staging: unisys: visorbus: Make visordriver_callback_lock  a mutex Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
  [PATCH v2 27/27] drivers: Add visorbus to the drivers directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 17/27] staging: unisys: visorbus: vbusdeviceinfo function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 12/27] staging: unisys: visorbus: remove periodic_work.h/.c David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 06/27] staging: unisys: visorbus: modify format string to match argument David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 16/27] staging: unisys: Remove reference to unused STANDALONE_CLIENT David Kershner <david.kershner@unisys.com> - 2016-06-01 04:30 +0200
  [PATCH v2 05/27] staging: unisys: visorbus: remove unused struct David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 07/27] staging: unisys: visornic: Correct comment spelling mistake David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 24/27] staging: unisys: Move vbushelper.h to visorbus directory David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
      RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-03 06:40 +0200
    Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 16:20 +0200
      RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-01 17:10 +0200
        Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-01 20:50 +0200
          RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking "Sell, Timothy C" <Timothy.Sell@unisys.com> - 2016-06-02 07:10 +0200
            Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary  locking Neil Horman <nhorman@redhat.com> - 2016-06-02 14:50 +0200
  [PATCH v2 15/27] staging: unisys: visorbus: Remove unused functions David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 20/27] staging: unisys: visorbus: make visorchannel  function descriptions more kerneldoc-like Thomas Gleixner <tglx@linutronix.de> - 2016-06-01 08:50 +0200
  [PATCH v2 23/27] staging: unisys: visorbus: Add kerneldoc-style comments for visorbus API David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 04/27] staging: unisys: visorbus: remove unused module parameters David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 18/27] staging: unisys: visorbus: make function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 14/27] staging: unisys: visorbus: Remove unnecessary EXPORT_SYMBOL statements David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 02/27] staging: unisys: visorchipset change -1 return value David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 02/27] staging: unisys: visorchipset change -1 return  value Neil Horman <nhorman@redhat.com> - 2016-06-01 15:20 +0200
  [PATCH v2 08/27] staging: unisys: include: Remove thread-related enum members David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 22/27] staging: unisys: visorbus: Move visorbus-unique functions to private header David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 03/27] staging: unisys: iovmcall_gnuc.h change -1  return values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:40 +0200
  [PATCH v2 21/27] staging: unisys: visorbus: make visorchipset function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 01/27] staging: unisys: visorbus change -1 return values David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
    Re: [PATCH v2 01/27] staging: unisys: visorbus change -1 return  values Neil Horman <nhorman@redhat.com> - 2016-06-01 15:30 +0200
  [PATCH v2 19/27] staging: unisys: visorbus: make visorbus_private.h function descriptions more kerneldoc-like David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 11/27] staging: unisys: visorbus: use kernel timer instead of workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200
  [PATCH v2 09/27] staging: unisys: visorbus: removed unused periodic_test_workqueue David Kershner <david.kershner@unisys.com> - 2016-06-01 04:40 +0200

csiph-web