On Wed, Jul 31 2013, Marek Szyprowski wrote:
This patch cleans the initialization of dma contiguous framework. The all-in-one dma_declare_contiguous() function is now separated into dma_contiguous_reserve_area() which only steals the the memory from memblock allocator and dma_contiguous_add_device() function, which assigns given device to the specified reserved memory area. This improves the flexibility in defining contiguous memory areas and assigning device to them, because now it is possible to assign more than one device to the given contiguous memory area. Such split in initialization procedure is also required for upcoming device tree support.
Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Acked-by: Kyungmin Park kyungmin.park@samsung.com
Acked-by: Michal Nazarewicz mina86@mina86.com
Even though I have some comments.
@@ -124,22 +124,29 @@ void __init dma_contiguous_reserve(phys_addr_t limit) #endif }
- if (selected_size) {
- if (selected_size && !dma_contiguous_default_area) {
Perhaps check this earlier in the function? Also, maybe WARN would be in order when dma_contiguous_reserve is called with dma_contiguous_default_area already reserved.
pr_debug("%s: reserving %ld MiB for global area\n", __func__, (unsigned long)selected_size / SZ_1M);
dma_declare_contiguous(NULL, selected_size, 0, limit);
dma_contiguous_reserve_area(selected_size, 0, limit,&dma_contiguous_default_area);}; static DEFINE_MUTEX(cma_mutex);
-int __init dma_declare_contiguous(struct device *dev, phys_addr_t size,
phys_addr_t base, phys_addr_t limit)+int __init dma_contiguous_reserve_area(phys_addr_t size, phys_addr_t base,
phys_addr_t limit, struct cma **res_cma)
@@ -277,10 +245,11 @@ int __init dma_declare_contiguous(struct device *dev, phys_addr_t size, * Each reserved area must be initialised later, when more kernel * subsystems (like slab allocator) are available. */
- r->start = base;
- r->size = size;
- r->dev = dev;
- cma_reserved_count++;
- cma->base_pfn = PFN_DOWN(base);
- cma->count = size >> PAGE_SHIFT;
- *res_cma = cma;
Alternatively it could return a pointer, but either way…
- cma_area_count++;
- pr_info("CMA: reserved %ld MiB at %08lx\n", (unsigned long)size / SZ_1M, (unsigned long)base);
+int __init dma_contiguous_add_device(struct device *dev, struct cma *cma) +{
- dev_set_cma_area(dev, cma);
- return 0;
+}
This never fails and it is used in dma_declare_contiguous w/o checking for errors so perhaps it should return void? Applies to some other functions as well.
Moreover, this function is so tiny, it makes me wonder why does it exist in the first place (why would anyone call it rather then dev_set_cma_area? is it just matter of name?), and since it exists, why not put it to a header file as a static inline.
+int __init dma_contiguous_set_default_area(struct cma *cma) +{
- dma_contiguous_default_area = cma;
- return 0;
}
Ditto.
+static inline int dma_declare_contiguous(struct device *dev, phys_addr_t size,
phys_addr_t base, phys_addr_t limit)+{
- struct cma *cma;
- int ret;
+ if (WARN_ON(!dev)) + return -EINVAL;
- ret = dma_contiguous_reserve_area(size, base, limit, &cma);
- if (ret == 0)
dma_contiguous_add_device(dev, cma);- return ret;
+}