> -----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 > > > - up(&dev->visordriver_callback_lock); > > - if (!visor_periodic_work_nextperiod(dev->periodic_work)) > > - put_device(&dev->device); > > + mod_timer(&dev->timer, jiffies + POLLJIFFIES_NORMALCHANNEL); > > } > > > > static void > > dev_start_periodic_work(struct visor_device *dev) > > { > > - if (dev->being_removed) > > + if (dev->being_removed || dev->timer_active) > > return; > > /* now up by at least 2 */ > > get_device(&dev->device); > > - if (!visor_periodic_work_start(dev->periodic_work)) > > - put_device(&dev->device); > > + dev->timer.expires = jiffies + POLLJIFFIES_NORMALCHANNEL; > > + add_timer(&dev->timer); > > + dev->timer_active = true; > > } > > > > static void > > dev_stop_periodic_work(struct visor_device *dev) > > { > > - if (visor_periodic_work_stop(dev->periodic_work)) > > - put_device(&dev->device); > > + if (!dev->timer_active) > > + return; > > + del_timer_sync(&dev->timer); > > + dev->timer_active = false; > > + put_device(&dev->device); > > } > > > > /** This is called automatically upon adding a visor_device (device_add), > or > > @@ -776,17 +771,9 @@ create_visor_device(struct visor_device *dev) > > dev->device.release = visorbus_release_device; > > /* keep a reference just for us (now 2) */ > > get_device(&dev->device); > > - dev->periodic_work = > > - visor_periodic_work_create(POLLJIFFIES_NORMALCHANNEL, > > - periodic_dev_workqueue, > > - dev_periodic_work, > > - dev, dev_name(&dev->device)); > > - if (!dev->periodic_work) { > > - POSTCODE_LINUX_3(DEVICE_CREATE_FAILURE_PC, > chipset_dev_no, > > - DIAG_SEVERITY_ERR); > > - err = -EINVAL; > > - goto err_put; > > - } > > + init_timer(&dev->timer); > > + dev->timer.data = (unsigned long)(dev); > > + dev->timer.function = dev_periodic_work; > > > > /* bus_id must be a unique name with respect to this bus TYPE > > * (NOT bus instance). That's why we need to include the bus > > @@ -1268,13 +1255,6 @@ visorbus_init(void) > > goto error; > > } > > > > - periodic_dev_workqueue = > create_singlethread_workqueue("visorbus_dev"); > > - if (!periodic_dev_workqueue) { > > - POSTCODE_LINUX_2(CREATE_WORKQUEUE_PC, > DIAG_SEVERITY_ERR); > > - err = -ENOMEM; > > - goto error; > > - } > > - > > /* This enables us to receive notifications when devices appear for > > * which this service partition is to be a server for. > > */ > > @@ -1297,10 +1277,6 @@ visorbus_exit(void) > > visorchipset_register_busdev(NULL, NULL, NULL); > > remove_all_visor_devices(); > > > > - flush_workqueue(periodic_dev_workqueue); /* better not be any > work! */ > > - destroy_workqueue(periodic_dev_workqueue); > > - periodic_dev_workqueue = NULL; > > - > > list_for_each_safe(listentry, listtmp, &list_all_bus_instances) { > > struct visor_device *dev = list_entry(listentry, > > struct visor_device, > > -- > > 1.9.1 > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
