hvc_console & interrupts: reworked patch

[Ryan Arnold]

On Tue, 2004-04-13 at 03:21, Benjamin Herrenschmidt wrote:

> This patch is _TOTALLY UNTESTED_, I'll do that tomorrow, but at least
> the chosen approach can be discussed here in the meantime. So I rewrote
> Ryan's patch in a different way that, of course, I personally prefer ;)

Ben, I've given your patch a little look and see some things I'm
concerned about but for the most part I think your solution is quite
clever.

1.) A printk test reveals that several hvc_open calls are made before
any hvc_close calls are paired against them this brings up the following
concerns:

drivers/char/hvc_console.c line 109
        if (irq != NO_IRQ)
                request_irq(irq, hvc_handle_interrupt, SA_INTERRUPT,
"hvc_console", hp);

What are the implications of requesting the irq with every hvc_open
rather than when hvc_count == 1?  Is a redundant request harmless?  This
call is made every time a user program intends to output something to
the console.

2.) drivers/char/hvc_console.c line 140

 bail:
        spin_unlock_irqrestore(&hp->lock, flags);
        if (irq != NO_IRQ)
                free_irq(irq, hp);

Additionally the irq is freed with every hvc_close call that is made.
What is the effect of freeing the irq if there are still several
instances of programs that have the tty open?

This kind of behavior was the cause of a bug which prevented proper
interrupt behavior in my removed patch.  I'm interested if the same
behavior is exhibited in your patch (I will test it when my system is
available).

3.) I've experienced runtime debug warning messages when requesting the
irq in hvc_open in my patch and you seem to do the same thing.  I never
did figure out how/why the system thinks that request_irq is sleeping
inside of a spinlock when called from within hvc_open().  If we don't
request the irq there I don't know of an appropriate place.

4.) arch/ppc64/kernel/hvconsole.c line 740

irq_p = (unsigned int *)get_property(vty, "interrupts", 0);

This seems to return a valid [yet unexpected/undocumented] IRQ on
POWER-4 and this is how my patch broke antonb's system.  I'm have no
idea how firmware reacts with vty interrupts on POWER-4 and I'm
reluctant to use the interrupt driven mechanism on that hardware if it
does because I was led to believe that this wasn't actually supported
until recently on Power-5 systems, and not at all on Power-4.

Maybe we need to see what get_property() returns for bogus strings on
POWER-4.  If we can't just determine that the system is POWER-5 based on
the presence of a vty int some other method would have to be used.  Ben
do you have a power-4 to test this on?

5.)
> Ryan: I suppose the interrupt acts like an "edge" trigger that gets
> re-armed when HV returns 0 bytes on a read, right ?

Ben, yes this is the behavior.  Another interrupt will not be dispatched
by firmware (HV) until all the data from the original interrupt has been
removed from the firmware buffer (meaning HV returns 0 bytes on read).
This leads me to believe that we don't need to worry about enabling and
disabling interrupts.

6.) drivers/char/hvc_console.c line 357
                read_total += n;
                if (read_total >= 64) {
                        poll_mask |= HVC_POLL_QUICK;
                        break;
                }

This is an interesting technique which I like better than simply
rescheduling after every HV read which is wasteful.  Does the magic
value of 64 have something to do with TTY_THRESHOLD_THROTTLE being 128?
You obviously use it TTY throttle predictor anticipator.  The danger is
that when you push the data if you over estimated the amount of data you
could flip you just end up over writing the tty buffer.

If you underestimated then the TTY will never throttle and you'll never
get the TTY unthrottle callback, hence no hvc_kick() to continue reading
data.


7.) drivers/char/hvc_console.c line 328

                n = hvc_arch_get_chars(index, buf, count);
                if (n <= 0) {
                        /* XXX What is that supposed to do ? */
                        if (n == -EPIPE)
                                tty_hangup(tty);
                        break;
                }

On Power-5 systems the listening client adapter can go away at any time,
meaning there would be no-one sending chars and the HV returns H_Closed
which hvc_arch_get_chars() in arch/ppc64/hvconsole returns as 0.  So,
this n == -EPIPE doesn't seem to ever happen.

It may seem strange to ignore a negative return value on
hvc_arch_get_chars() but I think it may be necessary.

If it did return -EPIPE the hangup would the kill the connection of all
of the userland programs that have the tty open.  I don't think that the
tty_hangup is appropriate in this case since the vty is a passive server
who simply waits until there is a client adapter connected.  Hanging up
userland apps like this on Power-5 systems might cause problems when a
user switches between HMC console and HVCS console client adapters.

8.) Is there a reason you decided it would be better to converge the int
& polling mechanism into this single mechanism when we actually talked
about diverging the functions into separate tty_operation callbacks
[though I like your new method better]?  I'm still concerned about the
work_queue scheduling anomalies everyone hints at and I'm wondering if
that contributed to your decision since I still use them in another
driver.

I think it is overall a pretty interesting patch but I'd like to give it
a going over with some of my console tests on my power-5 system first.
I'll do that on Wednesday.

Ryan S. Arnold
IBM Linux Technology Center


** Sent via the linuxppc64-dev mail list. See http://lists.linuxppc.org/