Re: O2 VICE support

[Christoph Hellwig <hch@infradead.org>]

On Tue, Dec 10, 2002 at 11:11:20AM -0800, ilya@theIlya.com wrote:
> RCS file: /home/cvs/linux/arch/mips64/kernel/ioctl32.c,v
> retrieving revision 1.26
> diff -u -r1.26 ioctl32.c
> --- arch/mips64/kernel/ioctl32.c	12 Nov 2002 15:26:11 -0000	1.26
> +++ arch/mips64/kernel/ioctl32.c	10 Dec 2002 17:02:29 -0000
> @@ -55,6 +57,10 @@
>  
>  #include <linux/rtc.h>
>  
> +#ifdef CONFIG_O2_VICE
> +#include <linux/vice.h>
> +#endif

Just include the header unconditionally.

>  static int w_long(unsigned int fd, unsigned int cmd, unsigned long arg)
> @@ -827,6 +968,12 @@
>  COMPATIBLE_IOCTL(TIOCGSERIAL)
>  COMPATIBLE_IOCTL(TIOCSSERIAL)
>  COMPATIBLE_IOCTL(TIOCSERGETLSR)
> +#ifdef CONFIG_O2_VICE
> +COMPATIBLE_IOCTL(VICE_IOCTL_MAP_DMA)
> +COMPATIBLE_IOCTL(VICE_IOCTL_MSP_RUN)
> +COMPATIBLE_IOCTL(VICE_IOCTL_BSP_RUN)
> +COMPATIBLE_IOCTL(VICE_IOCTL_DO_DMA)
> +#endif

Dito.  COMPATIBLE_IOCTL() isn't a big burgden, so it doesn't harm to get
rid of the ifdefs.

> +source drivers/char/o2vice/Kconfig

I think it should be in drivers/media/vice (i.e. it's one of the media
devices, and I don't think you need the o2, especially as you don't
use it e.g. in the header name).

> --- /dev/null	Sun Jul 17 16:46:18 1994
> +++ drivers/char/o2vice/Makefile	Thu Sep 12 00:12:16 2002
> @@ -0,0 +1,21 @@
> +#
> +# drivers/char/o2vice/Makefile
> +#
> +# Makefile for the O2 VICE Engine driver.
> +#
> +
> +SUB_DIRS     := 
> +MOD_SUB_DIRS := $(SUB_DIRS)
> +ALL_SUB_DIRS := $(SUB_DIRS)
> +
> +#O_TARGET := vice.o
> +
> +obj-y		:=
> +obj-m		:=
> +obj-n		:=
> +obj-		:=

This is 2.2 crap.  Just get rid of it..

> +
> +obj-$(CONFIG_O2_VICE)	+= main.o msp.o bsp.o dma.o
> +#obj-$(CONFIG_O2_VICE_DBG) += vicedebug.o
> +
> +include $(TOPDIR)/Rules.make

The include isn't needed anymore either in 2.5.

> +#include <linux/ioctl.h> /* needed for the _IOW etc stuff used later */
> +#ifdef __KERNEL__
> +#include <asm/pci.h>
> +#endif
> +#include <linux/types.h>	/* size_t */
> +
> +#ifndef __KERNEL__
> +typedef __u64 u64;
> +typedef __u32 u32;
> +typedef __u16 u16;
> +#endif /* __KERNEL */

Urgg.  Usually kernel headers aren't supposed to be used in userspace.
If you want to use a copy anyway it should be done with much less burden
on the kernel code.

> +#ifdef VICE_DEBUG
> +# define DPRINTK(fmt, args...) printk( KERN_DEBUG "vice: " fmt, ## args)
> +#else
> +# define DPRINTK(fmt, args...) /* not debugging: nothing */
> +#endif
> +
> +#define PDEBUG DPRINTK

Just use pr_debug?

> +
> +#ifndef VICE_MAJOR
> +#define VICE_MAJOR 240   /* dynamic major by default */
> +#endif

Comment doesn't match code.

> +/*
> + * Prototypes for shared functions
> + */
> +ssize_t vice_read (struct file *filp, char *buf, size_t count,
> +                    loff_t *f_pos);
> +ssize_t vice_write (struct file *filp, const char *buf, size_t count,
> +                     loff_t *f_pos);
> +loff_t  vice_llseek (struct file *filp, loff_t off, int whence);
> +int     vice_ioctl (struct inode *inode, struct file *filp,
> +                     unsigned int cmd, unsigned long arg);

