| Message ID | 20181216095755.10503-3-hch@lst.de |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] sparc/io-unit: fix ->map_sg return value | expand |
Hi Christoph. On Sun, Dec 16, 2018 at 10:57:55AM +0100, Christoph Hellwig wrote: > Just decrementing the sz value will lead to an incorrect return value. > Instead of just introducing a local variable switch to the standard > for_each_sg helper and standard naming of the arguments. > > Fixes: ce65d36f3e ("sparc: remove the sparc32_dma_ops indirection") > Reported-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Christoph Hellwig <hch@lst.de> Some random comments below - nothing that calls for any change of this patch. Acked-by: Sam Ravnborg <sam@ravnborg.org> > --- > arch/sparc/mm/iommu.c | 39 ++++++++++++++++++--------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) > > diff --git a/arch/sparc/mm/iommu.c b/arch/sparc/mm/iommu.c > index 3599485717e7..25ab0fa042ef 100644 > --- a/arch/sparc/mm/iommu.c > +++ b/arch/sparc/mm/iommu.c > @@ -241,32 +241,31 @@ static dma_addr_t sbus_iommu_map_page_pflush(struct device *dev, > return __sbus_iommu_map_page(dev, page, offset, len); > } > > -static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist *sg, > - int sz, enum dma_data_direction dir, unsigned long attrs) > +static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist *sgl, > + int nents, enum dma_data_direction dir, unsigned long attrs) > { > - int n; > + struct scatterlist *sg; > + int j, n; Nit - in the other use of for_each_sg() you used "i" as the variable named. It confused me a little to see "j". sg-lenght and sg->offsett are both unsigned int. So n should looking at this piece of code be unsigned int. But then iommu_get_one() takes an int as argument. So the real issue seems to be that iommu_get_one() should have npages be an unsigned int. And your code is fine. If you had named n for npages it would have been a little more readable. > > flush_page_for_dma(0); > - while (sz != 0) { > - --sz; > + > + for_each_sg(sgl, sg, nents, j) { > n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; > sg->dma_address = iommu_get_one(dev, sg_page(sg), n) + sg->offset; > sg->dma_length = sg->length; > - sg = sg_next(sg); > } > > - return sz; > + return nents; > } > > -static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sg, > - int sz, enum dma_data_direction dir, unsigned long attrs) > +static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sgl, > + int nents, enum dma_data_direction dir, unsigned long attrs) > { > unsigned long page, oldpage = 0; > - int n, i; > - > - while(sz != 0) { > - --sz; > + struct scatterlist *sg; > + int i, j, n; > > + for_each_sg(sgl, sg, nents, j) { > n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; > > /* > @@ -286,10 +285,9 @@ static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sg, > > sg->dma_address = iommu_get_one(dev, sg_page(sg), n) + sg->offset; > sg->dma_length = sg->length; > - sg = sg_next(sg); > } > > - return sz; > + return nents; > } > > static void iommu_release_one(struct device *dev, u32 busa, int npages) > @@ -318,17 +316,16 @@ static void sbus_iommu_unmap_page(struct device *dev, dma_addr_t dma_addr, > iommu_release_one(dev, dma_addr & PAGE_MASK, npages); > } > > -static void sbus_iommu_unmap_sg(struct device *dev, struct scatterlist *sg, > - int sz, enum dma_data_direction dir, unsigned long attrs) > +static void sbus_iommu_unmap_sg(struct device *dev, struct scatterlist *sgl, > + int nents, enum dma_data_direction dir, unsigned long attrs) > { > - int n; > + struct scatterlist *sg; > + int j, n; > > - while(sz != 0) { > - --sz; > + for_each_sg(sgl, sg, nents, j) { > n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; > iommu_release_one(dev, sg->dma_address & PAGE_MASK, n); > sg->dma_address = 0x21212121; > - sg = sg_next(sg); > } > } > > -- > 2.19.2
On Wed, Dec 19, 2018 at 05:28:25PM +0100, Sam Ravnborg wrote: > > + struct scatterlist *sg; > > + int j, n; > Nit - in the other use of for_each_sg() you used "i" as the variable named. > It confused me a little to see "j". I think this was a copy and paste from the one function where i was already taken. I can remove it here. > sg-lenght and sg->offsett are both unsigned int. > So n should looking at this piece of code be unsigned int. > But then iommu_get_one() takes an int as argument. > So the real issue seems to be that iommu_get_one() should > have npages be an unsigned int. And your code is fine. > > If you had named n for npages it would have been a little more readable. Well, n isn't really new here but from the existing code. I see plenty of potential for the usual cleanups in that code..
On Wed, Dec 19, 2018 at 05:30:12PM +0100, Christoph Hellwig wrote: > On Wed, Dec 19, 2018 at 05:28:25PM +0100, Sam Ravnborg wrote: > > > + struct scatterlist *sg; > > > + int j, n; > > Nit - in the other use of for_each_sg() you used "i" as the variable named. > > It confused me a little to see "j". > > I think this was a copy and paste from the one function where i was > already taken. I can remove it here. > > > sg-lenght and sg->offsett are both unsigned int. > > So n should looking at this piece of code be unsigned int. > > But then iommu_get_one() takes an int as argument. > > So the real issue seems to be that iommu_get_one() should > > have npages be an unsigned int. And your code is fine. > > > > If you had named n for npages it would have been a little more readable. > > Well, n isn't really new here but from the existing code. I see > plenty of potential for the usual cleanups in that code.. Yep. So better leave it as it is. Then someone with a little more sparc love could do it one day. Sam
diff --git a/arch/sparc/mm/iommu.c b/arch/sparc/mm/iommu.c index 3599485717e7..25ab0fa042ef 100644 --- a/arch/sparc/mm/iommu.c +++ b/arch/sparc/mm/iommu.c @@ -241,32 +241,31 @@ static dma_addr_t sbus_iommu_map_page_pflush(struct device *dev, return __sbus_iommu_map_page(dev, page, offset, len); } -static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist *sg, - int sz, enum dma_data_direction dir, unsigned long attrs) +static int sbus_iommu_map_sg_gflush(struct device *dev, struct scatterlist *sgl, + int nents, enum dma_data_direction dir, unsigned long attrs) { - int n; + struct scatterlist *sg; + int j, n; flush_page_for_dma(0); - while (sz != 0) { - --sz; + + for_each_sg(sgl, sg, nents, j) { n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; sg->dma_address = iommu_get_one(dev, sg_page(sg), n) + sg->offset; sg->dma_length = sg->length; - sg = sg_next(sg); } - return sz; + return nents; } -static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sg, - int sz, enum dma_data_direction dir, unsigned long attrs) +static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sgl, + int nents, enum dma_data_direction dir, unsigned long attrs) { unsigned long page, oldpage = 0; - int n, i; - - while(sz != 0) { - --sz; + struct scatterlist *sg; + int i, j, n; + for_each_sg(sgl, sg, nents, j) { n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; /* @@ -286,10 +285,9 @@ static int sbus_iommu_map_sg_pflush(struct device *dev, struct scatterlist *sg, sg->dma_address = iommu_get_one(dev, sg_page(sg), n) + sg->offset; sg->dma_length = sg->length; - sg = sg_next(sg); } - return sz; + return nents; } static void iommu_release_one(struct device *dev, u32 busa, int npages) @@ -318,17 +316,16 @@ static void sbus_iommu_unmap_page(struct device *dev, dma_addr_t dma_addr, iommu_release_one(dev, dma_addr & PAGE_MASK, npages); } -static void sbus_iommu_unmap_sg(struct device *dev, struct scatterlist *sg, - int sz, enum dma_data_direction dir, unsigned long attrs) +static void sbus_iommu_unmap_sg(struct device *dev, struct scatterlist *sgl, + int nents, enum dma_data_direction dir, unsigned long attrs) { - int n; + struct scatterlist *sg; + int j, n; - while(sz != 0) { - --sz; + for_each_sg(sgl, sg, nents, j) { n = (sg->length + sg->offset + PAGE_SIZE-1) >> PAGE_SHIFT; iommu_release_one(dev, sg->dma_address & PAGE_MASK, n); sg->dma_address = 0x21212121; - sg = sg_next(sg); } }
Just decrementing the sz value will lead to an incorrect return value. Instead of just introducing a local variable switch to the standard for_each_sg helper and standard naming of the arguments. Fixes: ce65d36f3e ("sparc: remove the sparc32_dma_ops indirection") Reported-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/sparc/mm/iommu.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-)