[PATCH] CMA: Don't return a valid cma for non-cma dev

[PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

When system(one x86 soc) boot, we saw many normal dma allocation requests
goes to cma area. The call chain is
	dma_generic_alloc_coherent
	    dma_alloc_from_contiguous	-- arch/x86/kernel/pci-dma.c
	        cma_alloc(dev_get_cma_area(dev), count, align)

Current dev_get_cma_area() will return a valid "cma" anyway. Then all
these requests will be taken as valid cma request, and get pages from
cma area, which has 2 problems:
1. make the cma area fragmented
2. confuse the cma reservation, usually cma memory size is set according
   to the expectation of system scenario, these unexpected requests
   will affect the designed cma usage.

So this patch will enforce the judgement, and only return valid "cma"
for real cma user, thus make normal user like IO device driver not
abuse cma reserved region.

Signed-off-by: Feng Tang <feng.tang@intel.com>
---
 include/linux/dma-contiguous.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
index 569bbd0..d6ccc19 100644
--- a/include/linux/dma-contiguous.h
+++ b/include/linux/dma-contiguous.h
@@ -66,7 +66,8 @@ static inline struct cma *dev_get_cma_area(struct device *dev)
 {
 	if (dev && dev->cma_area)
 		return dev->cma_area;
-	return dma_contiguous_default_area;
+	else
+		return NULL;
 }
 
 static inline void dev_set_cma_area(struct device *dev, struct cma *cma)
-- 
1.7.9.5

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Michal Nazarewicz]

On Thu, Jul 30 2015, Feng Tang wrote:
> When system(one x86 soc) boot, we saw many normal dma allocation requests
> goes to cma area. The call chain is
> 	dma_generic_alloc_coherent
> 	    dma_alloc_from_contiguous	-- arch/x86/kernel/pci-dma.c
> 	        cma_alloc(dev_get_cma_area(dev), count, align)
>
> Current dev_get_cma_area() will return a valid "cma" anyway. Then all
> these requests will be taken as valid cma request, and get pages from
> cma area, which has 2 problems:
> 1. make the cma area fragmented
> 2. confuse the cma reservation, usually cma memory size is set according
>    to the expectation of system scenario, these unexpected requests
>    will affect the designed cma usage.
>
> So this patch will enforce the judgement, and only return valid "cma"
> for real cma user, thus make normal user like IO device driver not
> abuse cma reserved region.

Just don’t set dma_contiguous_default_area.  This patch defeats the
purpose of a *default* area.

> Signed-off-by: Feng Tang <feng.tang@intel.com>
> ---
>  include/linux/dma-contiguous.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/dma-contiguous.h b/include/linux/dma-contiguous.h
> index 569bbd0..d6ccc19 100644
> --- a/include/linux/dma-contiguous.h
> +++ b/include/linux/dma-contiguous.h
> @@ -66,7 +66,8 @@ static inline struct cma *dev_get_cma_area(struct device *dev)
>  {
>  	if (dev && dev->cma_area)
>  		return dev->cma_area;
> -	return dma_contiguous_default_area;
> +	else
> +		return NULL;
>  }
>  
>  static inline void dev_set_cma_area(struct device *dev, struct cma *cma)
> -- 
> 1.7.9.5
>

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Tang, Feng]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2034 bytes --]

Hi Michal Nazarewicz,

Thanks for the review.

On Thu, 2015-07-30 at 15:59 +0200, Michal Nazarewicz wrote:
> On Thu, Jul 30 2015, Feng Tang wrote:
> > When system(one x86 soc) boot, we saw many normal dma allocation requests
> > goes to cma area. The call chain is
> > 	dma_generic_alloc_coherent
> > 	    dma_alloc_from_contiguous	-- arch/x86/kernel/pci-dma.c
> > 	        cma_alloc(dev_get_cma_area(dev), count, align)
> >
> > Current dev_get_cma_area() will return a valid "cma" anyway. Then all
> > these requests will be taken as valid cma request, and get pages from
> > cma area, which has 2 problems:
> > 1. make the cma area fragmented
> > 2. confuse the cma reservation, usually cma memory size is set according
> >    to the expectation of system scenario, these unexpected requests
> >    will affect the designed cma usage.
> >
> > So this patch will enforce the judgement, and only return valid "cma"
> > for real cma user, thus make normal user like IO device driver not
> > abuse cma reserved region.
> 
> Just don’t set dma_contiguous_default_area.  This patch defeats the
> purpose of a *default* area.

