![]() | |||||||||||||
Messages in this thread |
Mozilla is being annoying and not quoting your patch, so bear with me. comments: 1) return an error instead of BUG() (and no error return) in the generic DMA routines that can return a meaningful value 2) num_req_outstanding accessed without lock in do_viodasd_request (driver's request_fn). all other accesses are inside spinlock. 3) is viodasd_revalidate really needed? 4) why do you call blkdev_dequeue_request() in do_viodasd_request() rather than viodasd_end_request() ? Or just use end_request() ? 5) is it really OK to call viodasd_open() and viodasd_release() multiple times? These functions do not look guarded against multiple openers. 6) access to a struct viodasd_device in viodasd_ioctl() is completely unprotected. OK, or asking for trouble? 7) use sg_dma_address() and sg_dma_len() accessors instead of directly referencing the struct scatterlist elements. (several places) 8) send_request() probably wants a common error-exit+cleanup path, instead of duplicating the same cleanup code multiple times 9) viodasd_restart_all_queues_starting_from -- are you sure you don't want to make the function name even longer? Maybe try for a new record? 10) in viodasd_handleReadWrite() you obtain the queue lock via spin_lock(), but the rest of the kernel uses spin_lock_irq() or spin_lock_irqsave() 11) viodasd_handleReadWrite, vioHandleBlockEvent -- follow the style in the rest of the driver, and eliminate the StudlyCaps. 12) don't you need to set blk_queue_max_phys_segments() too? 13) in viodasd_init(), don't you need to undo the effects of vio_set_hostlp() if an error occurs? 14) why does vio_set_dma_mask() always return an error? That seems rather useless and unwanted. Hey, I just merged iSeries veth, so I had to give you some more work... ;-) Jeff P.S. I so wish that people had named the API function dma_alloc_incoherent() rather than dma_alloc_noncoherent :) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ | ||||||||||||
| Last update: 2005-03-22 13:01 [from the cache] ©2003-2008 | |||||||||||||