[PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-23 Thread Grygorii Strashko 
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 
Signed-off-by: Grygorii Strashko 
---
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 
 #include 
 #include 
-
+#include 
 #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_fnhandler;
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);
+   }
+}
+
 /*
  * 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(poo

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-23 Thread Ivan Khoronzhuk 



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 
Signed-off-by: Grygorii Strashko 
---
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 
  #include 
  #include 
-
+#include 
  #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_fnhandler;
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?


  /*
   * 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;

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-23 Thread ivan.khoronzhuk 



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 
Signed-off-by: Grygorii Strashko 


Tested-by: Ivan Khoronzhuk 


---
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 
  #include 
  #include 
-
+#include 
  #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_fnhandler;
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);
+   }
+}
+
  /*
   * 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_vi

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-23 Thread Mugunthan V N 
On Thursday 23 June 2016 06:06 PM, 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 
> Signed-off-by: Grygorii Strashko 

Acked-by: Mugunthan V N 

Regards
Mugunthan V N

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-23 Thread Mugunthan V N 
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 
>> Signed-off-by: Grygorii Strashko 
>> ---
>> 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 
>>   #include 
>>   #include 
>> -
>> +#include 
>>   #include "davinci_cpdma.h"
>>
>>   /* DMA Registers */
>> @@ -87,9 +87,8 @@ struct cpdma_desc_pool {
>>   void*cpumap;/* dma_alloc map */
>>   intdesc_size, mem_size;
>>   intnum_desc, used_desc;
>> -unsigned long*bitmap;
>>   struct device*dev;
>> -spinlock_tlock;
>> +struct gen_pool*gen_pool;
>>   };
>>
>>   enum cpdma_state {
>> @@ -117,6 +116,7 @@ struct cpdma_chan {
>>   intchan_num;
>>   spinlock_tlock;
>>   intcount;
>> +u32desc_num;
>>   u32mask;
>>   cpdma_handler_fnhandler;
>>   enum dma_data_directiondir;
>> @@ -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);
>> +

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-24 Thread Afzal Mohammed 
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

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-24 Thread Lennart Sorensen 
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

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-24 Thread Grygorii Strashko 
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

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-24 Thread Lennart Sorensen 
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

Re: [PATCH] net: ethernet: ti: cpdma: switch to use genalloc

2016-06-25 Thread Afzal Mohammed 
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