Yes! This is exactly what I tried when I first saw this problem, but
this failed as there 2 places inside drivers/base/dma-contiguous.c
which set the dma_contiguous_default_area

1) 
void __init dma_contiguous_reserve(phys_addr_t limit)
{
    ....
    dma_contiguous_reserve_area(selected_size, selected_base,
                                            selected_limit,
                                   &dma_contiguous_default_area,
                                            fixed);
     ....
}

2) 
static int __init rmem_cma_setup(struct reserved_mem *rmem)
{
    ...
    if (of_get_flat_dt_prop(node, "linux,cma-default", NULL))
                dma_contiguous_set_default(cma);
    ...
}

Are you suggesting me to remove them?

Thanks,
Feng


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Michal Nazarewicz]

On Fri, Jul 31 2015, Feng Tang wrote:
> Hi Michal Nazarewicz,
>
> Thanks for the review.
>
> On Thu, 2015-07-30 at 15:59 +0200, Michal Nazarewicz wrote:
>> On Thu, Jul 30 2015, Feng Tang wrote:
>> > When system(one x86 soc) boot, we saw many normal dma allocation requests
>> > goes to cma area. The call chain is
>> > 	dma_generic_alloc_coherent
>> > 	    dma_alloc_from_contiguous	-- arch/x86/kernel/pci-dma.c
>> > 	        cma_alloc(dev_get_cma_area(dev), count, align)
>> >
>> > Current dev_get_cma_area() will return a valid "cma" anyway. Then all
>> > these requests will be taken as valid cma request, and get pages from
>> > cma area, which has 2 problems:
>> > 1. make the cma area fragmented
>> > 2. confuse the cma reservation, usually cma memory size is set according
>> >    to the expectation of system scenario, these unexpected requests
>> >    will affect the designed cma usage.
>> >
>> > So this patch will enforce the judgement, and only return valid "cma"
>> > for real cma user, thus make normal user like IO device driver not
>> > abuse cma reserved region.
>> 
>> Just don’t set dma_contiguous_default_area.  This patch defeats the
>> purpose of a *default* area.
>
> Yes! This is exactly what I tried when I first saw this problem, but
> this failed as there 2 places inside drivers/base/dma-contiguous.c
> which set the dma_contiguous_default_area
>
> 1) 
> void __init dma_contiguous_reserve(phys_addr_t limit)
> {
>     ....
>     dma_contiguous_reserve_area(selected_size, selected_base,
>                                             selected_limit,
>                                    &dma_contiguous_default_area,
>                                             fixed);
>      ....
> }
>
> 2) 
> static int __init rmem_cma_setup(struct reserved_mem *rmem)
> {
>     ...
>     if (of_get_flat_dt_prop(node, "linux,cma-default", NULL))
>                 dma_contiguous_set_default(cma);
>     ...
> }
>
> Are you suggesting me to remove them?

Set CONFIG_CMA_SIZE_MBYTES to zero (in Kconfig) or add ‘cma=0’ on
command line, and don’t use ‘linux,cma-default’ in device tree.  Then
you will end up with no default CMA area.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

