From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hugh Dickins Subject: Re: [PATCH] ipr: don't doublefree pages from scatterlist Date: Mon, 6 Feb 2006 09:32:54 +0000 (GMT) Message-ID: References: <20060104172727.GA320@tau.solarneutrino.net> <20060105201249.GB1795@tau.solarneutrino.net> <20060109033149.GC283@tau.solarneutrino.net> <20060109185350.GG283@tau.solarneutrino.net> <20060118001252.GB821@tau.solarneutrino.net> <43E3D3EC.3040006@us.ibm.com> <43E66FB6.6070303@us.ibm.com> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: Received: from silver.veritas.com ([143.127.12.111]:54035 "EHLO silver.veritas.com") by vger.kernel.org with ESMTP id S1750843AbWBFJcY (ORCPT ); Mon, 6 Feb 2006 04:32:24 -0500 Received: from [172.21.56.1]([172.21.56.1]) (3553 bytes) by megami.veritas.com via sendmail with P:esmtp/R:smart_host/T:smtp (sender: ) id for ; Mon, 6 Feb 2006 01:32:23 -0800 (PST) (Smail-3.2.0.101 1997-Dec-17 #15 built 2001-Aug-30) In-Reply-To: <43E66FB6.6070303@us.ibm.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Brian King Cc: James Bottomley , Andrew Morton , linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org 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