Re: [PATCH] iSeries virtual disk

[Jeff Garzik]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/