Re: [PATCH 2/3] mmc: Export ios settings for a host through debugfs

[Haavard Skinnemoen]

Pierre Ossman <drzeus-mmc@xxxxxxxxx> wrote:
> On Thu, 26 Jun 2008 13:09:48 +0200
> Haavard Skinnemoen <haavard.skinnemoen@xxxxxxxxx> wrote:

> > +static void mmc_remove_ios_debugfs(struct mmc_ios *ios)
> > +{
> > +	if (ios->dbg_root) {
> 
> You can save a bit of indentation if you do:
> 
> 	if (!ios->dbg_root)
> 		return;

Good point.

> > +static int mmc_add_ios_debugfs(struct mmc_ios *ios, struct dentry *parent)
> > +{
> > +	struct dentry *dir;
> > +
> > +	dir = debugfs_create_dir("ios", parent);
> > +	if (!dir)
> > +		return -EBUSY;	/* or whatever */
> 
> Is it undefined what a NULL return means here? I would have expected an
> ERRPTR or ENOMEM.

Yeah...debugfs_create_dir() may return an ERRPTR only if debugfs is
disabled. All other errors return NULL.

We wouldn't be here unless the caller managed to create the host
directory, so checking IS_ERR() is redundant. Maybe ENOMEM would make
more sense, but I could also be that the directory already exists,
hence EBUSY is appropriate. We really don't know as long as debugfs
keeps us in the dark...

> > +#ifdef CONFIG_MMC_DEBUG_FS
> > +	struct dentry		*dbg_root;
> > +	struct dentry		*dbg_clock;
> > +	struct dentry		*dbg_vdd;
> > +	struct dentry		*dbg_bus_mode;
> > +	struct dentry		*dbg_chip_select;
> > +	struct dentry		*dbg_power_mode;
> > +	struct dentry		*dbg_bus_width;
> > +	struct dentry		*dbg_timing;
> > +#endif
> 
> Can't we use debugfs' own bookkeeping to keep track of them? Saves us a
> lot of noise in these structures.

You mean d_subdirs in struct dentry? I guess we could do that...though
I was sort of trying not to dig too deply into VFS internals...

Haavard
--
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/