Re: [PATCH 5/6] mtdblock: Replace array of block cache info with pointers in struct mtd_info

[Ben Hutchings <bhutchings@solarflare.com>]

On Sun, 2010-01-10 at 12:34 +0200, Artem Bityutskiy wrote:
> On Tue, 2010-01-05 at 15:22 +0000, Ben Hutchings wrote:
> >  	mutex_lock(&mtdblk->cache_mutex);
> >  	write_cached_data(mtdblk);
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> > index 0f32a9b..5c1b1ac 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -177,6 +177,10 @@ struct mtd_info {
> >  	 */
> >  	struct backing_dev_info *backing_dev_info;
> >  
> > +#if defined(CONFIG_MTD_BLOCK) || defined(CONFIG_MTD_BLOCK_MODULE)
> > +	/* Block device state */
> > +	struct mtdblk_dev *blk_dev;
> > +#endif
> 
> I think injecting this to mtd_info is conceptually very wrong. It breaks
> layering. Block devices sit on top of MTD devices, not vice-versa. Lower
> layer (MTD) should not know anything about the higher layer (block).

I agree in principle, but it already does know about block and char
devices.

> I think you should a global list or rb-tree instead of the
> 'mtdblks[MAX_MTD_DEVICES]' array.

I can try that.

> But you should not inject block device
> fields to 'struct mtd_info'.
> 
> Moreover, all this mtdblock stuff is legacy, and coupling it tighter to
> 'struct mtd_info' is not a good idea.

I don't think this makes it significantly worse.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.