Re: [PATCH] ipr: don't doublefree pages from scatterlist

[Hugh Dickins <hugh@veritas.com>]

On Sun, 5 Feb 2006, Brian King wrote:
> 
> No. I can't find any code in any architecture that modifies either the length,
> page, or offset fields in the *_map_sg routines.

>From looking at the source, the architectures I found to be doing
scatterlist coalescing in some cases were alpha, ia64, parisc (some
code under drivers), powerpc, sparc64 and x86_64.

I agree with you that it would be possible for them to do the coalescing
by just adjusting dma_address and dma_length (though it's architecture-
dependent whether there are such fields at all), not interfering with
the input page and length; and maybe some of them do proceed that way.
I didn't find the coalescing code in any of them very easy to follow.

So please examine arch/x86_64/kernel/pci_gart.c gart_map_sg (and
dma_map_cont which it calls): x86_64 was the architecture on which
the problem was really found with drivers/scsi/st.c, and avoided
by that boot option iommu=nomerge.

Lines like "*sout = *s;" and "*sout = sg[start];" are structure-
copying whole scallerlist entries from one position in the list
to another, without explicit mention of the page and length fields.

> I'm still failing to see an actual bug in the ipr driver. Unless there is a
> good reason for pushing additional complexity on every caller of pci_map_sg
> to do additional bookkeeping, such as an architecture that is actually
> modifying
> fields in the scatterlist other than the dma_* fields, then I think it makes
> more sense to clarify the API to say that pci_map_sg will not modify these
> fields.

The API is commendably clear that they may be modified.  Like you I'd
prefer it to be otherwise, and instead guarantee that they won't be:
that would prevent this kind of surprise.  But it's not how it is;
and looking at the various pieces of coalescing code, I quickly
abandoned my first inclination, to adjust them to make it so.

Hugh