Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Anup Patel]

On Fri, Jul 28, 2017 at 8:43 AM, Vinod Koul  wrote:
> On Thu, Jul 27, 2017 at 09:42:33AM +0530, Anup Patel wrote:
>> On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
>> > On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
>
>> >>  drivers/dma/bcm-sba-raid.c | 439 
>> >> +++--
>> >>  1 file changed, 226 insertions(+), 213 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> >> index e41bbc7..6d15fed 100644
>> >> --- a/drivers/dma/bcm-sba-raid.c
>> >> +++ b/drivers/dma/bcm-sba-raid.c
>> >> @@ -48,7 +48,8 @@
>> >>
>> >>  #include "dmaengine.h"
>> >>
>> >> -/* SBA command related defines */
>> >> +/* == Driver macros and defines = */
>> >
>> > why this noise, seems unrelated to the change!
>>
>> This is just minor beautification. Again, I will put this
>> in separate patch.
>
> Well you can't shove garlands under an unrelated change. By all means throw
> the whole garden out there, but please as a separate patch

Sure, I will have separate patch for this beautification.

>
>>
>> >
>> >> +
>> >>  #define SBA_TYPE_SHIFT   48
>> >>  #define SBA_TYPE_MASKGENMASK(1, 
>> >> 0)
>> >>  #define SBA_TYPE_A   0x0
>> >> @@ -82,39 +83,41 @@
>> >>  #define SBA_CMD_WRITE_BUFFER 0xc
>> >>  #define SBA_CMD_GALOIS   0xe
>> >>
>> >> -/* Driver helper macros */
>> >> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
>> >> +
>> >>  #define to_sba_request(tx)   \
>> >>   container_of(tx, struct sba_request, tx)
>> >>  #define to_sba_device(dchan) \
>> >>   container_of(dchan, struct sba_device, dma_chan)
>> >>
>> >> -enum sba_request_state {
>> >> - SBA_REQUEST_STATE_FREE = 1,
>> >> - SBA_REQUEST_STATE_ALLOCED = 2,
>> >> - SBA_REQUEST_STATE_PENDING = 3,
>> >> - SBA_REQUEST_STATE_ACTIVE = 4,
>> >> - SBA_REQUEST_STATE_RECEIVED = 5,
>> >> - SBA_REQUEST_STATE_COMPLETED = 6,
>> >> - SBA_REQUEST_STATE_ABORTED = 7,
>> >> +/* = Driver data structures = */
>> >> +
>> >> +enum sba_request_flags {
>> >> + SBA_REQUEST_STATE_FREE  = 0x001,
>> >> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
>> >> + SBA_REQUEST_STATE_PENDING   = 0x004,
>> >> + SBA_REQUEST_STATE_ACTIVE= 0x008,
>> >> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
>> >> + SBA_REQUEST_STATE_COMPLETED = 0x020,
>> >> + SBA_REQUEST_STATE_ABORTED   = 0x040,
>> >> + SBA_REQUEST_STATE_MASK  = 0x0ff,
>> >> + SBA_REQUEST_FENCE   = 0x100,
>> >
>> > how does this help in mem alloctn?
>
> ??

Ahh, I missed to address this comment.

Currently, we have separate "bool" flag for fenced
sba_request and separate "state" variable in
sba_request. We are have merged this two things
in common "u32 flags" in sba_request. In future,
we can use more bits in "u32 flags" as required
without disturbing the sba_request.

I will make this separate patch.

I agree, I have covered many changes in PATCH1
which makes it hard for you to review.

Thanks,
Anup

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Anup Patel]

On Fri, Jul 28, 2017 at 8:43 AM, Vinod Koul  wrote:
> On Thu, Jul 27, 2017 at 09:42:33AM +0530, Anup Patel wrote:
>> On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
>> > On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
>
>> >>  drivers/dma/bcm-sba-raid.c | 439 
>> >> +++--
>> >>  1 file changed, 226 insertions(+), 213 deletions(-)
>> >>
>> >> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> >> index e41bbc7..6d15fed 100644
>> >> --- a/drivers/dma/bcm-sba-raid.c
>> >> +++ b/drivers/dma/bcm-sba-raid.c
>> >> @@ -48,7 +48,8 @@
>> >>
>> >>  #include "dmaengine.h"
>> >>
>> >> -/* SBA command related defines */
>> >> +/* == Driver macros and defines = */
>> >
>> > why this noise, seems unrelated to the change!
>>
>> This is just minor beautification. Again, I will put this
>> in separate patch.
>
> Well you can't shove garlands under an unrelated change. By all means throw
> the whole garden out there, but please as a separate patch

Sure, I will have separate patch for this beautification.

>
>>
>> >
>> >> +
>> >>  #define SBA_TYPE_SHIFT   48
>> >>  #define SBA_TYPE_MASKGENMASK(1, 
>> >> 0)
>> >>  #define SBA_TYPE_A   0x0
>> >> @@ -82,39 +83,41 @@
>> >>  #define SBA_CMD_WRITE_BUFFER 0xc
>> >>  #define SBA_CMD_GALOIS   0xe
>> >>
>> >> -/* Driver helper macros */
>> >> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
>> >> +
>> >>  #define to_sba_request(tx)   \
>> >>   container_of(tx, struct sba_request, tx)
>> >>  #define to_sba_device(dchan) \
>> >>   container_of(dchan, struct sba_device, dma_chan)
>> >>
>> >> -enum sba_request_state {
>> >> - SBA_REQUEST_STATE_FREE = 1,
>> >> - SBA_REQUEST_STATE_ALLOCED = 2,
>> >> - SBA_REQUEST_STATE_PENDING = 3,
>> >> - SBA_REQUEST_STATE_ACTIVE = 4,
>> >> - SBA_REQUEST_STATE_RECEIVED = 5,
>> >> - SBA_REQUEST_STATE_COMPLETED = 6,
>> >> - SBA_REQUEST_STATE_ABORTED = 7,
>> >> +/* = Driver data structures = */
>> >> +
>> >> +enum sba_request_flags {
>> >> + SBA_REQUEST_STATE_FREE  = 0x001,
>> >> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
>> >> + SBA_REQUEST_STATE_PENDING   = 0x004,
>> >> + SBA_REQUEST_STATE_ACTIVE= 0x008,
>> >> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
>> >> + SBA_REQUEST_STATE_COMPLETED = 0x020,
>> >> + SBA_REQUEST_STATE_ABORTED   = 0x040,
>> >> + SBA_REQUEST_STATE_MASK  = 0x0ff,
>> >> + SBA_REQUEST_FENCE   = 0x100,
>> >
>> > how does this help in mem alloctn?
>
> ??

Ahh, I missed to address this comment.

Currently, we have separate "bool" flag for fenced
sba_request and separate "state" variable in
sba_request. We are have merged this two things
in common "u32 flags" in sba_request. In future,
we can use more bits in "u32 flags" as required
without disturbing the sba_request.

I will make this separate patch.

I agree, I have covered many changes in PATCH1
which makes it hard for you to review.

Thanks,
Anup

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Vinod Koul]

On Thu, Jul 27, 2017 at 09:42:33AM +0530, Anup Patel wrote:
> On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
> > On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:

> >>  drivers/dma/bcm-sba-raid.c | 439 
> >> +++--
> >>  1 file changed, 226 insertions(+), 213 deletions(-)
> >>
> >> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> >> index e41bbc7..6d15fed 100644
> >> --- a/drivers/dma/bcm-sba-raid.c
> >> +++ b/drivers/dma/bcm-sba-raid.c
> >> @@ -48,7 +48,8 @@
> >>
> >>  #include "dmaengine.h"
> >>
> >> -/* SBA command related defines */
> >> +/* == Driver macros and defines = */
> >
> > why this noise, seems unrelated to the change!
> 
> This is just minor beautification. Again, I will put this
> in separate patch.

Well you can't shove garlands under an unrelated change. By all means throw
the whole garden out there, but please as a separate patch

> 
> >
> >> +
> >>  #define SBA_TYPE_SHIFT   48
> >>  #define SBA_TYPE_MASKGENMASK(1, 0)
> >>  #define SBA_TYPE_A   0x0
> >> @@ -82,39 +83,41 @@
> >>  #define SBA_CMD_WRITE_BUFFER 0xc
> >>  #define SBA_CMD_GALOIS   0xe
> >>
> >> -/* Driver helper macros */
> >> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
> >> +
> >>  #define to_sba_request(tx)   \
> >>   container_of(tx, struct sba_request, tx)
> >>  #define to_sba_device(dchan) \
> >>   container_of(dchan, struct sba_device, dma_chan)
> >>
> >> -enum sba_request_state {
> >> - SBA_REQUEST_STATE_FREE = 1,
> >> - SBA_REQUEST_STATE_ALLOCED = 2,
> >> - SBA_REQUEST_STATE_PENDING = 3,
> >> - SBA_REQUEST_STATE_ACTIVE = 4,
> >> - SBA_REQUEST_STATE_RECEIVED = 5,
> >> - SBA_REQUEST_STATE_COMPLETED = 6,
> >> - SBA_REQUEST_STATE_ABORTED = 7,
> >> +/* = Driver data structures = */
> >> +
> >> +enum sba_request_flags {
> >> + SBA_REQUEST_STATE_FREE  = 0x001,
> >> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
> >> + SBA_REQUEST_STATE_PENDING   = 0x004,
> >> + SBA_REQUEST_STATE_ACTIVE= 0x008,
> >> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
> >> + SBA_REQUEST_STATE_COMPLETED = 0x020,
> >> + SBA_REQUEST_STATE_ABORTED   = 0x040,
> >> + SBA_REQUEST_STATE_MASK  = 0x0ff,
> >> + SBA_REQUEST_FENCE   = 0x100,
> >
> > how does this help in mem alloctn?

??

-- 
~Vinod

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Vinod Koul]

On Thu, Jul 27, 2017 at 09:42:33AM +0530, Anup Patel wrote:
> On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
> > On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:

> >>  drivers/dma/bcm-sba-raid.c | 439 
> >> +++--
> >>  1 file changed, 226 insertions(+), 213 deletions(-)
> >>
> >> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> >> index e41bbc7..6d15fed 100644
> >> --- a/drivers/dma/bcm-sba-raid.c
> >> +++ b/drivers/dma/bcm-sba-raid.c
> >> @@ -48,7 +48,8 @@
> >>
> >>  #include "dmaengine.h"
> >>
> >> -/* SBA command related defines */
> >> +/* == Driver macros and defines = */
> >
> > why this noise, seems unrelated to the change!
> 
> This is just minor beautification. Again, I will put this
> in separate patch.

Well you can't shove garlands under an unrelated change. By all means throw
the whole garden out there, but please as a separate patch

> 
> >
> >> +
> >>  #define SBA_TYPE_SHIFT   48
> >>  #define SBA_TYPE_MASKGENMASK(1, 0)
> >>  #define SBA_TYPE_A   0x0
> >> @@ -82,39 +83,41 @@
> >>  #define SBA_CMD_WRITE_BUFFER 0xc
> >>  #define SBA_CMD_GALOIS   0xe
> >>
> >> -/* Driver helper macros */
> >> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
> >> +
> >>  #define to_sba_request(tx)   \
> >>   container_of(tx, struct sba_request, tx)
> >>  #define to_sba_device(dchan) \
> >>   container_of(dchan, struct sba_device, dma_chan)
> >>
> >> -enum sba_request_state {
> >> - SBA_REQUEST_STATE_FREE = 1,
> >> - SBA_REQUEST_STATE_ALLOCED = 2,
> >> - SBA_REQUEST_STATE_PENDING = 3,
> >> - SBA_REQUEST_STATE_ACTIVE = 4,
> >> - SBA_REQUEST_STATE_RECEIVED = 5,
> >> - SBA_REQUEST_STATE_COMPLETED = 6,
> >> - SBA_REQUEST_STATE_ABORTED = 7,
> >> +/* = Driver data structures = */
> >> +
> >> +enum sba_request_flags {
> >> + SBA_REQUEST_STATE_FREE  = 0x001,
> >> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
> >> + SBA_REQUEST_STATE_PENDING   = 0x004,
> >> + SBA_REQUEST_STATE_ACTIVE= 0x008,
> >> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
> >> + SBA_REQUEST_STATE_COMPLETED = 0x020,
> >> + SBA_REQUEST_STATE_ABORTED   = 0x040,
> >> + SBA_REQUEST_STATE_MASK  = 0x0ff,
> >> + SBA_REQUEST_FENCE   = 0x100,
> >
> > how does this help in mem alloctn?

??

-- 
~Vinod

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Anup Patel]

On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
> On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
>> This patch improves memory allocation in SBA RAID driver in
>> following ways:
>> 1. Simplify struct sba_request to reduce memory consumption
>
> what is the simplification?? You need to document that

OK, will make it a separate patch with detailed commit description.

>
>> 2. Allocate sba resources before registering dma device
>
> what is the motivation for that
>
> So, reading this log doesnt help me to know what to expect in this patch

OK, this also requires separate patch with detailed commit description.

>
>>
>> Signed-off-by: Anup Patel 
>> Reviewed-by: Scott Branden 
>> Reviewed-by: Ray Jui 
>> Reviewed-by: Vikram Prakash 
>> ---
>>  drivers/dma/bcm-sba-raid.c | 439 
>> +++--
>>  1 file changed, 226 insertions(+), 213 deletions(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index e41bbc7..6d15fed 100644
>> --- a/drivers/dma/bcm-sba-raid.c
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -48,7 +48,8 @@
>>
>>  #include "dmaengine.h"
>>
>> -/* SBA command related defines */
>> +/* == Driver macros and defines = */
>
> why this noise, seems unrelated to the change!

This is just minor beautification. Again, I will put this
in separate patch.

>
>> +
>>  #define SBA_TYPE_SHIFT   48
>>  #define SBA_TYPE_MASKGENMASK(1, 0)
>>  #define SBA_TYPE_A   0x0
>> @@ -82,39 +83,41 @@
>>  #define SBA_CMD_WRITE_BUFFER 0xc
>>  #define SBA_CMD_GALOIS   0xe
>>
>> -/* Driver helper macros */
>> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
>> +
>>  #define to_sba_request(tx)   \
>>   container_of(tx, struct sba_request, tx)
>>  #define to_sba_device(dchan) \
>>   container_of(dchan, struct sba_device, dma_chan)
>>
>> -enum sba_request_state {
>> - SBA_REQUEST_STATE_FREE = 1,
>> - SBA_REQUEST_STATE_ALLOCED = 2,
>> - SBA_REQUEST_STATE_PENDING = 3,
>> - SBA_REQUEST_STATE_ACTIVE = 4,
>> - SBA_REQUEST_STATE_RECEIVED = 5,
>> - SBA_REQUEST_STATE_COMPLETED = 6,
>> - SBA_REQUEST_STATE_ABORTED = 7,
>> +/* = Driver data structures = */
>> +
>> +enum sba_request_flags {
>> + SBA_REQUEST_STATE_FREE  = 0x001,
>> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
>> + SBA_REQUEST_STATE_PENDING   = 0x004,
>> + SBA_REQUEST_STATE_ACTIVE= 0x008,
>> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
>> + SBA_REQUEST_STATE_COMPLETED = 0x020,
>> + SBA_REQUEST_STATE_ABORTED   = 0x040,
>> + SBA_REQUEST_STATE_MASK  = 0x0ff,
>> + SBA_REQUEST_FENCE   = 0x100,
>
> how does this help in mem alloctn?
>
>>  };
>>
>>  struct sba_request {
>>   /* Global state */
>>   struct list_head node;
>>   struct sba_device *sba;
>> - enum sba_request_state state;
>> - bool fence;
>> + u32 flags;
>>   /* Chained requests management */
>>   struct sba_request *first;
>>   struct list_head next;
>> - unsigned int next_count;
>>   atomic_t next_pending_count;
>>   /* BRCM message data */
>> - void *resp;
>> - dma_addr_t resp_dma;
>> - struct brcm_sba_command *cmds;
>>   struct brcm_message msg;
>>   struct dma_async_tx_descriptor tx;
>> + /* SBA commands */
>> + struct brcm_sba_command cmds[0];
>>  };
>>
>>  enum sba_version {
>> @@ -128,11 +131,11 @@ struct sba_device {
>>   /* DT configuration parameters */
>>   enum sba_version ver;
>>   /* Derived configuration parameters */
>> - u32 max_req;
>>   u32 hw_buf_size;
>>   u32 hw_resp_size;
>>   u32 max_pq_coefs;
>>   u32 max_pq_srcs;
>> + u32 max_req;
>>   u32 max_cmd_per_req;
>>   u32 max_xor_srcs;
>>   u32 max_resp_pool_size;
>> @@ -152,7 +155,6 @@ struct sba_device {
>>   void *cmds_base;
>>   dma_addr_t cmds_dma_base;
>>   spinlock_t reqs_lock;
>> - struct sba_request *reqs;
>>   bool reqs_fence;
>>   struct list_head reqs_alloc_list;
>>   struct list_head reqs_pending_list;
>> @@ -161,10 +163,9 @@ struct sba_device {
>>   struct list_head reqs_completed_list;
>>   struct list_head reqs_aborted_list;
>>   struct list_head reqs_free_list;
>> - int reqs_free_count;
>>  };
>>
>> -/* == SBA command helper routines = */
>> +/* == Command helper routines = */
>
> more noise..
>
>>
>>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>>  {
>> @@ -196,7 +197,7 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 
>> b1, u32 b0)
>>  ((d & SBA_C_MDATA_DNUM_MASK) << 

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Anup Patel]

On Wed, Jul 26, 2017 at 10:39 PM, Vinod Koul  wrote:
> On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
>> This patch improves memory allocation in SBA RAID driver in
>> following ways:
>> 1. Simplify struct sba_request to reduce memory consumption
>
> what is the simplification?? You need to document that

OK, will make it a separate patch with detailed commit description.

>
>> 2. Allocate sba resources before registering dma device
>
> what is the motivation for that
>
> So, reading this log doesnt help me to know what to expect in this patch

OK, this also requires separate patch with detailed commit description.

>
>>
>> Signed-off-by: Anup Patel 
>> Reviewed-by: Scott Branden 
>> Reviewed-by: Ray Jui 
>> Reviewed-by: Vikram Prakash 
>> ---
>>  drivers/dma/bcm-sba-raid.c | 439 
>> +++--
>>  1 file changed, 226 insertions(+), 213 deletions(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index e41bbc7..6d15fed 100644
>> --- a/drivers/dma/bcm-sba-raid.c
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -48,7 +48,8 @@
>>
>>  #include "dmaengine.h"
>>
>> -/* SBA command related defines */
>> +/* == Driver macros and defines = */
>
> why this noise, seems unrelated to the change!

This is just minor beautification. Again, I will put this
in separate patch.

>
>> +
>>  #define SBA_TYPE_SHIFT   48
>>  #define SBA_TYPE_MASKGENMASK(1, 0)
>>  #define SBA_TYPE_A   0x0
>> @@ -82,39 +83,41 @@
>>  #define SBA_CMD_WRITE_BUFFER 0xc
>>  #define SBA_CMD_GALOIS   0xe
>>
>> -/* Driver helper macros */
>> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
>> +
>>  #define to_sba_request(tx)   \
>>   container_of(tx, struct sba_request, tx)
>>  #define to_sba_device(dchan) \
>>   container_of(dchan, struct sba_device, dma_chan)
>>
>> -enum sba_request_state {
>> - SBA_REQUEST_STATE_FREE = 1,
>> - SBA_REQUEST_STATE_ALLOCED = 2,
>> - SBA_REQUEST_STATE_PENDING = 3,
>> - SBA_REQUEST_STATE_ACTIVE = 4,
>> - SBA_REQUEST_STATE_RECEIVED = 5,
>> - SBA_REQUEST_STATE_COMPLETED = 6,
>> - SBA_REQUEST_STATE_ABORTED = 7,
>> +/* = Driver data structures = */
>> +
>> +enum sba_request_flags {
>> + SBA_REQUEST_STATE_FREE  = 0x001,
>> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
>> + SBA_REQUEST_STATE_PENDING   = 0x004,
>> + SBA_REQUEST_STATE_ACTIVE= 0x008,
>> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
>> + SBA_REQUEST_STATE_COMPLETED = 0x020,
>> + SBA_REQUEST_STATE_ABORTED   = 0x040,
>> + SBA_REQUEST_STATE_MASK  = 0x0ff,
>> + SBA_REQUEST_FENCE   = 0x100,
>
> how does this help in mem alloctn?
>
>>  };
>>
>>  struct sba_request {
>>   /* Global state */
>>   struct list_head node;
>>   struct sba_device *sba;
>> - enum sba_request_state state;
>> - bool fence;
>> + u32 flags;
>>   /* Chained requests management */
>>   struct sba_request *first;
>>   struct list_head next;
>> - unsigned int next_count;
>>   atomic_t next_pending_count;
>>   /* BRCM message data */
>> - void *resp;
>> - dma_addr_t resp_dma;
>> - struct brcm_sba_command *cmds;
>>   struct brcm_message msg;
>>   struct dma_async_tx_descriptor tx;
>> + /* SBA commands */
>> + struct brcm_sba_command cmds[0];
>>  };
>>
>>  enum sba_version {
>> @@ -128,11 +131,11 @@ struct sba_device {
>>   /* DT configuration parameters */
>>   enum sba_version ver;
>>   /* Derived configuration parameters */
>> - u32 max_req;
>>   u32 hw_buf_size;
>>   u32 hw_resp_size;
>>   u32 max_pq_coefs;
>>   u32 max_pq_srcs;
>> + u32 max_req;
>>   u32 max_cmd_per_req;
>>   u32 max_xor_srcs;
>>   u32 max_resp_pool_size;
>> @@ -152,7 +155,6 @@ struct sba_device {
>>   void *cmds_base;
>>   dma_addr_t cmds_dma_base;
>>   spinlock_t reqs_lock;
>> - struct sba_request *reqs;
>>   bool reqs_fence;
>>   struct list_head reqs_alloc_list;
>>   struct list_head reqs_pending_list;
>> @@ -161,10 +163,9 @@ struct sba_device {
>>   struct list_head reqs_completed_list;
>>   struct list_head reqs_aborted_list;
>>   struct list_head reqs_free_list;
>> - int reqs_free_count;
>>  };
>>
>> -/* == SBA command helper routines = */
>> +/* == Command helper routines = */
>
> more noise..
>
>>
>>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>>  {
>> @@ -196,7 +197,7 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 
>> b1, u32 b0)
>>  ((d & SBA_C_MDATA_DNUM_MASK) << SBA_C_MDATA_DNUM_SHIFT);
>>  }
>>
>> -/* == Channel resource management routines = */
>> +/* == General helper routines = */

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Vinod Koul]

On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
> This patch improves memory allocation in SBA RAID driver in
> following ways:
> 1. Simplify struct sba_request to reduce memory consumption

what is the simplification?? You need to document that

> 2. Allocate sba resources before registering dma device

what is the motivation for that

So, reading this log doesnt help me to know what to expect in this patch

> 
> Signed-off-by: Anup Patel 
> Reviewed-by: Scott Branden 
> Reviewed-by: Ray Jui 
> Reviewed-by: Vikram Prakash 
> ---
>  drivers/dma/bcm-sba-raid.c | 439 
> +++--
>  1 file changed, 226 insertions(+), 213 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index e41bbc7..6d15fed 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -48,7 +48,8 @@
>  
>  #include "dmaengine.h"
>  
> -/* SBA command related defines */
> +/* == Driver macros and defines = */

why this noise, seems unrelated to the change!

> +
>  #define SBA_TYPE_SHIFT   48
>  #define SBA_TYPE_MASKGENMASK(1, 0)
>  #define SBA_TYPE_A   0x0
> @@ -82,39 +83,41 @@
>  #define SBA_CMD_WRITE_BUFFER 0xc
>  #define SBA_CMD_GALOIS   0xe
>  
> -/* Driver helper macros */
> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
> +
>  #define to_sba_request(tx)   \
>   container_of(tx, struct sba_request, tx)
>  #define to_sba_device(dchan) \
>   container_of(dchan, struct sba_device, dma_chan)
>  
> -enum sba_request_state {
> - SBA_REQUEST_STATE_FREE = 1,
> - SBA_REQUEST_STATE_ALLOCED = 2,
> - SBA_REQUEST_STATE_PENDING = 3,
> - SBA_REQUEST_STATE_ACTIVE = 4,
> - SBA_REQUEST_STATE_RECEIVED = 5,
> - SBA_REQUEST_STATE_COMPLETED = 6,
> - SBA_REQUEST_STATE_ABORTED = 7,
> +/* = Driver data structures = */
> +
> +enum sba_request_flags {
> + SBA_REQUEST_STATE_FREE  = 0x001,
> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
> + SBA_REQUEST_STATE_PENDING   = 0x004,
> + SBA_REQUEST_STATE_ACTIVE= 0x008,
> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
> + SBA_REQUEST_STATE_COMPLETED = 0x020,
> + SBA_REQUEST_STATE_ABORTED   = 0x040,
> + SBA_REQUEST_STATE_MASK  = 0x0ff,
> + SBA_REQUEST_FENCE   = 0x100,

how does this help in mem alloctn?

>  };
>  
>  struct sba_request {
>   /* Global state */
>   struct list_head node;
>   struct sba_device *sba;
> - enum sba_request_state state;
> - bool fence;
> + u32 flags;
>   /* Chained requests management */
>   struct sba_request *first;
>   struct list_head next;
> - unsigned int next_count;
>   atomic_t next_pending_count;
>   /* BRCM message data */
> - void *resp;
> - dma_addr_t resp_dma;
> - struct brcm_sba_command *cmds;
>   struct brcm_message msg;
>   struct dma_async_tx_descriptor tx;
> + /* SBA commands */
> + struct brcm_sba_command cmds[0];
>  };
>  
>  enum sba_version {
> @@ -128,11 +131,11 @@ struct sba_device {
>   /* DT configuration parameters */
>   enum sba_version ver;
>   /* Derived configuration parameters */
> - u32 max_req;
>   u32 hw_buf_size;
>   u32 hw_resp_size;
>   u32 max_pq_coefs;
>   u32 max_pq_srcs;
> + u32 max_req;
>   u32 max_cmd_per_req;
>   u32 max_xor_srcs;
>   u32 max_resp_pool_size;
> @@ -152,7 +155,6 @@ struct sba_device {
>   void *cmds_base;
>   dma_addr_t cmds_dma_base;
>   spinlock_t reqs_lock;
> - struct sba_request *reqs;
>   bool reqs_fence;
>   struct list_head reqs_alloc_list;
>   struct list_head reqs_pending_list;
> @@ -161,10 +163,9 @@ struct sba_device {
>   struct list_head reqs_completed_list;
>   struct list_head reqs_aborted_list;
>   struct list_head reqs_free_list;
> - int reqs_free_count;
>  };
>  
> -/* == SBA command helper routines = */
> +/* == Command helper routines = */

more noise..

>  
>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>  {
> @@ -196,7 +197,7 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 
> b1, u32 b0)
>  ((d & SBA_C_MDATA_DNUM_MASK) << SBA_C_MDATA_DNUM_SHIFT);
>  }
>  
> -/* == Channel resource management routines = */
> +/* == General helper routines = */

and it keeps getting more interesting, sigh!!!

>  
>  static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  {
> @@ -204,24 +205,20 @@ static struct sba_request *sba_alloc_request(struct 
> sba_device *sba)
>   struct sba_request *req = NULL;
>  
>   

Re: [PATCH 1/6] dma: bcm-sba-raid: Improve memory allocation in SBA RAID driver

[Vinod Koul]

On Wed, Jul 26, 2017 at 11:06:39AM +0530, Anup Patel wrote:
> This patch improves memory allocation in SBA RAID driver in
> following ways:
> 1. Simplify struct sba_request to reduce memory consumption

what is the simplification?? You need to document that

> 2. Allocate sba resources before registering dma device

what is the motivation for that

So, reading this log doesnt help me to know what to expect in this patch

> 
> Signed-off-by: Anup Patel 
> Reviewed-by: Scott Branden 
> Reviewed-by: Ray Jui 
> Reviewed-by: Vikram Prakash 
> ---
>  drivers/dma/bcm-sba-raid.c | 439 
> +++--
>  1 file changed, 226 insertions(+), 213 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index e41bbc7..6d15fed 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -48,7 +48,8 @@
>  
>  #include "dmaengine.h"
>  
> -/* SBA command related defines */
> +/* == Driver macros and defines = */

why this noise, seems unrelated to the change!

> +
>  #define SBA_TYPE_SHIFT   48
>  #define SBA_TYPE_MASKGENMASK(1, 0)
>  #define SBA_TYPE_A   0x0
> @@ -82,39 +83,41 @@
>  #define SBA_CMD_WRITE_BUFFER 0xc
>  #define SBA_CMD_GALOIS   0xe
>  
> -/* Driver helper macros */
> +#define SBA_MAX_REQ_PER_MBOX_CHANNEL 8192
> +
>  #define to_sba_request(tx)   \
>   container_of(tx, struct sba_request, tx)
>  #define to_sba_device(dchan) \
>   container_of(dchan, struct sba_device, dma_chan)
>  
> -enum sba_request_state {
> - SBA_REQUEST_STATE_FREE = 1,
> - SBA_REQUEST_STATE_ALLOCED = 2,
> - SBA_REQUEST_STATE_PENDING = 3,
> - SBA_REQUEST_STATE_ACTIVE = 4,
> - SBA_REQUEST_STATE_RECEIVED = 5,
> - SBA_REQUEST_STATE_COMPLETED = 6,
> - SBA_REQUEST_STATE_ABORTED = 7,
> +/* = Driver data structures = */
> +
> +enum sba_request_flags {
> + SBA_REQUEST_STATE_FREE  = 0x001,
> + SBA_REQUEST_STATE_ALLOCED   = 0x002,
> + SBA_REQUEST_STATE_PENDING   = 0x004,
> + SBA_REQUEST_STATE_ACTIVE= 0x008,
> + SBA_REQUEST_STATE_RECEIVED  = 0x010,
> + SBA_REQUEST_STATE_COMPLETED = 0x020,
> + SBA_REQUEST_STATE_ABORTED   = 0x040,
> + SBA_REQUEST_STATE_MASK  = 0x0ff,
> + SBA_REQUEST_FENCE   = 0x100,

how does this help in mem alloctn?

>  };
>  
>  struct sba_request {
>   /* Global state */
>   struct list_head node;
>   struct sba_device *sba;
> - enum sba_request_state state;
> - bool fence;
> + u32 flags;
>   /* Chained requests management */
>   struct sba_request *first;
>   struct list_head next;
> - unsigned int next_count;
>   atomic_t next_pending_count;
>   /* BRCM message data */
> - void *resp;
> - dma_addr_t resp_dma;
> - struct brcm_sba_command *cmds;
>   struct brcm_message msg;
>   struct dma_async_tx_descriptor tx;
> + /* SBA commands */
> + struct brcm_sba_command cmds[0];
>  };
>  
>  enum sba_version {
> @@ -128,11 +131,11 @@ struct sba_device {
>   /* DT configuration parameters */
>   enum sba_version ver;
>   /* Derived configuration parameters */
> - u32 max_req;
>   u32 hw_buf_size;
>   u32 hw_resp_size;
>   u32 max_pq_coefs;
>   u32 max_pq_srcs;
> + u32 max_req;
>   u32 max_cmd_per_req;
>   u32 max_xor_srcs;
>   u32 max_resp_pool_size;
> @@ -152,7 +155,6 @@ struct sba_device {
>   void *cmds_base;
>   dma_addr_t cmds_dma_base;
>   spinlock_t reqs_lock;
> - struct sba_request *reqs;
>   bool reqs_fence;
>   struct list_head reqs_alloc_list;
>   struct list_head reqs_pending_list;
> @@ -161,10 +163,9 @@ struct sba_device {
>   struct list_head reqs_completed_list;
>   struct list_head reqs_aborted_list;
>   struct list_head reqs_free_list;
> - int reqs_free_count;
>  };
>  
> -/* == SBA command helper routines = */
> +/* == Command helper routines = */

more noise..

>  
>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>  {
> @@ -196,7 +197,7 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 
> b1, u32 b0)
>  ((d & SBA_C_MDATA_DNUM_MASK) << SBA_C_MDATA_DNUM_SHIFT);
>  }
>  
> -/* == Channel resource management routines = */
> +/* == General helper routines = */

and it keeps getting more interesting, sigh!!!

>  
>  static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  {
> @@ -204,24 +205,20 @@ static struct sba_request *sba_alloc_request(struct 
> sba_device *sba)
>   struct sba_request *req = NULL;
>  
>   spin_lock_irqsave(>reqs_lock, flags);
> -
>   req = list_first_entry_or_null(>reqs_free_list,
>