lkml.org 
[lkml]   [2008]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] ieee1394: sbp2: enforce s/g segment size limit
    From
    On Tue, 12 Aug 2008 10:04:14 -0700
    "Grant Grundler" <grundler@google.com> wrote:

    > On Sat, Aug 9, 2008 at 11:20 AM, Stefan Richter
    > <stefanr@s5r6.in-berlin.de> wrote:
    > > 1. We don't need to round the SBP-2 segment size limit down to a
    > > multiple of 4 kB (0xffff -> 0xf000). It is only necessary to
    > > ensure quadlet alignment (0xffff -> 0xfffc).
    > >
    > > 2. Use dma_set_max_seg_size() to tell the DMA mapping infrastructure
    > > and the block IO layer about the restriction. This way we can
    > > remove the size checks and segment splitting in the queuecommand
    > > path.
    > >
    > > This assumes that no other code in the ieee1394 stack uses
    > > dma_map_sg() with conflicting requirements. It furthermore assumes
    > > that the controller device's platform actually allows us to set the
    > > segment size to our liking. Assert the latter with a BUG_ON().
    > >
    > > 3. Also use blk_queue_max_segment_size() to tell the block IO layer
    > > about it. It cannot know it because our scsi_add_host() does not
    > > point to the FireWire controller's device.
    > >
    > > We can also uniformly use dma_map_sg() for the single segment case just
    > > like for the multi segment case, to further simplify the code.
    > >
    > > Also clean up how the page table is converted to big endian.
    > >
    > > Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
    > > ---
    > >
    > > Applicable after
    > > [PATCH update] ieee1394: sbp2: stricter dma_sync
    > > [PATCH] ieee1394: sbp2: check for DMA mapping failures
    > > from a few minutes ago.
    > >
    > > drivers/ieee1394/sbp2.c | 106 +++++++++++++---------------------------
    > > drivers/ieee1394/sbp2.h | 37 +++++--------
    > > 2 files changed, 52 insertions(+), 91 deletions(-)
    > ...
    > > + for_each_sg(sg, sg, sg_count, i) {
    > > + len = sg_dma_len(sg);
    > > + if (len == 0)
    > > + continue;
    > >
    > > - orb->misc |= ORB_SET_DATA_SIZE(sg_count);
    > > + pt[j].high = cpu_to_be32(len << 16);
    > > + pt[j].low = cpu_to_be32(sg_dma_address(sg));
    > > + j++;
    > > + }
    >
    > While this code will probably work correctly, I believe if the
    > sg_dma_len() returns 0, one has walked off the end of the
    > "bus address" list.
    >
    > pci_map_sg() returns how many DMA addr/len pairs are used
    > and that count could (should?) be used when pulling the
    > dma_len/addr pairs out of the sg list.

    Yeah, from a quick look, seems that this patch wrongly handles
    sg_count.

    This patch sets scsi_sg_count(sc) to sg_count, right? for_each_sg is
    expected to be used with a return value of pci_map_sg.

    Then this patch can simply do something like:

    for_each_sg(sg, sg, sg_count, i) {
    pt[i].high = cpu_to_be32(sg_dma_len(sg) << 16);
    pt[i].low = cpu_to_be32(sg_dma_address(sg));
    }


    \
     
     \ /
      Last update: 2008-08-13 01:47    [W:0.024 / U:0.936 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site