hvconsole.c : possible problem

[Benjamin Herrenschmidt]

> Sorry, didn't follow that. The problem being solved is this: you do an
> hvsi_read() and store 15 bytes into 'buf' as follows:
> 	ff05000065 ff05000165 ff05000265
> (that's "AAA" in three packets). We just put all three of those packets
> into a single ring buffer, so now we have to go back and copy two and
> three into subsequent buffers.
>
> Another possibility is having an intermediary 256-byte buffer that we
> read into, then copy from there into buffers. That would probably
> simplify the code a bit, at the expense of always requiring at least
> one memcpy (which is not the case now) even if we just get one packet.
> I'll try that.

Why ? You don't _have_ to read 16 bytes by 16 bytes, do you ? You can
just read the right amount to fill the current packet header or data
payload.

Something like

	if (head_complete)
		size = remaining_data > 16 : 16 : remaining_data;
	else
		size = sizeof(header) - current_count;

> > A much simpler loop basically, asking only as much is needed to fill
> > the header if that isn't filled, then as much is needed to fill the
> > data (maxed out at 16) until the packet is filled. That would allow
> > to always deal only with packet boundaries, let throttling work
> > properly
> > at HV level, and simplify the code significantly...
>
> We don't get to ask for a specific number of bytes; we're told after we
> have them (up to 16). I'm noticing that the '16' parameter you're
> looking at is never used, and should be removed all the way back to
> drivers/char/hvc_console.c.

Ah, we always ask for 16 bytes... that I didn't get. Ok, then yes, use
a static 16 bytes buffer, and then move data from there to packets and
leave data in there if you have no packet available. The overflow logic
is dodgy, you should be able to do a single nice loop.

> >  - There is historical hunt for null characters in hvterm_get_chars(),
> > that paulus added because xmon was getting confused. Where do those
> > NULL
> > chars come from ? I doubt HV itself is adding anything to the stream so
> > I suppose it's whatever console program sits at the other hand ?
>
> That must be the case, yeah. HVSI data passes through the HV in the
> same way as normal console data, but if any munging occured there we
> would be screwed. So it must be that java "vterm" on the HMC.
>
> > I'm
> > asking because hvsi_read() doesn't do that, and so we end up with 2
> > routines having the exact same prototype: hvsi_read() and
> > hvc_hvterm_get_chars(), both calling the same HV routine, but not
> > doing exactly the same thing and I was wondering if this inconsistency
> > was still necessary.
>
> I've verified that those NULLs do still appear in the normal case, if
> that's what you mean. I have not seen any NULLs come through the HVSI
> code and they would be pretty noticeable. That is consistent with the
> vterm-did-it theory, and yes, that means we have different behavior.
>
> >  - The whole wait_for_packet() is completely synchronous (doesn't
> > schedule), that doesn't sound very good.
>
> True, hmm... I must have meant to come back to that later.
>
> I guess I'll find out, but is there a limit on how early you can call
> schedule() during boot (like kmalloc)? wait_for_packet() is only used
> during initialization which happens pretty early.

If you are really concerned, then check system_state == SYSTEM_RUNNING

Ben.


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