Re: [PATCH 2/2][MTD] Add support for > 2GiB MTD devices

[David Woodhouse <dwmw2@infradead.org>]

On Wed, 2008-08-27 at 17:25 +0200, Jörn Engel wrote:
> On Wed, 27 August 2008 17:47:43 +0300, Artem Bityutskiy wrote:
> > 
> > The plus of sysfs I see is that I can add more files to expose more
> > information in sysfs, while I can not change MEMGETINFO ioctl. E.g., I
> > need to expose sub-page size to user-space, and I cannot do this with
> > MEMGETINFO.
> 
> sysfs makes adding new attributes easier, yes.  But once added you
> cannot remove the attribute again - ever.  Which means that either way
> you need to tread carefully and think twice before making a rash
> decision.

That's not _necessarily_ true, although it should certainly be done with
care. Attributes in sysfs can be optional (in a way that they can't
really be optional if they're part of a binary ioctl payload), and
userspace can cope with them being absent. The sub-page size attribute
is something which wouldn't always be present, and we could happily just
drop it and forget about it in future if we really wanted to.

> > > So what was the reason again why mtd needs two userspace interfaces
> > > instead of just one?
> > 
> > I would like to make udev creating MTD devices, instead of creating them
> > by hands. Adding MTD to LDM would anyway introduce corresponding sysfs
> > files, right? This means we would have one more interface anyway.
> 
> Could be useful, I don't mind you sending a patch.  However, does this
> means that MEMGETINFO64 or some other ioctl should not be done?  Should
> flash_erase open, read and close 8 seperate files instead of doing a
> single ioctl?

It's hardly a fast path. And we don't have to worry about the fact that
it's non-atomic -- these things aren't exactly _changing_ over time.

> And should our support for large devices wait for the sysfs support
> that has been talked about and not done for about two years already?

You whine too much, Jörn. It doesn't take very long, as a proof of
concept, to add some attributes to the existing class support in
mtdchar.c ...

--- drivers/mtd/mtdchar.c~	2008-07-13 22:51:29.000000000 +0100
+++ drivers/mtd/mtdchar.c	2008-08-29 13:15:08.000000000 +0100
@@ -22,12 +22,32 @@
 
 static struct class *mtd_class;
 
+static ssize_t mtd_show_size(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	return sprintf(buf, "0x%x\n", mtd->size);
+}
+static ssize_t mtd_show_erasesize(struct device *dev, struct device_attribute *attr,
+				  char *buf)
+{
+	struct mtd_info *mtd = dev_get_drvdata(dev);
+	return sprintf(buf, "0x%x\n", mtd->erasesize);
+}
+
+static DEVICE_ATTR(size, S_IRUGO, mtd_show_size, NULL);
+static DEVICE_ATTR(erasesize, S_IRUGO, mtd_show_erasesize, NULL);
+
 static void mtd_notify_add(struct mtd_info* mtd)
 {
+	struct device *dev;
 	if (!mtd)
 		return;
 
-	device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+	dev = device_create(mtd_class, NULL, MKDEV(MTD_CHAR_MAJOR, mtd->index*2), "mtd%d", mtd->index);
+	dev_set_drvdata(dev, mtd);
+	device_create_file(dev, &dev_attr_size);
+	device_create_file(dev, &dev_attr_erasesize);
 
 	device_create(mtd_class, NULL,
 		      MKDEV(MTD_CHAR_MAJOR, mtd->index*2+1), "mtd%dro", mtd->index);


Ok, so it shouldn't be only for mtdchar -- it should be generic, so we
should shift some of that into the mtd core code. And we should let
people hook up the 'parent' correctly, and there are a few other things
we should do to tidy it up. But it isn't exactly hard.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation