lkml.org 
[lkml]   [2004]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2005-03-22 14:01    [from the cache]
©2003-2011 Jasper Spaans