hvc_console & interrupts: reworked patch

[Benjamin Herrenschmidt]

On Wed, 2004-04-14 at 08:28, Ryan Arnold wrote:
> 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);

No, if you look properly, you'll notice that I only fill the "irq" local
variable on the first open and last close.

> 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?

Same comment

> 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.

Which is why I do it outside of the spinlock. request_irq can allocate
memory and get deep into procfs when setting up the irq affinity stuff,
so it must not be called within a spinlock.

> 4.) arch/ppc64/kernel/hvconsole.c line 740
>
> irq_p = (unsigned int *)get_property(vty, "interrupts", 0);

The above was just copied from your patch ;)

> 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.

Ok, I'll have a look. What does the RPA says ?

> 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?

I do.

> 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.

Yup.

> 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?

Yes, it looks good to use half, but testing may lead us toward a
different value.

> 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.

How so ? I don't need kick to continue reading data on a non-throttled
line. Just the interrupt or the 10ms timeout on non-interrupt setups.
Or is there a bug ?

> 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.

Yes. If we had some kind of hotplug, it would make sense, I don't think
we really want to do a hangup which is a bit too drastic imho.

> 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.

Yup.

> 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.

Separate tty operations would have been justified if the underlying
implementation was really different. After looking at it, it seems the
write path can be exactly the same, the only difference is the
interrupt 'notification' on reads, so I choose the least changes
approach, it's also the better on the line-counter ;)

Also, I prefer not loading the workqueues too much.

> 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.

Sure.

Ben.


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