Andrea Arcangeli (arcangeli@mbox.queen.it)
Mon, 13 Apr 1998 17:11:31 +0200 (CEST)
On Sun, 12 Apr 1998, Philip Blundell wrote:
>Some work is still needed. The `preempt' stuff is still almost certainly
>broken - I haven't worked out yet whether I'm being stupid and not
>understanding how it worked, or whether it was really as disastrous as it it
>appeared.
The current 2.1.95 preempt stuff could be ugly but works fine and I
understand how it works since I wrote it myself.
> static inline int lp_char(char lpchar, int minor)
> {
>- int status;
> unsigned int wait = 0;
> unsigned long count = 0;
> struct lp_stats *stats;
>
>- for (;;) {
>- lp_yield(minor);
>- status = r_str (minor);
>- if (++count == LP_CHAR(minor))
>+ while (!LP_READY(minor, r_str(minor))) {
>+ if (++count == (LP_POLLING(minor)?LP_CHAR(minor):LP_INT_CHAR))
> return 0;
>- if (LP_POLLING(minor))
>- {
>- if (LP_READY(minor, status))
>- break;
>- } else {
>- if (!LP_READY(minor, status))
>- return 0;
>- else
>- break;
>- }
>+ lp_yield(minor);
> }
Wrong. If lp is not ready in interrupt printing must return without
continue to poll, I fixed this bug some days ago. We have the interrupt
that tell us when the printer is ready so we don' t need to poll. If there
is a bug is not here. Could be that the interrupt happens before we cli()
to interruptible_sleep_on() and so we miss the interrupt?
As second it' s better lp_yeld() at first of lp_char() before read the
status of the printer.
> w_dtr(minor, lpchar);
>@@ -222,9 +212,7 @@
> static void lp_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> {
> 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);
>+ up(&lp_dev->sem);
> }
I don't know the semaphore implementation. What are the advantages
against a wait_queue?
>-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)
Right.
> {
> unsigned long copy_size;
> unsigned long total_bytes_written = 0;
> unsigned long bytes_written;
> struct lp_struct *lp = &lp_table[minor];
>- unsigned char status;
>
> if (minor >= LP_NO)
> return -ENXIO;
>@@ -301,25 +288,24 @@
> current->timeout = jiffies + LP_TIME(minor);
> lp_schedule (minor);
> } else {
>- cli();
>- if (LP_PREEMPTED(minor)) {
>- sti();
This point is buggy since ppa is timer driven and could claim parport
after the LP_PREEMPTED() test (if interrupts are enabled) and so goes to
sleep waiting for a wakeup. Then lp will not detect that parport need
to be released and it will gain parport waiting for an interrupt that
could not occour since the printer could be offline or so on and ppa will
wait in starvation.
I have not time to check all your patch in detail now, I will try to look
in details tomorrow or this night, but please be not aggressive against
the preemptive stuff of lp since it works fine and it is delicate.
Andrea[s] Arcangeli
-- 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:36 EST