Re: [PATCH] mtd: export mtd->writebufsize attribute over sysfs

[Artem Bityutskiy <dedekind1@gmail.com>]

On Fri, 2011-02-11 at 16:35 +0100, Anatolij Gustschin wrote:
> On Fri, 11 Feb 2011 17:09:54 +0200
> Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 
> > On Fri, 2011-02-11 at 12:46 +0100, Anatolij Gustschin wrote:
> > > +static ssize_t mtd_writebufsize_store(struct device *dev,
> > > +				struct device_attribute *attr,
> > > +				const char *buf, size_t size)
> > > +{
> > > +	struct mtd_info *mtd = dev_to_mtd(dev);
> > > +	unsigned long value;
> > > +	int ret;
> > > +
> > > +	ret = strict_strtoul(buf, 0, &value);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	mtd->writebufsize = value;
> > > +	return size;
> > > +}
> > > +static DEVICE_ATTR(writebufsize, S_IRUGO, mtd_writebufsize_show,
> > > +		   mtd_writebufsize_store);
> > 
> > 
> > I think writebufsize should be read-only. This is characteristic of the
> > flash and should not be changed. OK, if the chip allows to change it, it
> > could be done, but this is not what the patch seems to be about.
> 
> Yes, it is read-only by default.
> 
> > I mean, writebufsize is like writesize. Why writesize sysfs attribut is
> > R/O but writebufsize should be RW?
> 
> This attribute is R/O when the appropriate file is created. For
> mtd-ram test device it will be set RW by the fixup routine. I used
> this to set the writebufsize to different values when working with
> different UBIFS images. If I can set it dynamically, I do not need
> to recompile the mtdram module or the kernel.

But this is bad architecture, you are putting policy into the kernel and
hard-coding device-type specific checks with things like:

+                       sysfs_chmod_file(&mtd->dev.kobj, attr,
+                                        S_IRUGO | S_IWUSR | S_IWGRP);

and

+       if (mtd->type != MTD_RAM)
+               return;

If you are doing something mtdram-specific, make it mtdram-specific. How
about this: add the "writebufsize" mtdram module parameter, then you
will have a /sys/modules/mtdram/parameters/writebufsize file, which you
then can change run-time.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)