Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1430385 > unrolled thread
| Started by | Mugunthan V N <mugunthanvnm@ti.com> |
|---|---|
| First post | 2016-06-24 08:10 +0200 |
| Last post | 2016-06-25 10:40 +0200 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc Mugunthan V N <mugunthanvnm@ti.com> - 2016-06-24 08:10 +0200
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc Afzal Mohammed <afzal.mohd.ma@gmail.com> - 2016-06-24 14:00 +0200
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc "Lennart Sorensen" <lsorense@csclub.uwaterloo.ca> - 2016-06-24 18:20 +0200
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc Grygorii Strashko <grygorii.strashko@ti.com> - 2016-06-24 19:00 +0200
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc "Lennart Sorensen" <lsorense@csclub.uwaterloo.ca> - 2016-06-24 20:40 +0200
Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc Afzal Mohammed <afzal.mohd.ma@gmail.com> - 2016-06-25 10:40 +0200
| From | Mugunthan V N <mugunthanvnm@ti.com> |
|---|---|
| Date | 2016-06-24 08:10 +0200 |
| Subject | Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc |
| Message-ID | <rNstr-4rr-11@gated-at.bofh.it> |
On Thursday 23 June 2016 06:26 PM, Ivan Khoronzhuk wrote:
>
>
> On 23.06.16 15:36, Grygorii Strashko wrote:
>> TI CPDMA currently uses a bitmap for tracking descriptors alloactions
>> allocations, but The genalloc already handles the same and can be used
>> as with special memory (SRAM) as with DMA cherent memory chank
>> (dma_alloc_coherent()). Hence, switch to using genalloc and add
>> desc_num property for each channel for limitation of max number of
>> allowed descriptors for each CPDMA channel. This patch do not affect
>> on net throuput.
>>
>> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> Testing
>> TCP window: 256K, bandwidth in Mbits/sec:
>> host: iperf -s
>> device: iperf -c 172.22.39.17 -t600 -i5 -d -w128K
>>
>> AM437x-idk, 1Gbps link
>> before: : 341.60, after: 232+123=355
>> am57xx-beagle-x15, 1Gbps link
>> before: : 1112.80, after: 814+321=1135
>> am335x-boneblack, 100Mbps link
>> before: : 162.40, after: 72+93=165
>>
>> drivers/net/ethernet/ti/davinci_cpdma.c | 136
>> +++++++++++++++-----------------
>> 1 file changed, 62 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c
>> b/drivers/net/ethernet/ti/davinci_cpdma.c
>> index 18bf3a8..03b9882 100644
>> --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> @@ -21,7 +21,7 @@
>> #include <linux/dma-mapping.h>
>> #include <linux/io.h>
>> #include <linux/delay.h>
>> -
>> +#include <linux/genalloc.h>
>> #include "davinci_cpdma.h"
>>
>> /* DMA Registers */
>> @@ -87,9 +87,8 @@ struct cpdma_desc_pool {
>> void *cpumap; /* dma_alloc map */
>> int desc_size, mem_size;
>> int num_desc, used_desc;
>> - unsigned long *bitmap;
>> struct device *dev;
>> - spinlock_t lock;
>> + struct gen_pool *gen_pool;
>> };
>>
>> enum cpdma_state {
>> @@ -117,6 +116,7 @@ struct cpdma_chan {
>> int chan_num;
>> spinlock_t lock;
>> int count;
>> + u32 desc_num;
>> u32 mask;
>> cpdma_handler_fn handler;
>> enum dma_data_direction dir;
>> @@ -145,6 +145,20 @@ struct cpdma_chan {
>> (directed << CPDMA_TO_PORT_SHIFT)); \
>> } while (0)
>>
>> +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
>> +{
>> + if (!pool)
>> + return;
>> +
>> + WARN_ON(pool->used_desc);
>> + if (pool->cpumap) {
>> + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
>> + pool->phys);
>> + } else {
>> + iounmap(pool->iomap);
>> + }
>> +}
>> +
> single if, brackets?
if() has multiple line statement, so brackets are must.
Regards
Mugunthan V N
>
>> /*
>> * Utility constructs for a cpdma descriptor pool. Some devices
>> (e.g. davinci
>> * emac) have dedicated on-chip memory for these descriptors. Some
>> other
>> @@ -155,24 +169,25 @@ static struct cpdma_desc_pool *
>> cpdma_desc_pool_create(struct device *dev, u32 phys, dma_addr_t
>> hw_addr,
>> int size, int align)
>> {
>> - int bitmap_size;
>> struct cpdma_desc_pool *pool;
>> + int ret;
>>
>> pool = devm_kzalloc(dev, sizeof(*pool), GFP_KERNEL);
>> if (!pool)
>> - goto fail;
>> -
>> - spin_lock_init(&pool->lock);
>> + goto gen_pool_create_fail;
>>
>> pool->dev = dev;
>> pool->mem_size = size;
>> pool->desc_size = ALIGN(sizeof(struct cpdma_desc), align);
>> pool->num_desc = size / pool->desc_size;
>>
>> - bitmap_size = (pool->num_desc / BITS_PER_LONG) * sizeof(long);
>> - pool->bitmap = devm_kzalloc(dev, bitmap_size, GFP_KERNEL);
>> - if (!pool->bitmap)
>> - goto fail;
>> + pool->gen_pool = devm_gen_pool_create(dev,
>> ilog2(pool->desc_size), -1,
>> + "cpdma");
>> + if (IS_ERR(pool->gen_pool)) {
>> + dev_err(dev, "pool create failed %ld\n",
>> + PTR_ERR(pool->gen_pool));
>> + goto gen_pool_create_fail;
>> + }
>>
>> if (phys) {
>> pool->phys = phys;
>> @@ -185,24 +200,22 @@ cpdma_desc_pool_create(struct device *dev, u32
>> phys, dma_addr_t hw_addr,
>> pool->phys = pool->hw_addr; /* assumes no IOMMU, don't use
>> this value */
>> }
>>
>> - if (pool->iomap)
>> - return pool;
>> -fail:
>> - return NULL;
>> -}
>> -
>> -static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
>> -{
>> - if (!pool)
>> - return;
>> + if (!pool->iomap)
>> + goto gen_pool_create_fail;
>>
>> - WARN_ON(pool->used_desc);
>> - if (pool->cpumap) {
>> - dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
>> - pool->phys);
>> - } else {
>> - iounmap(pool->iomap);
>> + ret = gen_pool_add_virt(pool->gen_pool, (unsigned long)pool->iomap,
>> + pool->phys, pool->mem_size, -1);
>> + if (ret < 0) {
>> + dev_err(dev, "pool add failed %d\n", ret);
>> + goto gen_pool_add_virt_fail;
>> }
>> +
>> + return pool;
>> +
>> +gen_pool_add_virt_fail:
>> + cpdma_desc_pool_destroy(pool);
>> +gen_pool_create_fail:
>> + return NULL;
>> }
>>
>> static inline dma_addr_t desc_phys(struct cpdma_desc_pool *pool,
>> @@ -210,7 +223,7 @@ static inline dma_addr_t desc_phys(struct
>> cpdma_desc_pool *pool,
>> {
>> if (!desc)
>> return 0;
>> - return pool->hw_addr + (__force long)desc - (__force
>> long)pool->iomap;
>> + return gen_pool_virt_to_phys(pool->gen_pool, (unsigned long)desc);
>> }
>>
>> static inline struct cpdma_desc __iomem *
>> @@ -220,47 +233,23 @@ desc_from_phys(struct cpdma_desc_pool *pool,
>> dma_addr_t dma)
>> }
>>
>> static struct cpdma_desc __iomem *
>> -cpdma_desc_alloc(struct cpdma_desc_pool *pool, int num_desc, bool is_rx)
>> +cpdma_desc_alloc(struct cpdma_desc_pool *pool)
>> {
>> - unsigned long flags;
>> - int index;
>> - int desc_start;
>> - int desc_end;
>> struct cpdma_desc __iomem *desc = NULL;
>>
>> - spin_lock_irqsave(&pool->lock, flags);
>> -
>> - if (is_rx) {
>> - desc_start = 0;
>> - desc_end = pool->num_desc/2;
>> - } else {
>> - desc_start = pool->num_desc/2;
>> - desc_end = pool->num_desc;
>> - }
>> -
>> - index = bitmap_find_next_zero_area(pool->bitmap,
>> - desc_end, desc_start, num_desc, 0);
>> - if (index < desc_end) {
>> - bitmap_set(pool->bitmap, index, num_desc);
>> - desc = pool->iomap + pool->desc_size * index;
>> + desc = (struct cpdma_desc __iomem *)gen_pool_alloc(pool->gen_pool,
>> + pool->desc_size);
>> + if (desc)
>> pool->used_desc++;
>> - }
>>
>> - spin_unlock_irqrestore(&pool->lock, flags);
>> return desc;
>> }
>>
>> static void cpdma_desc_free(struct cpdma_desc_pool *pool,
>> struct cpdma_desc __iomem *desc, int num_desc)
>> {
>> - unsigned long flags, index;
>> -
>> - index = ((unsigned long)desc - (unsigned long)pool->iomap) /
>> - pool->desc_size;
>> - spin_lock_irqsave(&pool->lock, flags);
>> - bitmap_clear(pool->bitmap, index, num_desc);
>> + gen_pool_free(pool->gen_pool, (unsigned long)desc, pool->desc_size);
>> pool->used_desc--;
>> - spin_unlock_irqrestore(&pool->lock, flags);
>> }
>>
>> struct cpdma_ctlr *cpdma_ctlr_create(struct cpdma_params *params)
>> @@ -516,6 +505,7 @@ struct cpdma_chan *cpdma_chan_create(struct
>> cpdma_ctlr *ctlr, int chan_num,
>> chan->state = CPDMA_STATE_IDLE;
>> chan->chan_num = chan_num;
>> chan->handler = handler;
>> + chan->desc_num = ctlr->pool->num_desc / 2;
>>
>> if (is_rx_chan(chan)) {
>> chan->hdp = ctlr->params.rxhdp + offset;
>> @@ -675,7 +665,13 @@ int cpdma_chan_submit(struct cpdma_chan *chan,
>> void *token, void *data,
>> goto unlock_ret;
>> }
>>
>> - desc = cpdma_desc_alloc(ctlr->pool, 1, is_rx_chan(chan));
>> + if (chan->count >= chan->desc_num) {
>> + chan->stats.desc_alloc_fail++;
>> + ret = -ENOMEM;
>> + goto unlock_ret;
>> + }
>> +
>> + desc = cpdma_desc_alloc(ctlr->pool);
>> if (!desc) {
>> chan->stats.desc_alloc_fail++;
>> ret = -ENOMEM;
>> @@ -721,24 +717,16 @@ EXPORT_SYMBOL_GPL(cpdma_chan_submit);
>>
>> bool cpdma_check_free_tx_desc(struct cpdma_chan *chan)
>> {
>> - unsigned long flags;
>> - int index;
>> - bool ret;
>> struct cpdma_ctlr *ctlr = chan->ctlr;
>> struct cpdma_desc_pool *pool = ctlr->pool;
>> + unsigned long flags;
>> + bool free_tx_desc;
> longer first
>
>>
>> - spin_lock_irqsave(&pool->lock, flags);
>> -
>> - index = bitmap_find_next_zero_area(pool->bitmap,
>> - pool->num_desc, pool->num_desc/2, 1, 0);
>> -
>> - if (index < pool->num_desc)
>> - ret = true;
>> - else
>> - ret = false;
>> -
>> - spin_unlock_irqrestore(&pool->lock, flags);
>> - return ret;
>> + spin_lock_irqsave(&chan->lock, flags);
>> + free_tx_desc = (chan->count < chan->desc_num) &&
>> + gen_pool_avail(pool->gen_pool);
>> + spin_unlock_irqrestore(&chan->lock, flags);
>> + return free_tx_desc;
>> }
>> EXPORT_SYMBOL_GPL(cpdma_check_free_tx_desc);
>>
>> @@ -772,7 +760,6 @@ static int __cpdma_chan_process(struct cpdma_chan
>> *chan)
>> unsigned long flags;
>>
>> spin_lock_irqsave(&chan->lock, flags);
>> -
> ?
>
>> desc = chan->head;
>> if (!desc) {
>> chan->stats.empty_dequeue++;
>> @@ -796,6 +783,7 @@ static int __cpdma_chan_process(struct cpdma_chan
>> *chan)
>> CPDMA_DESC_PORT_MASK);
>>
>> chan->head = desc_from_phys(pool, desc_read(desc, hw_next));
>> +
> ?
>
>> chan_write(chan, cp, desc_dma);
>> chan->count--;
>> chan->stats.good_dequeue++;
>>
>
[toc] | [next] | [standalone]
| From | Afzal Mohammed <afzal.mohd.ma@gmail.com> |
|---|---|
| Date | 2016-06-24 14:00 +0200 |
| Message-ID | <rNxW9-7AL-1@gated-at.bofh.it> |
| In reply to | #1430385 |
Hi,
On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote:
> On Thursday 23 June 2016 06:26 PM, Ivan Khoronzhuk wrote:
> >> + if (pool->cpumap) {
> >> + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
> >> + pool->phys);
> >> + } else {
> >> + iounmap(pool->iomap);
> >> + }
> > single if, brackets?
>
> if() has multiple line statement, so brackets are must.
Another paint to the bikeshed,
seems documented coding style mentions otherwise.
Regards
afzal
[toc] | [prev] | [next] | [standalone]
| From | "Lennart Sorensen" <lsorense@csclub.uwaterloo.ca> |
|---|---|
| Date | 2016-06-24 18:20 +0200 |
| Message-ID | <rNBZL-1W6-3@gated-at.bofh.it> |
| In reply to | #1430385 |
On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote:
> >> +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
> >> +{
> >> + if (!pool)
> >> + return;
> >> +
> >> + WARN_ON(pool->used_desc);
> >> + if (pool->cpumap) {
> >> + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
> >> + pool->phys);
> >> + } else {
> >> + iounmap(pool->iomap);
> >> + }
> >> +}
> >> +
> > single if, brackets?
>
> if() has multiple line statement, so brackets are must.
It is line wrapped, it is still one statement. And you can't argue the
else being multiple lines, although the style does require using brackets
for the else if the if required them.
Style says "Do not unnecessarily use braces where a single statement will do."
It says statement, not line. A multiline wrapped statement is still
one statement.
I may personally hate the lack of brackets, but style wise it seems very
clear that the linux kernel only uses brakcets when required, which is
only when there is more than one statement. I prefer what you did,
but not as much as I prefer consistency.
--
Len Sorensen
[toc] | [prev] | [next] | [standalone]
| From | Grygorii Strashko <grygorii.strashko@ti.com> |
|---|---|
| Date | 2016-06-24 19:00 +0200 |
| Message-ID | <rNCCt-2dO-19@gated-at.bofh.it> |
| In reply to | #1430793 |
On 06/24/2016 07:15 PM, Lennart Sorensen wrote:
> On Fri, Jun 24, 2016 at 11:35:15AM +0530, Mugunthan V N wrote:
>>>> +static void cpdma_desc_pool_destroy(struct cpdma_desc_pool *pool)
>>>> +{
>>>> + if (!pool)
>>>> + return;
>>>> +
>>>> + WARN_ON(pool->used_desc);
>>>> + if (pool->cpumap) {
>>>> + dma_free_coherent(pool->dev, pool->mem_size, pool->cpumap,
>>>> + pool->phys);
>>>> + } else {
>>>> + iounmap(pool->iomap);
>>>> + }
>>>> +}
>>>> +
>>> single if, brackets?
>>
>> if() has multiple line statement, so brackets are must.
>
> It is line wrapped, it is still one statement. And you can't argue the
> else being multiple lines, although the style does require using brackets
> for the else if the if required them.
>
> Style says "Do not unnecessarily use braces where a single statement will do."
> It says statement, not line. A multiline wrapped statement is still
> one statement.
>
> I may personally hate the lack of brackets, but style wise it seems very
> clear that the linux kernel only uses brakcets when required, which is
> only when there is more than one statement. I prefer what you did,
> but not as much as I prefer consistency.
>
Oh. nice :( So, seems, I'd need to send v3. Right?
By the way, this code hasn't been introduced by this patch - I've
just moved whole function from one place to another.
--
regards,
-grygorii
[toc] | [prev] | [next] | [standalone]
| From | "Lennart Sorensen" <lsorense@csclub.uwaterloo.ca> |
|---|---|
| Date | 2016-06-24 20:40 +0200 |
| Message-ID | <rNEbg-3lO-13@gated-at.bofh.it> |
| In reply to | #1430814 |
On Fri, Jun 24, 2016 at 07:58:32PM +0300, Grygorii Strashko wrote: > Oh. nice :( So, seems, I'd need to send v3. Right? > By the way, this code hasn't been introduced by this patch - I've > just moved whole function from one place to another. Well since it is moving I would think that was a handy time to fix the coding style violation too, since it got noticed. That leaves just one place in that file violating that part of the coding style (the other is in cpdma_chan_dump). Somehow it wasn't spotted when the code was put in back in 2010, and since they were wrapped lines, they don't stand out quite as much visually. -- Len Sorensen
[toc] | [prev] | [next] | [standalone]
| From | Afzal Mohammed <afzal.mohd.ma@gmail.com> |
|---|---|
| Date | 2016-06-25 10:40 +0200 |
| Message-ID | <rNRia-3cb-25@gated-at.bofh.it> |
| In reply to | #1430793 |
Hi, On Fri, Jun 24, 2016 at 12:15:41PM -0400, Lennart Sorensen wrote: > although the style does require using brackets for the else if the > if required them. As an aside, though most of the style rationale is K & R, K & R consistently uses unbalanced braces for if-else-* For a one that learns C unadultered from K & R, probably Kernel coding style comes naturally, except for trivial things like above. ...a brick for the shed. Regards afzal
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web