On Fri, Jul 31, 2015 at 02:05:17PM +0200, Michal Nazarewicz wrote:
> On Fri, Jul 31 2015, Feng Tang wrote:
> > Hi Michal Nazarewicz,
> >
> > Thanks for the review.
> >
> > On Thu, 2015-07-30 at 15:59 +0200, Michal Nazarewicz wrote:
> >> On Thu, Jul 30 2015, Feng Tang wrote:
> >> > When system(one x86 soc) boot, we saw many normal dma allocation requests
> >> > goes to cma area. The call chain is
> >> > 	dma_generic_alloc_coherent
> >> > 	    dma_alloc_from_contiguous	-- arch/x86/kernel/pci-dma.c
> >> > 	        cma_alloc(dev_get_cma_area(dev), count, align)
> >> >
> >> > Current dev_get_cma_area() will return a valid "cma" anyway. Then all
> >> > these requests will be taken as valid cma request, and get pages from
> >> > cma area, which has 2 problems:
> >> > 1. make the cma area fragmented
> >> > 2. confuse the cma reservation, usually cma memory size is set according
> >> >    to the expectation of system scenario, these unexpected requests
> >> >    will affect the designed cma usage.
> >> >
> >> > So this patch will enforce the judgement, and only return valid "cma"
> >> > for real cma user, thus make normal user like IO device driver not
> >> > abuse cma reserved region.
> >> 
> >> Just don’t set dma_contiguous_default_area.  This patch defeats the
> >> purpose of a *default* area.
> >
> > Yes! This is exactly what I tried when I first saw this problem, but
> > this failed as there 2 places inside drivers/base/dma-contiguous.c
> > which set the dma_contiguous_default_area
> >
> > 1) 
> > void __init dma_contiguous_reserve(phys_addr_t limit)
> > {
> >     ....
> >     dma_contiguous_reserve_area(selected_size, selected_base,
> >                                             selected_limit,
> >                                    &dma_contiguous_default_area,
> >                                             fixed);
> >      ....
> > }
> >
> > 2) 
> > static int __init rmem_cma_setup(struct reserved_mem *rmem)
> > {
> >     ...
> >     if (of_get_flat_dt_prop(node, "linux,cma-default", NULL))
> >                 dma_contiguous_set_default(cma);
> >     ...
> > }
> >
> > Are you suggesting me to remove them?
> 
> Set CONFIG_CMA_SIZE_MBYTES to zero (in Kconfig) or add ‘cma=0’ on
> command line, and don’t use ‘linux,cma-default’ in device tree.  Then
> you will end up with no default CMA area.

Maybe I didn't make my problem clear, for our platform, we do need
to use cma as we have camera ISP which has no IOMMU, so we cannot
set "cma=0".

The current situation is we have 2 kinds of request to call 
dma_alloc_from_contiguous():
1. Those general IO drivers like spi/i2c/emmc/audio which will
request 1 to dozen of pages for normal DMA use 
2. The camera ISP and other HW which needs from seveal MB to 
 20MB CMA buffer, total CMA number may reach 180MB or more

We don't want the 1st customers to go into CMA area, to fragment
the CMA area from time to time. That's why I finally came up
with this patch.

Any suggestions?

Thanks,
Feng

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Michal Nazarewicz]

On Fri, Jul 31 2015, Feng Tang wrote:
> Maybe I didn't make my problem clear, for our platform, we do need to
> use cma as we have camera ISP which has no IOMMU, so we cannot set
> "cma=0".

Then specify a CMA region for the camera in platform initialisation code
or device trees or whatever else is the rave nowadays.

I’m assuming that you have a piece of code (or configuration of some
sort) that assigns a CMA region to the device (otherwise ‘dev->cma_area’
would be NULL and your patch would just always get you NULL CMA area).
Simply create a CMA area there and assign it to the device.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

On Fri, Jul 31, 2015 at 07:46:30PM +0200, Michal Nazarewicz wrote:
> On Fri, Jul 31 2015, Feng Tang wrote:
> > Maybe I didn't make my problem clear, for our platform, we do need to
> > use cma as we have camera ISP which has no IOMMU, so we cannot set
> > "cma=0".
> 
> Then specify a CMA region for the camera in platform initialisation code
> or device trees or whatever else is the rave nowadays.
> 
> I’m assuming that you have a piece of code (or configuration of some
> sort) that assigns a CMA region to the device (otherwise ‘dev->cma_area’
> would be NULL and your patch would just always get you NULL CMA area).
> Simply create a CMA area there and assign it to the device.

