Re: [PATCH] iSeries virtual disk

[Stephen Rothwell]

Hi Jeff,

On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <jgarzik@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx
http://www.canb.auug.org.au/~sfr/

Attachment: pgp00000.pgp
Description: PGP signature