On Tue, Aug 10, 2010 at 05:39:13PM +0800, Lv, Zhiyuan wrote:
> Ian is on travel and I am sending this patch on behalf of him.
>
> The patch is to add a kernel device driver of virtio-gl, which is used for
> data communication between QEMU emulator host and client OS running in it.
> This virtual device is used for GL acceleration in QEMU.
Kernel patches need a "signed-off-by:" line, right?
And why not post this to the proper development mailing list for the
kernel to get it reviewed by the subsystem maintainers?
>
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 7cfcc62..c074b73 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -1113,6 +1113,14 @@ config TELCLOCK
> /sys/devices/platform/telco_clock, with a number of files for
> controlling the behavior of this hardware.
>
> +config VIRTIOGL
> + tristate "Virtio userspace memory transport"
> + depends on VIRTIO_PCI
> + default n
> + help
> + A Driver to facilitate transferring data from userspace to a
> + hypervisor (eg. qemu)
Like Arjan said, this should not be a character device, use the proper
kernel subsystem instead.
> +//#include <asm/uaccess.h> /* for put_user */
> +//#include <asm/current.h>
Please run the scripts/checkpatch.pl script on all kernel patch
submissions.
> +
> +struct virtio_gl_data
> +{
> + char *buffer;
> + int pages;
> + unsigned int pid;
> +};
> +
> +#define SIZE_OUT_HEADER (4*3)
> +#define SIZE_IN_HEADER 4
> +
> +#define to_virtio_gl_data(a) ((struct virtio_gl_data *)(a)->private_data)
> +
> +int init_module(void);
> +void cleanup_module(void);
Don't do this, you just caused a global symbol problem if you build the
driver into the kernel.
> +static int device_open(struct inode *, struct file *);
> +static int device_release(struct inode *, struct file *);
> +static int device_mmap(struct file *, struct vm_area_struct *);
> +static int device_fsync (struct file *, struct dentry *, int datasync);
> +
> +#define DEVICE_NAME "glmem" /* Dev name as it appears in /proc/devices
> */
> +
> +static struct virtqueue *vq;
> +
> +/*
> + * Global variables are declared as static, so are global within the file.
> + */
> +
> +static int major; /* Major number assigned to our device driver
> */
You really need a whole major? Why? Why not just a single minor number
that is created dynamically?
> + major = register_chrdev(0, DEVICE_NAME, &fops);
> +
> + if (major < 0) {
> + printk(KERN_ALERT "Registering char device failed with %d\n",
> major);
> + return major;
> + }
> +
> + virtiogl_class = class_create(THIS_MODULE, "virtio_gl");
> + if (IS_ERR(virtiogl_class))
> + return PTR_ERR(virtiogl_class);
Don't create new classes for no reason please, that's not allowed.
I think I know why you are creating this class, but if you use the
proper kernel interfaces instead, you will get udev support "for free",
no need to roll it yourself.
thanks,
greg k-h
_______________________________________________
MeeGo-dev mailing list
MeeGo-dev@meego.com
http://lists.meego.com/listinfo/meego-dev
