Re: [PATCH] Open Firmware device tree virtual filesystem

Pekka Enberg
2006-12-31T13:24:18Z

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?