Re: [PATCH 08/10] [SG] Update arch/ to use sg helpers

[Jens Axboe <jens.axboe@oracle.com>]

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