On Mon, Oct 22 2007, Benny Halevy wrote: > It looks like it could be nice to define and use a helper for > page_address(sg_page(sg)) (although 11 call sites could use it > after this patch) > > #define sg_pgaddr(sg) page_address(sg_page(sg)) > > Note that mips sg_{un,}map_sg checked for page_address(sg->page) != 0 > before calling __dma_sync(addr + sg->offset, sg->length, direction) > and you changed it to addr = (unsigned long) sg_virt(sg) which > takes sg->offset into account. That said I'm not sure if the original > code was correct for the (page_address(sg->page) == 0 && sg->offset != 0) > case...
I initially thought that may have been a bug, but most of the other arch iommu code handle it in the same way. I don't think it's a bug, since they want to clear full page ranges anyway. I should not have changed this code to take the offset into account - I don't think it's an issue, but it should have been left as-is. Will do that. > > diff --git a/arch/x86/kernel/pci-calgary_64.c > > b/arch/x86/kernel/pci-calgary_64.c > > index 5098f58..1a20fe3 100644 > > --- a/arch/x86/kernel/pci-calgary_64.c > > +++ b/arch/x86/kernel/pci-calgary_64.c > > @@ -411,8 +411,10 @@ static int calgary_nontranslate_map_sg(struct device* > > dev, > > int i; > > > > for_each_sg(sg, s, nelems, i) { > > - BUG_ON(!s->page); > > - s->dma_address = virt_to_bus(page_address(s->page) +s->offset); > > + struct page *p = sg_page(s); > > + > > + BUG_ON(!p); > > why not just BUG_ON(!sg_page(s))? > > > + s->dma_address = virt_to_bus(sg_virt(s)); > > s->dma_length = s->length; > > } > > return nelems; I think because of a two stage conversion, the page variable addition predates the sg_virt() helper. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/