Hello, On Wednesday, March 07, 2012 8:16 AM Hiroshi Doyu wrote:
> From: Hiroshi DOYU <[email protected]> > Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper > Date: Wed, 07 Mar 2012 08:37:06 +0200 (EET) > Message-ID: <[email protected]> > > > From: Hiroshi DOYU <[email protected]> > > Subject: Re: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper > > Date: Wed, 07 Mar 2012 08:09:52 +0200 (EET) > > Message-ID: <[email protected]> > > > > > From: Krishna Reddy <[email protected]> > > > Subject: RE: [PATCHv7 9/9] ARM: dma-mapping: add support for IOMMU mapper > > > Date: Tue, 6 Mar 2012 23:48:42 +0100 > > > Message-ID: > > > <[email protected]> > > > > > > > > > +struct dma_iommu_mapping * > > > > > > +arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, > > > > > > size_t size, > > > > > > + int order) > > > > > > +{ > > > > > > + unsigned int count = (size >> PAGE_SHIFT) - order; > > > > > > + unsigned int bitmap_size = BITS_TO_LONGS(count) * > > > > > > sizeof(long); > > > > > > > > The count calculation doesn't seem correct. "order" is log2 number and > > > > size >> PAGE_SHIFT is number of pages. > > > > > > > > If size is passed as 64*4096(256KB) and order is 6(allocation > > > > granularity is 2^6 > pages=256KB), > > > > just 1 bit is enough to manage allocations. So it should be 4 bytes > > > > or one long. > > > > > > Good catch! > > > > > > > But the calculation gives count = 64 - 6 = 58 and > > > > Bitmap_size gets set to (58/(4*8)) * 4 = 8 bytes, which is incorrect. > > > > > > "order" isn't the order of size passed, which is minimal *page* > > > allocation order which client decides whatever, just in case. > > > > > > > It should be as follows. > > > > unsigned int count = 1 << get_order(size) - order; > > > > To be precise, as below? > > > > unsigned int count = 1 << (get_order(size) - order); > > This could be: > > From fd40740ef4bc4a3924fe1188ea6dd785be0fe859 Mon Sep 17 00:00:00 2001 > From: Hiroshi DOYU <[email protected]> > Date: Wed, 7 Mar 2012 08:14:38 +0200 > Subject: [PATCH 1/1] dma-mapping: Fix count calculation of iova space > > Fix count calculation of iova space. > Pointed by Krishna Reddy <[email protected]> > > Signed-off-by: Hiroshi DOYU <[email protected]> > --- > arch/arm/mm/dma-mapping.c | 11 +++++++++-- > 1 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 6c2f104..56f0af5 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -1483,11 +1483,18 @@ struct dma_iommu_mapping * > arm_iommu_create_mapping(struct bus_type *bus, dma_addr_t base, size_t size, > int order) > { > - unsigned int count = (size >> PAGE_SHIFT) - order; > - unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); > + unsigned int n, count; > + unsigned int bitmap_size; > struct dma_iommu_mapping *mapping; > int err = -ENOMEM; > > + n = get_order(size); > + if (n < order) > + return ERR_PTR(-EINVAL); > + > + count = 1 << (n - order); > + bitmap_size = BITS_TO_LONGS(count) * sizeof(long); > + > mapping = kzalloc(sizeof(struct dma_iommu_mapping), GFP_KERNEL); > if (!mapping) > goto err; Thanks again for finding another bug. I thought that I've checked that code more than twice, but it looks that I've missed something again. IMHO the size of virtual memory area doesn't need to be aligned to the power of two, so I will simplify it to the following code: unsigned int count = size >> (PAGE_SHIFT + order); unsigned int bitmap_size = BITS_TO_LONGS(count) * sizeof(long); if (!count) return ERR_PTR(-EINVAL); ... Best regards -- Marek Szyprowski Samsung Poland R&D Center _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
