Re: [PATCH update] ieee1394: sbp2: enforce s/g segment size limit

[FUJITA Tomonori]

On Wed, 13 Aug 2008 12:19:59 +0200 (CEST)
Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx> wrote:

> On 13 Aug, FUJITA Tomonori wrote:
> > On Tue, 12 Aug 2008 10:04:14 -0700
> > "Grant Grundler" <grundler@xxxxxxxxxx> wrote:
> > 
> >> On Sat, Aug 9, 2008 at 11:20 AM, Stefan Richter
> >> <stefanr@xxxxxxxxxxxxxxxxx> wrote:
> >> >  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?
> 
> Well, I keep the original scsi_sg_count to call dma_unmap_sg with it
> later.

You need to keep it? You can access to it via scsi_sg_count when
calling dma_unmap_sg? Seems that firewire code does.


> According to Corbet/ Rubini/ Kroah-Hartman: LDD3, dma_unmap_sg
> is to be called with the number of s/g elements before DMA-mapping.

Are you talking about?

=
To unmap a scatterlist, just call:

	pci_unmap_sg(pdev, sglist, nents, direction);

Again, make sure DMA activity has already finished.

PLEASE NOTE:  The 'nents' argument to the pci_unmap_sg call must be
              the _same_ one you passed into the pci_map_sg call,
	      it should _NOT_ be the 'count' value _returned_ from the
              pci_map_sg call.


This is from Documentation/DMA-mapping.txt


> From a quick look at some dma_unmap_sg implementations, this seems still
> to be the case; or it doesn't actually matter.

I think that all the IOMMUs are fine even if you call dma_unmap_sg
with a 'nents' value that dma_map_sg returned. But, well, it's against
the rule.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/