![]() | |||||||||||||
Messages in this thread |
Hi Jeff, On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@pobox.com> wrote: > > 1) return an error instead of BUG() (and no error return) in the generic > DMA routines that can return a meaningful value I have removed the DMA changes from this patch. > 2) num_req_outstanding accessed without lock in do_viodasd_request > (driver's request_fn). all other accesses are inside spinlock. This is actually OK because: 1) if we see a value too large, when it get decremented by handle_read_write, all the queue requst functions will get rerun. 2) in send_request, if we get an error and decrement the count to zero, then the count could have been at most 1 (sonce sends are serialised) so in the request funtion, we would not have stopped processing requests. > 3) is viodasd_revalidate really needed? Not any more - its gone. > 4) why do you call blkdev_dequeue_request() in do_viodasd_request() > rather than viodasd_end_request() ? Or just use end_request() ? See Jens' response. > 5) is it really OK to call viodasd_open() and viodasd_release() multiple > times? These functions do not look guarded against multiple openers. It is OK. > 6) access to a struct viodasd_device in viodasd_ioctl() is completely > unprotected. OK, or asking for trouble? Its OK, beacuse the viodasd_request is completely static data after the disk is probed - and before that you cannot get the ioctl ... > 7) use sg_dma_address() and sg_dma_len() accessors instead of directly > referencing the struct scatterlist elements. (several places) done. > 8) send_request() probably wants a common error-exit+cleanup path, > instead of duplicating the same cleanup code multiple times done. > 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() fixed. > 11) viodasd_handleReadWrite, vioHandleBlockEvent -- follow the style in > the rest of the driver, and eliminate the StudlyCaps. done. > 12) don't you need to set blk_queue_max_phys_segments() too? done. > 13) in viodasd_init(), don't you need to undo the effects of > vio_set_hostlp() if an error occurs? No and, in fact, we can't. > 14) why does vio_set_dma_mask() always return an error? That seems > rather useless and unwanted. dma stuff removed ... New patch following soon. -- Cheers, Stephen Rothwell sfr@canb.auug.org.au http://www.canb.auug.org.au/~sfr/ [unhandled content-type:application/pgp-signature] | ||||||||||||
| Last update: 2005-03-22 13:01 [from the cache] ©2003-2008 | |||||||||||||