WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-ppc-devel

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

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