Your suggestion remind me one more thing, that for a system which needs
multiple cma heaps (like for security reason), they may have to share
one struct device *dev, as in ion_cma_heap_create()

struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
{
	struct ion_cma_heap *cma_heap;

	cma_heap = kzalloc(sizeof(struct ion_cma_heap), GFP_KERNEL);

	if (!cma_heap)
		return ERR_PTR(-ENOMEM);

	cma_heap->heap.ops = &ion_cma_ops;
	/* get device from private heaps data, later it will be
	 * used to make the link with reserved CMA memory */
	cma_heap->dev = data->priv;
	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
	return &cma_heap->heap;
}

Usually the platform data's priv points to the same ion platform device,
so the several heap's dev will be same.

Then when the time comes to allocate for each cma heap, the "dev->cma_area"
may have to be dynamically switched, and casue many syncing trouble.

I'm working on a patch for this. 

Thanks,
Feng

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Michal Nazarewicz]

On Wed, Aug 05 2015, Feng Tang wrote:
> that for a system which needs multiple cma heaps (like for security
> reason), they may have to share one struct device *dev, as in
> ion_cma_heap_create()

If you need several CMA areas to allocate from, create multiple struct
devices.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> On Wed, Aug 05 2015, Feng Tang wrote:
> > that for a system which needs multiple cma heaps (like for security
> > reason), they may have to share one struct device *dev, as in
> > ion_cma_heap_create()
> 
> If you need several CMA areas to allocate from, create multiple struct
> devices.

Yes, that's my thought too. The normal cma case for SOCs are one platform
device is created in drivers/staging/android/ion/xxx_ion.c, then that
device's pointer will be transferred into ion_cma_heap_create() inside
of the struct ion_platform_heap data.

And it's not easy to create multiple platform devices. And it may be
better to give each cma_heap one dedicated device in struct ion_cma_heap
which could be used as a parameter for 
	dma_alloc_from_contigous() --> cma_alloc()

Thanks,
Feng
 
> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> On Wed, Aug 05 2015, Feng Tang wrote:
> > that for a system which needs multiple cma heaps (like for security
> > reason), they may have to share one struct device *dev, as in
> > ion_cma_heap_create()
> 
> If you need several CMA areas to allocate from, create multiple struct
> devices.

I've made a quick patch, which works ok on our multiple cma heap cases.

Thanks,
Feng

---
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index f4211f1..ee9c5d1 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -29,6 +29,7 @@
 struct ion_cma_heap {
 	struct ion_heap heap;
 	struct device *dev;
+	struct device default_dma_dev;
 };
 
 #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
@@ -180,9 +181,14 @@ struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
 		return ERR_PTR(-ENOMEM);
 
 	cma_heap->heap.ops = &ion_cma_ops;
-	/* get device from private heaps data, later it will be
-	 * used to make the link with reserved CMA memory */
-	cma_heap->dev = data->priv;
+
+	cma_heap->dev = &cma_heap->default_dma_dev;
+	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
+	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
+
+	/* data->priv contains a pointer to struct cma */
+	dev_set_cma_area(cma_heap->dev, data->priv);
+
 	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
 	return &cma_heap->heap;
 }


> -- 
> Best regards,                                         _     _
> .o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
> ..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
> ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Michal Nazarewicz]

> On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
>> If you need several CMA areas to allocate from, create multiple struct
>> devices.

On Wed, Aug 05 2015, Feng Tang wrote:
> I've made a quick patch, which works ok on our multiple cma heap cases.

