Hello Christoph,
> This function is small and very useful, I think it should be included unconditional
> and the prototype maybe moved to kernel.h.
That's ok for me, although I don't know what the exact criteria for
adding a function to kernel.h are.
>>+int
>>+generic_usercopy(struct inode *inode, struct file *file,
>>+ unsigned int cmd, unsigned long arg,
>>+ int (*func)(struct inode *inode, struct file *file,
>>+ unsigned int cmd, void *arg))
>
>
> The name is a bit mislead. maybe ioctl_usercopy? ioctl_uaccess?
These are both fine IMHO.
> Also file/inode should go away from the prototype (and the callback).
> Only file is needed because inode == file->f_dentry->d_inode, and even
> that one should be just some void *data instead.
I only copied the function from videodev.c and renamed it, so please
don't blame me for the way stuff is done there. 8-)
Perhaps Gerd Knorr or Alan Cox can comment on your changes -- I'll have
to investigate if all of these arguments are used anyway.
>>+ case _IOC_READ: /* some v4l ioctls are marked wrong ... */
> That's crap. Please move this workaround to v4l not into generic code.
Is it possible to fix the definitons of the v4l ioctls instead without
breaking binary compatiblity?
>>+ case _IOC_WRITE:
>>+ case (_IOC_WRITE | _IOC_READ):
>>+ if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
>>+ parg = sbuf;
>>+ } else {
>>+ /* too big to allocate from stack */
>>+ mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
>>+ if (NULL == mbuf)
>>+ return -ENOMEM;
>>+ parg = mbuf;
>
>
> I wonder whether you should just kmalloc always.
Since this function is always used in user context and the memory is
always freed afterwards, it should be possible to use vmalloc() or a
static buffer (what's the maximum size?) instead.
>>+ /* call driver */
>>+ err = func(inode, file, cmd, parg);
>>+ if (err == -ENOIOCTLCMD)
>>+ err = -EINVAL;
>
>
> I don't think this is the right place for this substitution - leave it to
> the drivers.
Agreed.
CU
Michael.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri May 23 2003 - 22:00:54 EST