[PARPORT] Re: lp interrupt bug (fwd)


Andrea Arcangeli (arcangeli@mbox.queen.it)
Thu, 7 May 1998 10:42:58 +0200 (CEST)


Hi Linus,
        here is my latest diff against lp.

1. lp_open() fix for SMP using test_and_set_bit() instead of using a not
   atomic operation.

2. Removed the unused and unuseful enable/disable_irq().

3. Added a private wait_queue to lp_struct to not depend on parport
   internals.

4. Fix the _static_ variable 'last' that if used static will break lp if
   used with more than one printer.

5. Mask all the STATS and CAREFUL stuff with two #undef. That stuff is
   not useful for the most of people I think, if somebody need it he need
   only to #define LP_STATS and LP_CAREFUL to 1.

6. Updated TODO-parport. I removed lp is a _mess_ since it' s not a mess
   for me ;-).

7. Some other minor cleanup.

I am working with three alpha-guys in order to fix the interrupt driven
lp. They report a complete stall. I made a patch that workaround that
stall adding a polling stage in lp_char() but I don' t like this
workaround. Now I am developing a program that will give me the exact
handshake diagram of their printers at runtime to try to understand better
where the problem is. They are using HP Deskjet printer in all cases.
Sound me really strange that HP printers talk with a buggy handshake!
Maybe the problem is the parallel port...

If you like to have a _working_ on all hardware lp I can send you the diff
against this one that implement the _working_ workaround.

Andrea[s] Arcangeli

--- linux/drivers/char/lp.c 1998/04/24 15:45:53 1.1
+++ linux/drivers/char/lp.c 1998/05/05 22:46:36
@@ -13,7 +13,7 @@
  * lp_read (Status readback) support added by Carsten Gross,
  * carsten@sol.wohnheim.uni-ulm.de
  * Support for parport by Philip Blundell <Philip.Blundell@pobox.com>
- * parport_sharing hacking by Andrea Arcangeli <arcangeli@mbox.queen.it>
+ * Parport sharing hacking by Andrea Arcangeli <arcangeli@mbox.queen.it>
  * Fixed kernel_(to/from)_user memory copy to check for errors
  * by Riccardo Facchetti <fizban@tin.it>
  */
@@ -75,6 +75,8 @@
 #include <linux/delay.h>
 
 #include <linux/parport.h>
+#undef LP_STATS
+#undef LP_NEED_CAREFUL
 #include <linux/lp.h>
 
 #include <asm/irq.h>
@@ -87,17 +89,23 @@
 struct lp_struct lp_table[LP_NO] =
 {
         [0 ... LP_NO-1] = {NULL, 0, LP_INIT_CHAR, LP_INIT_TIME, LP_INIT_WAIT,
- NULL, 0, 0, 0, {0}}
+ NULL,
+#ifdef LP_STATS
+ 0, 0, {0},
+#endif
+ NULL, 0}
 };
 
 /* Test if printer is ready (and optionally has no error conditions) */
+#ifdef LP_NEED_CAREFUL
 #define LP_READY(minor, status) \
- ((LP_F(minor) & LP_CAREFUL) ? _LP_CAREFUL_READY(status) : (status & LP_PBUSY))
-#define LP_CAREFUL_READY(minor, status) \
- ((LP_F(minor) & LP_CAREFUL) ? _LP_CAREFUL_READY(status) : 1)
+ ((LP_F(minor) & LP_CAREFUL) ? _LP_CAREFUL_READY(status) : ((status) & LP_PBUSY))
 #define _LP_CAREFUL_READY(status) \
- (status & (LP_PBUSY|LP_POUTPA|LP_PSELECD|LP_PERRORP)) == \
+ ((status) & (LP_PBUSY|LP_POUTPA|LP_PSELECD|LP_PERRORP)) == \
       (LP_PBUSY|LP_PSELECD|LP_PERRORP)
+#else
+#define LP_READY(minor, status) ((status) & LP_PBUSY)
+#endif
 
 #undef LP_DEBUG
 #undef LP_READ_DEBUG
