| 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: <20160607135122.GC13083@neilslaptop.think-freely.org>
Date: Tue, 7 Jun 2016 09:51:22 -0400
From: Neil Horman <nhorman@...hat.com>
To: "Sell, Timothy C" <Timothy.Sell@...sys.com>
Cc: "Kershner, David A" <David.Kershner@...sys.com>,
"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 v3 11/30] staging: unisys: visorbus: use kernel timer
instead of workqueue
On Tue, Jun 07, 2016 at 01:47:13PM +0000, Sell, Timothy C wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@...hat.com]
> > Sent: Tuesday, June 07, 2016 9:40 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 v3 11/30] staging: unisys: visorbus: use kernel timer
> > instead of workqueue
> >
> > On Sat, Jun 04, 2016 at 01:27:11PM -0400, David Kershner wrote:
> > > From: Tim Sell <Timothy.Sell@...sys.com>
> > >
> > > A kernel timer is now used as the vehicle to periodically call the
> > > channel_interrupt function of registered visor drivers, instead of a
> > > workqueue.
> > >
> > > This simplifies a lot of things by making periodic_work.c and
> > > periodic_work.h no longer necessary. This change also means that the
> > > channel_interrupt() callbacks registered by visor drivers (via
> > > visorbus_register_visor_driver()) will now be called in atomic context
> > > (i.e., canNOT sleep) rather than kernel thread context (CAN sleep).
> > > Fortunately this did NOT necessitate any change to the existing
> > > channel_interrupt() callbacks, because none of them ever perform any
> > > operations that would be invalid in atomic context.
> > >
> > > Signed-off-by: Tim Sell <Timothy.Sell@...sys.com>
> > > Signed-off-by: David Kershner <david.kershner@...sys.com>
> > > ---
> > > drivers/staging/unisys/include/visorbus.h | 10 +++--
> > > drivers/staging/unisys/visorbus/visorbus_main.c | 54 +++++++---------------
> > ---
> > > 2 files changed, 21 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/drivers/staging/unisys/include/visorbus.h
> > b/drivers/staging/unisys/include/visorbus.h
> > > index 9baf1ec..9bb88bb 100644
> > > --- a/drivers/staging/unisys/include/visorbus.h
> > > +++ b/drivers/staging/unisys/include/visorbus.h
> > > @@ -34,8 +34,9 @@
> > > #include <linux/poll.h>
> > > #include <linux/kernel.h>
> > > #include <linux/uuid.h>
> > > +#include <linux/seq_file.h>
> > > +#include <linux/slab.h>
> > >
> > > -#include "periodic_work.h"
> > > #include "channel.h"
> > >
> > > struct visor_driver;
> > > @@ -126,8 +127,8 @@ struct visor_driver {
> > > * device: Device struct meant for use by the bus driver
> > > * only.
> > > * list_all: Used by the bus driver to enumerate
> > devices.
> > > - * periodic_work: Device work queue. Private use by bus driver
> > > - * only.
> > > + * timer: Timer fired periodically to do interrupt-type
> > > + * activity.
> > > * being_removed: Indicates that the device is being removed
> > from
> > > * the bus. Private bus driver use only.
> > > * visordriver_callback_lock: Used by the bus driver to lock when handling
> > > @@ -157,7 +158,8 @@ struct visor_device {
> > > /* These fields are for private use by the bus driver only. */
> > > struct device device;
> > > struct list_head list_all;
> > > - struct periodic_work *periodic_work;
> > > + struct timer_list timer;
> > > + bool timer_active;
> > > bool being_removed;
> > > struct semaphore visordriver_callback_lock;
> > > bool pausing;
> > > diff --git a/drivers/staging/unisys/visorbus/visorbus_main.c
> > b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > index ebdd5de..e98e720 100644
> > > --- a/drivers/staging/unisys/visorbus/visorbus_main.c
> > > +++ b/drivers/staging/unisys/visorbus/visorbus_main.c
> > > @@ -19,7 +19,6 @@
> > > #include "visorbus.h"
> > > #include "visorbus_private.h"
> > > #include "version.h"
> > > -#include "periodic_work.h"
> > > #include "vbuschannel.h"
> > > #include "guestlinuxdebug.h"
> > > #include "vmcallinterface.h"
> > > @@ -116,7 +115,6 @@ struct bus_type visorbus_type = {
> > > .bus_groups = visorbus_bus_groups,
> > > };
> > >
> > > -static struct workqueue_struct *periodic_dev_workqueue;
> > > static long long bus_count; /** number of bus instances */
> > > /** ever-increasing */
> > >
> > > @@ -222,10 +220,6 @@ visorbus_release_device(struct device *xdev)
> > > {
> > > struct visor_device *dev = to_visor_device(xdev);
> > >
> > > - if (dev->periodic_work) {
> > > - visor_periodic_work_destroy(dev->periodic_work);
> > > - dev->periodic_work = NULL;
> > > - }
> > > if (dev->visorchannel) {
> > > visorchannel_destroy(dev->visorchannel);
> > > dev->visorchannel = NULL;
> > > @@ -530,35 +524,36 @@ unregister_driver_attributes(struct visor_driver
> > *drv)
> > > }
> > >
> > > static void
> > > -dev_periodic_work(void *xdev)
> > > +dev_periodic_work(unsigned long __opaque)
> > > {
> > > - struct visor_device *dev = xdev;
> > > + struct visor_device *dev = (struct visor_device *)__opaque;
> > > struct visor_driver *drv = to_visor_driver(dev->device.driver);
> > >
> > > - down(&dev->visordriver_callback_lock);
> > > if (drv->channel_interrupt)
> > > drv->channel_interrupt(dev);
> > visorinput_channel_interrupt uses down_write (and other
> > channel_interrupt
> > methods may do the same). While its legal to use semaphores in interrupt
> > context, you need to convert that/those calls to the trylock variants and
> > handle
> > a failed lock to avoid blocking when in interrupt context, which moving to a
> > timer handler puts you in.
>
> visorinput_channel_interrupt() USED to use down_write(), but I removed
> this in the previous patch in this set ([PATCH v3 10/30] staging: unisys:
> visorinput: remove unnecessary locking). None of the other
> channel_interrupt functions use non-atomic locking. (It's possible they
> USED TO, but not anymore.)
>
> Tim Sell
>
Ah, so you did, thanks for the clarification.
Neil
Powered by blists - more mailing lists