lkml.org 
[lkml]   [2007]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: sata_sil24 broken since 2.6.23-rc4-mm1
On Thu, Oct 11 2007, Tejun Heo wrote:
> Jens Axboe wrote:
> > 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?
>
> Because that makes ata_sg iterator macros have hidden side effects
> (nothing in the interface suggests it can't be nested and when somebody
> actually nests it, finding what went wrong can be pretty difficult). I
> think it would be better to have explicit ata_sg_iter passed around if
> sg itself isn't enough to walk the sg list.

I think it's implicit in the interface, since you don't pass an sgtable
in. But it's not a big deal to me, if you want it changed, go ahead :-)

> > 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.
>
> Yeap, it can be removed but having "is this the last one?" test is just
> nicer to low level drivers. With ata_sg_iter, I think we can do it in
> O(N) by looking up and caching the next entry.

Sure, of you could just lookup sg_last() in the beginning of the loop.
Still seems a bit silly to me just to keep the ata_sg_is_last()
interface, since it can be done for free basically by maintaining an
sglast element in the loop that you then use when the loop is done to
set your SG_END marker (or whatever the driver needs).

--
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 12:31    [W:0.045 / U:3.580 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site