[parisc-linux-cvs] [PATCH] 32-bit PDC wrapper patch

[Matthew Wilcox]

On Thu, Mar 15, 2001 at 10:31:54AM -0700, Paul Bame wrote:
> = Too much magic, unnecessary
> = use of varargs... ick.  This only makes it worse :-)
> 
> Some clues in addition to loaded terms like "too much", "magic",
> "unnecessary" would be helpful guidance for someone to re-design
> our PDC interface.

Fair point.  My apologies.

> IMO, the use of varargs for PDC is almost as sensible as using it for
> printk.  The first two PDC args tell PDC how to interpret the
> rest of the args, much as the prink format string does.  Varargs
> leads to rather compact code (both C and object) which
> is amenable to doing algorithmic translations of its arguments when
> needed (and it's needed for the 64 on 32 case).  It see no compelling
> reason to replicate for each number of arguments passable to a PDC call.

I disagree.  I think it's entirely possible & cleaner to  have the
individual wrappers (pdc_*) do the argument setup before calling the
real_call function.  If we are going to keep the varargs, we should
pass the number of arguments which should be popped off the stack.
My recollection is that there's no guarantee that calling va_arg for
more arguments than were passed will work.

> Unfortunately ugliness and magic are absolutely required at some level
> of our code, because calling the firmware is unavoidably complex.  IMO
> (but then I wrote it) we are at a decent compromise where most of the
> ugliness (and "magic": constructing a stack frame by hand in C!) is
> hidden under mem_pdc_call() [I wish the alignment and phys/virt issues
> were hidden there too alas] Not that it's necessarily a good example, but
> hp-ux settled on a similar design.

> Or, to take this to logical extreme, if we had mem_pdc_call3()
> through mem_pdc_call_11() as separate calls, I could make the argument
> that we should also have mem_pdc_call3i(int p1, int p2, long arg1)
> plus mem_pdc_call3p(int p1, int p2, void *arg1) plus
> mem_pdc_call3r(int p1, int p2, RADDR *arg1) so that we
> could dispense further with the "magic" of passing everything as
> unsigned longs and gain stronger type checking.  But
> at this point the explosion of mem_pdc_call*()s gets so large that
> it makes sense to only craft the ones you need, defined
> with interfaces containing correct number of arguments and types,
> which is roughly what's in pdc.c today.

The way I look at this is that we have a (relatively) typesafe interface
which then communicates with the type-unsafe PDC.  The parts which
know about the type should handle all the checking, then convert into
unsigned longs which is what PDC accepts.  the current situation has
pdc_* doing some of the work, encoding some of the information into
the call to mem_pdc_call which then does more of the work, passing that
along to real32_call which does more of the work and then finally calls
real32_call_asm which is completely type-unsafe.  I just collapsed two
of the layers into one.


-- 
Revolutions do not require corporate support.