Re: [PATCH] Open Firmware device tree virtual filesystem

From: Pekka Enberg
Date: Sun Dec 31 2006 - 08:24:33 EST


On 12/31/06, Mitch Bradley <wmb@xxxxxxxxxxxxx> wrote:
diff --git a/arch/i386/kernel/ofw_fs.c b/arch/i386/kernel/ofw_fs.c
new file mode 100644
index 0000000..30ca359
--- /dev/null
+++ b/arch/i386/kernel/ofw_fs.c
@@ -0,0 +1,261 @@
+/* 1275 in little-endian ASCII (for IEEE 1275 - the Open Firmware Standard) */
+#define OFWFS_MAGIC 0x35373231

Please put this in <linux/magic.h>

On 12/31/06, Mitch Bradley <wmb@xxxxxxxxxxxxx> wrote:
+ case S_IFREG:
+ inode->i_fop = &ofwfs_property_operations;
+ inode->i_bytes = ((struct propval *)data)->length;
+ inode->i_size = (loff_t)(((struct propval *)data)->length);

As you need the struct twice, use a local variable.

On 12/31/06, Mitch Bradley <wmb@xxxxxxxxxxxxx> wrote:
+static int ofwfs_create_props(struct super_block *sb, struct dentry *dir,
+ phandle node)
+{
+ char propname[32];
+ int security, len;
+ struct propval *propval;
+ int ret = 0;
+ int flag;
+
+ propname[0] = '\0';
+
+ while ((void)callofw("nextprop", 3, 1, node, propname, propname,
+ &flag), flag == 1) {

Please do the call within the loop body and use an explicit bereak.

+ security = strncmp(propname, "security-", 9) == 0;
+ len = 0;

Redundant assignment, no?

+ if (!security)
+ (void)callofw("getproplen", 2, 1, node, propname, &len);

We don't use void casts to suppress return value in Linux. You can
just drop it (elsewhere too).

On 12/31/06, Mitch Bradley <wmb@xxxxxxxxxxxxx> wrote:
+out:
+ if (ret) {
+ WARN_ON(1);
+/*
+ ofwfs_remove_props(sb, dir);
+*/
+ }

Hmm?

On 12/31/06, Mitch Bradley <wmb@xxxxxxxxxxxxx> wrote:
+static int __init ofwfs_init(void)
+{
+ int ret;
+
+ ret = register_filesystem(&ofwfs_type);
+ if (ret)
+ return ret;
+
+ kern_mount(&ofwfs_type);

kern_mount can fail so you'd better do IS_ERR/PTR_ERR here.

diff --git a/include/asm-i386/callofw.h b/include/asm-i386/callofw.h
new file mode 100644
index 0000000..594cb63
--- /dev/null
+++ b/include/asm-i386/callofw.h
+#include <linux/types.h>
+
+typedef void *phandle;

The typedef seems useless and confusing at best. Can we drop it?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/