lkml.org 
[lkml]   [2007]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: sata_sil24 broken since 2.6.23-rc4-mm1
    On Thu, Oct 11 2007, Tejun Heo wrote:
    > Torsten Kaiser wrote:
    > > Looking closer at
    > > http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git;a=commitdiff;h=ec6fdded4d76aa54aa57341e5dfdd61c507b1dcd
    > > the change to libata.h seems bogus :
    > >
    > > in ata_qc_first_sg:
    > > old new
    > > return qc->__sg return qc->__sg
    > > qc->__sg - qc->__sg == 0 qc->n_iter=0
    > > -> sg - qc->__sg corresponds to qc->n_iter
    > >
    > > in ata_qc_next_sg:
    > > sg++; sg_next(sg); qc->n_iter++;
    > > sg - qc->__sg < qc->n_elem qc->n_iter < qc->nelem
    > > -> sg - qc->__sg corresponds to qc->n_iter
    > >
    > > but in ata_sg_is_last:
    > > (sg - qc->__sg) +1 == qc->n_elem qc->n_iter == qc->n_elem
    > > if sg - qc->__sg corresponds to qc->n_iter then shoudn't it be
    > > qc->n_iter+1 == qc->n_elem?
    > >
    > > That missing +1 would explain, why the SGE_TRM never gets set.
    >
    > Thanks a lot for tracking this down. Does changing the above code fix
    > your problem?
    >
    > Jens, Torsten's analysis looks correct && depending on qc state (n_iter)
    > during iteration doesn't look like a good idea. Those iterators are not
    > supposed to have side effects. Would it be difficult to implement
    > sg_last() test?

    This is the old ata_sg_is_last:

    static inline int
    ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
    {
    if (sg == &qc->pad_sgent)
    return 1;
    if (qc->pad_len)
    return 0;
    if (((sg - qc->__sg) + 1) == qc->n_elem)
    return 1;
    return 0;
    }

    and the new one:

    static inline int
    ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
    {
    if (sg == &qc->pad_sgent)
    return 1;
    if (qc->pad_len)
    return 0;
    if (qc->n_iter == qc->n_elem)
    return 1;
    return 0;
    }

    ->n_iter is how ata_qc_next_sg() walks over the sglist, I don't
    understand your reference to why depending on that during iteration
    would be bad?

    So we could add a test for sg_last() there, but that would turn sg table
    iteration into an O(N^2) operation for those drivers that use
    ata_sg_is_last() with chained sg tables. I'd much rather just get rid of
    ata_sg_is_last(), it's only used to mark end-of-table entries for
    hardware. That logic can be performed cheaper.

    Torsten, your analysis does look correct. Does it work with this simple
    patch?

    diff --git a/include/linux/libata.h b/include/linux/libata.h
    index 2784163..0152bf4 100644
    --- a/include/linux/libata.h
    +++ b/include/linux/libata.h
    @@ -947,7 +947,7 @@ ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
    return 1;
    if (qc->pad_len)
    return 0;
    - if (qc->n_iter == qc->n_elem)
    + if ((qc->n_iter + 1) == qc->n_elem)
    return 1;
    return 0;
    }
    --
    Jens Axboe

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-10-11 10:29    [W:0.025 / U:29.676 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site