[bug] block subsystem related crash with latest -git - Kernel

This is a discussion on [bug] block subsystem related crash with latest -git - Kernel ; On Thu, Oct 18 2007, Jeff Garzik wrote: > Mark Lord wrote: >> Okay, mine is dying with EIP at blk_rq_map_sg+0xcb/0x160. >> Screen photo is at http://rtr.ca/recent/2.6.23-git12-crash.jpg , >> but the top was cut off (isn't there a new config ...

+ Reply to Thread
Page 5 of 8 FirstFirst ... 3 4 5 6 7 ... LastLast
Results 81 to 100 of 151

Thread: [bug] block subsystem related crash with latest -git

  1. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Mark Lord wrote:
    >> Okay, mine is dying with EIP at blk_rq_map_sg+0xcb/0x160.
    >> Screen photo is at http://rtr.ca/recent/2.6.23-git12-crash.jpg,
    >> but the top was cut off (isn't there a new config option or patch
    >> to do double-columns or scrollback or something ???.

    >
    > Is this a sata_mv box? If so, could you try this patch?


    If anything, that shrinks the size of the resulting request. Did this
    patch make any difference to you?

    Now sata_mv should not be doing this (already discussed), but as long as
    it's only lowering the physical sg count then it should not cause any
    bugs at least.

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

  2. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Mark Lord wrote:
    >> Mark Lord wrote:
    >>> Linus Torvalds wrote:
    >>>>
    >>>> On Wed, 17 Oct 2007, Mark Lord wrote:
    >>>>> It would be good to have something soon-ish.
    >>>>> This "dead at boot time" issue is impacting the general ability to test
    >>>>> patches against latest -git in time for the current merge window.
    >>>>
    >>>> In the meantime, does the patch I sent out help people?
    >>>
    >>> Your patch from this posting http://lkml.org/lkml/2007/10/17/285
    >>> does not seem to make much difference here.
    >>>
    >>> It still crashes at exactly the same place.

    >> However, Jens's patch from that same thread:
    >> http://lkml.org/lkml/2007/10/17/269
    >> ..allowed me to boot and post this followup message from -git12
    >> Jeff: try that one.

    >
    > That's already in my upstream kernel, here. commits
    > ba951841ceb7fa5b06ad48caa5270cc2ae17941e and
    > a3bec5c5aea0da263111c4d8f8eabc1f8560d7bf.
    >
    > sata_mv and sata_nv still reliably poop themselves here, whereas its rock
    > solid with 2.6.23.1. Sounds like different issues from yours, as I see a
    > stream of SATA errors on the bad kernels, errors which are often a symptom
    > of something whacked in the DMA engine (misprogramming causes the silicon
    > to generate bogus FIS's, which the device then chokes on)


    Do you know if this poop involves the segment padding that sometimes
    goes on in libata?

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

  3. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Ingo Molnar wrote:
    >
    > * Jens Axboe wrote:
    >
    > > --- a/drivers/scsi/scsi_lib.c
    > > +++ b/drivers/scsi/scsi_lib.c
    > > @@ -39,7 +39,7 @@
    > > * (unless chaining is used). Should ideally fit inside a single page, to
    > > * avoid a higher order allocation.
    > > */
    > > -#define SCSI_MAX_SG_SEGMENTS 128
    > > +#define SCSI_MAX_SG_SEGMENTS 129

    >
    > this one finally made the trick and it's booting fine now, without any
    > crashes!


    Super, that validates the theory the theory that Linus put forth. So the
    bug is clear now, this morning I'll work on proper sg looping.

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

  4. Re: [bug] ata subsystem related crash with latest -git

    Jens Axboe wrote:
    > On Thu, Oct 18 2007, Jeff Garzik wrote:
    >> Mark Lord wrote:
    >>> Mark Lord wrote:
    >>>> Linus Torvalds wrote:
    >>>>> On Wed, 17 Oct 2007, Mark Lord wrote:
    >>>>>> It would be good to have something soon-ish.
    >>>>>> This "dead at boot time" issue is impacting the general ability to test
    >>>>>> patches against latest -git in time for the current merge window.
    >>>>> In the meantime, does the patch I sent out help people?
    >>>> Your patch from this posting http://lkml.org/lkml/2007/10/17/285
    >>>> does not seem to make much difference here.
    >>>>
    >>>> It still crashes at exactly the same place.
    >>> However, Jens's patch from that same thread:
    >>> http://lkml.org/lkml/2007/10/17/269
    >>> ..allowed me to boot and post this followup message from -git12
    >>> Jeff: try that one.

    >> That's already in my upstream kernel, here. commits
    >> ba951841ceb7fa5b06ad48caa5270cc2ae17941e and
    >> a3bec5c5aea0da263111c4d8f8eabc1f8560d7bf.
    >>
    >> sata_mv and sata_nv still reliably poop themselves here, whereas its rock
    >> solid with 2.6.23.1. Sounds like different issues from yours, as I see a
    >> stream of SATA errors on the bad kernels, errors which are often a symptom
    >> of something whacked in the DMA engine (misprogramming causes the silicon
    >> to generate bogus FIS's, which the device then chokes on)

    >
    > Do you know if this poop involves the segment padding that sometimes
    > goes on in libata?


    Definitely not, in this case -- it's all ATA, nothing ATAPI.

    It throws SError { Handshk } which then triggers the EH to reset the
    link, and so it goes, over and over The same thing happens when I
    intentionally screw up the PRD tables. Not much more data points than
    that, so far.

    I'll try the SCSI_MAX_SG_SEGMENTS patch too, to see if that fixes things.

    Jeff



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

  5. Re: [bug] ata subsystem related crash with latest -git

    On Wed, Oct 17 2007, David Miller wrote:
    > From: Linus Torvalds
    > Date: Wed, 17 Oct 2007 18:07:19 -0700 (PDT)
    >
    > > sg_next() - as it stands now - never actually looks at the SG that its
    > > argument points to: it explicitly *only* looks at the next one.
    > >
    > > That's the bug. If sg_next() looked at the actual *current* sg entry, we
    > > wouldn't have any issues to begin with, and that's what I'm arguing we
    > > should do in the longer run (where "longer run" is defined as "when Jens
    > > does it asap").

    >
    > What the thing really wants is some kind of indication of state,
    > without having to bloat up the scatterlist structure.
    >
    > I believe that we have enough of a limited set of accessors to
    > sg->page that we can more aggressively encode things in the lower
    > bits.
    >
    > I'm thinking of encoding the low two bits of sg->page as
    > follows:
    >
    > 1) bits == 0
    >
    > then the SG list is linear and sg_next() is sg++
    >
    > 2) bits == 1
    >
    > the nest SG is an indirect chunk, sg_next() is
    > therefore something like:
    >
    > next = *((struct scatterlist **)(sg + 1));
    >
    > 3) bits == 2
    >
    > this is the last entry in the scatterlist, sg_next() is NULL
    >
    > So for the cases of ARCH_HAS_SG_CHAIN not being set (ie. back
    > compatible), we can do no bit encoding in page->flags and just do
    > sg_next() == sg++, as is done now.
    >
    > When doing SG chaining, in each non-linear chunk we have to allocate
    > one more pointer past the end of the scatterlist array (instead of a
    > full extra scatterlist entry for the indirect pointer encode). Next,
    > all sg->page accesses have to be guarded to clear the state bits
    > out first.
    >
    > I don't know, maybe it would work, and would make the loop termination
    > issues easier to handle properly.


    I like it. Basically the only real change is using bit 2 as a
    termination point, so we avoid going beyond the end of the sgtable.
    Here's a starting point, it actually booted for me in the first go
    (boggle). Only x86 so far, archs will need to be converted. And lots
    more drivers I'm sure, I only fixed up the ones that botched my compile.

    So just consider this a directional patch.

    diff --git a/arch/x86/kernel/pci-gart_64.c b/arch/x86/kernel/pci-gart_64.c
    index 5cdfab6..daaf636 100644
    --- a/arch/x86/kernel/pci-gart_64.c
    +++ b/arch/x86/kernel/pci-gart_64.c
    @@ -302,7 +302,7 @@ static int dma_map_sg_nonforce(struct device *dev, struct scatterlist *sg,
    #endif

    for_each_sg(sg, s, nents, i) {
    - unsigned long addr = page_to_phys(s->page) + s->offset;
    + unsigned long addr = page_to_phys(sg_page(s)) + s->offset;
    if (nonforced_iommu(dev, addr, s->length)) {
    addr = dma_map_area(dev, addr, s->length, dir);
    if (addr == bad_dma_address) {
    @@ -397,7 +397,7 @@ static int gart_map_sg(struct device *dev, struct scatterlist *sg, int nents,
    start_sg = sgmap = sg;
    ps = NULL; /* shut up gcc */
    for_each_sg(sg, s, nents, i) {
    - dma_addr_t addr = page_to_phys(s->page) + s->offset;
    + dma_addr_t addr = page_to_phys(sg_page(s)) + s->offset;
    s->dma_address = addr;
    BUG_ON(s->length == 0);

    diff --git a/arch/x86/kernel/pci-nommu_64.c b/arch/x86/kernel/pci-nommu_64.c
    index e85d436..d64a4a5 100644
    --- a/arch/x86/kernel/pci-nommu_64.c
    +++ b/arch/x86/kernel/pci-nommu_64.c
    @@ -62,8 +62,8 @@ static int nommu_map_sg(struct device *hwdev, struct scatterlist *sg,
    int i;

    for_each_sg(sg, s, nents, i) {
    - BUG_ON(!s->page);
    - s->dma_address = virt_to_bus(page_address(s->page) +s->offset);
    + BUG_ON(!sg_page(s));
    + s->dma_address = virt_to_bus(page_address(sg_page(s)) +s->offset);
    if (!check_addr("map_sg", hwdev, s->dma_address, s->length))
    return 0;
    s->dma_length = s->length;
    diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
    index 3935469..d02783c 100644
    --- a/block/ll_rw_blk.c
    +++ b/block/ll_rw_blk.c
    @@ -1354,10 +1354,11 @@ new_segment:
    else
    sg = sg_next(sg);

    - memset(sg, 0, sizeof(*sg));
    - sg->page = bvec->bv_page;
    + sg_set_page(sg, bvec->bv_page);
    sg->length = nbytes;
    sg->offset = bvec->bv_offset;
    + sg_dma_len(sg) = 0;
    + sg_dma_address(sg) = 0;
    nsegs++;
    }
    bvprv = bvec;
    diff --git a/crypto/digest.c b/crypto/digest.c
    index e56de67..8871dec 100644
    --- a/crypto/digest.c
    +++ b/crypto/digest.c
    @@ -41,7 +41,7 @@ static int update2(struct hash_desc *desc,
    return 0;

    for (; {
    - struct page *pg = sg->page;
    + struct page *pg = sg_page(sg);
    unsigned int offset = sg->offset;
    unsigned int l = sg->length;

    diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
    index d6852c3..b9bbda0 100644
    --- a/crypto/scatterwalk.c
    +++ b/crypto/scatterwalk.c
    @@ -54,7 +54,7 @@ static void scatterwalk_pagedone(struct scatter_walk *walk, int out,
    if (out) {
    struct page *page;

    - page = walk->sg->page + ((walk->offset - 1) >> PAGE_SHIFT);
    + page = sg_page(walk->sg) + ((walk->offset - 1) >> PAGE_SHIFT);
    flush_dcache_page(page);
    }

    diff --git a/crypto/scatterwalk.h b/crypto/scatterwalk.h
    index 9c73e37..87ed681 100644
    --- a/crypto/scatterwalk.h
    +++ b/crypto/scatterwalk.h
    @@ -22,13 +22,13 @@

    static inline struct scatterlist *scatterwalk_sg_next(struct scatterlist *sg)
    {
    - return (++sg)->length ? sg : (void *)sg->page;
    + return (++sg)->length ? sg : (void *) sg_page(sg);
    }

    static inline unsigned long scatterwalk_samebuf(struct scatter_walk *walk_in,
    struct scatter_walk *walk_out)
    {
    - return !(((walk_in->sg->page - walk_out->sg->page) << PAGE_SHIFT) +
    + return !(((sg_page(walk_in->sg) - sg_page(walk_out->sg)) << PAGE_SHIFT) +
    (int)(walk_in->offset - walk_out->offset));
    }

    @@ -60,7 +60,7 @@ static inline unsigned int scatterwalk_aligned(struct scatter_walk *walk,

    static inline struct page *scatterwalk_page(struct scatter_walk *walk)
    {
    - return walk->sg->page + (walk->offset >> PAGE_SHIFT);
    + return sg_page(walk->sg) + (walk->offset >> PAGE_SHIFT);
    }

    static inline void scatterwalk_unmap(void *vaddr, int out)
    diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
    index bbaa545..b1fa70a 100644
    --- a/drivers/ata/libata-core.c
    +++ b/drivers/ata/libata-core.c
    @@ -4296,7 +4296,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc)
    sg_last(sg, qc->orig_n_elem)->length += qc->pad_len;
    if (pad_buf) {
    struct scatterlist *psg = &qc->pad_sgent;
    - void *addr = kmap_atomic(psg->page, KM_IRQ0);
    + void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
    memcpy(addr + psg->offset, pad_buf, qc->pad_len);
    kunmap_atomic(addr, KM_IRQ0);
    }
    @@ -4686,11 +4686,11 @@ static int ata_sg_setup(struct ata_queued_cmd *qc)
    * data in this function or read data in ata_sg_clean.
    */
    offset = lsg->offset + lsg->length - qc->pad_len;
    - psg->page = nth_page(lsg->page, offset >> PAGE_SHIFT);
    + sg_set_page(psg, nth_page(sg_page(lsg), offset >> PAGE_SHIFT));
    psg->offset = offset_in_page(offset);

    if (qc->tf.flags & ATA_TFLAG_WRITE) {
    - void *addr = kmap_atomic(psg->page, KM_IRQ0);
    + void *addr = kmap_atomic(sg_page(psg), KM_IRQ0);
    memcpy(pad_buf, addr + psg->offset, qc->pad_len);
    kunmap_atomic(addr, KM_IRQ0);
    }
    @@ -4836,7 +4836,7 @@ static void ata_pio_sector(struct ata_queued_cmd *qc)
    if (qc->curbytes == qc->nbytes - qc->sect_size)
    ap->hsm_task_state = HSM_ST_LAST;

    - page = qc->cursg->page;
    + page = sg_page(qc->cursg);
    offset = qc->cursg->offset + qc->cursg_ofs;

    /* get the current page and offset */
    @@ -4988,7 +4988,7 @@ next_sg:

    sg = qc->cursg;

    - page = sg->page;
    + page = sg_page(sg);
    offset = sg->offset + qc->cursg_ofs;

    /* get the current page and offset */
    diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
    index 9fbb39c..5b758b9 100644
    --- a/drivers/ata/libata-scsi.c
    +++ b/drivers/ata/libata-scsi.c
    @@ -1544,7 +1544,7 @@ static unsigned int ata_scsi_rbuf_get(struct scsi_cmnd *cmd, u8 **buf_out)
    struct scatterlist *sg = scsi_sglist(cmd);

    if (sg) {
    - buf = kmap_atomic(sg->page, KM_IRQ0) + sg->offset;
    + buf = kmap_atomic(sg_page(sg), KM_IRQ0) + sg->offset;
    buflen = sg->length;
    } else {
    buf = NULL;
    diff --git a/drivers/ieee1394/dma.c b/drivers/ieee1394/dma.c
    index 45d6055..25e113b 100644
    --- a/drivers/ieee1394/dma.c
    +++ b/drivers/ieee1394/dma.c
    @@ -111,7 +111,7 @@ int dma_region_alloc(struct dma_region *dma, unsigned long n_bytes,
    unsigned long va =
    (unsigned long)dma->kvirt + (i << PAGE_SHIFT);

    - dma->sglist[i].page = vmalloc_to_page((void *)va);
    + sg_set_page(&dma->sglist[i], vmalloc_to_page((void *)va));
    dma->sglist[i].length = PAGE_SIZE;
    }

    diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
    index 72ee4c9..46cae5a 100644
    --- a/drivers/scsi/scsi_debug.c
    +++ b/drivers/scsi/scsi_debug.c
    @@ -625,7 +625,7 @@ static int fill_from_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
    scsi_for_each_sg(scp, sg, scp->use_sg, k) {
    if (active) {
    kaddr = (unsigned char *)
    - kmap_atomic(sg->page, KM_USER0);
    + kmap_atomic(sg_page(sg), KM_USER0);
    if (NULL == kaddr)
    return (DID_ERROR << 16);
    kaddr_off = (unsigned char *)kaddr + sg->offset;
    @@ -672,7 +672,7 @@ static int fetch_to_dev_buffer(struct scsi_cmnd * scp, unsigned char * arr,
    sg = scsi_sglist(scp);
    req_len = fin = 0;
    for (k = 0; k < scp->use_sg; ++k, sg = sg_next(sg)) {
    - kaddr = (unsigned char *)kmap_atomic(sg->page, KM_USER0);
    + kaddr = (unsigned char *)kmap_atomic(sg_page(sg), KM_USER0);
    if (NULL == kaddr)
    return -1;
    kaddr_off = (unsigned char *)kaddr + sg->offset;
    diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
    index aac8a02..4ee32e5 100644
    --- a/drivers/scsi/scsi_lib.c
    +++ b/drivers/scsi/scsi_lib.c
    @@ -295,7 +295,7 @@ static int scsi_req_map_sg(struct request *rq, struct scatterlist *sgl,
    int i, err, nr_vecs = 0;

    for_each_sg(sgl, sg, nsegs, i) {
    - page = sg->page;
    + page = sg_page(sg);
    off = sg->offset;
    len = sg->length;
    data_len += len;
    @@ -781,6 +781,13 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
    sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);

    /*
    + * if we have nothing left, mark the last segment as
    + * end-of-list
    + */
    + if (!left)
    + sg_mark_end(sgl, this);
    +
    + /*
    * don't allow subsequent mempool allocs to sleep, it would
    * violate the mempool principle.
    */
    @@ -2353,7 +2360,7 @@ void *scsi_kmap_atomic_sg(struct scatterlist *sgl, int sg_count,
    *offset = *offset - len_complete + sg->offset;

    /* Assumption: contiguous pages can be accessed as "page + i" */
    - page = nth_page(sg->page, (*offset >> PAGE_SHIFT));
    + page = nth_page(sg_page(sg), (*offset >> PAGE_SHIFT));
    *offset &= ~PAGE_MASK;

    /* Bytes in this sg-entry from *offset to the end of the page */
    diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
    index 7238b2d..cc19710 100644
    --- a/drivers/scsi/sg.c
    +++ b/drivers/scsi/sg.c
    @@ -1169,7 +1169,7 @@ sg_vma_nopage(struct vm_area_struct *vma, unsigned long addr, int *type)
    len = vma->vm_end - sa;
    len = (len < sg->length) ? len : sg->length;
    if (offset < len) {
    - page = virt_to_page(page_address(sg->page) + offset);
    + page = virt_to_page(page_address(sg_page(sg)) + offset);
    get_page(page); /* increment page count */
    break;
    }
    @@ -1717,13 +1717,13 @@ st_map_user_pages(struct scatterlist *sgl, const unsigned int max_pages,
    goto out_unlock; */
    }

    - sgl[0].page = pages[0];
    + sg_set_page(sgl, pages[0]);
    sgl[0].offset = uaddr & ~PAGE_MASK;
    if (nr_pages > 1) {
    sgl[0].length = PAGE_SIZE - sgl[0].offset;
    count -= sgl[0].length;
    for (i=1; i < nr_pages ; i++) {
    - sgl[i].page = pages[i];
    + sg_set_page(&sgl[i], pages[i]);
    sgl[i].length = count < PAGE_SIZE ? count : PAGE_SIZE;
    count -= PAGE_SIZE;
    }
    @@ -1754,7 +1754,7 @@ st_unmap_user_pages(struct scatterlist *sgl, const unsigned int nr_pages,
    int i;

    for (i=0; i < nr_pages; i++) {
    - struct page *page = sgl[i].page;
    + struct page *page = sg_page(&sgl[i]);

    if (dirtied)
    SetPageDirty(page);
    @@ -1854,7 +1854,7 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
    scatter_elem_sz_prev = ret_sz;
    }
    }
    - sg->page = p;
    + sg_set_page(sg, p);
    sg->length = (ret_sz > num) ? num : ret_sz;

    SCSI_LOG_TIMEOUT(5, printk("sg_build_indirect: k=%d, num=%d, "
    @@ -1907,14 +1907,14 @@ sg_write_xfer(Sg_request * srp)
    onum = 1;

    ksglen = sg->length;
    - p = page_address(sg->page);
    + p = page_address(sg_page(sg));
    for (j = 0, k = 0; j < onum; ++j) {
    res = sg_u_iovec(hp, iovec_count, j, 1, &usglen, &up);
    if (res)
    return res;

    for (; p; sg = sg_next(sg), ksglen = sg->length,
    - p = page_address(sg->page)) {
    + p = page_address(sg_page(sg))) {
    if (usglen <= 0)
    break;
    if (ksglen > usglen) {
    @@ -1991,12 +1991,12 @@ sg_remove_scat(Sg_scatter_hold * schp)
    } else {
    int k;

    - for (k = 0; (k < schp->k_use_sg) && sg->page;
    + for (k = 0; (k < schp->k_use_sg) && sg_page(sg);
    ++k, sg = sg_next(sg)) {
    SCSI_LOG_TIMEOUT(5, printk(
    "sg_remove_scat: k=%d, pg=0x%p, len=%d\n",
    - k, sg->page, sg->length));
    - sg_page_free(sg->page, sg->length);
    + k, sg_page(sg), sg->length));
    + sg_page_free(sg_page(sg), sg->length);
    }
    }
    kfree(schp->buffer);
    @@ -2038,7 +2038,7 @@ sg_read_xfer(Sg_request * srp)
    } else
    onum = 1;

    - p = page_address(sg->page);
    + p = page_address(sg_page(sg));
    ksglen = sg->length;
    for (j = 0, k = 0; j < onum; ++j) {
    res = sg_u_iovec(hp, iovec_count, j, 0, &usglen, &up);
    @@ -2046,7 +2046,7 @@ sg_read_xfer(Sg_request * srp)
    return res;

    for (; p; sg = sg_next(sg), ksglen = sg->length,
    - p = page_address(sg->page)) {
    + p = page_address(sg_page(sg))) {
    if (usglen <= 0)
    break;
    if (ksglen > usglen) {
    @@ -2092,15 +2092,15 @@ sg_read_oxfer(Sg_request * srp, char __user *outp, int num_read_xfer)
    if ((!outp) || (num_read_xfer <= 0))
    return 0;

    - for (k = 0; (k < schp->k_use_sg) && sg->page; ++k, sg = sg_next(sg)) {
    + for (k = 0; (k < schp->k_use_sg) && sg_page(sg); ++k, sg = sg_next(sg)) {
    num = sg->length;
    if (num > num_read_xfer) {
    - if (__copy_to_user(outp, page_address(sg->page),
    + if (__copy_to_user(outp, page_address(sg_page(sg)),
    num_read_xfer))
    return -EFAULT;
    break;
    } else {
    - if (__copy_to_user(outp, page_address(sg->page),
    + if (__copy_to_user(outp, page_address(sg_page(sg)),
    num))
    return -EFAULT;
    num_read_xfer -= num;
    diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
    index c021af3..98c455d 100644
    --- a/drivers/usb/core/message.c
    +++ b/drivers/usb/core/message.c
    @@ -443,7 +443,7 @@ int usb_sg_init (
    } else {
    /* hc may use _only_ transfer_buffer */
    io->urbs [i]->transfer_buffer =
    - page_address (sg [i].page) + sg [i].offset;
    + page_address(sg_page(&sg[i])) + sg [i].offset;
    len = sg [i].length;
    }

    diff --git a/drivers/usb/storage/protocol.c b/drivers/usb/storage/protocol.c
    index cc8f7c5..889622b 100644
    --- a/drivers/usb/storage/protocol.c
    +++ b/drivers/usb/storage/protocol.c
    @@ -195,7 +195,7 @@ unsigned int usb_stor_access_xfer_buf(unsigned char *buffer,
    * the *offset and *index values for the next loop. */
    cnt = 0;
    while (cnt < buflen) {
    - struct page *page = sg->page +
    + struct page *page = sg_page(sg) +
    ((sg->offset + *offset) >> PAGE_SHIFT);
    unsigned int poff =
    (sg->offset + *offset) & (PAGE_SIZE-1);
    diff --git a/include/asm-x86/dma-mapping_32.h b/include/asm-x86/dma-mapping_32.h
    index 6a2d26c..e0d38d8 100644
    --- a/include/asm-x86/dma-mapping_32.h
    +++ b/include/asm-x86/dma-mapping_32.h
    @@ -45,9 +45,9 @@ dma_map_sg(struct device *dev, struct scatterlist *sglist, int nents,
    WARN_ON(nents == 0 || sglist[0].length == 0);

    for_each_sg(sglist, sg, nents, i) {
    - BUG_ON(!sg->page);
    + BUG_ON(!sg_page(sg));

    - sg->dma_address = page_to_phys(sg->page) + sg->offset;
    + sg->dma_address = page_to_phys(sg_page(sg)) + sg->offset;
    }

    flush_write_buffers();
    diff --git a/include/asm-x86/scatterlist_32.h b/include/asm-x86/scatterlist_32.h
    index bd5164a..0e7d997 100644
    --- a/include/asm-x86/scatterlist_32.h
    +++ b/include/asm-x86/scatterlist_32.h
    @@ -4,7 +4,10 @@
    #include

    struct scatterlist {
    - struct page *page;
    +#ifdef CONFIG_DEBUG_SG
    + unsigned long sg_magic;
    +#endif
    + unsigned long page_link;
    unsigned int offset;
    dma_addr_t dma_address;
    unsigned int length;
    diff --git a/include/asm-x86/scatterlist_64.h b/include/asm-x86/scatterlist_64.h
    index ef3986b..1847c72 100644
    --- a/include/asm-x86/scatterlist_64.h
    +++ b/include/asm-x86/scatterlist_64.h
    @@ -4,7 +4,10 @@
    #include

    struct scatterlist {
    - struct page *page;
    +#ifdef CONFIG_DEBUG_SG
    + unsigned long sg_magic;
    +#endif
    + unsigned long page_link;
    unsigned int offset;
    unsigned int length;
    dma_addr_t dma_address;
    diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
    index 2dc7464..cbc2b57 100644
    --- a/include/linux/scatterlist.h
    +++ b/include/linux/scatterlist.h
    @@ -5,10 +5,24 @@
    #include
    #include

    +#define SG_MAGIC 0x87654321
    +
    +static inline void sg_set_page(struct scatterlist *sg, struct page *page)
    +{
    + unsigned long page_link = sg->page_link & 0x3;
    +
    + sg->page_link = page_link | (unsigned long) page;
    +}
    +
    +#define sg_page(sg) ((struct page *) ((sg)->page_link & ~0x3))
    +
    static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
    unsigned int buflen)
    {
    - sg->page = virt_to_page(buf);
    +#ifdef CONFIG_DEBUG_SG
    + sg->sg_magic = SG_MAGIC;
    +#endif
    + sg_set_page(sg, virt_to_page(buf));
    sg->offset = offset_in_page(buf);
    sg->length = buflen;
    }
    @@ -25,9 +39,9 @@ static inline void sg_init_one(struct scatterlist *sg, const void *buf,
    * a valid sg entry, or whether it points to the start of a new scatterlist.
    * Those low bits are there for everyone! (thanks mason :-)
    */
    -#define sg_is_chain(sg) ((unsigned long) (sg)->page & 0x01)
    -#define sg_chain_ptr(sg) \
    - ((struct scatterlist *) ((unsigned long) (sg)->page & ~0x01))
    +#define sg_is_chain(sg) ((sg)->page_link & 0x01)
    +#define sg_is_last(sg) ((sg)->page_link & 0x02)
    +#define sg_chain_ptr(sg) ((struct scatterlist *) ((sg)->page_link & ~0x01))

    /**
    * sg_next - return the next scatterlist entry in a list
    @@ -43,10 +57,15 @@ static inline void sg_init_one(struct scatterlist *sg, const void *buf,
    */
    static inline struct scatterlist *sg_next(struct scatterlist *sg)
    {
    - sg++;
    -
    - if (unlikely(sg_is_chain(sg)))
    +#ifdef CONFIG_DEBUG_SG
    + BUG_ON(sg->sg_magic != SG_MAGIC);
    +#endif
    + if (sg_is_last(sg))
    + sg = NULL;
    + else if (sg_is_chain(sg))
    sg = sg_chain_ptr(sg);
    + else
    + sg++;

    return sg;
    }
    @@ -83,6 +102,9 @@ static inline struct scatterlist *sg_last(struct scatterlist *sgl,
    ret = sg;

    #endif
    +#ifdef CONFIG_DEBUG_SG
    + BUG_ON(sgl[0].sg_magic != SG_MAGIC);
    +#endif
    return ret;
    }

    @@ -101,7 +123,41 @@ static inline void sg_chain(struct scatterlist *prv, unsigned int prv_nents,
    #ifndef ARCH_HAS_SG_CHAIN
    BUG();
    #endif
    - prv[prv_nents - 1].page = (struct page *) ((unsigned long) sgl | 0x01);
    + prv[prv_nents - 1].page_link = (unsigned long) sgl | 0x01;
    +}
    +
    +/**
    + * sg_mark_end - Mark the end of the scatterlist
    + * @sgl: Scatterlist
    + * @nents: Number of entries in sgl
    + *
    + * Marks the last entry as the termination point for sg_next()
    + *
    + */
    +static inline void sg_mark_end(struct scatterlist *sgl, unsigned int nents)
    +{
    + sgl[nents - 1].page_link = 0x02;
    +}
    +
    +/**
    + * sg_init_table - Initialize an sg table
    + * @sgl: Scatterlist
    + * @nents: Number of entries in sgl
    + *
    + * Marks the last entry as the termination point for sg_next()
    + *
    + */
    +static inline void sg_init_table(struct scatterlist *sgl, unsigned int nents)
    +{
    + memset(sgl, 0, sizeof(*sgl) * nents);
    + sg_mark_end(sgl, nents);
    +#ifdef CONFIG_DEBUG_SG
    + {
    + int i;
    + for (i = 0; i < nents; i++)
    + sgl[i].sg_magic = SG_MAGIC;
    + }
    +#endif
    }

    #endif /* _LINUX_SCATTERLIST_H */
    diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
    index 7d16e64..c17aace 100644
    --- a/lib/Kconfig.debug
    +++ b/lib/Kconfig.debug
    @@ -389,6 +389,17 @@ config DEBUG_LIST

    If unsure, say N.

    +config DEBUG_SG
    + bool "Debug SG table operations"
    + depends on DEBUG_KERNEL
    + help
    + Enable this to turn on checks on scatter-gather tables. This can
    + help find problems with drivers that do not properly initialize
    + their sg tables. Runtime overhead is very small, but it does
    + increase an sg entry by sizeof(unsigned long).
    +
    + If unsure, say N.
    +
    config FRAME_POINTER
    bool "Compile the kernel with frame pointers"
    depends on DEBUG_KERNEL && (X86 || CRIS || M68K || M68KNOMMU || FRV || UML || S390 || AVR32 || SUPERH || BFIN)
    diff --git a/lib/swiotlb.c b/lib/swiotlb.c
    index 752fd95..e58909e 100644
    --- a/lib/swiotlb.c
    +++ b/lib/swiotlb.c
    @@ -35,7 +35,7 @@
    #define OFFSET(val,align) ((unsigned long) \
    ( (val) & ( (align) - 1)))

    -#define SG_ENT_VIRT_ADDRESS(sg) (page_address((sg)->page) + (sg)->offset)
    +#define SG_ENT_VIRT_ADDRESS(sg) (page_address(sg_page((sg)) + (sg)->offset))
    #define SG_ENT_PHYS_ADDRESS(sg) virt_to_bus(SG_ENT_VIRT_ADDRESS(sg))

    /*
    diff --git a/net/core/skbuff.c b/net/core/skbuff.c
    index 70d9b5d..4e2c84f 100644
    --- a/net/core/skbuff.c
    +++ b/net/core/skbuff.c
    @@ -2045,7 +2045,7 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)
    if (copy > 0) {
    if (copy > len)
    copy = len;
    - sg[elt].page = virt_to_page(skb->data + offset);
    + sg_set_page(&sg[elt], virt_to_page(skb->data + offset));
    sg[elt].offset = (unsigned long)(skb->data + offset) % PAGE_SIZE;
    sg[elt].length = copy;
    elt++;
    @@ -2065,7 +2065,7 @@ skb_to_sgvec(struct sk_buff *skb, struct scatterlist *sg, int offset, int len)

    if (copy > len)
    copy = len;
    - sg[elt].page = frag->page;
    + sg_set_page(&sg[elt], frag->page);
    sg[elt].offset = frag->page_offset+offset-start;
    sg[elt].length = copy;
    elt++;
    diff --git a/net/ieee80211/ieee80211_crypt_wep.c b/net/ieee80211/ieee80211_crypt_wep.c
    index 8d18245..1a77877 100644
    --- a/net/ieee80211/ieee80211_crypt_wep.c
    +++ b/net/ieee80211/ieee80211_crypt_wep.c
    @@ -170,7 +170,8 @@ static int prism2_wep_encrypt(struct sk_buff *skb, int hdr_len, void *priv)
    icv[3] = crc >> 24;

    crypto_blkcipher_setkey(wep->tx_tfm, key, klen);
    - sg.page = virt_to_page(pos);
    + sg_init_table(&sg, 1);
    + sg_set_page(&sg, virt_to_page(pos));
    sg.offset = offset_in_page(pos);
    sg.length = len + 4;
    return crypto_blkcipher_encrypt(&desc, &sg, &sg, len + 4);
    @@ -212,7 +213,8 @@ static int prism2_wep_decrypt(struct sk_buff *skb, int hdr_len, void *priv)
    plen = skb->len - hdr_len - 8;

    crypto_blkcipher_setkey(wep->rx_tfm, key, klen);
    - sg.page = virt_to_page(pos);
    + sg_init_table(&sg, 1);
    + sg_set_page(&sg, virt_to_page(pos));
    sg.offset = offset_in_page(pos);
    sg.length = plen + 4;
    if (crypto_blkcipher_decrypt(&desc, &sg, &sg, plen + 4))
    diff --git a/net/mac80211/wep.c b/net/mac80211/wep.c
    index 6675261..9857961 100644
    --- a/net/mac80211/wep.c
    +++ b/net/mac80211/wep.c
    @@ -138,7 +138,8 @@ void ieee80211_wep_encrypt_data(struct crypto_blkcipher *tfm, u8 *rc4key,
    *icv = cpu_to_le32(~crc32_le(~0, data, data_len));

    crypto_blkcipher_setkey(tfm, rc4key, klen);
    - sg.page = virt_to_page(data);
    + sg_init_table(&sg, 1);
    + sg_set_page(&sg, virt_to_page(data));
    sg.offset = offset_in_page(data);
    sg.length = data_len + WEP_ICV_LEN;
    crypto_blkcipher_encrypt(&desc, &sg, &sg, sg.length);
    @@ -204,7 +205,8 @@ int ieee80211_wep_decrypt_data(struct crypto_blkcipher *tfm, u8 *rc4key,
    __le32 crc;

    crypto_blkcipher_setkey(tfm, rc4key, klen);
    - sg.page = virt_to_page(data);
    + sg_init_table(&sg, 1);
    + sg_set_page(&sg, virt_to_page(data));
    sg.offset = offset_in_page(data);
    sg.length = data_len + WEP_ICV_LEN;
    crypto_blkcipher_decrypt(&desc, &sg, &sg, sg.length);
    diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
    index bfb6a29..32be431 100644
    --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
    +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
    @@ -197,9 +197,9 @@ encryptor(struct scatterlist *sg, void *data)
    int i = (page_pos + outbuf->page_base) >> PAGE_CACHE_SHIFT;
    in_page = desc->pages[i];
    } else {
    - in_page = sg->page;
    + in_page = sg_page(sg);
    }
    - desc->infrags[desc->fragno].page = in_page;
    + sg_set_page(&desc->infrags[desc->fragno], in_page);
    desc->fragno++;
    desc->fraglen += sg->length;
    desc->pos += sg->length;
    @@ -215,11 +215,11 @@ encryptor(struct scatterlist *sg, void *data)
    if (ret)
    return ret;
    if (fraglen) {
    - desc->outfrags[0].page = sg->page;
    + sg_set_page(&desc->outfrags[0], sg_page(sg));
    desc->outfrags[0].offset = sg->offset + sg->length - fraglen;
    desc->outfrags[0].length = fraglen;
    desc->infrags[0] = desc->outfrags[0];
    - desc->infrags[0].page = in_page;
    + sg_set_page(&desc->infrags[0], in_page);
    desc->fragno = 1;
    desc->fraglen = fraglen;
    } else {
    @@ -287,7 +287,7 @@ decryptor(struct scatterlist *sg, void *data)
    if (ret)
    return ret;
    if (fraglen) {
    - desc->frags[0].page = sg->page;
    + sg_set_page(&desc->frags[0], sg_page(sg));
    desc->frags[0].offset = sg->offset + sg->length - fraglen;
    desc->frags[0].length = fraglen;
    desc->fragno = 1;
    diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
    index 6a59180..3d1f7cd 100644
    --- a/net/sunrpc/xdr.c
    +++ b/net/sunrpc/xdr.c
    @@ -1059,7 +1059,7 @@ xdr_process_buf(struct xdr_buf *buf, unsigned int offset, unsigned int len,
    do {
    if (thislen > page_len)
    thislen = page_len;
    - sg->page = buf->pages[i];
    + sg_set_page(sg, buf->pages[i]);
    sg->offset = page_offset;
    sg->length = thislen;
    ret = actor(sg, data);
    diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
    index 5ced62c..fb2220a 100644
    --- a/net/xfrm/xfrm_algo.c
    +++ b/net/xfrm/xfrm_algo.c
    @@ -552,7 +552,7 @@ int skb_icv_walk(const struct sk_buff *skb, struct hash_desc *desc,
    if (copy > len)
    copy = len;

    - sg.page = virt_to_page(skb->data + offset);
    + sg_set_page(&sg, virt_to_page(skb->data + offset));
    sg.offset = (unsigned long)(skb->data + offset) % PAGE_SIZE;
    sg.length = copy;

    @@ -577,7 +577,7 @@ int skb_icv_walk(const struct sk_buff *skb, struct hash_desc *desc,
    if (copy > len)
    copy = len;

    - sg.page = frag->page;
    + sg_set_page(&sg, frag->page);
    sg.offset = frag->page_offset + offset-start;
    sg.length = copy;


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

  6. Re: [bug] ata subsystem related crash with latest -git

    Ingo Molnar wrote:
    > * Jens Axboe wrote:
    >
    >> --- a/drivers/scsi/scsi_lib.c
    >> +++ b/drivers/scsi/scsi_lib.c
    >> @@ -39,7 +39,7 @@
    >> * (unless chaining is used). Should ideally fit inside a single page, to
    >> * avoid a higher order allocation.
    >> */
    >> -#define SCSI_MAX_SG_SEGMENTS 128
    >> +#define SCSI_MAX_SG_SEGMENTS 129

    >
    > this one finally made the trick and it's booting fine now, without any
    > crashes!


    Alas, this didn't help me here. I did manage to capture the error
    messages this time, and in the two machines I'm actively testing on,
    sata_mv is the driver that's dying in both cases. Machine A
    additionally has sata_nv, which is working. Machine B additionally has
    ata_piix, which is working. So in both cases, its sata_mv that is
    throwing SError complaints:

    > ata7.00: exception Emask 0x0 SAct 0x0 SErr 0x400000 action 0x6 frozen
    > ata7.00: edma_err 0x04000080, EDMA self-disable
    > ata7: SError: { Handshk }
    > ata7.00: cmd ca/00:08:c7:40:00/00:00:00:00:00/e0 tag 0 cdb 0x0 data 4096 out
    > res 50/00:00:ce:40:00/00:00:00:00:00/e0 Emask 0x100 (unknown error)
    > ata7.00: status: { DRDY }
    > ata7: hard resetting link
    > ata7: SATA link up 1.5 Gbps (SStatus 113 SControl 300)


    Still digging... this behavior showed up after libata changes went in,
    FWIW.

    Jeff


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

  7. Re: [bug] ata subsystem related crash with latest -git

    Jens Axboe wrote:
    > On Thu, Oct 18 2007, Jeff Garzik wrote:
    >> Ingo Molnar wrote:
    >>> * Jens Axboe wrote:
    >>>> --- a/drivers/scsi/scsi_lib.c
    >>>> +++ b/drivers/scsi/scsi_lib.c
    >>>> @@ -39,7 +39,7 @@
    >>>> * (unless chaining is used). Should ideally fit inside a single page,
    >>>> to
    >>>> * avoid a higher order allocation.
    >>>> */
    >>>> -#define SCSI_MAX_SG_SEGMENTS 128
    >>>> +#define SCSI_MAX_SG_SEGMENTS 129
    >>> this one finally made the trick and it's booting fine now, without any
    >>> crashes!

    >> Alas, this didn't help me here. I did manage to capture the error messages
    >> this time, and in the two machines I'm actively testing on, sata_mv is the
    >> driver that's dying in both cases. Machine A additionally has sata_nv,
    >> which is working. Machine B additionally has ata_piix, which is working.
    >> So in both cases, its sata_mv that is throwing SError complaints:

    >
    > Theory - ata_sg_is_last() isn't returning true for the last entry. Can
    > you double check that it correcly marks the last entry in mv_fill_sg()?
    > Alternatively, just try this patch.


    Will check in a few minutes, after my current test: I noticed that
    sg_tablesize in sata_mv is not LIBATA_MAX_PRD. This is expected
    behavior, but I wonder if that difference -- most notably being smaller
    than SCSI_MAX_SG_SEGMENTS -- would trigger any latent bugs.

    Anyway, we will both have answers momentarily...

    Jeff



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

  8. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Ingo Molnar wrote:
    >> * Jens Axboe wrote:
    >>> --- a/drivers/scsi/scsi_lib.c
    >>> +++ b/drivers/scsi/scsi_lib.c
    >>> @@ -39,7 +39,7 @@
    >>> * (unless chaining is used). Should ideally fit inside a single page,
    >>> to
    >>> * avoid a higher order allocation.
    >>> */
    >>> -#define SCSI_MAX_SG_SEGMENTS 128
    >>> +#define SCSI_MAX_SG_SEGMENTS 129

    >> this one finally made the trick and it's booting fine now, without any
    >> crashes!

    >
    > Alas, this didn't help me here. I did manage to capture the error messages
    > this time, and in the two machines I'm actively testing on, sata_mv is the
    > driver that's dying in both cases. Machine A additionally has sata_nv,
    > which is working. Machine B additionally has ata_piix, which is working.
    > So in both cases, its sata_mv that is throwing SError complaints:


    Theory - ata_sg_is_last() isn't returning true for the last entry. Can
    you double check that it correcly marks the last entry in mv_fill_sg()?
    Alternatively, just try this patch.


    diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    index 4df8311..b858183 100644
    --- a/drivers/ata/sata_mv.c
    +++ b/drivers/ata/sata_mv.c
    @@ -1139,6 +1139,7 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    struct mv_port_priv *pp = qc->ap->private_data;
    struct scatterlist *sg;
    struct mv_sg *mv_sg;
    + int end_marked = 0;

    mv_sg = pp->sg_tbl;
    ata_for_each_sg(sg, qc) {
    @@ -1159,13 +1160,15 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    sg_len -= len;
    addr += len;

    - if (!sg_len && ata_sg_is_last(sg, qc))
    + if (!sg_len && ata_sg_is_last(sg, qc)) {
    mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    + end_marked++;
    + }

    mv_sg++;
    }
    -
    }
    + BUG_ON(end_marked != 1);
    }

    static inline void mv_crqb_pack_cmd(__le16 *cmdw, u8 data, u8 addr, unsigned last)

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

  9. Re: [bug] ata subsystem related crash with latest -git

    Jeff Garzik wrote:
    > I noticed that
    > sg_tablesize in sata_mv is not LIBATA_MAX_PRD. This is expected
    > behavior, but I wonder if that difference -- most notably being smaller
    > than SCSI_MAX_SG_SEGMENTS -- would trigger any latent bugs.


    Another sata_mv difference from the rest: the chip does not support
    ATAPI, so we never care about qc->pad

    Jeff


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

  10. Re: [bug] ata subsystem related crash with latest -git

    Jens Axboe wrote:
    > index 4df8311..b858183 100644
    > --- a/drivers/ata/sata_mv.c
    > +++ b/drivers/ata/sata_mv.c
    > @@ -1139,6 +1139,7 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    > struct mv_port_priv *pp = qc->ap->private_data;
    > struct scatterlist *sg;
    > struct mv_sg *mv_sg;
    > + int end_marked = 0;
    >
    > mv_sg = pp->sg_tbl;
    > ata_for_each_sg(sg, qc) {
    > @@ -1159,13 +1160,15 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    > sg_len -= len;
    > addr += len;
    >
    > - if (!sg_len && ata_sg_is_last(sg, qc))
    > + if (!sg_len && ata_sg_is_last(sg, qc)) {
    > mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    > + end_marked++;
    > + }
    >
    > mv_sg++;
    > }
    > -
    > }
    > + BUG_ON(end_marked != 1);



    Your BUG_ON() does indeed trip, here.

    Its surprising that other folks don't explode, considering that
    mv_fill_sg() intentionally mirrors the logic in ata_fill_sg().

    Jeff


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

  11. Re: [bug] ata subsystem related crash with latest -git

    Benny Halevy wrote:
    >
    >
    > On 10/18/07, *Jeff Garzik* > wrote:
    >
    > Jens Axboe wrote:
    > > index 4df8311..b858183 100644
    > > --- a/drivers/ata/sata_mv.c
    > > +++ b/drivers/ata/sata_mv.c
    > > @@ -1139,6 +1139,7 @@ static void mv_fill_sg(struct

    > ata_queued_cmd *qc)
    > > struct mv_port_priv *pp = qc->ap->private_data;
    > > struct scatterlist *sg;
    > > struct mv_sg *mv_sg;
    > > + int end_marked = 0;
    > >
    > > mv_sg = pp->sg_tbl;
    > > ata_for_each_sg(sg, qc) {
    > > @@ -1159,13 +1160,15 @@ static void mv_fill_sg(struct

    > ata_queued_cmd *qc)
    > > sg_len -= len;
    > > addr += len;
    > >
    > > - if (!sg_len && ata_sg_is_last(sg, qc))
    > > + if (!sg_len && ata_sg_is_last(sg, qc)) {

    >
    >
    > I'm not sure, but shouldn't that be || rather than &&?


    sg_len is zero at the end of each scatterlist entry, so we need to test
    the additional ata_sg_is_last() condition to determine if we are really
    at the end of the PRD table.

    Jeff



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

  12. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Jens Axboe wrote:
    >> index 4df8311..b858183 100644
    >> --- a/drivers/ata/sata_mv.c
    >> +++ b/drivers/ata/sata_mv.c
    >> @@ -1139,6 +1139,7 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    >> struct mv_port_priv *pp = qc->ap->private_data;
    >> struct scatterlist *sg;
    >> struct mv_sg *mv_sg;
    >> + int end_marked = 0;
    >> mv_sg = pp->sg_tbl;
    >> ata_for_each_sg(sg, qc) {
    >> @@ -1159,13 +1160,15 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    >> sg_len -= len;
    >> addr += len;
    >> - if (!sg_len && ata_sg_is_last(sg, qc))
    >> + if (!sg_len && ata_sg_is_last(sg, qc)) {
    >> mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    >> + end_marked++;
    >> + }
    >> mv_sg++;
    >> }
    >> -
    >> }
    >> + BUG_ON(end_marked != 1);

    >
    >
    > Your BUG_ON() does indeed trip, here.
    >
    > Its surprising that other folks don't explode, considering that
    > mv_fill_sg() intentionally mirrors the logic in ata_fill_sg().


    The sata_mv construct looks a bit odd. Does this work? That last
    end_mv_sg test should always be true, just being paranoid...

    diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    index 4df8311..5397eea 100644
    --- a/drivers/ata/sata_mv.c
    +++ b/drivers/ata/sata_mv.c
    @@ -1138,8 +1138,9 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    {
    struct mv_port_priv *pp = qc->ap->private_data;
    struct scatterlist *sg;
    - struct mv_sg *mv_sg;
    + struct mv_sg *mv_sg, *end_mv_sg;

    + end_mv_sg = NULL;
    mv_sg = pp->sg_tbl;
    ata_for_each_sg(sg, qc) {
    dma_addr_t addr = sg_dma_address(sg);
    @@ -1158,14 +1159,12 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)

    sg_len -= len;
    addr += len;
    -
    - if (!sg_len && ata_sg_is_last(sg, qc))
    - mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    -
    + end_mv_sg = mv_sg;
    mv_sg++;
    }
    -
    }
    + if (end_mv_sg)
    + end_mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    }

    static inline void mv_crqb_pack_cmd(__le16 *cmdw, u8 data, u8 addr, unsigned last)

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

  13. Re: [bug] ata subsystem related crash with latest -git

    Jens Axboe wrote:
    > The sata_mv construct looks a bit odd. Does this work? That last


    The sata_mv construct worked just fine before sg chaining


    > end_mv_sg test should always be true, just being paranoid...
    >
    > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    > index 4df8311..5397eea 100644
    > --- a/drivers/ata/sata_mv.c
    > +++ b/drivers/ata/sata_mv.c
    > @@ -1138,8 +1138,9 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    > {
    > struct mv_port_priv *pp = qc->ap->private_data;
    > struct scatterlist *sg;
    > - struct mv_sg *mv_sg;
    > + struct mv_sg *mv_sg, *end_mv_sg;
    >
    > + end_mv_sg = NULL;
    > mv_sg = pp->sg_tbl;
    > ata_for_each_sg(sg, qc) {
    > dma_addr_t addr = sg_dma_address(sg);
    > @@ -1158,14 +1159,12 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    >
    > sg_len -= len;
    > addr += len;
    > -
    > - if (!sg_len && ata_sg_is_last(sg, qc))
    > - mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    > -
    > + end_mv_sg = mv_sg;
    > mv_sg++;
    > }
    > -
    > }
    > + if (end_mv_sg)
    > + end_mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    > }
    >


    I'm testing a similar patch based on ata_fill_sg()'s method, which
    basically does something similar to what you've done here (see
    attached). I had noticed that ata_fill_sg() did not call ata_sg_is_last().

    If this fixes the problem, I think the best solution would be to delete
    ata_sg_is_last(). In the few users that exist, we should be able to
    eliminate the test programmatically as you and ata_fill_sg() have done
    -- thereby eliminating a branch per loop in a hotpath.

    Off to test the attached... if that doesn't work I'll try your version,
    though there shouldn't be much difference.

    Jeff



    diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    index 4df8311..42b5a9e 100644
    --- a/drivers/ata/sata_mv.c
    +++ b/drivers/ata/sata_mv.c
    @@ -421,7 +421,6 @@ static void mv_error_handler(struct ata_port *ap);
    static void mv_post_int_cmd(struct ata_queued_cmd *qc);
    static void mv_eh_freeze(struct ata_port *ap);
    static void mv_eh_thaw(struct ata_port *ap);
    -static int mv_slave_config(struct scsi_device *sdev);
    static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);

    static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
    @@ -459,7 +458,7 @@ static struct scsi_host_template mv5_sht = {
    .use_clustering = 1,
    .proc_name = DRV_NAME,
    .dma_boundary = MV_DMA_BOUNDARY,
    - .slave_configure = mv_slave_config,
    + .slave_configure = ata_scsi_slave_config,
    .slave_destroy = ata_scsi_slave_destroy,
    .bios_param = ata_std_bios_param,
    };
    @@ -477,7 +476,7 @@ static struct scsi_host_template mv6_sht = {
    .use_clustering = 1,
    .proc_name = DRV_NAME,
    .dma_boundary = MV_DMA_BOUNDARY,
    - .slave_configure = mv_slave_config,
    + .slave_configure = ata_scsi_slave_config,
    .slave_destroy = ata_scsi_slave_destroy,
    .bios_param = ata_std_bios_param,
    };
    @@ -756,17 +755,6 @@ static void mv_irq_clear(struct ata_port *ap)
    {
    }

    -static int mv_slave_config(struct scsi_device *sdev)
    -{
    - int rc = ata_scsi_slave_config(sdev);
    - if (rc)
    - return rc;
    -
    - blk_queue_max_phys_segments(sdev->request_queue, MV_MAX_SG_CT / 2);
    -
    - return 0; /* scsi layer doesn't check return value, sigh */
    -}
    -
    static void mv_set_edma_ptrs(void __iomem *port_mmio,
    struct mv_host_priv *hpriv,
    struct mv_port_priv *pp)
    @@ -1138,34 +1126,35 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    {
    struct mv_port_priv *pp = qc->ap->private_data;
    struct scatterlist *sg;
    - struct mv_sg *mv_sg;
    + struct mv_sg *mv_sg = pp->sg_tbl;
    + unsigned int idx = 0;

    - mv_sg = pp->sg_tbl;
    ata_for_each_sg(sg, qc) {
    - dma_addr_t addr = sg_dma_address(sg);
    - u32 sg_len = sg_dma_len(sg);
    + u64 addr;
    + u32 offset, sg_len, len;
    +
    + addr = sg_dma_address(sg);
    + sg_len = sg_dma_len(sg);

    while (sg_len) {
    - u32 offset = addr & 0xffff;
    - u32 len = sg_len;
    + offset = addr & 0xffff;
    + len = sg_len;

    if ((offset + sg_len > 0x10000))
    len = 0x10000 - offset;

    - mv_sg->addr = cpu_to_le32(addr & 0xffffffff);
    - mv_sg->addr_hi = cpu_to_le32((addr >> 16) >> 16);
    - mv_sg->flags_size = cpu_to_le32(len & 0xffff);
    + mv_sg[idx].addr = cpu_to_le32(addr & 0xffffffff);
    + mv_sg[idx].addr_hi = cpu_to_le32(addr >> 32);
    + mv_sg[idx].flags_size = cpu_to_le32(len & 0xffff);

    + idx++;
    sg_len -= len;
    addr += len;
    -
    - if (!sg_len && ata_sg_is_last(sg, qc))
    - mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    -
    - mv_sg++;
    }
    -
    }
    +
    + if (idx)
    + mv_sg[idx - 1].flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    }

    static inline void mv_crqb_pack_cmd(__le16 *cmdw, u8 data, u8 addr, unsigned last)


  14. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Jens Axboe wrote:
    >> The sata_mv construct looks a bit odd. Does this work? That last

    >
    > The sata_mv construct worked just fine before sg chaining


    Yes I know, but I'm trying to works towards getting rid of sg_last() and
    ata_sg_is_last() anyway :-)

    >> end_mv_sg test should always be true, just being paranoid...
    >> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    >> index 4df8311..5397eea 100644
    >> --- a/drivers/ata/sata_mv.c
    >> +++ b/drivers/ata/sata_mv.c
    >> @@ -1138,8 +1138,9 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    >> {
    >> struct mv_port_priv *pp = qc->ap->private_data;
    >> struct scatterlist *sg;
    >> - struct mv_sg *mv_sg;
    >> + struct mv_sg *mv_sg, *end_mv_sg;
    >> + end_mv_sg = NULL;
    >> mv_sg = pp->sg_tbl;
    >> ata_for_each_sg(sg, qc) {
    >> dma_addr_t addr = sg_dma_address(sg);
    >> @@ -1158,14 +1159,12 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    >> sg_len -= len;
    >> addr += len;
    >> -
    >> - if (!sg_len && ata_sg_is_last(sg, qc))
    >> - mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    >> -
    >> + end_mv_sg = mv_sg;
    >> mv_sg++;
    >> }
    >> -
    >> }
    >> + if (end_mv_sg)
    >> + end_mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    >> }
    >>

    >
    > I'm testing a similar patch based on ata_fill_sg()'s method, which
    > basically does something similar to what you've done here (see attached).
    > I had noticed that ata_fill_sg() did not call ata_sg_is_last().
    >
    > If this fixes the problem, I think the best solution would be to delete
    > ata_sg_is_last(). In the few users that exist, we should be able to
    > eliminate the test programmatically as you and ata_fill_sg() have done --
    > thereby eliminating a branch per loop in a hotpath.
    >
    > Off to test the attached... if that doesn't work I'll try your version,
    > though there shouldn't be much difference.


    That should work as well. WRT ata_sg_is_last(), if we go ahead with my
    recent sg chaining updates, we can keep the test as it would be a single
    conditional and not require any looping.

    Let me know when you have tested this!

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

  15. Re: [bug] ata subsystem related crash with latest -git

    Jens Axboe wrote:
    > That should work as well. WRT ata_sg_is_last(), if we go ahead with my
    > recent sg chaining updates, we can keep the test as it would be a single
    > conditional and not require any looping.
    >
    > Let me know when you have tested this!


    The patch I attached to the last email got both sata_mv test boxes
    working reliably (so far).

    I worked up a patch that kills ata_sg_is_last() (plus the
    max_phys_segments sata_mv fix), see attached. I'm thinking this is what
    I like to see in upstream.

    Of course, this doesn't explain why ata_sg_is_last() was broken, but
    since it's working _and_ slightly more efficient, I don't really care

    Jeff



    diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
    index 8d1b03d..199f7e1 100644
    --- a/drivers/ata/pdc_adma.c
    +++ b/drivers/ata/pdc_adma.c
    @@ -318,7 +318,7 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)
    struct scatterlist *sg;
    struct ata_port *ap = qc->ap;
    struct adma_port_priv *pp = ap->private_data;
    - u8 *buf = pp->pkt;
    + u8 *buf = pp->pkt, *last_buf = NULL;
    int i = (2 + buf[3]) * 8;
    u8 pFLAGS = pORD | ((qc->tf.flags & ATA_TFLAG_WRITE) ? pDIRO : 0);

    @@ -334,8 +334,7 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)
    *(__le32 *)(buf + i) = cpu_to_le32(len);
    i += 4;

    - if (ata_sg_is_last(sg, qc))
    - pFLAGS |= pEND;
    + last_buf = &buf[i];
    buf[i++] = pFLAGS;
    buf[i++] = qc->dev->dma_mode & 0xf;
    buf[i++] = 0; /* pPKLW */
    @@ -348,6 +347,10 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)
    VPRINTK("PRD[%u] = (0x%lX, 0x%X)\n", i/4,
    (unsigned long)addr, len);
    }
    +
    + if (likely(last_buf))
    + *last_buf |= pEND;
    +
    return i;
    }

    diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
    index 4df8311..7f1b13e 100644
    --- a/drivers/ata/sata_mv.c
    +++ b/drivers/ata/sata_mv.c
    @@ -421,7 +421,6 @@ static void mv_error_handler(struct ata_port *ap);
    static void mv_post_int_cmd(struct ata_queued_cmd *qc);
    static void mv_eh_freeze(struct ata_port *ap);
    static void mv_eh_thaw(struct ata_port *ap);
    -static int mv_slave_config(struct scsi_device *sdev);
    static int mv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent);

    static void mv5_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
    @@ -459,7 +458,7 @@ static struct scsi_host_template mv5_sht = {
    .use_clustering = 1,
    .proc_name = DRV_NAME,
    .dma_boundary = MV_DMA_BOUNDARY,
    - .slave_configure = mv_slave_config,
    + .slave_configure = ata_scsi_slave_config,
    .slave_destroy = ata_scsi_slave_destroy,
    .bios_param = ata_std_bios_param,
    };
    @@ -477,7 +476,7 @@ static struct scsi_host_template mv6_sht = {
    .use_clustering = 1,
    .proc_name = DRV_NAME,
    .dma_boundary = MV_DMA_BOUNDARY,
    - .slave_configure = mv_slave_config,
    + .slave_configure = ata_scsi_slave_config,
    .slave_destroy = ata_scsi_slave_destroy,
    .bios_param = ata_std_bios_param,
    };
    @@ -756,17 +755,6 @@ static void mv_irq_clear(struct ata_port *ap)
    {
    }

    -static int mv_slave_config(struct scsi_device *sdev)
    -{
    - int rc = ata_scsi_slave_config(sdev);
    - if (rc)
    - return rc;
    -
    - blk_queue_max_phys_segments(sdev->request_queue, MV_MAX_SG_CT / 2);
    -
    - return 0; /* scsi layer doesn't check return value, sigh */
    -}
    -
    static void mv_set_edma_ptrs(void __iomem *port_mmio,
    struct mv_host_priv *hpriv,
    struct mv_port_priv *pp)
    @@ -1138,7 +1126,7 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    {
    struct mv_port_priv *pp = qc->ap->private_data;
    struct scatterlist *sg;
    - struct mv_sg *mv_sg;
    + struct mv_sg *mv_sg, *last_sg = NULL;

    mv_sg = pp->sg_tbl;
    ata_for_each_sg(sg, qc) {
    @@ -1159,13 +1147,13 @@ static void mv_fill_sg(struct ata_queued_cmd *qc)
    sg_len -= len;
    addr += len;

    - if (!sg_len && ata_sg_is_last(sg, qc))
    - mv_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    -
    + last_sg = mv_sg;
    mv_sg++;
    }
    -
    }
    +
    + if (likely(last_sg))
    + last_sg->flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
    }

    static inline void mv_crqb_pack_cmd(__le16 *cmdw, u8 data, u8 addr, unsigned last)
    diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
    index b061927..26ebffc 100644
    --- a/drivers/ata/sata_sil24.c
    +++ b/drivers/ata/sata_sil24.c
    @@ -796,16 +796,19 @@ static inline void sil24_fill_sg(struct ata_queued_cmd *qc,
    struct sil24_sge *sge)
    {
    struct scatterlist *sg;
    + struct sil24_sge *last_sge = NULL;

    ata_for_each_sg(sg, qc) {
    sge->addr = cpu_to_le64(sg_dma_address(sg));
    sge->cnt = cpu_to_le32(sg_dma_len(sg));
    - if (ata_sg_is_last(sg, qc))
    - sge->flags = cpu_to_le32(SGE_TRM);
    - else
    - sge->flags = 0;
    + sge->flags = 0;
    +
    + last_sge = sge;
    sge++;
    }
    +
    + if (likely(last_sge))
    + last_sge->flags = cpu_to_le32(SGE_TRM);
    }

    static int sil24_qc_defer(struct ata_queued_cmd *qc)
    diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
    index b41dfb5..c316a0b 100644
    --- a/drivers/scsi/ipr.c
    +++ b/drivers/scsi/ipr.c
    @@ -5134,6 +5134,7 @@ static void ipr_build_ata_ioadl(struct ipr_cmnd *ipr_cmd,
    u32 ioadl_flags = 0;
    struct ipr_ioarcb *ioarcb = &ipr_cmd->ioarcb;
    struct ipr_ioadl_desc *ioadl = ipr_cmd->ioadl;
    + struct ipr_ioadl_desc *last_ioadl = NULL;
    int len = qc->nbytes + qc->pad_len;
    struct scatterlist *sg;

    @@ -5156,11 +5157,13 @@ static void ipr_build_ata_ioadl(struct ipr_cmnd *ipr_cmd,
    ata_for_each_sg(sg, qc) {
    ioadl->flags_and_data_len = cpu_to_be32(ioadl_flags | sg_dma_len(sg));
    ioadl->address = cpu_to_be32(sg_dma_address(sg));
    - if (ata_sg_is_last(sg, qc))
    - ioadl->flags_and_data_len |= cpu_to_be32(IPR_IOADL_FLAGS_LAST);
    - else
    - ioadl++;
    +
    + last_ioadl = ioadl;
    + ioadl++;
    }
    +
    + if (likely(last_ioadl))
    + last_ioadl->flags_and_data_len |= cpu_to_be32(IPR_IOADL_FLAGS_LAST);
    }

    /**
    diff --git a/include/linux/libata.h b/include/linux/libata.h
    index 377e6d4..bc3b6fc 100644
    --- a/include/linux/libata.h
    +++ b/include/linux/libata.h
    @@ -1037,18 +1037,6 @@ extern void ata_port_pbar_desc(struct ata_port *ap, int bar, ssize_t offset,
    /*
    * qc helpers
    */
    -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;
    -}
    -
    static inline struct scatterlist *
    ata_qc_first_sg(struct ata_queued_cmd *qc)
    {


  16. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jens Axboe wrote:
    > On Thu, Oct 18 2007, Ingo Molnar wrote:
    > >
    > > * Jens Axboe wrote:
    > >
    > > > > Of course, this doesn't explain why ata_sg_is_last() was broken, but
    > > > > since it's working _and_ slightly more efficient, I don't really
    > > > > care
    > > >
    > > > Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
    > > > perfectly fine with me.

    > >
    > > it would still be nice to figure out exactly why it was broken.

    >
    > It would always be nice. For this case I don't think it's very
    > interesting, if we pursue the improved sg iteration setup.


    BTW, I think it's pretty clear that ata_sg_is_last() is broken. It's
    likely a one-off in the n_iter test.

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

  17. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Ingo Molnar wrote:
    >
    > * Jens Axboe wrote:
    >
    > > > Of course, this doesn't explain why ata_sg_is_last() was broken, but
    > > > since it's working _and_ slightly more efficient, I don't really
    > > > care

    > >
    > > Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
    > > perfectly fine with me.

    >
    > it would still be nice to figure out exactly why it was broken.


    It would always be nice. For this case I don't think it's very
    interesting, if we pursue the improved sg iteration setup.

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

  18. Re: [bug] ata subsystem related crash with latest -git

    On Thu, Oct 18 2007, Jeff Garzik wrote:
    > Jens Axboe wrote:
    >> That should work as well. WRT ata_sg_is_last(), if we go ahead with my
    >> recent sg chaining updates, we can keep the test as it would be a single
    >> conditional and not require any looping.
    >> Let me know when you have tested this!

    >
    > The patch I attached to the last email got both sata_mv test boxes working
    > reliably (so far).
    >
    > I worked up a patch that kills ata_sg_is_last() (plus the max_phys_segments
    > sata_mv fix), see attached. I'm thinking this is what I like to see in
    > upstream.


    Great!

    > Of course, this doesn't explain why ata_sg_is_last() was broken, but since
    > it's working _and_ slightly more efficient, I don't really care


    Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
    perfectly fine with me.

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

  19. Re: [bug] ata subsystem related crash with latest -git


    * Jens Axboe wrote:

    > > Of course, this doesn't explain why ata_sg_is_last() was broken, but
    > > since it's working _and_ slightly more efficient, I don't really
    > > care

    >
    > Tomo and I agreed to kill sg_last() a few days ago anyways, so this is
    > perfectly fine with me.


    it would still be nice to figure out exactly why it was broken.

    Ingo
    -
    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/

  20. Re: [bug] ata subsystem related crash with latest -git


    * Jens Axboe wrote:

    > > It would always be nice. For this case I don't think it's very
    > > interesting, if we pursue the improved sg iteration setup.

    >
    > BTW, I think it's pretty clear that ata_sg_is_last() is broken. It's
    > likely a one-off in the n_iter test.


    fixing that would be a -stable candidate, as a potential data corruptor
    - or is it more benign?

    Ingo
    -
    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/

+ Reply to Thread
Page 5 of 8 FirstFirst ... 3 4 5 6 7 ... LastLast