Messages in this thread |  | | Date | Thu, 09 Nov 2000 12:02:45 -0500 | From | Jeff Garzik <> | Subject | Re: OOPS loading cs46xx module, test11-pre1 |
| |
> I have an (untested) update for the cs46xx driver in Linux 2.4. > It includes Nils' 2.2 changes, use of initcalls (so compiled-in > should work) and use of the 2.4 PCI interface.
Patch Generally looks ok. Comments:
1) This code is weird: > if (copy_to_user(buffer, dmabuf->rawbuf + swptr, cnt)) { >- if (!ret) ret = -EFAULT; >- return ret; >+ if (!ret) >+ ret = -EFAULT; >+ break; > }
If you have an error condition (ret != 0), then you should not attempt the copy_to_user at all. If you do not have an error condition, you should unconditionally assign -EFAULT to 'ret'.
There is code like this around calls to copy_{to,from}_user and signal_pending that I see at a quick glance.
2) this patch enabled cs_mmap, and removes a check for vma->vm_offset != 0. Also that is clearly 2.2.x code, simply removing the check is wrong. You should replace the check with one that checks vma->vm_pgoff != 0.
3) is there method or madness to the delay changes? they are not explained, just changed...
4) cs_probe is marked __init but cs_remove is marked __devexit. on hotplug, cs_probe simply doesn't exist in the kernel anymore... boom.
5) there is no need to appear zeroes to the end of these cs_pci_tbl entries. Just end each with "PCI_ANY_ID,"... + { PCI_VENDOR_ID_CIRRUS, 0x6001, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 }, + { PCI_VENDOR_ID_CIRRUS, 0x6003, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 }, + { PCI_VENDOR_ID_CIRRUS, 0x6004, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
6) remove check for pci_present(), redundant
7) use pci_module_init for hotplug. quite simply:
init_module() { return pci_module_init(&my_driver); }
of course for cs46xx, you will want to keep the version printk...
8) xxx_MODULE_NAME was a dumb and overly-lengthy creation of mine. Use instead MODNAME.
-- Jeff Garzik | Building 1024 | Would you like a Twinkie? MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org Please read the FAQ at http://www.tux.org/lkml/
|  |