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@xxxxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
Some random comments below - nothing that calls for any change of this patch.
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> ---
> 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
