| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1c4e9b86ed344191a2be886d571ca7b8@US-EXCH13-2.na.uis.unisys.com>
Date: Wed, 1 Jun 2016 15:09:13 +0000
From: "Sell, Timothy C" <Timothy.Sell@...sys.com>
To: Neil Horman <nhorman@...hat.com>,
"Kershner, David A" <David.Kershner@...sys.com>
CC: "corbet@....net" <corbet@....net>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"mingo@...hat.com" <mingo@...hat.com>,
"hpa@...or.com" <hpa@...or.com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"Arfvidson, Erik" <Erik.Arfvidson@...sys.com>,
"hofrat@...dl.org" <hofrat@...dl.org>,
"dzickus@...hat.com" <dzickus@...hat.com>,
"jes.sorensen@...hat.com" <jes.sorensen@...hat.com>,
"Curtin, Alexander Paul" <Alexander.Curtin@...sys.com>,
"janani.rvchndrn@...il.com" <janani.rvchndrn@...il.com>,
"sudipm.mukherjee@...il.com" <sudipm.mukherjee@...il.com>,
"prarit@...hat.com" <prarit@...hat.com>,
"Binder, David Anthony" <David.Binder@...sys.com>,
"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"driverdev-devel@...uxdriverproject.org"
<driverdev-devel@...uxdriverproject.org>,
*S-Par-Maintainer <SParMaintainer@...sys.com>
Subject: RE: [PATCH v2 10/27] staging: unisys: visorinput: remove unnecessary
locking
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@...hat.com]
> Sent: Wednesday, June 01, 2016 10:18 AM
> To: Kershner, David A
> Cc: corbet@....net; tglx@...utronix.de; mingo@...hat.com;
> hpa@...or.com; gregkh@...uxfoundation.org; Arfvidson, Erik; Sell, Timothy
> C; hofrat@...dl.org; dzickus@...hat.com; jes.sorensen@...hat.com;
> Curtin, Alexander Paul; janani.rvchndrn@...il.com;
> sudipm.mukherjee@...il.com; prarit@...hat.com; Binder, David Anthony;
> dan.j.williams@...el.com; linux-kernel@...r.kernel.org; linux-
> doc@...r.kernel.org; driverdev-devel@...uxdriverproject.org; *S-Par-
> Maintainer
> Subject: Re: [PATCH v2 10/27] staging: unisys: visorinput: remove
> unnecessary locking
>
> On Tue, May 31, 2016 at 10:26:36PM -0400, David Kershner wrote:
> > From: Tim Sell <Timothy.Sell@...sys.com>
> >
> > 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 <Timothy.Sell@...sys.com>
> > Signed-off-by: David Kershner <david.kershner@...sys.com>
> > ---
> > 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.
No. That's the intent of this code, to remember what the
state of interrupts SHOULD be (via devdata->interrupts_enabled), at
a point in time when interrupts can NOT be enabled, e.g., when
the device is paused (devdata->paused). After the device is resumed,
the real interrupt state (visorbus_enable_channel_interrupts())
will be synchronized with the remembered state.
>
> > 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
Ditto my response above.
>
> > 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.
>
You are correct that visorinput_pause and visorinput_open/close
can be called in parallel. However, as I alluded to in my comment
above, the intent of this code is to just restore the actual interrupt
state with the desired state (remembered in
devdata->interrupts_enabled). It's ok if interrupts don't get
enabled, because that would be our intent if there are no longer
any users of the device. (In this case visorinput_close() would have
been called and devdata->interrupts_enabled would have got set
false while the device was paused.)
Tim Sell
> > rc = 0;
> > out_locked:
> > up_write(&devdata->lock_visor_dev);
> > --
> > 1.9.1
> >
Powered by blists - more mailing lists