linux-mips
[Top] [All Lists]

Re: O2 VICE support

To: Christoph Hellwig <hch@infradead.org>
Subject: Re: O2 VICE support
From: ilya@theIlya.com
Date: Wed, 11 Dec 2002 14:16:29 -0800
Cc: linux-mips@linux-mips.org, ralf@linux-mips.org
In-reply-to: <20021211133831.A19300@infradead.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20021210191120.GE609@gateway.total-knowledge.com> <20021211133831.A19300@infradead.org>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.4i
> 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.
Well, most of manipulation of this device will be done bu user-space
code. Kernel's only job is synchronising access and managing memory.
Thus we would like to keep all definitions in one place.

> > +    if (!filp->private_data) {
> > +   filp->private_data = vice_device;
> > +    }
> 
> filp->private_data can't be set.
???
I can live without using it of course, since it isn't currently possible to have
more then one instance of VICE in same machine, but theoretical possibility does
exist. Where should I pass information about which specific device current
call is?

> 
> > +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?
This is more like reminder to myself that I should do that one day :)

> > +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.
I was wondering about that too, but this is the way i was in book :)

> 
> > +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.. :)
I'm not buying that one :)

> 
> > +#ifndef VICE_DEBUG
> > +    EXPORT_NO_SYMBOLS;             /* otherwise, leave global symbols 
> > visible */
> > +#endif
> 
> EXPORT_NO_SYMBOLS is a noop on 2.5
Driver was written for 2.4 first.


Thanks a lot for constructive criticism.

        Ilya.

Attachment: pgpN9vlXcDDPo.pgp
Description: PGP signature

<Prev in Thread] Current Thread [Next in Thread>