@@ -108,8 +116,8 @@
 {
        struct lp_struct *lps = (struct lp_struct *)handle;
 
- if (waitqueue_active (&lps->dev->wait_q))
- wake_up_interruptible(&lps->dev->wait_q);
+ if (waitqueue_active (&lps->wait_q))
+ wake_up_interruptible(&lps->wait_q);
 
        /* Don't actually release the port now */
        return 1;
@@ -157,31 +165,29 @@
 
 static inline int lp_char(char lpchar, int minor)
 {
- int status;
+ unsigned char status;
         unsigned int wait = 0;
         unsigned long count = 0;
+#ifdef LP_STATS
         struct lp_stats *stats;
+#endif
 
- for (;;) {
+ for (;;)
+ {
                 lp_yield(minor);
                 status = r_str (minor);
- if (++count == LP_CHAR(minor))
+ if (LP_READY(minor, status))
+ break;
+ if (!LP_POLLED(minor) || ++count == LP_CHAR(minor) ||
+ signal_pending(current))
                         return 0;
- if (LP_POLLING(minor))
- {
- if (LP_READY(minor, status))
- break;
- } else {
- if (!LP_READY(minor, status))
- return 0;
- else
- break;
- }
         }
 
         w_dtr(minor, lpchar);
+#ifdef LP_STATS
         stats = &LP_STAT(minor);
         stats->chars++;
+#endif
         /* must wait before taking strobe high, and after taking strobe
            low, according spec. Some printers need it, others don't. */
 #ifndef __sparc__
@@ -200,6 +206,8 @@
 #endif
         /* take strobe low */
         w_ctr(minor, LP_PSELECP | LP_PINITP);
+
+#ifdef LP_STATS
         /* update waittime statistics */
         if (count > stats->maxwait) {
 #ifdef LP_DEBUG
@@ -212,6 +220,7 @@
             stats->meanwait - count;
         stats->meanwait = (255 * stats->meanwait + count + 128) / 256;
         stats->mdev = ((127 * stats->mdev) + wait + 64) / 128;
+#endif
 
         return 1;
 }
@@ -220,13 +229,13 @@
 {
         struct lp_struct *lp_dev = (struct lp_struct *) dev_id;
 
- if (waitqueue_active (&lp_dev->dev->wait_q))
- wake_up_interruptible(&lp_dev->dev->wait_q);
+ if (waitqueue_active (&lp_dev->wait_q))
+ wake_up_interruptible(&lp_dev->wait_q);
 }
 
 static void lp_error(int minor)
 {
- if (LP_POLLING(minor) || LP_PREEMPTED(minor)) {
+ if (LP_POLLED(minor) || LP_PREEMPTED(minor)) {
                 current->state = TASK_INTERRUPTIBLE;
                 current->timeout = jiffies + LP_TIMEOUT_POLLED;
                 lp_parport_release(minor);
@@ -236,7 +245,7 @@
 }
 
 static int lp_check_status(int minor) {
- static unsigned char last = 0;
+ unsigned int last = lp_table[minor].last_error;
         unsigned char status = r_str(minor);
         if ((status & LP_POUTPA)) {
                 if (last != LP_POUTPA) {
@@ -256,6 +265,8 @@
         }
         else last = 0;
 
+ lp_table[minor].last_error = last;
+
         if (last != 0) {
                 if (LP_F(minor) & LP_ABORT)
                         return 1;
@@ -265,7 +276,7 @@
         return 0;
 }
 
-static inline int lp_write_buf(unsigned int minor, const char *buf, int count)
+static int lp_write_buf(unsigned int minor, const char *buf, int count)
 {
         unsigned long copy_size;
         unsigned long total_bytes_written = 0;
@@ -275,9 +286,11 @@
 
         if (minor >= LP_NO)
                 return -ENXIO;
- if (lp_table[minor].dev == NULL)
+ if (lp->dev == NULL)
                 return -ENXIO;
 
+ lp_table[minor].last_error = 0;
+
         do {
                 bytes_written = 0;
                 copy_size = (count <= LP_BUFFER_SIZE ? count : LP_BUFFER_SIZE);
@@ -289,55 +302,69 @@
                         if (lp_char(lp->lp_buffer[bytes_written], minor)) {
                                 --copy_size;
                                 ++bytes_written;
- lp_table[minor].runchars++;
+#ifdef LP_STATS
+ lp->runchars++;
+#endif
                         } else {
                                 int rc = total_bytes_written + bytes_written;
- if (lp_table[minor].runchars > LP_STAT(minor).maxrun)
- LP_STAT(minor).maxrun = lp_table[minor].runchars;
+
+#ifdef LP_STATS
+ if (lp->runchars > LP_STAT(minor).maxrun)
+ LP_STAT(minor).maxrun = lp->runchars;
                                 LP_STAT(minor).sleeps++;
+#endif
 
- if (LP_POLLING(minor)) {
- lp_polling:
+ if (signal_pending(current)) {
+ if (total_bytes_written + bytes_written)
+ return total_bytes_written + bytes_written;
+ else
+ return -EINTR;
+ }
+
+#ifdef LP_STATS
+ lp->runchars = 0;
+#endif
+
+ if (LP_POLLED(minor)) {
                                         if (lp_check_status(minor))
                                                 return rc ? rc : -EIO;
-#ifdef LP_DEBUG
- printk(KERN_DEBUG "lp%d sleeping at %d characters for %d jiffies\n", minor, lp_table[minor].runchars, LP_TIME(minor));
+ lp_polling:
+#if defined(LP_DEBUG) && defined(LP_STATS)
+ printk(KERN_DEBUG "lp%d sleeping at %d characters for %d jiffies\n", minor, lp->runchars, LP_TIME(minor));
 #endif
                                         current->state = TASK_INTERRUPTIBLE;
                                         current->timeout = jiffies + LP_TIME(minor);
                                         lp_schedule (minor);
                                 } else {
                                         cli();
- if (LP_PREEMPTED(minor)) {
+ if (LP_PREEMPTED(minor))
+ {
+ /*
+ * We can' t sleep on the interrupt
+ * since another pardevice need the port.
+ */
                                                 sti();
                                                 goto lp_polling;
                                         }
- enable_irq(lp->dev->port->irq);
- w_ctr(minor, LP_PSELECP|LP_PINITP|LP_PINTEN);
+ w_ctr(minor, LP_PSELECP | LP_PINITP | LP_PINTEN);
                                         status = r_str(minor);
- if ((!(status & LP_PACK) || (status & LP_PBUSY))
- && LP_CAREFUL_READY(minor, status)) {
+ if (!(status & LP_PACK) || (status & LP_PBUSY))
+ {
+ /*
+ * The interrupt is happened in the
+ * meantime so don' t wait for it.
+ */
                                                 w_ctr(minor, LP_PSELECP | LP_PINITP);
                                                 sti();
                                                 continue;
                                         }
                                         current->timeout = jiffies + LP_TIMEOUT_INTERRUPT;
- interruptible_sleep_on(&lp->dev->wait_q);
- disable_irq(lp->dev->port->irq);
+ interruptible_sleep_on(&lp->wait_q);
                                         w_ctr(minor, LP_PSELECP | LP_PINITP);
                                         sti();
                                         if (lp_check_status(minor))
                                                 return rc ? rc : -EIO;
                                 }
-
- lp_table[minor].runchars = 0;
-
- if (signal_pending(current)) {
- if (total_bytes_written + bytes_written)
- return total_bytes_written + bytes_written;
- else
- return -EINTR;
- }
                         }
                 }
 
@@ -356,10 +383,12 @@
         unsigned int minor = MINOR(file->f_dentry->d_inode->i_rdev);
         ssize_t retv;
 
+#ifdef LP_STATS
         if (jiffies-lp_table[minor].lastcall > LP_TIME(minor))
                 lp_table[minor].runchars = 0;
 
         lp_table[minor].lastcall = jiffies;
+#endif
 
          /* Claim Parport or sleep until it becomes available
           */
@@ -495,9 +524,8 @@
                 return -ENXIO;
         if ((LP_F(minor) & LP_EXIST) == 0)
                 return -ENXIO;
- if (LP_F(minor) & LP_BUSY)
+ if (test_and_set_bit(LP_BUSY_BIT_POS, &LP_F(minor)) & LP_BUSY)
                 return -EBUSY;
- LP_F(minor) |= LP_BUSY;
 
         MOD_INC_USE_COUNT;
 
@@ -543,8 +571,8 @@
 
         kfree_s(lp_table[minor].lp_buffer, LP_BUFFER_SIZE);
         lp_table[minor].lp_buffer = NULL;
- LP_F(minor) &= ~LP_BUSY;
         MOD_DEC_USE_COUNT;
+ LP_F(minor) &= ~LP_BUSY;
         return 0;
 }
 
@@ -581,12 +609,14 @@
                         else
                                 LP_F(minor) &= ~LP_ABORTOPEN;
                         break;
+#ifdef LP_NEED_CAREFUL
                 case LPCAREFUL:
                         if (arg)
                                 LP_F(minor) |= LP_CAREFUL;
                         else
                                 LP_F(minor) &= ~LP_CAREFUL;
                         break;
+#endif
                 case LPWAIT:
                         LP_WAIT(minor) = arg;
                         break;
@@ -609,6 +639,7 @@
                 case LPRESET:
                         lp_reset(minor);
                         break;
+#ifdef LP_STATS
                 case LPGETSTATS:
                         if (copy_to_user((int *) arg, &LP_STAT(minor),
                                         sizeof(struct lp_stats)))
@@ -617,6 +648,7 @@
                                 memset(&LP_STAT(minor), 0,
                                                 sizeof(struct lp_stats));
                         break;
+#endif
                  case LPGETFLAGS:
                          status = LP_F(minor);
                         if (copy_to_user((int *) arg, &status, sizeof(int)))
--- linux/include/linux/lp.h 1998/04/24 23:13:07 1.1
+++ linux/include/linux/lp.h 1998/05/05 22:12:43
@@ -20,11 +20,14 @@
 #define LP_EXIST 0x0001
 #define LP_SELEC 0x0002
 #define LP_BUSY 0x0004
+#define LP_BUSY_BIT_POS 2
 #define LP_OFFL 0x0008
 #define LP_NOPA 0x0010
 #define LP_ERR 0x0020
 #define LP_ABORT 0x0040
+#ifdef LP_NEED_CAREFUL
 #define LP_CAREFUL 0x0080
+#endif
 #define LP_ABORTOPEN 0x0100
 
 /* timeout for each character. This is relative to bus cycles -- it
@@ -67,14 +70,18 @@
                             or 0 for polling (no IRQ) */
 #define LPGETIRQ 0x0606 /* get the current IRQ number */
 #define LPWAIT 0x0608 /* corresponds to LP_INIT_WAIT */
+#ifdef LP_NEED_CAREFUL
 #define LPCAREFUL 0x0609 /* call with TRUE arg to require out-of-paper, off-
                             line, and error indicators good on all writes,
                             FALSE to ignore them. Default is ignore. */
+#endif
 #define LPABORTOPEN 0x060a /* call with TRUE arg to abort open() on error,
                             FALSE to ignore error. Default is ignore. */
 #define LPGETSTATUS 0x060b /* return LP_S(minor) */
 #define LPRESET 0x060c /* reset printer */
+#ifdef LP_STATS
 #define LPGETSTATS 0x060d /* get statistics (struct lp_stats) */
+#endif
 #define LPGETFLAGS 0x060e /* get status flags */
 
 /* timeout for printk'ing a timeout, in jiffies (100ths of a second).
@@ -90,11 +97,14 @@
 #define LP_WAIT(minor) lp_table[(minor)].wait /* strobe wait */
 #define LP_IRQ(minor) lp_table[(minor)].dev->port->irq /* interrupt # */
                                                         /* 0 means polled */
+#ifdef LP_STATS
 #define LP_STAT(minor) lp_table[(minor)].stats /* statistics area */
+#endif
 #define LP_BUFFER_SIZE 256
 
 #define LP_BASE(x) lp_table[(x)].dev->port->base
 
+#ifdef LP_STATS
 struct lp_stats {
         unsigned long chars;
         unsigned long sleeps;
@@ -103,6 +113,7 @@
         unsigned int meanwait;
         unsigned int mdev;
 };
+#endif
 
 struct lp_struct {
         struct pardevice *dev;
@@ -111,10 +122,13 @@
         unsigned int time;
         unsigned int wait;
         char *lp_buffer;
+#ifdef LP_STATS
         unsigned int lastcall;
         unsigned int runchars;
- unsigned int waittime;
         struct lp_stats stats;
+#endif
+ struct wait_queue *wait_q;
+ unsigned int last_error;
 };
 
 /*
@@ -160,7 +174,7 @@
  */
 #define LP_DELAY 50
 
-#define LP_POLLING(minor) (lp_table[(minor)].dev->port->irq == PARPORT_IRQ_NONE)
+#define LP_POLLED(minor) (lp_table[(minor)].dev->port->irq == PARPORT_IRQ_NONE)
 #define LP_PREEMPTED(minor) (lp_table[(minor)].dev->port->waithead != NULL)
 
 /*
--- linux/drivers/misc/TODO-parport 1998/04/25 10:40:28 1.1
+++ linux/drivers/misc/TODO-parport 1998/04/25 10:41:46
@@ -6,20 +6,13 @@
 
 2. A better lp.c:
 
- a) It's a _mess_
-
- b) ECP support would be nice. This can only work if both the port and
+ a) ECP support would be nice. This can only work if both the port and
       the printer support it.
 
- c) Errors could do with being handled better. There's no point logging a
- message every 10 seconds when the printer is out of paper.
-
- d) Handle status readback automatically. IEEE1284 printers can post status
+ b) Handle status readback automatically. IEEE1284 printers can post status
       bits when they have something to say. We should read out and deal
       with (maybe just log) whatever the printer wants to tell the world.
 
 3. Support more hardware (eg m68k, Sun bpp).
 
 4. A better PLIP (make use of bidirectional/ECP/EPP ports).
-
-

-- 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:42 EST