Messages in this thread | | | Date | Fri, 27 Feb 2004 11:42:40 +1100 | From | Stephen Rothwell <> | Subject | Re: [PATCH] iSeries virtual disk |
| |
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] | |