Philip Blundell (Philip.Blundell@pobox.com)
Mon, 29 Dec 1997 01:49:21 +0000
Hi.
As some of you know, there have been problems with sharing the port fairly
between parallel devices. This meant that, in particular a Zip drive would
tend to freeze the lp driver out for long periods of time, and printing would
essentially grind to a halt. It was hard to know when to release the port.
I've done some changes to the parport_share code, and I think I now have a
solution to this. Hopefully I've managed to implement it without breaking any
existing drivers, though they may need changes to actually make use of the new
features.
Two new functions have been introduced. One of them,
parport_claim_or_block(), just eliminates some of the boilerplate that was
ending up in every driver to handle having to wait when the port is in use.
Transient drivers should be able to do without a `wakeup' function again.
The other, parport_yield(), is the important one. It is intended to be
called by drivers whenever they _could_ release the port with no ill effects,
but could also go on working and make useful progress if they were allowed to.
It checks to see whether any other devices are currently sleeping in
parport_claim_or_block(), takes into account how long the current device has
been running for, and either allows it to continue or releases the port and
then calls parport_claim_or_block() to wait for it to become free again. If
need be you can request that parport_yield() should not block (in which case
it returns -EAGAIN when it would have done). Otherwise it returns 0 if the
port was yielded and 1 if not.
This scheme should, I hope, make simple drivers quite a lot simpler (no need
for any wakeup or preempt functions or wait queues), resolve Grant's various
worries about the features parport provides, and deal with Andrea's problem of
the slow printer. I have written the needed code in lp.c, I think, but as
ever I don't have the hardware to test it with.
(In apparent contradiction to what I said above, lp.c _does_ still have a
preempt() routine. This is needed because it can sleep waiting for an IRQ for
an indefinite length of time; there still needs to be some way for another
device to kick it off the bus.)
At the moment the timeslice for each device is hardwired to 10cs. This
probably ought to be tuneable at runtime, and maybe different for each device.
There are a couple of other ways the scheduling could be improved if it looks
like it might help.
Please check it out. I've attached the patch here; I'd be interested in any
feedback.
p.
Index: drivers/char/lp.c
===================================================================
RCS file: /disks/c7/repository/linux/drivers/char/lp.c,v
retrieving revision 1.5
diff -u -r1.5 lp.c
--- lp.c 1997/12/24 22:35:16 1.5
+++ lp.c 1997/12/29 01:30:27
@@ -17,13 +17,13 @@
*/
/* This driver should, in theory, work with any parallel port that has an
- * appropriate low-level driver; all I/O is done through the parport abstraction
- * layer. There is a performance penalty for this, but parallel ports are
- * comparitively low-speed devices anyway. It should be possible to eliminate or
- * reduce this overhead in the common case, as well.
+ * appropriate low-level driver; all I/O is done through the parport
+ * abstraction layer. There is a performance penalty for this, but parallel
+ * ports are comparitively low-speed devices anyway. It should be possible
+ * to eliminate or reduce this overhead in the common case, as well.
*
- * If this driver is built into the kernel, you can configure it using the kernel
- * command-line. For example:
+ * If this driver is built into the kernel, you can configure it using the
+ * kernel command-line. For example:
*
* lp=parport1,none,parport2 (bind lp0 to parport1, disable lp1 and
* bind lp2 to parport2)
@@ -32,16 +32,17 @@
* have printers attached, as determined
* by the IEEE-1284 autoprobe)
*
- * lp=reset (reset the printer during initialisation)
+ * lp=reset (reset the printer during
+ * initialisation)
*
* lp=off (disable the printer driver entirely)
*
- * If the driver is loaded as a module, similar functionality is available using
- * module parameters. The equivalent of the above commands would be:
+ * If the driver is loaded as a module, similar functionality is available
+ * using module parameters. The equivalent of the above commands would be:
*
- * # insmod lp.o parport=1,-1,2 (use -1 for disabled ports, since module
- * parameters do not allow you to mix textual
- * and numeric values)
+ * # insmod lp.o parport=1,-1,2 (use -1 for disabled ports, since
+ * module parameters do not allow you
+ * to mix textual and numeric values)
*
* # insmod lp.o autoprobe=1
*
@@ -102,75 +103,47 @@
#undef LP_DEBUG
#undef LP_READ_DEBUG
-/* --- parport support functions ------------------------------- */
-
-static inline void lp_parport_release (int minor)
-{
- parport_release (lp_table[minor].dev);
- lp_table[minor].should_relinquish = 0;
-}
-
-static inline void lp_parport_claim (int minor)
-{
- if (parport_claim (lp_table[minor].dev))
- sleep_on (&lp_table[minor].lp_wait_q);
-}
-
-static void lp_wakeup(void *ref)
-{
- struct lp_struct *lp_dev = (struct lp_struct *) ref;
-
- if (!waitqueue_active (&lp_dev->lp_wait_q))
- return; /* Wake up whom? */
-
- /* Claim the Parport */
- if (parport_claim(lp_dev->dev))
- return; /* Shouldn't happen */
-
- wake_up(&lp_dev->lp_wait_q);
-}
+/* --- parport support ----------------------------------------- */
static int lp_preempt(void *handle)
{
- struct lp_struct *lps = (struct lp_struct *)handle;
+ struct lp_struct *lps = (struct lp_struct *)handle;
- if (waitqueue_active (&lps->lp_wait_q))
- wake_up_interruptible(&lps->lp_wait_q);
+ /* Just remember that someone wants the port */
+ lps->should_relinquish = 1;
- /* Just remember that someone wants the port */
- lps->should_relinquish = 1;
+ if (waitqueue_active (&lps->lp_wait_q))
+ wake_up_interruptible(&lps->lp_wait_q);
- /* Don't actually release the port now */
- return 1;
+ /* Don't actually release the port now */
+ return 1;
}
+
/* --- low-level port access ----------------------------------- */
#define r_dtr(x) (parport_read_data(lp_table[(x)].dev->port))
#define r_str(x) (parport_read_status(lp_table[(x)].dev->port))
#define w_ctr(x,y) do { parport_write_control(lp_table[(x)].dev->port, (y)); } while (0)
#define w_dtr(x,y) do { parport_write_data(lp_table[(x)].dev->port, (y)); } while (0)
+#define lp_parport_release(x) do { parport_release(lp_table[(x)].dev); } while (0);
+#define lp_parport_claim(x) do { parport_claim_or_block(lp_table[(x)].dev); } while (0);
-static inline void lp_schedule (int minor)
+static __inline__ void lp_schedule (int minor)
{
- if (lp_table[minor].should_relinquish) {
- lp_parport_release (minor);
- schedule ();
- lp_parport_claim (minor);
- }
- else
+ if (parport_yield (lp_table[minor].dev, 1) == 1 && need_resched)
schedule ();
}
static int lp_reset(int minor)
{
int retval;
- lp_parport_claim(minor);
+ lp_parport_claim (minor);
w_ctr(minor, LP_PSELECP);
- udelay(LP_DELAY);
+ udelay (LP_DELAY);
w_ctr(minor, LP_PSELECP | LP_PINITP);
retval = r_str(minor);
- lp_parport_release(minor);
+ lp_parport_release (minor);
return retval;
}
@@ -188,10 +161,9 @@
struct lp_stats *stats;
do {
- status = r_str(minor);
+ status = r_str (minor);
count++;
- if (need_resched || lp_table[minor].should_relinquish)
- lp_schedule (minor);
+ lp_schedule (minor);
} while (!LP_READY(minor, status) && count < LP_CHAR(minor));
if (count == LP_CHAR(minor))
@@ -246,7 +218,7 @@
static void lp_error(int minor)
{
- if (LP_POLLING(minor) || lp_table[minor].should_relinquish) {
+ if (LP_POLLING(minor)) {
current->state = TASK_INTERRUPTIBLE;
current->timeout = jiffies + LP_TIMEOUT_POLLED;
lp_schedule (minor);
@@ -368,7 +340,6 @@
lp_table[minor].lastcall = jiffies;
/* Claim Parport or sleep until it becomes available
- * (see lp_wakeup() for details)
*/
lp_parport_claim (minor);
@@ -411,7 +382,6 @@
unsigned int minor=MINOR(file->f_dentry->d_inode->i_rdev);
/* Claim Parport or sleep until it becomes available
- * (see lp_wakeup() for details)
*/
lp_parport_claim (minor);
@@ -703,7 +673,7 @@
int lp_register(int nr, struct parport *port)
{
lp_table[nr].dev = parport_register_device(port, "lp",
- lp_preempt, lp_wakeup,
+ lp_preempt, NULL,
lp_interrupt,
PARPORT_DEV_TRAN,
(void *) &lp_table[nr]);
Index: drivers/misc/parport_share.c
===================================================================
RCS file: /disks/c7/repository/linux/drivers/misc/parport_share.c,v
retrieving revision 1.8
diff -u -r1.8 parport_share.c
--- parport_share.c 1997/12/24 22:37:33 1.8
+++ parport_share.c 1997/12/29 01:33:51
@@ -3,10 +3,11 @@
*
* Authors: David Campbell <campbell@tirian.che.curtin.edu.au>
* Tim Waugh <tim@cyberelk.demon.co.uk>
- * Jose Renau <renau@acm.org>
+ * Jose Renau <renau@acm.org>
+ * Philip Blundell <philb@gnu.org>
*
* based on work by Grant Guenther <grant@torque.net>
- * and Philip Blundell <Philip.Blundell@pobox.com>
+ * and Philip Blundell
*/
#include <linux/tasks.h>
@@ -18,6 +19,7 @@
#include <linux/ioport.h>
#include <linux/kernel.h>
#include <linux/malloc.h>
+#include <linux/sched.h>
#include <linux/config.h>
@@ -27,6 +29,8 @@
#undef PARPORT_PARANOID
+#define PARPORT_DEFAULT_TIMESLICE (HZ/10)
+
static struct parport *portlist = NULL, *portlist_tail = NULL;
static int portcount = 0;
@@ -48,8 +52,8 @@
void parport_null_intr_func(int irq, void *dev_id, struct pt_regs *regs)
{
- /* NULL function - Does nothing */
- return;
+ /* Null function - does nothing. IRQs are pointed here whenever
+ there is no real handler for them. */
}
struct parport *parport_register_port(unsigned long base, int irq, int dma,
@@ -58,9 +62,8 @@
struct parport *tmp;
/* Check for a previously registered port.
- * NOTE: we will ignore irq and dma if we find a previously
- * registered device.
- */
+ NOTE: we will ignore irq and dma if we find a previously
+ registered device. */
for (tmp = portlist; tmp; tmp = tmp->next) {
if (tmp->base == base)
return tmp;
@@ -91,7 +94,7 @@
}
sprintf(tmp->name, "parport%d", portcount);
- /* Here we chain the entry to our list. */
+ /* Chain the entry to our list. */
if (portlist_tail)
portlist_tail->next = tmp;
portlist_tail = tmp;
@@ -203,6 +206,9 @@
inc_parport_count();
port->ops->inc_use_count();
+ init_waitqueue(&tmp->wait_q);
+ tmp->timeslice = PARPORT_DEFAULT_TIMESLICE;
+
return tmp;
}
@@ -210,15 +216,18 @@
{
struct parport *port;
+#ifdef PARPORT_PARANOID
if (dev == NULL) {
printk(KERN_ERR "parport_unregister_device: passed NULL\n");
return;
}
+#endif
port = dev->port;
if (port->cad == dev) {
- printk(KERN_WARNING "%s: refused to unregister currently active device %s.\n", port->name, dev->name);
+ printk(KERN_WARNING "%s: refused to unregister "
+ "currently active device %s.\n", port->name, dev->name);
return;
}
@@ -247,94 +256,131 @@
int parport_claim(struct pardevice *dev)
{
- struct pardevice *pd1;
+ struct pardevice *oldcad;
+ struct parport *port = dev->port;
- if (dev->port->cad == dev) {
+ if (port->cad == dev) {
printk(KERN_INFO "%s: %s already owner\n",
dev->port->name,dev->name);
return 0;
}
/* Preempt any current device */
- pd1 = dev->port->cad;
- if (dev->port->cad) {
- if (dev->port->cad->preempt) {
- if (dev->port->cad->preempt(dev->port->cad->private))
+ if ((oldcad = port->cad)) {
+ if (oldcad->preempt) {
+ if (oldcad->preempt(oldcad->private))
return -EAGAIN;
- dev->port->ops->save_state(dev->port, dev->state);
+ port->ops->save_state(port, dev->state);
} else
return -EAGAIN;
- }
- /* Watch out for bad things */
- if (dev->port->cad != pd1) {
- printk(KERN_WARNING "%s: death while preempting %s\n",
- dev->port->name, dev->name);
- if (dev->port->cad)
- return -EAGAIN;
+ if (port->cad != oldcad) {
+ printk(KERN_WARNING
+ "%s: %s released port when preempted!\n",
+ port->name, oldcad->name);
+ if (port->cad)
+ return -EAGAIN;
+ }
}
/* Now we do the change of devices */
- dev->port->cad = dev;
+ port->cad = dev;
/* Swap the IRQ handlers. */
- if (dev->port->irq != PARPORT_IRQ_NONE) {
- if (pd1 && pd1->irq_func) {
- free_irq(dev->port->irq,pd1->private);
- request_irq(dev->port->irq, parport_null_intr_func,
- SA_INTERRUPT, dev->port->name, NULL);
+ if (port->irq != PARPORT_IRQ_NONE) {
+ if (oldcad && oldcad->irq_func) {
+ free_irq(port->irq, oldcad->private);
+ request_irq(port->irq, parport_null_intr_func,
+ SA_INTERRUPT, port->name, NULL);
}
if (dev->irq_func) {
- free_irq(dev->port->irq, NULL);
- request_irq(dev->port->irq, dev->irq_func,
+ free_irq(port->irq, NULL);
+ request_irq(port->irq, dev->irq_func,
SA_INTERRUPT, dev->name, dev->private);
}
}
/* Restore control registers */
- dev->port->ops->restore_state(dev->port, dev->state);
+ port->ops->restore_state(port, dev->state);
+
+ dev->time = jiffies;
return 0;
}
+int parport_claim_or_block(struct pardevice *dev)
+{
+ int r;
+ dev->time = jiffies;
+ atomic_inc(&dev->port->waiters);
+ r = parport_claim(dev);
+ if (r == -EAGAIN)
+ sleep_on(&dev->wait_q);
+ atomic_dec(&dev->port->waiters);
+ return r;
+}
+
void parport_release(struct pardevice *dev)
{
- struct pardevice *pd1;
+ struct parport *port = dev->port;
+ struct pardevice *pd, *oldest = NULL;
+ unsigned long int oldest_time = 0;
/* Make sure that dev is the current device */
- if (dev->port->cad != dev) {
- printk(KERN_WARNING "%s: %s tried to release parport when not owner\n", dev->port->name, dev->name);
+ if (port->cad != dev) {
+ printk(KERN_WARNING "%s: %s tried to release parport "
+ "when not owner\n", port->name, dev->name);
return;
}
- dev->port->cad = NULL;
+ port->cad = NULL;
/* Save control registers */
- dev->port->ops->save_state(dev->port, dev->state);
+ port->ops->save_state(port, dev->state);
/* Point IRQs somewhere harmless. */
- if (dev->port->irq != PARPORT_IRQ_NONE && dev->irq_func) {
- free_irq(dev->port->irq, dev->private);
- request_irq(dev->port->irq, parport_null_intr_func,
- SA_INTERRUPT, dev->port->name, NULL);
+ if (port->irq != PARPORT_IRQ_NONE && dev->irq_func) {
+ free_irq(port->irq, dev->private);
+ request_irq(port->irq, parport_null_intr_func,
+ SA_INTERRUPT, port->name, NULL);
}
+ /* If anybody is waiting, find out who's been there longest and
+ then wake them up. */
+ if (atomic_read(&port->waiters)) {
+ for (pd = port->devices; pd; pd = pd->next) {
+ if (waitqueue_active(&pd->wait_q)) {
+ if ((jiffies - pd->time) >= oldest_time) {
+ oldest_time = jiffies - pd->time;
+ oldest = pd;
+ }
+ }
+ }
+ if (oldest == NULL) {
+ printk("%s: nobody (%d) is waiting.\n", port->name,
+ atomic_read(&port->waiters));
+ atomic_set(&port->waiters, 0);
+ } else {
+ wake_up(&oldest->wait_q);
+ return;
+ }
+ }
+
/* Walk the list, offering a wakeup callback to everybody other
- * than the lurker and the device that called us.
- */
- for (pd1 = dev->next; pd1; pd1 = pd1->next) {
- if (!(pd1->flags & PARPORT_DEV_LURK)) {
- if (pd1->wakeup) {
- pd1->wakeup(pd1->private);
+ than the lurker and the device that called us. Do this in two
+ steps to get some semblance of fairness. */
+ for (pd = dev->next; pd; pd = pd->next) {
+ if (!(pd->flags & PARPORT_DEV_LURK)) {
+ if (pd->wakeup) {
+ pd->wakeup(pd->private);
if (dev->port->cad)
return;
}
}
}
-
- for (pd1 = dev->port->devices; pd1 && pd1 != dev; pd1 = pd1->next) {
- if (!(pd1->flags & PARPORT_DEV_LURK)) {
- if (pd1->wakeup) {
- pd1->wakeup(pd1->private);
+ for (pd = dev->port->devices; pd; pd = pd->next) {
+ if (pd != dev && !(pd->flags & PARPORT_DEV_LURK)) {
+ if (pd->wakeup) {
+ pd->wakeup(pd->private);
if (dev->port->cad)
return;
}
@@ -342,10 +388,8 @@
}
/* Now give the lurker a chance.
- * There must be a wakeup callback because we checked for it
- * at registration.
- */
- if (dev->port->lurker && (dev->port->lurker != dev)) {
+ There must be a wakeup callback because we checked for it
+ at registration. */
+ if (dev->port->lurker && (dev->port->lurker != dev))
dev->port->lurker->wakeup(dev->port->lurker->private);
- }
}
Index: include/linux/parport.h
===================================================================
RCS file: /disks/c7/repository/linux/include/linux/parport.h,v
retrieving revision 1.5
diff -u -r1.5 parport.h
--- parport.h 1997/12/29 00:19:33 1.5
+++ parport.h 1997/12/29 01:19:12
@@ -149,6 +149,9 @@
struct pardevice *next;
struct pardevice *prev;
struct parport_state *state; /* saved status over preemption */
+ struct wait_queue *wait_q;
+ unsigned long int time;
+ unsigned long int timeslice;
};
/* Directory information for the /proc interface */
@@ -175,7 +178,8 @@
struct pardevice *lurker;
struct parport *next;
- unsigned int flags;
+ unsigned int flags;
+ atomic_t waiters;
struct parport_dir pdir;
struct parport_device_info probe_info;
@@ -230,6 +234,10 @@
*/
extern int parport_claim(struct pardevice *dev);
+/* parport_claim_or_block is the same, but sleeps if the port cannot be
+ claimed. It only fails in dire situations. */
+extern int parport_claim_or_block(struct pardevice *dev);
+
/* parport_release reverses a previous parport_claim. This can never fail,
* though the effects are undefined (except that they are bad) if you didn't
* previously own the port. Once you have released the port you should make
@@ -238,7 +246,19 @@
* If you mess with the port state (enabling ECP for example) you should
* clean up before releasing the port.
*/
+
extern void parport_release(struct pardevice *dev);
+
+extern __inline__ unsigned int parport_yield(struct pardevice *dev,
+ unsigned int block)
+{
+ unsigned long int timeslip = (jiffies - dev->time);
+ if ((timeslip < dev->timeslice) ||
+ (atomic_read(&dev->port->waiters) == 0))
+ return 1;
+ parport_release(dev);
+ return (block)?parport_claim_or_block(dev):parport_claim(dev);
+}
/* The "modes" entry in parport is a bit field representing the following
* modes.
-- To unsubscribe, send mail to: linux-parport-request@torque.net --
-- with the single word "unsubscribe" in the body of the message. --
This archive was generated by hypermail 2.0b3 on Wed 30 Dec 1998 - 10:17:15 EST