[lkml]   [2004]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] iSeries virtual disk
    Hi Jeff,

    On Thu, 26 Feb 2004 02:29:26 -0500 Jeff Garzik <> 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)


    > 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?

    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.
    Stephen Rothwell
    [unhandled content-type:application/pgp-signature]
     \ /
      Last update: 2005-03-22 14:01    [W:0.025 / U:6.696 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site