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

[Shanker Donthineni]

Hi Marc/Robert,

On 03/16/2017 08:41 AM, Marc Zyngier wrote:
> On 16/03/17 13:31, Robert Richter wrote:
>> On 15.03.17 18:46:22, Marc Zyngier wrote:
>>> On 15/03/17 18:37, Robert Richter wrote:
 On 14.03.17 12:40:45, Shanker Donthineni wrote:
>> @@ -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(>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?
>>> No, there is no such guarantee. Actually, there is strictly no need for
>>> coherency, as the ITS tables are only written by the ITS itself, for its
>>> own purpose.
>> So no need to change that, right?
> I don't think there is any. We just need to allocate memory with the
> relevant constraints (alignment and zeroing, mostly), and make sure we
> never access it directly. Of course, property tables and command queues
> would benefit from being allocated as DMA buffers, which would allow the
> cache flush to be dealt with at the DMA level.

Agree with Marc, only PROP tables and CMD queue buffers are touched by CPU 
during runtime. That means DMA coherent property set to true only when both the 
CMD_BASE (ITS_FLAGS_CMDQ_NEEDS_FLUSHING) and PROP_BASE 
(RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING) tables support coherency.

> Thanks,
>
>   M.

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

[Shanker Donthineni]

Hi Marc/Robert,

On 03/16/2017 08:41 AM, Marc Zyngier wrote:
> On 16/03/17 13:31, Robert Richter wrote:
>> On 15.03.17 18:46:22, Marc Zyngier wrote:
>>> On 15/03/17 18:37, Robert Richter wrote:
 On 14.03.17 12:40:45, Shanker Donthineni wrote:
>> @@ -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(>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?
>>> No, there is no such guarantee. Actually, there is strictly no need for
>>> coherency, as the ITS tables are only written by the ITS itself, for its
>>> own purpose.
>> So no need to change that, right?
> I don't think there is any. We just need to allocate memory with the
> relevant constraints (alignment and zeroing, mostly), and make sure we
> never access it directly. Of course, property tables and command queues
> would benefit from being allocated as DMA buffers, which would allow the
> cache flush to be dealt with at the DMA level.

Agree with Marc, only PROP tables and CMD queue buffers are touched by CPU 
during runtime. That means DMA coherent property set to true only when both the 
CMD_BASE (ITS_FLAGS_CMDQ_NEEDS_FLUSHING) and PROP_BASE 
(RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING) tables support coherency.

> Thanks,
>
>   M.

-- 
Shanker Donthineni
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

[Robert Richter]

On 15.03.17 18:46:22, Marc Zyngier wrote:
> On 15/03/17 18:37, Robert Richter wrote:
> > On 14.03.17 12:40:45, Shanker Donthineni wrote:

> >>> @@ -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(>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?
> 
> No, there is no such guarantee. Actually, there is strictly no need for
> coherency, as the ITS tables are only written by the ITS itself, for its
> own purpose.

So no need to change that, right?

Thanks,

-Robert

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

[Robert Richter]

On 15.03.17 18:46:22, Marc Zyngier wrote:
> On 15/03/17 18:37, Robert Richter wrote:
> > On 14.03.17 12:40:45, Shanker Donthineni wrote:

> >>> @@ -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(>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?
> 
> No, there is no such guarantee. Actually, there is strictly no need for
> coherency, as the ITS tables are only written by the ITS itself, for its
> own purpose.

So no need to change that, right?

Thanks,

-Robert

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

[Marc Zyngier]

On 16/03/17 13:31, Robert Richter wrote:
> On 15.03.17 18:46:22, Marc Zyngier wrote:
>> On 15/03/17 18:37, Robert Richter wrote:
>>> On 14.03.17 12:40:45, Shanker Donthineni wrote:
> 
> @@ -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(>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?
>>
>> No, there is no such guarantee. Actually, there is strictly no need for
>> coherency, as the ITS tables are only written by the ITS itself, for its
>> own purpose.
> 
> So no need to change that, right?

I don't think there is any. We just need to allocate memory with the
relevant constraints (alignment and zeroing, mostly), and make sure we
never access it directly. Of course, property tables and command queues
would benefit from being allocated as DMA buffers, which would allow the
cache flush to be dealt with at the DMA level.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 16/03/17 13:31, Robert Richter wrote:
> On 15.03.17 18:46:22, Marc Zyngier wrote:
>> On 15/03/17 18:37, Robert Richter wrote:
>>> On 14.03.17 12:40:45, Shanker Donthineni wrote:
> 
> @@ -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(>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?
>>
>> No, there is no such guarantee. Actually, there is strictly no need for
>> coherency, as the ITS tables are only written by the ITS itself, for its
>> own purpose.
> 
> So no need to change that, right?

I don't think there is any. We just need to allocate memory with the
relevant constraints (alignment and zeroing, mostly), and make sure we
never access it directly. Of course, property tables and command queues
would benefit from being allocated as DMA buffers, which would allow the
cache flush to be dealt with at the DMA level.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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 
> > ---
> >  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(>dev, GFP_KERNEL | __GFP_ZERO,
> > -  order);
> > -   if (!base)
> > +   base = dmam_alloc_coherent(>dev,
> > +   PAGE_ORDER_TO_SIZE(order),
> > +   _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(>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(>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(>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

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 
> > ---
> >  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(>dev, GFP_KERNEL | __GFP_ZERO,
> > -  order);
> > -   if (!base)
> > +   base = dmam_alloc_coherent(>dev,
> > +   PAGE_ORDER_TO_SIZE(order),
> > +   _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(>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(>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(>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

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

[Marc Zyngier]

On 15/03/17 18:37, Robert Richter wrote:
> 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 
>>> ---
>>>  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(>dev, GFP_KERNEL | __GFP_ZERO,
>>> -  order);
>>> -   if (!base)
>>> +   base = dmam_alloc_coherent(>dev,
>>> +   PAGE_ORDER_TO_SIZE(order),
>>> +   _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(>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(>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(>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?

No, there is no such guarantee. Actually, there is strictly no need for
coherency, as the ITS tables are only written by the ITS itself, for its
own purpose.

The only things that may benefit from coherency are the property table
and the command queue.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 15/03/17 18:37, Robert Richter wrote:
> 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 
>>> ---
>>>  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(>dev, GFP_KERNEL | __GFP_ZERO,
>>> -  order);
>>> -   if (!base)
>>> +   base = dmam_alloc_coherent(>dev,
>>> +   PAGE_ORDER_TO_SIZE(order),
>>> +   _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(>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(>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(>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?

No, there is no such guarantee. Actually, there is strictly no need for
coherency, as the ITS tables are only written by the ITS itself, for its
own purpose.

The only things that may benefit from coherency are the property table
and the command queue.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...

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

[Shanker Donthineni]

Hi Robert,

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.


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 
> ---
>  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
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -864,6 +865,7 @@ static int its_setup_baser(struct its_node *its, struct 
> its_baser *baser,
>   u64 type = GITS_BASER_TYPE(val);
>   u32 alloc_pages;
>   void *base;
> + dma_addr_t dma_handle;
>   u64 tmp;
>  
>  retry_alloc_baser:
> @@ -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(>dev, GFP_KERNEL | __GFP_ZERO,
> -order);
> - if (!base)
> + base = dmam_alloc_coherent(>dev,
> + PAGE_ORDER_TO_SIZE(order),
> + _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.
> +
> + if (!base && order >= MAX_ORDER) {
> + order = MAX_ORDER - 1;
> + dev_warn(>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(>dev, "Failed to allocate device table\n");
>   return -ENOMEM;
> + }
>  
>  retry_baser:
> - val = (virt_to_phys(base)|
> + val = (dma_handle|
>   (type << GITS_BASER_TYPE_SHIFT)  |
>   ((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)   |
>   ((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT)|
> @@ -923,29 +938,28 @@ static int its_setup_baser(struct its_node *its, struct 
> its_baser *baser,
>   goto retry_baser;
>   }
>  
> - if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
> - /*
> -  * Page size didn't stick. Let's try a smaller
> -  * size and retry. If we reach 4K, then
> -  * something is horribly wrong...
> -  */
> - devm_free_pages(>dev, (unsigned long)base);
> - baser->base = NULL;
> -
> - switch (psz) {
> - case SZ_16K:
> - psz = SZ_4K;
> - goto retry_alloc_baser;
> - case SZ_64K:
> - psz = SZ_16K;
> - goto retry_alloc_baser;
> + if (val != tmp) {
> + dmam_free_coherent(>dev, PAGE_ORDER_TO_SIZE(order),
> + base, dma_handle);
> +
> + if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
> + /*
> +  * Page size didn't stick. Let's try a smaller
> +  * size and retry. If we reach 4K, then
> +  * something is horribly wrong...
> +  */
> + switch (psz) {
> + case SZ_16K:
> + psz = SZ_4K;
> + goto retry_alloc_baser;
> + case SZ_64K:
> + psz = SZ_16K;
> + goto retry_alloc_baser;
> + }
>   }
> - }
>  
> - if (val != tmp) {
>   dev_err(>dev, "%s doesn't stick: %llx %llx\n",
>  its_base_type_string[type], val, tmp);
> - devm_free_pages(>dev, (unsigned long)base);
>   return -ENXIO;
>   }
>  
> @@ -1003,12 +1017,6 @@ static bool its_parse_baser_device(struct its_node 
> *its, struct its_baser *baser
>* feature is not supported by hardware.
>*/
>   

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

[Shanker Donthineni]

Hi Robert,

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.


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 
> ---
>  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
> @@ -21,6 +21,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -864,6 +865,7 @@ static int its_setup_baser(struct its_node *its, struct 
> its_baser *baser,
>   u64 type = GITS_BASER_TYPE(val);
>   u32 alloc_pages;
>   void *base;
> + dma_addr_t dma_handle;
>   u64 tmp;
>  
>  retry_alloc_baser:
> @@ -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(>dev, GFP_KERNEL | __GFP_ZERO,
> -order);
> - if (!base)
> + base = dmam_alloc_coherent(>dev,
> + PAGE_ORDER_TO_SIZE(order),
> + _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.
> +
> + if (!base && order >= MAX_ORDER) {
> + order = MAX_ORDER - 1;
> + dev_warn(>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(>dev, "Failed to allocate device table\n");
>   return -ENOMEM;
> + }
>  
>  retry_baser:
> - val = (virt_to_phys(base)|
> + val = (dma_handle|
>   (type << GITS_BASER_TYPE_SHIFT)  |
>   ((esz - 1) << GITS_BASER_ENTRY_SIZE_SHIFT)   |
>   ((alloc_pages - 1) << GITS_BASER_PAGES_SHIFT)|
> @@ -923,29 +938,28 @@ static int its_setup_baser(struct its_node *its, struct 
> its_baser *baser,
>   goto retry_baser;
>   }
>  
> - if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
> - /*
> -  * Page size didn't stick. Let's try a smaller
> -  * size and retry. If we reach 4K, then
> -  * something is horribly wrong...
> -  */
> - devm_free_pages(>dev, (unsigned long)base);
> - baser->base = NULL;
> -
> - switch (psz) {
> - case SZ_16K:
> - psz = SZ_4K;
> - goto retry_alloc_baser;
> - case SZ_64K:
> - psz = SZ_16K;
> - goto retry_alloc_baser;
> + if (val != tmp) {
> + dmam_free_coherent(>dev, PAGE_ORDER_TO_SIZE(order),
> + base, dma_handle);
> +
> + if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
> + /*
> +  * Page size didn't stick. Let's try a smaller
> +  * size and retry. If we reach 4K, then
> +  * something is horribly wrong...
> +  */
> + switch (psz) {
> + case SZ_16K:
> + psz = SZ_4K;
> + goto retry_alloc_baser;
> + case SZ_64K:
> + psz = SZ_16K;
> + goto retry_alloc_baser;
> + }
>   }
> - }
>  
> - if (val != tmp) {
>   dev_err(>dev, "%s doesn't stick: %llx %llx\n",
>  its_base_type_string[type], val, tmp);
> - devm_free_pages(>dev, (unsigned long)base);
>   return -ENXIO;
>   }
>  
> @@ -1003,12 +1017,6 @@ static bool its_parse_baser_device(struct its_node 
> *its, struct its_baser *baser
>* feature is not supported by hardware.
>*/
>   new_order = max_t(u32,