On Tue, Jun 07, 2016 at 01:47:13PM +0000, Sell, Timothy C wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@xxxxxxxxxx] > > Sent: Tuesday, June 07, 2016 9:40 AM > > To: Kershner, David A > > Cc: corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; > > hpa@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Arfvidson, Erik; Sell, Timothy > > C; hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; jes.sorensen@xxxxxxxxxx; > > Curtin, Alexander Paul; janani.rvchndrn@xxxxxxxxx; > > sudipm.mukherjee@xxxxxxxxx; prarit@xxxxxxxxxx; Binder, David Anthony; > > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > Signed-off-by: David Kershner <david.kershner@xxxxxxxxxx> > > > --- > > > 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
