The MTD ioctls are all specific to mtdchar.c, so the
compat code for them should be there as well.
Also, some of the ioctl commands used in that driver
were previously not marked as compatible.
The conversion handlers could be further simplified
by not using compat_alloc_user_space any more.
CC: [email protected]
CC: [email protected]
Signed-off-by: Arnd Bergmann <[email protected]>
Index: linux-2.6.14-rc/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6.14-rc.orig/drivers/mtd/mtdchar.c 2005-11-05 02:41:10.000000000 +0100
+++ linux-2.6.14-rc/drivers/mtd/mtdchar.c 2005-11-05 02:41:30.000000000 +0100
@@ -6,6 +6,7 @@
*/
#include <linux/config.h>
+#include <linux/compat.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mtd/mtd.h>
@@ -300,8 +301,7 @@
wake_up((wait_queue_head_t *)instr->priv);
}
-static int mtd_ioctl(struct inode *inode, struct file *file,
- u_int cmd, u_long arg)
+static int mtd_do_ioctl(struct file *file, u_int cmd, u_long arg)
{
struct mtd_info *mtd = TO_MTD(file);
void __user *argp = (void __user *)arg;
@@ -626,12 +626,100 @@
return ret;
} /* memory_ioctl */
+static long mtd_ioctl(struct inode *inode, struct file *file,
+ unsigned int cmd, unsigned long arg)
+{
+ int ret;
+ lock_kernel();
+ ret = mtd_do_ioctl(file, cmd, arg);
+ unlock_kernel();
+ return ret;
+}
+
+#ifdef CONFIG_COMPAT
+struct mtd_oob_buf32 {
+ u_int32_t start;
+ u_int32_t length;
+ compat_caddr_t ptr; /* unsigned char* */
+};
+
+#define MEMWRITEOOB32 _IOWR('M',3,struct mtd_oob_buf32)
+#define MEMREADOOB32 _IOWR('M',4,struct mtd_oob_buf32)
+
+static int compat_mtd_rw_oob(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ struct mtd_oob_buf __user *buf = compat_alloc_user_space(sizeof(*buf));
+ struct mtd_oob_buf32 __user *buf32 = compat_ptr(arg);
+ u32 data;
+ char __user *datap;
+ unsigned int real_cmd;
+ int err;
+
+ real_cmd = (cmd == MEMREADOOB32) ?
+ MEMREADOOB : MEMWRITEOOB;
+
+ if (copy_in_user(&buf->start, &buf32->start,
+ 2 * sizeof(u32)) ||
+ get_user(data, &buf32->ptr))
+ return -EFAULT;
+ datap = compat_ptr(data);
+ if (put_user(datap, &buf->ptr))
+ return -EFAULT;
+
+ err = mtd_do_ioctl(file, real_cmd, (unsigned long) buf);
+
+ if (!err) {
+ if (copy_in_user(&buf32->start, &buf->start,
+ 2 * sizeof(u32)))
+ err = -EFAULT;
+ }
+
+ return err;
+}
+
+static long compat_mtd_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret = -ENOIOCTLCMD;
+
+ lock_kernel();
+ switch (cmd) {
+ case MEMWRITEOOB32:
+ case MEMREADOOB32:
+ ret = compat_mtd_rw_oob(file, cmd, arg);
+ break;
+
+ case MEMGETINFO:
+ case MEMERASE:
+ case MEMLOCK:
+ case MEMUNLOCK:
+ case MEMGETREGIONCOUNT:
+ case MEMGETREGIONINFO:
+ case MEMSETOOBSEL:
+ case MEMGETOOBSEL:
+ case MEMGETBADBLOCK:
+ case MEMSETBADBLOCK:
+ case OTPSELECT:
+ case OTPGETREGIONCOUNT:
+ case OTPGETREGIONINFO:
+ case OTPLOCK:
+ ret = mtd_do_ioctl(file, cmd, arg);
+ break;
+ }
+ unlock_kernel();
+
+ return ret;
+}
+#endif
+
static struct file_operations mtd_fops = {
.owner = THIS_MODULE,
.llseek = mtd_lseek,
.read = mtd_read,
.write = mtd_write,
- .ioctl = mtd_ioctl,
+ .unlocked_ioctl = mtd_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = compat_mtd_ioctl,
+#endif
.open = mtd_open,
.release = mtd_close,
};
Index: linux-2.6.14-rc/fs/compat_ioctl.c
===================================================================
--- linux-2.6.14-rc.orig/fs/compat_ioctl.c 2005-11-05 02:41:18.000000000 +0100
+++ linux-2.6.14-rc/fs/compat_ioctl.c 2005-11-05 02:41:30.000000000 +0100
@@ -1551,46 +1551,6 @@
return err;
}
-struct mtd_oob_buf32 {
- u_int32_t start;
- u_int32_t length;
- compat_caddr_t ptr; /* unsigned char* */
-};
-
-#define MEMWRITEOOB32 _IOWR('M',3,struct mtd_oob_buf32)
-#define MEMREADOOB32 _IOWR('M',4,struct mtd_oob_buf32)
-
-static int mtd_rw_oob(unsigned int fd, unsigned int cmd, unsigned long arg)
-{
- struct mtd_oob_buf __user *buf = compat_alloc_user_space(sizeof(*buf));
- struct mtd_oob_buf32 __user *buf32 = compat_ptr(arg);
- u32 data;
- char __user *datap;
- unsigned int real_cmd;
- int err;
-
- real_cmd = (cmd == MEMREADOOB32) ?
- MEMREADOOB : MEMWRITEOOB;
-
- if (copy_in_user(&buf->start, &buf32->start,
- 2 * sizeof(u32)) ||
- get_user(data, &buf32->ptr))
- return -EFAULT;
- datap = compat_ptr(data);
- if (put_user(datap, &buf->ptr))
- return -EFAULT;
-
- err = sys_ioctl(fd, real_cmd, (unsigned long) buf);
-
- if (!err) {
- if (copy_in_user(&buf32->start, &buf->start,
- 2 * sizeof(u32)))
- err = -EFAULT;
- }
-
- return err;
-}
-
#define VFAT_IOCTL_READDIR_BOTH32 _IOR('r', 1, struct compat_dirent[2])
#define VFAT_IOCTL_READDIR_SHORT32 _IOR('r', 2, struct compat_dirent[2])
@@ -2144,8 +2104,6 @@
#endif
#ifdef DECLARES
-HANDLE_IOCTL(MEMREADOOB32, mtd_rw_oob)
-HANDLE_IOCTL(MEMWRITEOOB32, mtd_rw_oob)
HANDLE_IOCTL(HDIO_GETGEO, hdio_getgeo)
HANDLE_IOCTL(BLKRAGET, w_long)
HANDLE_IOCTL(BLKGETSIZE, w_long)
Index: linux-2.6.14-rc/include/linux/compat_ioctl.h
===================================================================
--- linux-2.6.14-rc.orig/include/linux/compat_ioctl.h 2005-11-05 02:41:18.000000000 +0100
+++ linux-2.6.14-rc/include/linux/compat_ioctl.h 2005-11-05 02:41:30.000000000 +0100
@@ -656,13 +656,6 @@
COMPATIBLE_IOCTL(USBDEVFS_REAPURB32)
COMPATIBLE_IOCTL(USBDEVFS_REAPURBNDELAY32)
COMPATIBLE_IOCTL(USBDEVFS_CLEAR_HALT)
-/* MTD */
-COMPATIBLE_IOCTL(MEMGETINFO)
-COMPATIBLE_IOCTL(MEMERASE)
-COMPATIBLE_IOCTL(MEMLOCK)
-COMPATIBLE_IOCTL(MEMUNLOCK)
-COMPATIBLE_IOCTL(MEMGETREGIONCOUNT)
-COMPATIBLE_IOCTL(MEMGETREGIONINFO)
/* NBD */
ULONG_IOCTL(NBD_SET_SOCK)
ULONG_IOCTL(NBD_SET_BLKSIZE)
--
On Sat, 5 November 2005 17:26:56 +0100, Arnd Bergmann wrote:
>
> The MTD ioctls are all specific to mtdchar.c, so the
> compat code for them should be there as well.
>
> Also, some of the ioctl commands used in that driver
> were previously not marked as compatible.
>
> The conversion handlers could be further simplified
> by not using compat_alloc_user_space any more.
>
> CC: [email protected]
> CC: [email protected]
> Signed-off-by: Arnd Bergmann <[email protected]>
Acked-by: J?rn Engel <[email protected]>
Moving crap over to mtdchar.c is a good thing. Complete removal of
mtdchar.c might be even better, but at least the crap is relatively
self-contained now.
J?rn
--
When in doubt, punt. When somebody actually complains, go back and fix it...
The 90% solution is a good thing.
-- Rob Landley
J?rn Engel <[email protected]> writes:
> Moving crap over to mtdchar.c is a good thing. Complete removal of
> mtdchar.c might be even better, but at least the crap is relatively
> self-contained now.
Agreed moving the compat_ioctl to mtdchar now that it is possible
sounds good.
I can see that argument with respect to mtdblock. But why would
removal of mtdchar be a good thing? It is a simple raw access
interface. For those whose flash parts are too small for a filesystem
or when you are doing things that you can't do with a filesystem
like making or checking it you need something like mtd char. For
embedded folks who don't care you can just compile it out.
Eric
On Tue, 8 November 2005 11:10:27 -0700, Eric W. Biederman wrote:
> J?rn Engel <[email protected]> writes:
>
> > Moving crap over to mtdchar.c is a good thing. Complete removal of
> > mtdchar.c might be even better, but at least the crap is relatively
> > self-contained now.
>
> Agreed moving the compat_ioctl to mtdchar now that it is possible
> sounds good.
>
> I can see that argument with respect to mtdblock. But why would
> removal of mtdchar be a good thing? It is a simple raw access
> interface. For those whose flash parts are too small for a filesystem
> or when you are doing things that you can't do with a filesystem
> like making or checking it you need something like mtd char. For
> embedded folks who don't care you can just compile it out.
mtdchar.c is one of the worst drivers inside the kernel. The concept
of having a simple char device driver for flash may have its charm,
but the actual implementation is horrible. And things like the
read-only devices are even unfixable.
mtdblock.c, while being quite a bit more complicated, does not require
a ton of ioctls, will not confuse users with minor number 7 actually
being device number 3 and magically being read-only despite unix
permissions and hardware properties. Plus, it is just a file.
Can you name a few examples, where mtdchar.c makes sense? I've found
it to be quite useless.
J?rn
--
A quarrel is quickly settled when deserted by one party; there is
no battle unless there be two.
-- Seneca
On Tue, 2005-11-08 at 19:33 +0100, Jörn Engel wrote:
> On Tue, 8 November 2005 11:10:27 -0700, Eric W. Biederman wrote:
> > I can see that argument with respect to mtdblock. But why would
> > removal of mtdchar be a good thing? It is a simple raw access
> > interface. For those whose flash parts are too small for a filesystem
> > or when you are doing things that you can't do with a filesystem
> > like making or checking it you need something like mtd char. For
> > embedded folks who don't care you can just compile it out.
>
> mtdchar.c is one of the worst drivers inside the kernel. The concept
> of having a simple char device driver for flash may have its charm,
> but the actual implementation is horrible. And things like the
> read-only devices are even unfixable.
Sucky implementation isn't a reason for removal. It's a reason to fix
sucky implementation.
>
> mtdblock.c, while being quite a bit more complicated, does not require
> a ton of ioctls, will not confuse users with minor number 7 actually
> being device number 3 and magically being read-only despite unix
> permissions and hardware properties. Plus, it is just a file.
>
> Can you name a few examples, where mtdchar.c makes sense? I've found
> it to be quite useless.
It allows users to write an image to a NAND device that has bad blocks
and not totally screw it up. This is possible because of those ioctls.
The mtdblock stuff knows nothing of this.
I do agree that the read-only devices don't make sense. The code itself
probably needs work too. But until someone takes the time to make the
mtdblock devices have equivalent functionality, mtdchar needs to stay.
(And mtdblock has it's own set of issues as well. I'd be happy to
discuss them, but I think it would be offtopic for this thread.)
josh
On Tue, 2005-11-08 at 19:33 +0100, J?rn Engel wrote:
> Can you name a few examples, where mtdchar.c makes sense? I've found
> it to be quite useless.
How do you access FLASH specific functionality otherwise ?
case MEMGETINFO:
....
case OTPLOCK:
It may be useless for you, but I doubt that you represent the majority
of the MTD user base. Using a RAM simulator is a bit different to the
usage of _real_ FLASH.
Look into mtd/utils and figure yourself why it _is_ useful.
The code _is_ ugly and ioctls are out of fashion, but your "remove it"
request is just silly as long as you dont provide a reasonable
alternative access to those bits.
tglx
On Tue, 2005-11-08 at 19:33 +0100, Jörn Engel wrote:
> mtdblock.c, while being quite a bit more complicated, does not require
> a ton of ioctls, will not confuse users with minor number 7 actually
> being device number 3 and magically being read-only despite unix
> permissions and hardware properties.
MAKEDEV and the documentation and udev ought to be perfectly sufficient
to deal with the slight inconvenience caused by the use of minor
numbers. The protection afforded by the read-only devices has its uses.
> Can you name a few examples, where mtdchar.c makes sense? I've found
> it to be quite useless.
mtdchar allows you to explicitly control erases and per-byte writes. You
can't do that with mtdblock. And you might not even want CONFIG_BLK_DEV
enabled.
--
dwmw2
On Tue, 8 November 2005 19:57:49 +0100, Thomas Gleixner wrote:
>
> The code _is_ ugly and ioctls are out of fashion, but your "remove it"
> request is just silly as long as you dont provide a reasonable
> alternative access to those bits.
You may have noticed the missing patch and figured that it was not a
formal request for removal. Still, I'll be happy when it's gone and
am also happy that mtdchar mess is not spread over yet another file
anymore. Thanks, Arnd.
J?rn
--
Mac is for working,
Linux is for Networking,
Windows is for Solitaire!
-- stolen from dc
On Tue, 2005-11-08 at 23:21 +0100, J?rn Engel wrote:
> On Tue, 8 November 2005 19:57:49 +0100, Thomas Gleixner wrote:
> >
> > The code _is_ ugly and ioctls are out of fashion, but your "remove it"
> > request is just silly as long as you dont provide a reasonable
> > alternative access to those bits.
>
> You may have noticed the missing patch and figured that it was not a
> formal request for removal. Still, I'll be happy when it's gone and
> am also happy that mtdchar mess is not spread over yet another file
> anymore. Thanks, Arnd.
I'm deeply impressed by your insight and caring about Linux source code
quality. Thanks, Joern.
Complaining about legacy mess is easy blurb. Providing a clean solution
is obviously not on your agenda.
tglx
J?rn Engel <[email protected]> writes:
> mtdchar.c is one of the worst drivers inside the kernel. The concept
> of having a simple char device driver for flash may have its charm,
> but the actual implementation is horrible. And things like the
> read-only devices are even unfixable.
This is a confusing statement, you complain that the implementation is horrible
and then go on to complain about the interface to the character device.
The implementation appears small and concise. There are a couple of
FIXMEs but they are all about wishing the interface to the flash chips
mapped better to a user space interface. I routinely fix things in
the kernel whose implementations are much worse than mtdchar.
> Can you name a few examples, where mtdchar.c makes sense? I've found
> it to be quite useless.
I have found just the opposite. It happens to be the only interface
to mtd devices I use. In general when you have flash devices small
enough that you can't use a filesystem without waisting a lot of space
(keeping 1 free erase block out of 4 or 8 is a problem). Or when you are
doing low-level mucking mtdchar is invaluable.
As for the interface to mtdchar. I agree that the readonly character
device is silly, and does weird things to the mtd device minor numbers.
I agree that ioctls are not the prettiest interface around, however
the raw functionality the ioctls export is needed, and interesting. Some
of the functionality would be hard to export even in sysfs the cool ascii
replacement for ioctl.
Long term it does look like a sysfs interface to the mtd functionality
could suffice.
Eric
On Wed, 9 November 2005 08:37:16 -0700, Eric W. Biederman wrote:
> J?rn Engel <[email protected]> writes:
>
> > Can you name a few examples, where mtdchar.c makes sense? I've found
> > it to be quite useless.
>
> I have found just the opposite. It happens to be the only interface
> to mtd devices I use. In general when you have flash devices small
> enough that you can't use a filesystem without waisting a lot of space
> (keeping 1 free erase block out of 4 or 8 is a problem). Or when you are
> doing low-level mucking mtdchar is invaluable.
Josh already convinced me with the Bad Block argument. The hardware
already used an OOB scheme to define them. Regular unix files without
some sort of OOB data access don't map well to NAND.
> As for the interface to mtdchar. I agree that the readonly character
> device is silly, and does weird things to the mtd device minor numbers.
> I agree that ioctls are not the prettiest interface around, however
> the raw functionality the ioctls export is needed, and interesting. Some
> of the functionality would be hard to export even in sysfs the cool ascii
> replacement for ioctl.
>
> Long term it does look like a sysfs interface to the mtd functionality
> could suffice.
It could. Some time ago I starting coding something up, but got
quickly distracted. Might be easier to start from scratch again.
J?rn
--
Happiness isn't having what you want, it's wanting what you have.
-- unknown