Exported file operations are usually a very bad sign.  But it looks like
you could just make them static anyway :)

> +/*
> + * VICE ioctl commands
> + */
> +#ifdef __KERNEL__
> +#define VICE_IOCTL_MAGIC	0x96
> +#define VICE_IOCTL_MAP_DMA	_IOR(VICE_IOCTL_MAGIC,1,unsigned long)
> +#define VICE_IOCTL_MSP_RUN	_IOR(VICE_IOCTL_MAGIC,2,msp_run)
> +#define VICE_IOCTL_BSP_RUN	_IOR(VICE_IOCTL_MAGIC,3,bsp_run)
> +#define VICE_IOCTL_DO_DMA	_IOWR(VICE_IOCTL_MAGIC,4,dma_run)
> +#else
> +#define VICE_IOCTL_MAP_DMA	0x40089601
> +#define VICE_IOCTL_MSP_RUN	0x400c9602
> +#define VICE_IOCTL_BSP_RUN	0x400c9603
> +#define VICE_IOCTL_DO_DMA	0xc00c9604
> +#endif

Ummm..

> +static inline int vice_dma_mem_init(vice_dev *vice)
> +{
> +    int i;
> +    for (i=0; i<64*2;i++){
> +	if(!(vice->dma_kmem[i]=pci_alloc_consistent(0,VICE_PAGE_SIZE,&(vice->dma_mem[i])))) {
> +	    DPRINTK("failed at page# %i\n",i);
> +    	    return -ENOMEM;
> +	}
> +    }
> +    return 0;

Please adopt to Documentation/CodingStyle

> +    DPRINTK("Falling asleep\n");
> +    while(vice->msp_status==VICE_RES_STATUS_INPROGRESS) {
> +        interruptible_sleep_on(&wq);

sleep_on is racy.  You should at least use wait_even, but it doesn't look
like vice->msp_status is atomically updated anyway.

> --- /dev/null	Sun Jul 17 16:46:18 1994
> +++ drivers/char/o2vice/Makefile	Thu Sep 12 00:12:16 2002

The diff seems to include the same hunk twice..

> +/* In vice_open, the fop_array is used according to TYPE(dev) */
> +int vice_open(struct inode *inode, struct file *filp)
> +{
> +    vice_dev *vice=vice_device;		/* device information */
> +    int res;
> +
> +    MOD_INC_USE_COUNT;		/* Before we maybe sleep */

Please set .owner in the file_operations instead.

> +
> +    if(vice->is_open)
> +	return -EBUSY;

This seems pretty racy.

> +
> +    if (!filp->private_data) {
> +	filp->private_data = vice_device;
> +    }

filp->private_data can't be set.

> +ssize_t vice_read(struct file * filp, char *buf, size_t count, loff_t * f_pos)
> +{
> +    printk(KERN_WARNING
> +	   "Processing bit streams through reading/writing is not supported yet\n");
> +    return -ENOSYS;
> +}
> +
> +ssize_t vice_write(struct file * filp, const char *buf, size_t count,
> +	   loff_t * f_pos)
> +{
> +    printk(KERN_WARNING
> +	   "Processing bit streams through reading/writing is not supported (yet)\n");
> +    return -ENOSYS;
> +}

What about just not implementing the methods instead?

> +static void vice_vma_open(struct vm_area_struct *vma)
> +{ MOD_INC_USE_COUNT; }
> +
> +static void vice_vma_close(struct vm_area_struct *vma)
> +{ MOD_DEC_USE_COUNT; }

This is silly.  You get a reference for vma->vm_file as long as you
have any mmaps.  That's enough for the refcounting.

> +static struct vm_operations_struct vice_vm_ops = {
> +    open:  vice_vma_open,
> +    close: vice_vma_close,
> +    nopage: vice_vma_nopage,
> +};

Please use C99-initializers (i.e. .foo = bar)

> +void vice_cleanup_module(void)
> +{
> +#ifndef CONFIG_DEVFS_FS
> +    /* cleanup_module is never called if registering failed */
> +    unregister_chrdev(vice_major, "vice");
> +#endif

Umm, just because someone makes the mistake of enabling devfs he
doesn't have to use it.. :)

> +#ifndef VICE_DEBUG
> +    EXPORT_NO_SYMBOLS;		/* otherwise, leave global symbols visible */
> +#endif

EXPORT_NO_SYMBOLS is a noop on 2.5