Re: [XenPPC] [Patch] Find serial device and platform type

[Maria Butrico <butrico@xxxxxxxxxxxxx>]

I'll address only some of the stuff which I think might survive my next 
revision (and is not a nit.)  It's too early to nit pick, although a 
good discussion on style would save time later on the nits.

Jimi Xenidis wrote:
> comments:
> On Apr 24, 2006, at 4:22 PM, Maria Butrico wrote:
>
> > This patch is intended as a preliminary request for comment, status 
> > update and a good place to keep it, in case I destroy my tree.  It's 
> > work in progress.
> Usually people prepend "[rfc]" to the subject.
> Thanks for being the community tester on this :)
> > > +struct global_hardware_stuff {
> > > +    int platform_type;
> > > +    #define UNKNOWN_PLATFORM        1
> > > +    #define PMAC_HARDWARE           2
> > > +    #define MAPLE_SIM               3
> > > +    #define MAPLE_HARDWARE          4
> > > +    #define JS2X_HARDWARE           5
> Hollis made a good point about the array with strings and init 
> function pointers.
> tho I did not want anyone to spend a lot of time on this code, I think 
> what use here will last a looong time.
> Also, I'm not a Huge fan of #define's in structures and indenting 
> _before_ the '#' is not proper C and am surprised it works without at 
> least a warning.
> > > +    struct {
> [...]
> > > +    } serial_port;
> this struct is valid outside of the structure you are defining here so 
> please define it elsewhere.
> You might also want to include other members that are in struct 
> ns16550_defaults.
Probably so, but would not this be a good time to rename this struct 
(ns16550)  to some serial_defaults or similar?
> > > @@ -152,6 +182,7 @@ static int __init of_getprop(int ph, con
> > >       if (rets[0] == OF_FAILURE) {
> > >          DBG("getprop 0x%x %s -> FAILURE\n", ph, name);
> > > +        if (buflen > 0) memset(buf, 0, 1);
> > >          return OF_FAILURE;
> > >      }
> hmm, I think we need to be able to depend on the interface not messing 
> with anything on failure.
> I'm not sure this would protect you from anything, particularly if the 
> property is expected to be an non-string.
> The callers need to check for failure.
Yes the caller need to check for failure, but there was code in boot_of 
that goes something like
string[0] = '\0'
of-getprotp(string)

the string is set by get prop unless the call fails.  This simplify the 
caller.  Of course one must take care of empty string returned as well 
(not in the patch you see, but the in next one if I remember), but again 
I prefer to simply the caller. 

> > > +static void search_by_property_value(int p, const char *prop_name,
> > > +                const char *prop_value)
> Not sure what you plan on doing with this function...
>
> > > +/*
> > > + * Returns the value of the property '/#size-cells'
> > > + */
> > > +int __init boot_of_get_sizecells_prop()
> Sadly, it does not work which way.
> The size cell of a property is either defined by your package or an 
> ancestral package, not necessarily in the root. it gets even trickier 
> for the "regs" property where an ancestor's definition over-rides the 
> package's
> > > +{
> > > +    int of_root;
> > > +    u32 cell = 1;
> > > +    +    of_root = of_finddevice("/");
> > > +    if (OF_FAILURE != of_root)
> if this fails, you panic()
Is this a statement, a suggestion or a problem?

> > > +int __init boot_of_serial()
> > >
> > > +    char ofout_path[256] = {0,};
> this is a call to memset please just assign ofout_path[0] = '\0' later.
>
> > > +    const char serial[] = "serial";
> > > +    const char macio[] = "mac-io";
> > > +    const char escc[] = "escc";
> These are copies, since you need to know how big they are (with the 
> possible exception of serial) they should be pointers rather than arrays.
> The discovery code seems a little dubious but I know its heritage and 
> will let it slide.
> > > diff -r 56e1802303e5 xen/include/xen/sched.h
> > > --- a/xen/include/xen/sched.h    Wed Apr 12 15:41:55 2006 -0500
> > > +++ b/xen/include/xen/sched.h    Fri Apr 21 10:41:42 2006 -0400
> > > @@ -392,6 +392,10 @@ extern struct domain *domain_list;
> > >   /* Domain is being debugged by controller software. */
> > >  #define _DOMF_debugging        4
> > >  #define DOMF_debugging         (1UL<<_DOMF_debugging)
> > > + /* run domain in prob mode */
> > > +#define _DOMF_prob      7
> > > +#define DOMF_prob       (1UL<<_DOMF_prob)
> > > +
> There needs to be a new member in "struct arch_domain" and a new bit 
> representing this state should be there.
> -JX


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel