Re: [PATCH v2 8/8] irqchip, gicv3-its, cma: Use CMA for allocation of large device tables

[Robert Richter]

On 14.03.17 12:40:45, Shanker Donthineni wrote:

> I don't see anywhere in this patch, code calls explicitly CMA API to
> allocate memory for device table.  The CMA feature is an optional in
> kernel, and will be handled transparently inside the the DMA
> layer. It would be nicer to not mention CMA word in the commit
> subject.

Still CMA is *essential* and used for large tables. IMO this needs to
be emphasized. That's the reason for using devm_alloc_coherent().

> On 03/06/2017 06:57 AM, Robert Richter wrote:
> > The gicv3-its device table may have a size of up to 16MB. With 4k
> > pagesize the maximum size of memory allocation is 4MB. Use CMA for
> > allocation of large tables.
> Just say use devm_alloc_coherent() to allocate memory.
> 
> > We use the device managed version of dma_alloc_coherent(). Thus, we
> > don't need to release it manually on device removal.
> >
> > Signed-off-by: Robert Richter <rrichter@xxxxxxxxxx>
> > ---
> >  drivers/irqchip/irq-gic-v3-its.c | 69 +++++++++++++++++++++++-----------------
> >  1 file changed, 40 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> > index 6625b3a505f0..6d293a0165b0 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c

> > @@ -876,13 +878,26 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
> >  		order = get_order(GITS_BASER_PAGES_MAX * psz);
> >  	}
> >  
> > -	base = (void *)devm_get_free_pages(&its->dev, GFP_KERNEL | __GFP_ZERO,
> > -					   order);
> > -	if (!base)
> > +	base = dmam_alloc_coherent(&its->dev,
> > +				PAGE_ORDER_TO_SIZE(order),
> > +				&dma_handle,
> > +				GFP_KERNEL | __GFP_ZERO);

> Not just for 1st level device table, you have do a similar code
> change when allocating memory for 2nd level device table.

The 2nd level tables are much smaller, so no need for
dmam_alloc_coherent() there.

Though, we could use device managed devm_get_free_pages() there too.

> > +
> > +	if (!base && order >= MAX_ORDER) {
> > +		order = MAX_ORDER - 1;
> > +		dev_warn(&its->dev, "Device Table too large, reduce ids %u->%u, no CMA memory available\n",
> > +			its->device_ids,
> > +			ilog2(PAGE_ORDER_TO_SIZE(order) / (int)esz));
> > +		goto retry_alloc_baser;
> > +	}
> > +
> > +	if (!base) {
> > +		dev_err(&its->dev, "Failed to allocate device table\n");
> >  		return -ENOMEM;
> > +	}

> > @@ -1698,6 +1706,9 @@ static int __init its_init_one(struct its_node *its)
> >  		return err;
> >  	}
> >  
> > +	/* Setup dma_ops for dmam_alloc_coherent() */
> > +	arch_setup_dma_ops(&its->dev, 0, 0, NULL, true);
> > +

> Why you are hard-coding DMA coherent property to true here ? It
> breaks the MSI(x) functionally on systems where ITS hardware doesn't
> support coherency.

Aren't current ITS tables coherent only?

Thanks,

-Robert