Re: [PATCH] Open Firmware device tree virtual filesystem

["Pekka Enberg" <penberg@cs.helsinki.fi>]

On 12/31/06, Mitch Bradley <wmb@firmworks.com> 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@firmworks.com> 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@firmworks.com> 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@firmworks.com> wrote:
> +out:
> +       if (ret) {
> +               WARN_ON(1);
> +/*
> +               ofwfs_remove_props(sb, dir);
> +*/
> +       }

Hmm?

On 12/31/06, Mitch Bradley <wmb@firmworks.com> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/