> ---
> diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> index f4211f1..ee9c5d1 100644
> --- a/drivers/staging/android/ion/ion_cma_heap.c
> +++ b/drivers/staging/android/ion/ion_cma_heap.c
> @@ -29,6 +29,7 @@
>  struct ion_cma_heap {
>  	struct ion_heap heap;
>  	struct device *dev;
> +	struct device default_dma_dev;

I’m unfamiliar with ION code so cannot comment in great detail, butwhy
do you need dev and default_dma_dev fields?  Just make dev a non-pointer
and use that.

>  };
>  
>  #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
> @@ -180,9 +181,14 @@ struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
>  		return ERR_PTR(-ENOMEM);
>  
>  	cma_heap->heap.ops = &ion_cma_ops;
> -	/* get device from private heaps data, later it will be
> -	 * used to make the link with reserved CMA memory */
> -	cma_heap->dev = data->priv;
> +
> +	cma_heap->dev = &cma_heap->default_dma_dev;
> +	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
> +
> +	/* data->priv contains a pointer to struct cma */
> +	dev_set_cma_area(cma_heap->dev, data->priv);

In the previous code, data->priv seemed to be struct device*, but in
this code it is used as struct cma*.

> +
>  	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
>  	return &cma_heap->heap;
>  }

But yeah, in general, from CMA’s point of view, this looks good.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

Re: [PATCH] CMA: Don't return a valid cma for non-cma dev

[Feng Tang]

On Wed, Aug 05, 2015 at 01:15:50PM +0200, Michal Nazarewicz wrote:
> > On Wed, Aug 05, 2015 at 12:28:03PM +0200, Michal Nazarewicz wrote:
> >> If you need several CMA areas to allocate from, create multiple struct
> >> devices.
> 
> On Wed, Aug 05 2015, Feng Tang wrote:
> > I've made a quick patch, which works ok on our multiple cma heap cases.
> 
> > ---
> > diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
> > index f4211f1..ee9c5d1 100644
> > --- a/drivers/staging/android/ion/ion_cma_heap.c
> > +++ b/drivers/staging/android/ion/ion_cma_heap.c
> > @@ -29,6 +29,7 @@
> >  struct ion_cma_heap {
> >  	struct ion_heap heap;
> >  	struct device *dev;
> > +	struct device default_dma_dev;
> 
> I’m unfamiliar with ION code so cannot comment in great detail, butwhy
> do you need dev and default_dma_dev fields?  Just make dev a non-pointer
> and use that.

Good point. I've thought about keeping dev to be back compatible
with current code, and only use default_dma_dev when no "dev" is
passed in platform data. But from your other comments, it may
be not necessary to keep the "dev" 

> 
> >  };
> >  
> >  #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap)
> > @@ -180,9 +181,14 @@ struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data)
> >  		return ERR_PTR(-ENOMEM);
> >  
> >  	cma_heap->heap.ops = &ion_cma_ops;
> > -	/* get device from private heaps data, later it will be
> > -	 * used to make the link with reserved CMA memory */
> > -	cma_heap->dev = data->priv;
> > +
> > +	cma_heap->dev = &cma_heap->default_dma_dev;
> > +	cma_heap->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> > +	cma_heap->dev->dma_mask = &dev->coherent_dma_mask;
> > +
> > +	/* data->priv contains a pointer to struct cma */
> > +	dev_set_cma_area(cma_heap->dev, data->priv);
> 
> In the previous code, data->priv seemed to be struct device*, but in
> this code it is used as struct cma*.

As we are going to use per cma-heap's own "default_dma_dev",
the "data->priv" should be a pointer to "struct cma*", which
makes more sense, as when a cma heap is created, we'd better
provide a "struct cma *" to tell it where to request cma buffer.

> 
> > +
> >  	cma_heap->heap.type = ION_HEAP_TYPE_DMA;
> >  	return &cma_heap->heap;
> >  }
> 
> But yeah, in general, from CMA’s point of view, this looks good.

Thanks! Will try to clean the code and add more comments, then
send it out for review.

- Feng