From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758241AbcFAORz (ORCPT ); Wed, 1 Jun 2016 10:17:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34332 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750697AbcFAORy (ORCPT ); Wed, 1 Jun 2016 10:17:54 -0400 Date: Wed, 1 Jun 2016 10:17:48 -0400 From: Neil Horman To: David Kershner Cc: corbet@lwn.net, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, gregkh@linuxfoundation.org, erik.arfvidson@unisys.com, timothy.sell@unisys.com, hofrat@osadl.org, dzickus@redhat.com, jes.sorensen@redhat.com, alexander.curtin@unisys.com, janani.rvchndrn@gmail.com, sudipm.mukherjee@gmail.com, prarit@redhat.com, david.binder@unisys.com, dan.j.williams@intel.com, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, driverdev-devel@linuxdriverproject.org, sparmaintainer@unisys.com Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary locking Message-ID: <20160601141748.GD4578@hmsreliant.think-freely.org> References: <1464748013-31309-1-git-send-email-david.kershner@unisys.com> <1464748013-31309-11-git-send-email-david.kershner@unisys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1464748013-31309-11-git-send-email-david.kershner@unisys.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Wed, 01 Jun 2016 14:17:53 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote: > From: Tim Sell > > Locking in the _interrupt() function is NOT necessary so long as we ensure > that interrupts have been stopped whenever we need to pause or resume the > device, which we now do. > > While a device is paused, we ensure that interrupts stay disabled, i.e. > that the _interrupt() function will NOT be called, yet remember the desired > state in devdata->interrupts_enabled if open() or close() are called are > called while the device is paused. Then when the device is resumed, we > restore the actual state of interrupts (i.e., whether _interrupt() is going > to be called or not) to the desired state in devdata->interrupts_enabled. > > Signed-off-by: Tim Sell > Signed-off-by: David Kershner > --- > drivers/staging/unisys/visorinput/visorinput.c | 57 +++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 10 deletions(-) > > diff --git a/drivers/staging/unisys/visorinput/visorinput.c b/drivers/staging/unisys/visorinput/visorinput.c > index 12a3570..9c00710 100644 > --- a/drivers/staging/unisys/visorinput/visorinput.c > +++ b/drivers/staging/unisys/visorinput/visorinput.c > @@ -66,6 +66,7 @@ struct visorinput_devdata { > struct rw_semaphore lock_visor_dev; /* lock for dev */ > struct input_dev *visorinput_dev; > bool paused; > + bool interrupts_enabled; > unsigned int keycode_table_bytes; /* size of following array */ > /* for keyboard devices: visorkbd_keycode[] + visorkbd_ext_keycode[] */ > unsigned char keycode_table[0]; > @@ -228,7 +229,21 @@ static int visorinput_open(struct input_dev *visorinput_dev) > return -EINVAL; > } > dev_dbg(&visorinput_dev->dev, "%s opened\n", __func__); > + > + /* > + * 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); > + devdata->interrupts_enabled = true; > + if (devdata->paused) > + goto out_unlock; Don't you want to wait until you actually enable interrupts here to set interrupts_enabled to true? Otherwise, if devdata->paused is true, you will be out of sync. > visorbus_enable_channel_interrupts(devdata->dev); > + > +out_unlock: > + up_write(&devdata->lock_visor_dev); > return 0; > } > > @@ -243,7 +258,22 @@ static void visorinput_close(struct input_dev *visorinput_dev) > return; > } > dev_dbg(&visorinput_dev->dev, "%s closed\n", __func__); > + > + /* > + * If we're not paused, really disable interrupts. > + * Regardless of whether we are paused, set a flag indicating > + * interrupts should be disabled so when we resume we will > + * not re-enable them. > + */ > + > + down_write(&devdata->lock_visor_dev); > + devdata->interrupts_enabled = false; > + if (devdata->paused) > + goto out_unlock; Ditto to my above comment > visorbus_disable_channel_interrupts(devdata->dev); > + > +out_unlock: > + up_write(&devdata->lock_visor_dev); > } > > /* > @@ -438,10 +468,8 @@ visorinput_remove(struct visor_device *dev) > * in visorinput_channel_interrupt() > */ > > - down_write(&devdata->lock_visor_dev); > dev_set_drvdata(&dev->device, NULL); > unregister_client_input(devdata->visorinput_dev); > - up_write(&devdata->lock_visor_dev); > kfree(devdata); > } > > @@ -529,13 +557,7 @@ visorinput_channel_interrupt(struct visor_device *dev) > if (!devdata) > return; > > - down_write(&devdata->lock_visor_dev); > - if (devdata->paused) /* don't touch device/channel when paused */ > - goto out_locked; > - > visorinput_dev = devdata->visorinput_dev; > - if (!visorinput_dev) > - goto out_locked; > > while (visorchannel_signalremove(dev->visorchannel, 0, &r)) { > scancode = r.activity.arg1; > @@ -611,8 +633,6 @@ visorinput_channel_interrupt(struct visor_device *dev) > break; > } > } > -out_locked: > - up_write(&devdata->lock_visor_dev); > } > > static int > @@ -632,6 +652,14 @@ visorinput_pause(struct visor_device *dev, > rc = -EBUSY; > goto out_locked; > } > + if (devdata->interrupts_enabled) > + visorbus_disable_channel_interrupts(dev); > + > + /* > + * due to above, at this time no thread of execution will be > + * in visorinput_channel_interrupt() > + */ > + > devdata->paused = true; > complete_func(dev, 0); > rc = 0; > @@ -659,6 +687,15 @@ visorinput_resume(struct visor_device *dev, > } > devdata->paused = false; > complete_func(dev, 0); > + > + /* > + * Re-establish calls to visorinput_channel_interrupt() if that is > + * the desired state that we've kept track of in interrupts_enabled > + * while the device was paused. > + */ > + if (devdata->interrupts_enabled) > + visorbus_enable_channel_interrupts(dev); > + Unless I'm mistaken, it seems that visorinput_pause and visorinput_open or close can be called in parallel on different cpus. As such the state of interrupts_enabled may change during the execution of this function, which would lead to interrupts not getting properly enabled. > rc = 0; > out_locked: > up_write(&devdata->lock_visor_dev); > -- > 1.9.1 >