> 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:
pgp00245.pgp
Description: PGP signature
