[PATCH v2 00/16] Broadcom SBA-RAID driver improvements

[PATCH v2 00/16] Broadcom SBA-RAID driver improvements

[Anup Patel]

This patchset does various improvments to Broadcom SBA-RAID
driver.

The patches are based on Linux-4.13-rc3 and can also be found
at sba-raid-imp-v2 branch of
https://github.com/Broadcom/arm64-linux.git

Changes since v1:
 - Split PATCH1 into 10 smaller patches for easier review
 - Added more info to commit description of PATCH2
 - Split PATCH4 into 2 patches for easier review
 - Dropped PATCH6 because it is now part of
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1456425.html

Anup Patel (16):
  dmaengine: bcm-sba-raid: Minor improvments in comments
  dmaengine: bcm-sba-raid: Reduce locking context in sba_alloc_request()
  dmaengine: bcm-sba-raid: Common flags for sba_request state and fence
  dmaengine: bcm-sba-raid: Remove redundant next_count from sba_request
  dmaengine: bcm-sba-raid: Remove redundant resp_dma from sba_request
  dmaengine: bcm-sba-raid: Remove reqs_free_count from sba_device
  dmaengine: bcm-sba-raid: Allow arbitrary number free sba_request
  dmaengine: bcm-sba-raid: Increase number of free sba_request
  dmaengine: bcm-sba-raid: Improve sba_issue_pending() run duration
  dmaengine: bcm-sba-raid: Alloc resources before registering DMA device
  dmaengine: bcm-sba-raid: Peek mbox when we have no free requests
  dmaengine: bcm-sba-raid: Pre-ack async tx descriptor
  dmaengine: bcm-sba-raid: Re-factor sba_process_deferred_requests()
  dmaengine: bcm-sba-raid: Remove redundant SBA_REQUEST_STATE_RECEIVED
  dmaengine: bcm-sba-raid: Add debugfs support
  dmaengine: bcm-sba-raid: Explicitly ACK mailbox message after sending

 drivers/dma/bcm-sba-raid.c | 539 ++++++++++++++++++++++++++-------------------
 1 file changed, 308 insertions(+), 231 deletions(-)

-- 
2.7.4

[PATCH v2 01/16] dmaengine: bcm-sba-raid: Minor improvments in comments

[Anup Patel]

Make section comments consistent across the Broadcom SBA RAID driver
by avoiding " SBA " in some of the comments.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index e41bbc7..76999b7 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 ===== */
+
 #define SBA_TYPE_SHIFT					48
 #define SBA_TYPE_MASK					GENMASK(1, 0)
 #define SBA_TYPE_A					0x0
@@ -88,6 +89,8 @@
 #define to_sba_device(dchan)		\
 	container_of(dchan, struct sba_device, dma_chan)
 
+/* ===== Driver data structures ===== */
+
 enum sba_request_state {
 	SBA_REQUEST_STATE_FREE = 1,
 	SBA_REQUEST_STATE_ALLOCED = 2,
@@ -164,7 +167,7 @@ struct sba_device {
 	int reqs_free_count;
 };
 
-/* ====== SBA command helper routines ===== */
+/* ====== Command helper routines ===== */
 
 static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
 {
@@ -196,7 +199,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 ===== */
 
 static struct sba_request *sba_alloc_request(struct sba_device *sba)
 {
-- 
2.7.4

Re: [PATCH v2 01/16] dmaengine: bcm-sba-raid: Minor improvments in comments

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:45PM +0530, Anup Patel wrote:
> Make section comments consistent across the Broadcom SBA RAID driver
> by avoiding " SBA " in some of the comments.

and you add more comments..

> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index e41bbc7..76999b7 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 ===== */
> +
>  #define SBA_TYPE_SHIFT					48
>  #define SBA_TYPE_MASK					GENMASK(1, 0)
>  #define SBA_TYPE_A					0x0
> @@ -88,6 +89,8 @@
>  #define to_sba_device(dchan)		\
>  	container_of(dchan, struct sba_device, dma_chan)
>  
> +/* ===== Driver data structures ===== */

like this!!

> +
>  enum sba_request_state {
>  	SBA_REQUEST_STATE_FREE = 1,
>  	SBA_REQUEST_STATE_ALLOCED = 2,
> @@ -164,7 +167,7 @@ struct sba_device {
>  	int reqs_free_count;
>  };
>  
> -/* ====== SBA command helper routines ===== */
> +/* ====== Command helper routines ===== */
>  
>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>  {
> @@ -196,7 +199,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 ===== */
>  
>  static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  {
> -- 
> 2.7.4
> 

-- 
~Vinod

Re: [PATCH v2 01/16] dmaengine: bcm-sba-raid: Minor improvments in comments

[Anup Patel]

On Thu, Aug 17, 2017 at 9:14 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:45PM +0530, Anup Patel wrote:
>> Make section comments consistent across the Broadcom SBA RAID driver
>> by avoiding " SBA " in some of the comments.
>
> and you add more comments..

OK, I will add this to commit description.

>
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> ---
>>  drivers/dma/bcm-sba-raid.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index e41bbc7..76999b7 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 ===== */
>> +
>>  #define SBA_TYPE_SHIFT                                       48
>>  #define SBA_TYPE_MASK                                        GENMASK(1, 0)
>>  #define SBA_TYPE_A                                   0x0
>> @@ -88,6 +89,8 @@
>>  #define to_sba_device(dchan)         \
>>       container_of(dchan, struct sba_device, dma_chan)
>>
>> +/* ===== Driver data structures ===== */
>
> like this!!
>
>> +
>>  enum sba_request_state {
>>       SBA_REQUEST_STATE_FREE = 1,
>>       SBA_REQUEST_STATE_ALLOCED = 2,
>> @@ -164,7 +167,7 @@ struct sba_device {
>>       int reqs_free_count;
>>  };
>>
>> -/* ====== SBA command helper routines ===== */
>> +/* ====== Command helper routines ===== */
>>
>>  static inline u64 __pure sba_cmd_enc(u64 cmd, u32 val, u32 shift, u32 mask)
>>  {
>> @@ -196,7 +199,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 ===== */
>>

Regards,
Anup

[PATCH v2 02/16] dmaengine: bcm-sba-raid: Reduce locking context in sba_alloc_request()

[Anup Patel]

We don't require to hold "sba->reqs_lock" for long-time
in sba_alloc_request() because lock protection is not
required when initializing members of "struct sba_request".

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 76999b7..f81d5ac 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -207,24 +207,24 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	struct sba_request *req = NULL;
 
 	spin_lock_irqsave(&sba->reqs_lock, flags);
-
 	req = list_first_entry_or_null(&sba->reqs_free_list,
 				       struct sba_request, node);
 	if (req) {
 		list_move_tail(&req->node, &sba->reqs_alloc_list);
-		req->state = SBA_REQUEST_STATE_ALLOCED;
-		req->fence = false;
-		req->first = req;
-		INIT_LIST_HEAD(&req->next);
-		req->next_count = 1;
-		atomic_set(&req->next_pending_count, 1);
-
 		sba->reqs_free_count--;
-
-		dma_async_tx_descriptor_init(&req->tx, &sba->dma_chan);
 	}
-
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+	if (!req)
+		return NULL;
+
+	req->state = SBA_REQUEST_STATE_ALLOCED;
+	req->fence = false;
+	req->first = req;
+	INIT_LIST_HEAD(&req->next);
+	req->next_count = 1;
+	atomic_set(&req->next_pending_count, 1);
+
+	dma_async_tx_descriptor_init(&req->tx, &sba->dma_chan);
 
 	return req;
 }
-- 
2.7.4

[PATCH v2 03/16] dmaengine: bcm-sba-raid: Common flags for sba_request state and fence

[Anup Patel]

This patch merges sba_request state and fence into common
sba_request flags. Also, in-future we can extend sba_request
flags as required.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 66 ++++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index f81d5ac..6fa3df1 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -91,22 +91,23 @@
 
 /* ===== Driver data structures ===== */
 
-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,
+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,
 };
 
 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;
@@ -217,8 +218,7 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	if (!req)
 		return NULL;
 
-	req->state = SBA_REQUEST_STATE_ALLOCED;
-	req->fence = false;
+	req->flags = SBA_REQUEST_STATE_ALLOCED;
 	req->first = req;
 	INIT_LIST_HEAD(&req->next);
 	req->next_count = 1;
@@ -234,7 +234,8 @@ static void _sba_pending_request(struct sba_device *sba,
 				 struct sba_request *req)
 {
 	lockdep_assert_held(&sba->reqs_lock);
-	req->state = SBA_REQUEST_STATE_PENDING;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_PENDING;
 	list_move_tail(&req->node, &sba->reqs_pending_list);
 	if (list_empty(&sba->reqs_active_list))
 		sba->reqs_fence = false;
@@ -249,9 +250,10 @@ static bool _sba_active_request(struct sba_device *sba,
 		sba->reqs_fence = false;
 	if (sba->reqs_fence)
 		return false;
-	req->state = SBA_REQUEST_STATE_ACTIVE;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_ACTIVE;
 	list_move_tail(&req->node, &sba->reqs_active_list);
-	if (req->fence)
+	if (req->flags & SBA_REQUEST_FENCE)
 		sba->reqs_fence = true;
 	return true;
 }
@@ -261,7 +263,8 @@ static void _sba_abort_request(struct sba_device *sba,
 			       struct sba_request *req)
 {
 	lockdep_assert_held(&sba->reqs_lock);
-	req->state = SBA_REQUEST_STATE_ABORTED;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_ABORTED;
 	list_move_tail(&req->node, &sba->reqs_aborted_list);
 	if (list_empty(&sba->reqs_active_list))
 		sba->reqs_fence = false;
@@ -272,7 +275,8 @@ static void _sba_free_request(struct sba_device *sba,
 			      struct sba_request *req)
 {
 	lockdep_assert_held(&sba->reqs_lock);
-	req->state = SBA_REQUEST_STATE_FREE;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_FREE;
 	list_move_tail(&req->node, &sba->reqs_free_list);
 	if (list_empty(&sba->reqs_active_list))
 		sba->reqs_fence = false;
@@ -285,7 +289,8 @@ static void sba_received_request(struct sba_request *req)
 	struct sba_device *sba = req->sba;
 
 	spin_lock_irqsave(&sba->reqs_lock, flags);
-	req->state = SBA_REQUEST_STATE_RECEIVED;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_RECEIVED;
 	list_move_tail(&req->node, &sba->reqs_received_list);
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
@@ -298,10 +303,12 @@ static void sba_complete_chained_requests(struct sba_request *req)
 
 	spin_lock_irqsave(&sba->reqs_lock, flags);
 
-	req->state = SBA_REQUEST_STATE_COMPLETED;
+	req->flags &= ~SBA_REQUEST_STATE_MASK;
+	req->flags |= SBA_REQUEST_STATE_COMPLETED;
 	list_move_tail(&req->node, &sba->reqs_completed_list);
 	list_for_each_entry(nreq, &req->next, next) {
-		nreq->state = SBA_REQUEST_STATE_COMPLETED;
+		nreq->flags &= ~SBA_REQUEST_STATE_MASK;
+		nreq->flags |= SBA_REQUEST_STATE_COMPLETED;
 		list_move_tail(&nreq->node, &sba->reqs_completed_list);
 	}
 	if (list_empty(&sba->reqs_active_list))
@@ -576,7 +583,7 @@ sba_prep_dma_interrupt(struct dma_chan *dchan, unsigned long flags)
 	 * Force fence so that no requests are submitted
 	 * until DMA callback for this request is invoked.
 	 */
-	req->fence = true;
+	req->flags |= SBA_REQUEST_FENCE;
 
 	/* Fillup request message */
 	sba_fillup_interrupt_msg(req, req->cmds, &req->msg);
@@ -659,7 +666,8 @@ sba_prep_dma_memcpy_req(struct sba_device *sba,
 	req = sba_alloc_request(sba);
 	if (!req)
 		return NULL;
-	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
+	if (flags & DMA_PREP_FENCE)
+		req->flags |= SBA_REQUEST_FENCE;
 
 	/* Fillup request message */
 	sba_fillup_memcpy_msg(req, req->cmds, &req->msg,
@@ -796,7 +804,8 @@ sba_prep_dma_xor_req(struct sba_device *sba,
 	req = sba_alloc_request(sba);
 	if (!req)
 		return NULL;
-	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
+	if (flags & DMA_PREP_FENCE)
+		req->flags |= SBA_REQUEST_FENCE;
 
 	/* Fillup request message */
 	sba_fillup_xor_msg(req, req->cmds, &req->msg,
@@ -1005,7 +1014,8 @@ sba_prep_dma_pq_req(struct sba_device *sba, dma_addr_t off,
 	req = sba_alloc_request(sba);
 	if (!req)
 		return NULL;
-	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
+	if (flags & DMA_PREP_FENCE)
+		req->flags |= SBA_REQUEST_FENCE;
 
 	/* Fillup request messages */
 	sba_fillup_pq_msg(req, dmaf_continue(flags),
@@ -1258,7 +1268,8 @@ sba_prep_dma_pq_single_req(struct sba_device *sba, dma_addr_t off,
 	req = sba_alloc_request(sba);
 	if (!req)
 		return NULL;
-	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
+	if (flags & DMA_PREP_FENCE)
+		req->flags |= SBA_REQUEST_FENCE;
 
 	/* Fillup request messages */
 	sba_fillup_pq_single_msg(req,  dmaf_continue(flags),
@@ -1425,7 +1436,7 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
 	req = req->first;
 
 	/* Update request */
-	if (req->state == SBA_REQUEST_STATE_RECEIVED)
+	if (req->flags & SBA_REQUEST_STATE_RECEIVED)
 		sba_dma_tx_actions(req);
 	else
 		sba_free_chained_requests(req);
@@ -1488,11 +1499,10 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 		req = &sba->reqs[i];
 		INIT_LIST_HEAD(&req->node);
 		req->sba = sba;
-		req->state = SBA_REQUEST_STATE_FREE;
+		req->flags = SBA_REQUEST_STATE_FREE;
 		INIT_LIST_HEAD(&req->next);
 		req->next_count = 1;
 		atomic_set(&req->next_pending_count, 0);
-		req->fence = false;
 		req->resp = sba->resp_base + p;
 		req->resp_dma = sba->resp_dma_base + p;
 		p += sba->hw_resp_size;
-- 
2.7.4

Re: [PATCH v2 03/16] dmaengine: bcm-sba-raid: Common flags for sba_request state and fence

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:47PM +0530, Anup Patel wrote:
> This patch merges sba_request state and fence into common
> sba_request flags. Also, in-future we can extend sba_request
> flags as required.

and it also changes the flag values to bits, which I have no idea why that
was done, care to explain that please...

> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 66 ++++++++++++++++++++++++++--------------------
>  1 file changed, 38 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index f81d5ac..6fa3df1 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -91,22 +91,23 @@
>  
>  /* ===== Driver data structures ===== */
>  
> -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,
> +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,
>  };
>  
>  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;
> @@ -217,8 +218,7 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  	if (!req)
>  		return NULL;
>  
> -	req->state = SBA_REQUEST_STATE_ALLOCED;
> -	req->fence = false;
> +	req->flags = SBA_REQUEST_STATE_ALLOCED;
>  	req->first = req;
>  	INIT_LIST_HEAD(&req->next);
>  	req->next_count = 1;
> @@ -234,7 +234,8 @@ static void _sba_pending_request(struct sba_device *sba,
>  				 struct sba_request *req)
>  {
>  	lockdep_assert_held(&sba->reqs_lock);
> -	req->state = SBA_REQUEST_STATE_PENDING;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_PENDING;
>  	list_move_tail(&req->node, &sba->reqs_pending_list);
>  	if (list_empty(&sba->reqs_active_list))
>  		sba->reqs_fence = false;
> @@ -249,9 +250,10 @@ static bool _sba_active_request(struct sba_device *sba,
>  		sba->reqs_fence = false;
>  	if (sba->reqs_fence)
>  		return false;
> -	req->state = SBA_REQUEST_STATE_ACTIVE;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_ACTIVE;
>  	list_move_tail(&req->node, &sba->reqs_active_list);
> -	if (req->fence)
> +	if (req->flags & SBA_REQUEST_FENCE)
>  		sba->reqs_fence = true;
>  	return true;
>  }
> @@ -261,7 +263,8 @@ static void _sba_abort_request(struct sba_device *sba,
>  			       struct sba_request *req)
>  {
>  	lockdep_assert_held(&sba->reqs_lock);
> -	req->state = SBA_REQUEST_STATE_ABORTED;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_ABORTED;
>  	list_move_tail(&req->node, &sba->reqs_aborted_list);
>  	if (list_empty(&sba->reqs_active_list))
>  		sba->reqs_fence = false;
> @@ -272,7 +275,8 @@ static void _sba_free_request(struct sba_device *sba,
>  			      struct sba_request *req)
>  {
>  	lockdep_assert_held(&sba->reqs_lock);
> -	req->state = SBA_REQUEST_STATE_FREE;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_FREE;
>  	list_move_tail(&req->node, &sba->reqs_free_list);
>  	if (list_empty(&sba->reqs_active_list))
>  		sba->reqs_fence = false;
> @@ -285,7 +289,8 @@ static void sba_received_request(struct sba_request *req)
>  	struct sba_device *sba = req->sba;
>  
>  	spin_lock_irqsave(&sba->reqs_lock, flags);
> -	req->state = SBA_REQUEST_STATE_RECEIVED;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_RECEIVED;
>  	list_move_tail(&req->node, &sba->reqs_received_list);
>  	spin_unlock_irqrestore(&sba->reqs_lock, flags);
>  }
> @@ -298,10 +303,12 @@ static void sba_complete_chained_requests(struct sba_request *req)
>  
>  	spin_lock_irqsave(&sba->reqs_lock, flags);
>  
> -	req->state = SBA_REQUEST_STATE_COMPLETED;
> +	req->flags &= ~SBA_REQUEST_STATE_MASK;
> +	req->flags |= SBA_REQUEST_STATE_COMPLETED;
>  	list_move_tail(&req->node, &sba->reqs_completed_list);
>  	list_for_each_entry(nreq, &req->next, next) {
> -		nreq->state = SBA_REQUEST_STATE_COMPLETED;
> +		nreq->flags &= ~SBA_REQUEST_STATE_MASK;
> +		nreq->flags |= SBA_REQUEST_STATE_COMPLETED;
>  		list_move_tail(&nreq->node, &sba->reqs_completed_list);
>  	}
>  	if (list_empty(&sba->reqs_active_list))
> @@ -576,7 +583,7 @@ sba_prep_dma_interrupt(struct dma_chan *dchan, unsigned long flags)
>  	 * Force fence so that no requests are submitted
>  	 * until DMA callback for this request is invoked.
>  	 */
> -	req->fence = true;
> +	req->flags |= SBA_REQUEST_FENCE;
>  
>  	/* Fillup request message */
>  	sba_fillup_interrupt_msg(req, req->cmds, &req->msg);
> @@ -659,7 +666,8 @@ sba_prep_dma_memcpy_req(struct sba_device *sba,
>  	req = sba_alloc_request(sba);
>  	if (!req)
>  		return NULL;
> -	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
> +	if (flags & DMA_PREP_FENCE)
> +		req->flags |= SBA_REQUEST_FENCE;
>  
>  	/* Fillup request message */
>  	sba_fillup_memcpy_msg(req, req->cmds, &req->msg,
> @@ -796,7 +804,8 @@ sba_prep_dma_xor_req(struct sba_device *sba,
>  	req = sba_alloc_request(sba);
>  	if (!req)
>  		return NULL;
> -	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
> +	if (flags & DMA_PREP_FENCE)
> +		req->flags |= SBA_REQUEST_FENCE;
>  
>  	/* Fillup request message */
>  	sba_fillup_xor_msg(req, req->cmds, &req->msg,
> @@ -1005,7 +1014,8 @@ sba_prep_dma_pq_req(struct sba_device *sba, dma_addr_t off,
>  	req = sba_alloc_request(sba);
>  	if (!req)
>  		return NULL;
> -	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
> +	if (flags & DMA_PREP_FENCE)
> +		req->flags |= SBA_REQUEST_FENCE;
>  
>  	/* Fillup request messages */
>  	sba_fillup_pq_msg(req, dmaf_continue(flags),
> @@ -1258,7 +1268,8 @@ sba_prep_dma_pq_single_req(struct sba_device *sba, dma_addr_t off,
>  	req = sba_alloc_request(sba);
>  	if (!req)
>  		return NULL;
> -	req->fence = (flags & DMA_PREP_FENCE) ? true : false;
> +	if (flags & DMA_PREP_FENCE)
> +		req->flags |= SBA_REQUEST_FENCE;
>  
>  	/* Fillup request messages */
>  	sba_fillup_pq_single_msg(req,  dmaf_continue(flags),
> @@ -1425,7 +1436,7 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
>  	req = req->first;
>  
>  	/* Update request */
> -	if (req->state == SBA_REQUEST_STATE_RECEIVED)
> +	if (req->flags & SBA_REQUEST_STATE_RECEIVED)
>  		sba_dma_tx_actions(req);
>  	else
>  		sba_free_chained_requests(req);
> @@ -1488,11 +1499,10 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
>  		req = &sba->reqs[i];
>  		INIT_LIST_HEAD(&req->node);
>  		req->sba = sba;
> -		req->state = SBA_REQUEST_STATE_FREE;
> +		req->flags = SBA_REQUEST_STATE_FREE;
>  		INIT_LIST_HEAD(&req->next);
>  		req->next_count = 1;
>  		atomic_set(&req->next_pending_count, 0);
> -		req->fence = false;
>  		req->resp = sba->resp_base + p;
>  		req->resp_dma = sba->resp_dma_base + p;
>  		p += sba->hw_resp_size;
> -- 
> 2.7.4
> 

-- 
~Vinod

Re: [PATCH v2 03/16] dmaengine: bcm-sba-raid: Common flags for sba_request state and fence

[Anup Patel]

On Thu, Aug 17, 2017 at 9:15 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:47PM +0530, Anup Patel wrote:
>> This patch merges sba_request state and fence into common
>> sba_request flags. Also, in-future we can extend sba_request
>> flags as required.
>
> and it also changes the flag values to bits, which I have no idea why that
> was done, care to explain that please...

I thought its better to have separate bit each sba_request state so
that when a sba_request is accidentally in two states then we can
debug better.

I will restore state values.

Regards,
Anup

Re: [PATCH v2 03/16] dmaengine: bcm-sba-raid: Common flags for sba_request state and fence

[Vinod Koul]

On Fri, Aug 18, 2017 at 10:26:54AM +0530, Anup Patel wrote:
> On Thu, Aug 17, 2017 at 9:15 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Tue, Aug 01, 2017 at 04:07:47PM +0530, Anup Patel wrote:
> >> This patch merges sba_request state and fence into common
> >> sba_request flags. Also, in-future we can extend sba_request
> >> flags as required.
> >
> > and it also changes the flag values to bits, which I have no idea why that
> > was done, care to explain that please...
> 
> I thought its better to have separate bit each sba_request state so
> that when a sba_request is accidentally in two states then we can
> debug better.

that is fine, but you need to comminucate the motivation behind such a
change!!

> 
> I will restore state values.

either ways am okay, but if we are not using bits smartly then why to change

-- 
~Vinod

Re: [PATCH v2 03/16] dmaengine: bcm-sba-raid: Common flags for sba_request state and fence

[Anup Patel]

On Fri, Aug 18, 2017 at 10:55 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Aug 18, 2017 at 10:26:54AM +0530, Anup Patel wrote:
>> On Thu, Aug 17, 2017 at 9:15 AM, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Tue, Aug 01, 2017 at 04:07:47PM +0530, Anup Patel wrote:
>> >> This patch merges sba_request state and fence into common
>> >> sba_request flags. Also, in-future we can extend sba_request
>> >> flags as required.
>> >
>> > and it also changes the flag values to bits, which I have no idea why that
>> > was done, care to explain that please...
>>
>> I thought its better to have separate bit each sba_request state so
>> that when a sba_request is accidentally in two states then we can
>> debug better.
>
> that is fine, but you need to comminucate the motivation behind such a
> change!!

Okay, I will add this info to commit description.

>
>>
>> I will restore state values.
>
> either ways am okay, but if we are not using bits smartly then why to change

Okay, I will keep new state values as-is and only update commit
description.

Regards,
Anup

[PATCH v2 04/16] dmaengine: bcm-sba-raid: Remove redundant next_count from sba_request

[Anup Patel]

The next_count in sba_request is redundant because same information
is captured by next_pending_count. This patch removes next_count
from sba_request.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 6fa3df1..e8863e9 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -111,7 +111,6 @@ struct sba_request {
 	/* 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;
@@ -221,7 +220,6 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	req->flags = SBA_REQUEST_STATE_ALLOCED;
 	req->first = req;
 	INIT_LIST_HEAD(&req->next);
-	req->next_count = 1;
 	atomic_set(&req->next_pending_count, 1);
 
 	dma_async_tx_descriptor_init(&req->tx, &sba->dma_chan);
@@ -342,8 +340,7 @@ static void sba_chain_request(struct sba_request *first,
 
 	list_add_tail(&req->next, &first->next);
 	req->first = first;
-	first->next_count++;
-	atomic_set(&first->next_pending_count, first->next_count);
+	atomic_inc(&first->next_pending_count);
 
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
@@ -1501,7 +1498,6 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 		req->sba = sba;
 		req->flags = SBA_REQUEST_STATE_FREE;
 		INIT_LIST_HEAD(&req->next);
-		req->next_count = 1;
 		atomic_set(&req->next_pending_count, 0);
 		req->resp = sba->resp_base + p;
 		req->resp_dma = sba->resp_dma_base + p;
-- 
2.7.4

[PATCH v2 05/16] dmaengine: bcm-sba-raid: Remove redundant resp_dma from sba_request

[Anup Patel]

Both resp and resp_dma are redundant in sba_request because
resp is unused and resp_dma carries same information present
in tx.phys of sba_request. This patch removes both resp and
resp_dma from sba_request.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index e8863e9..7d08d4e 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -113,8 +113,6 @@ struct sba_request {
 	struct list_head next;
 	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;
@@ -513,6 +511,7 @@ static void sba_fillup_interrupt_msg(struct sba_request *req,
 {
 	u64 cmd;
 	u32 c_mdata;
+	dma_addr_t resp_dma = req->tx.phys;
 	struct brcm_sba_command *cmdsp = cmds;
 
 	/* Type-B command to load dummy data into buf0 */
@@ -528,7 +527,7 @@ static void sba_fillup_interrupt_msg(struct sba_request *req,
 	cmdsp->cmd = cmd;
 	*cmdsp->cmd_dma = cpu_to_le64(cmd);
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_B;
-	cmdsp->data = req->resp_dma;
+	cmdsp->data = resp_dma;
 	cmdsp->data_len = req->sba->hw_resp_size;
 	cmdsp++;
 
@@ -549,11 +548,11 @@ static void sba_fillup_interrupt_msg(struct sba_request *req,
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 	if (req->sba->hw_resp_size) {
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-		cmdsp->resp = req->resp_dma;
+		cmdsp->resp = resp_dma;
 		cmdsp->resp_len = req->sba->hw_resp_size;
 	}
 	cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
-	cmdsp->data = req->resp_dma;
+	cmdsp->data = resp_dma;
 	cmdsp->data_len = req->sba->hw_resp_size;
 	cmdsp++;
 
@@ -600,6 +599,7 @@ static void sba_fillup_memcpy_msg(struct sba_request *req,
 {
 	u64 cmd;
 	u32 c_mdata;
+	dma_addr_t resp_dma = req->tx.phys;
 	struct brcm_sba_command *cmdsp = cmds;
 
 	/* Type-B command to load data into buf0 */
@@ -636,7 +636,7 @@ static void sba_fillup_memcpy_msg(struct sba_request *req,
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 	if (req->sba->hw_resp_size) {
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-		cmdsp->resp = req->resp_dma;
+		cmdsp->resp = resp_dma;
 		cmdsp->resp_len = req->sba->hw_resp_size;
 	}
 	cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -719,6 +719,7 @@ static void sba_fillup_xor_msg(struct sba_request *req,
 	u64 cmd;
 	u32 c_mdata;
 	unsigned int i;
+	dma_addr_t resp_dma = req->tx.phys;
 	struct brcm_sba_command *cmdsp = cmds;
 
 	/* Type-B command to load data into buf0 */
@@ -774,7 +775,7 @@ static void sba_fillup_xor_msg(struct sba_request *req,
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 	if (req->sba->hw_resp_size) {
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-		cmdsp->resp = req->resp_dma;
+		cmdsp->resp = resp_dma;
 		cmdsp->resp_len = req->sba->hw_resp_size;
 	}
 	cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -863,6 +864,7 @@ static void sba_fillup_pq_msg(struct sba_request *req,
 	u64 cmd;
 	u32 c_mdata;
 	unsigned int i;
+	dma_addr_t resp_dma = req->tx.phys;
 	struct brcm_sba_command *cmdsp = cmds;
 
 	if (pq_continue) {
@@ -956,7 +958,7 @@ static void sba_fillup_pq_msg(struct sba_request *req,
 		cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 		if (req->sba->hw_resp_size) {
 			cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-			cmdsp->resp = req->resp_dma;
+			cmdsp->resp = resp_dma;
 			cmdsp->resp_len = req->sba->hw_resp_size;
 		}
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -983,7 +985,7 @@ static void sba_fillup_pq_msg(struct sba_request *req,
 		cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 		if (req->sba->hw_resp_size) {
 			cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-			cmdsp->resp = req->resp_dma;
+			cmdsp->resp = resp_dma;
 			cmdsp->resp_len = req->sba->hw_resp_size;
 		}
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -1037,6 +1039,7 @@ static void sba_fillup_pq_single_msg(struct sba_request *req,
 	u64 cmd;
 	u32 c_mdata;
 	u8 pos, dpos = raid6_gflog[scf];
+	dma_addr_t resp_dma = req->tx.phys;
 	struct brcm_sba_command *cmdsp = cmds;
 
 	if (!dst_p)
@@ -1115,7 +1118,7 @@ static void sba_fillup_pq_single_msg(struct sba_request *req,
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 	if (req->sba->hw_resp_size) {
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-		cmdsp->resp = req->resp_dma;
+		cmdsp->resp = resp_dma;
 		cmdsp->resp_len = req->sba->hw_resp_size;
 	}
 	cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -1236,7 +1239,7 @@ static void sba_fillup_pq_single_msg(struct sba_request *req,
 	cmdsp->flags = BRCM_SBA_CMD_TYPE_A;
 	if (req->sba->hw_resp_size) {
 		cmdsp->flags |= BRCM_SBA_CMD_HAS_RESP;
-		cmdsp->resp = req->resp_dma;
+		cmdsp->resp = resp_dma;
 		cmdsp->resp_len = req->sba->hw_resp_size;
 	}
 	cmdsp->flags |= BRCM_SBA_CMD_HAS_OUTPUT;
@@ -1458,7 +1461,7 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
 
 static int sba_prealloc_channel_resources(struct sba_device *sba)
 {
-	int i, j, p, ret = 0;
+	int i, j, ret = 0;
 	struct sba_request *req = NULL;
 
 	sba->resp_base = dma_alloc_coherent(sba->dma_dev.dev,
@@ -1492,16 +1495,13 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 		goto fail_free_cmds_pool;
 	}
 
-	for (i = 0, p = 0; i < sba->max_req; i++) {
+	for (i = 0; i < sba->max_req; i++) {
 		req = &sba->reqs[i];
 		INIT_LIST_HEAD(&req->node);
 		req->sba = sba;
 		req->flags = SBA_REQUEST_STATE_FREE;
 		INIT_LIST_HEAD(&req->next);
 		atomic_set(&req->next_pending_count, 0);
-		req->resp = sba->resp_base + p;
-		req->resp_dma = sba->resp_dma_base + p;
-		p += sba->hw_resp_size;
 		req->cmds = devm_kcalloc(sba->dev, sba->max_cmd_per_req,
 					 sizeof(*req->cmds), GFP_KERNEL);
 		if (!req->cmds) {
@@ -1519,7 +1519,7 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 		memset(&req->msg, 0, sizeof(req->msg));
 		dma_async_tx_descriptor_init(&req->tx, &sba->dma_chan);
 		req->tx.tx_submit = sba_tx_submit;
-		req->tx.phys = req->resp_dma;
+		req->tx.phys = sba->resp_dma_base + i * sba->hw_resp_size;
 		list_add_tail(&req->node, &sba->reqs_free_list);
 	}
 
-- 
2.7.4

[PATCH v2 06/16] dmaengine: bcm-sba-raid: Remove reqs_free_count from sba_device

[Anup Patel]

The reqs_free_count member of sba_device is not used anywhere
hence no point in tracking number of free sba_request.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 7d08d4e..59ef443 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -162,7 +162,6 @@ 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;
 };
 
 /* ====== Command helper routines ===== */
@@ -207,10 +206,8 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	spin_lock_irqsave(&sba->reqs_lock, flags);
 	req = list_first_entry_or_null(&sba->reqs_free_list,
 				       struct sba_request, node);
-	if (req) {
+	if (req)
 		list_move_tail(&req->node, &sba->reqs_alloc_list);
-		sba->reqs_free_count--;
-	}
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 	if (!req)
 		return NULL;
@@ -276,7 +273,6 @@ static void _sba_free_request(struct sba_device *sba,
 	list_move_tail(&req->node, &sba->reqs_free_list);
 	if (list_empty(&sba->reqs_active_list))
 		sba->reqs_fence = false;
-	sba->reqs_free_count++;
 }
 
 static void sba_received_request(struct sba_request *req)
@@ -1523,8 +1519,6 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 		list_add_tail(&req->node, &sba->reqs_free_list);
 	}
 
-	sba->reqs_free_count = sba->max_req;
-
 	return 0;
 
 fail_free_cmds_pool:
-- 
2.7.4

[PATCH v2 07/16] dmaengine: bcm-sba-raid: Allow arbitrary number free sba_request

[Anup Patel]

Currently, we cannot have any arbitrary number of free sba_request
because sba_prealloc_channel_resources() allocates an array of
sba_request using devm_kcalloc() and kcalloc() cannot provide
memory beyond certain size.

This patch removes "reqs" (sba_request array) from sba_device
and makes "cmds" as variable array (instead of pointer) in
sba_request. This helps sba_prealloc_channel_resources() to
allocate sba_request and associated SBA command in one allocation
which in-turn allows arbitrary number of free sba_request.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 59ef443..cd6e3cd 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -113,9 +113,10 @@ struct sba_request {
 	struct list_head next;
 	atomic_t next_pending_count;
 	/* BRCM message data */
-	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 {
@@ -153,7 +154,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;
@@ -1484,26 +1484,20 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 	INIT_LIST_HEAD(&sba->reqs_aborted_list);
 	INIT_LIST_HEAD(&sba->reqs_free_list);
 
-	sba->reqs = devm_kcalloc(sba->dev, sba->max_req,
-				 sizeof(*req), GFP_KERNEL);
-	if (!sba->reqs) {
-		ret = -ENOMEM;
-		goto fail_free_cmds_pool;
-	}
-
 	for (i = 0; i < sba->max_req; i++) {
-		req = &sba->reqs[i];
+		req = devm_kzalloc(sba->dev,
+				sizeof(*req) +
+				sba->max_cmd_per_req * sizeof(req->cmds[0]),
+				GFP_KERNEL);
+		if (!req) {
+			ret = -ENOMEM;
+			goto fail_free_cmds_pool;
+		}
 		INIT_LIST_HEAD(&req->node);
 		req->sba = sba;
 		req->flags = SBA_REQUEST_STATE_FREE;
 		INIT_LIST_HEAD(&req->next);
 		atomic_set(&req->next_pending_count, 0);
-		req->cmds = devm_kcalloc(sba->dev, sba->max_cmd_per_req,
-					 sizeof(*req->cmds), GFP_KERNEL);
-		if (!req->cmds) {
-			ret = -ENOMEM;
-			goto fail_free_cmds_pool;
-		}
 		for (j = 0; j < sba->max_cmd_per_req; j++) {
 			req->cmds[j].cmd = 0;
 			req->cmds[j].cmd_dma = sba->cmds_base +
-- 
2.7.4

[PATCH v2 08/16] dmaengine: bcm-sba-raid: Increase number of free sba_request

[Anup Patel]

Currently, we have only 1024 free sba_request created
by sba_prealloc_channel_resources(). This is too low
and the prep_xxx() callbacks start failing more often
at time of RAID array setup over NVMe disks.

This patch sets number of free sba_request created by
sba_prealloc_channel_resources() to be:
<number_of_mailbox_channels> x 8192

Due to above, we will have sufficient number of free
sba_request and prep_xxx() callbacks failing is very
unlikely.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index cd6e3cd..af6cc8e 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -83,6 +83,8 @@
 #define SBA_CMD_WRITE_BUFFER				0xc
 #define SBA_CMD_GALOIS					0xe
 
+#define SBA_MAX_REQ_PER_MBOX_CHANNEL			8192
+
 /* Driver helper macros */
 #define to_sba_request(tx)		\
 	container_of(tx, struct sba_request, tx)
@@ -1622,6 +1624,13 @@ static int sba_probe(struct platform_device *pdev)
 	sba->dev = &pdev->dev;
 	platform_set_drvdata(pdev, sba);
 
+	/* Number of channels equals number of mailbox channels */
+	ret = of_count_phandle_with_args(pdev->dev.of_node,
+					 "mboxes", "#mbox-cells");
+	if (ret <= 0)
+		return -ENODEV;
+	mchans_count = ret;
+
 	/* Determine SBA version from DT compatible string */
 	if (of_device_is_compatible(sba->dev->of_node, "brcm,iproc-sba"))
 		sba->ver = SBA_VER_1;
@@ -1634,14 +1643,12 @@ static int sba_probe(struct platform_device *pdev)
 	/* Derived Configuration parameters */
 	switch (sba->ver) {
 	case SBA_VER_1:
-		sba->max_req = 1024;
 		sba->hw_buf_size = 4096;
 		sba->hw_resp_size = 8;
 		sba->max_pq_coefs = 6;
 		sba->max_pq_srcs = 6;
 		break;
 	case SBA_VER_2:
-		sba->max_req = 1024;
 		sba->hw_buf_size = 4096;
 		sba->hw_resp_size = 8;
 		sba->max_pq_coefs = 30;
@@ -1655,6 +1662,7 @@ static int sba_probe(struct platform_device *pdev)
 	default:
 		return -EINVAL;
 	}
+	sba->max_req = SBA_MAX_REQ_PER_MBOX_CHANNEL * mchans_count;
 	sba->max_cmd_per_req = sba->max_pq_srcs + 3;
 	sba->max_xor_srcs = sba->max_cmd_per_req - 1;
 	sba->max_resp_pool_size = sba->max_req * sba->hw_resp_size;
@@ -1668,22 +1676,14 @@ static int sba_probe(struct platform_device *pdev)
 	sba->client.knows_txdone	= false;
 	sba->client.tx_tout		= 0;
 
-	/* Number of channels equals number of mailbox channels */
-	ret = of_count_phandle_with_args(pdev->dev.of_node,
-					 "mboxes", "#mbox-cells");
-	if (ret <= 0)
-		return -ENODEV;
-	mchans_count = ret;
-	sba->mchans_count = 0;
-	atomic_set(&sba->mchans_current, 0);
-
 	/* Allocate mailbox channel array */
-	sba->mchans = devm_kcalloc(&pdev->dev, sba->mchans_count,
+	sba->mchans = devm_kcalloc(&pdev->dev, mchans_count,
 				   sizeof(*sba->mchans), GFP_KERNEL);
 	if (!sba->mchans)
 		return -ENOMEM;
 
 	/* Request mailbox channels */
+	sba->mchans_count = 0;
 	for (i = 0; i < mchans_count; i++) {
 		sba->mchans[i] = mbox_request_channel(&sba->client, i);
 		if (IS_ERR(sba->mchans[i])) {
@@ -1692,6 +1692,7 @@ static int sba_probe(struct platform_device *pdev)
 		}
 		sba->mchans_count++;
 	}
+	atomic_set(&sba->mchans_current, 0);
 
 	/* Find-out underlying mailbox device */
 	ret = of_parse_phandle_with_args(pdev->dev.of_node,
-- 
2.7.4

[PATCH v2 09/16] dmaengine: bcm-sba-raid: Improve sba_issue_pending() run duration

[Anup Patel]

The pending sba_request list can become very long in real-life usage
(e.g. setting up RAID array) which can cause sba_issue_pending() to
run for long duration.

This patch adds common sba_process_deferred_requests() to process
few completed and pending requests so that it finishes in short
duration. We use this common sba_process_deferred_requests() in
both sba_issue_pending() and sba_receive_message().

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 234 ++++++++++++++++++++++++---------------------
 1 file changed, 125 insertions(+), 109 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index af6cc8e..f6616da 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -277,38 +277,28 @@ static void _sba_free_request(struct sba_device *sba,
 		sba->reqs_fence = false;
 }
 
-static void sba_received_request(struct sba_request *req)
+/* Note: Must be called with sba->reqs_lock held */
+static void _sba_complete_request(struct sba_device *sba,
+				  struct sba_request *req)
 {
-	unsigned long flags;
-	struct sba_device *sba = req->sba;
-
-	spin_lock_irqsave(&sba->reqs_lock, flags);
+	lockdep_assert_held(&sba->reqs_lock);
 	req->flags &= ~SBA_REQUEST_STATE_MASK;
-	req->flags |= SBA_REQUEST_STATE_RECEIVED;
-	list_move_tail(&req->node, &sba->reqs_received_list);
-	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+	req->flags |= SBA_REQUEST_STATE_COMPLETED;
+	list_move_tail(&req->node, &sba->reqs_completed_list);
+	if (list_empty(&sba->reqs_active_list))
+		sba->reqs_fence = false;
 }
 
-static void sba_complete_chained_requests(struct sba_request *req)
+/* Note: Must be called with sba->reqs_lock held */
+static void _sba_received_request(struct sba_device *sba,
+				  struct sba_request *req)
 {
-	unsigned long flags;
-	struct sba_request *nreq;
-	struct sba_device *sba = req->sba;
-
-	spin_lock_irqsave(&sba->reqs_lock, flags);
-
+	lockdep_assert_held(&sba->reqs_lock);
 	req->flags &= ~SBA_REQUEST_STATE_MASK;
-	req->flags |= SBA_REQUEST_STATE_COMPLETED;
-	list_move_tail(&req->node, &sba->reqs_completed_list);
-	list_for_each_entry(nreq, &req->next, next) {
-		nreq->flags &= ~SBA_REQUEST_STATE_MASK;
-		nreq->flags |= SBA_REQUEST_STATE_COMPLETED;
-		list_move_tail(&nreq->node, &sba->reqs_completed_list);
-	}
+	req->flags |= SBA_REQUEST_STATE_RECEIVED;
+	list_move_tail(&req->node, &sba->reqs_received_list);
 	if (list_empty(&sba->reqs_active_list))
 		sba->reqs_fence = false;
-
-	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
 
 static void sba_free_chained_requests(struct sba_request *req)
@@ -386,26 +376,6 @@ static void sba_cleanup_pending_requests(struct sba_device *sba)
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
 
-/* ====== DMAENGINE callbacks ===== */
-
-static void sba_free_chan_resources(struct dma_chan *dchan)
-{
-	/*
-	 * Channel resources are pre-alloced so we just free-up
-	 * whatever we can so that we can re-use pre-alloced
-	 * channel resources next time.
-	 */
-	sba_cleanup_nonpending_requests(to_sba_device(dchan));
-}
-
-static int sba_device_terminate_all(struct dma_chan *dchan)
-{
-	/* Cleanup all pending requests */
-	sba_cleanup_pending_requests(to_sba_device(dchan));
-
-	return 0;
-}
-
 static int sba_send_mbox_request(struct sba_device *sba,
 				 struct sba_request *req)
 {
@@ -431,17 +401,27 @@ static int sba_send_mbox_request(struct sba_device *sba,
 	return 0;
 }
 
-static void sba_issue_pending(struct dma_chan *dchan)
+static void sba_process_deferred_requests(struct sba_device *sba)
 {
 	int ret;
+	u32 count;
 	unsigned long flags;
-	struct sba_request *req, *req1;
-	struct sba_device *sba = to_sba_device(dchan);
+	struct sba_request *req;
+	struct dma_async_tx_descriptor *tx;
 
 	spin_lock_irqsave(&sba->reqs_lock, flags);
 
-	/* Process all pending request */
-	list_for_each_entry_safe(req, req1, &sba->reqs_pending_list, node) {
+	/* Count pending requests */
+	count = 0;
+	list_for_each_entry(req, &sba->reqs_pending_list, node)
+		count++;
+
+	/* Process pending requests */
+	while (!list_empty(&sba->reqs_pending_list) && count) {
+		/* Get the first pending request */
+		req = list_first_entry(&sba->reqs_pending_list,
+				       struct sba_request, node);
+
 		/* Try to make request active */
 		if (!_sba_active_request(sba, req))
 			break;
@@ -456,11 +436,102 @@ static void sba_issue_pending(struct dma_chan *dchan)
 			_sba_pending_request(sba, req);
 			break;
 		}
+
+		count--;
+	}
+
+	/* Count completed requests */
+	count = 0;
+	list_for_each_entry(req, &sba->reqs_completed_list, node)
+		count++;
+
+	/* Process completed requests */
+	while (!list_empty(&sba->reqs_completed_list) && count) {
+		req = list_first_entry(&sba->reqs_completed_list,
+					struct sba_request, node);
+		list_del_init(&req->node);
+		tx = &req->tx;
+
+		spin_unlock_irqrestore(&sba->reqs_lock, flags);
+
+		WARN_ON(tx->cookie < 0);
+		if (tx->cookie > 0) {
+			dma_cookie_complete(tx);
+			dmaengine_desc_get_callback_invoke(tx, NULL);
+			dma_descriptor_unmap(tx);
+			tx->callback = NULL;
+			tx->callback_result = NULL;
+		}
+
+		dma_run_dependencies(tx);
+
+		spin_lock_irqsave(&sba->reqs_lock, flags);
+
+		/* If waiting for 'ack' then move to completed list */
+		if (!async_tx_test_ack(&req->tx))
+			_sba_complete_request(sba, req);
+		else
+			_sba_free_request(sba, req);
+
+		count--;
 	}
 
+	/* Re-check pending and completed work */
+	count = 0;
+	if (!list_empty(&sba->reqs_pending_list) ||
+	    !list_empty(&sba->reqs_completed_list))
+		count = 1;
+
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
 
+static void sba_process_received_request(struct sba_device *sba,
+					 struct sba_request *req)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sba->reqs_lock, flags);
+
+	/* Mark request as received */
+	_sba_received_request(sba, req);
+
+	/* Update request */
+	if (!atomic_dec_return(&req->first->next_pending_count))
+		_sba_complete_request(sba, req->first);
+	if (req->first != req)
+		_sba_free_request(sba, req);
+
+	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+}
+
+/* ====== DMAENGINE callbacks ===== */
+
+static void sba_free_chan_resources(struct dma_chan *dchan)
+{
+	/*
+	 * Channel resources are pre-alloced so we just free-up
+	 * whatever we can so that we can re-use pre-alloced
+	 * channel resources next time.
+	 */
+	sba_cleanup_nonpending_requests(to_sba_device(dchan));
+}
+
+static int sba_device_terminate_all(struct dma_chan *dchan)
+{
+	/* Cleanup all pending requests */
+	sba_cleanup_pending_requests(to_sba_device(dchan));
+
+	return 0;
+}
+
+static void sba_issue_pending(struct dma_chan *dchan)
+{
+	struct sba_device *sba = to_sba_device(dchan);
+
+	/* Process deferred requests */
+	sba_process_deferred_requests(sba);
+}
+
 static dma_cookie_t sba_tx_submit(struct dma_async_tx_descriptor *tx)
 {
 	unsigned long flags;
@@ -1382,40 +1453,10 @@ sba_prep_dma_pq(struct dma_chan *dchan, dma_addr_t *dst, dma_addr_t *src,
 
 /* ====== Mailbox callbacks ===== */
 
-static void sba_dma_tx_actions(struct sba_request *req)
-{
-	struct dma_async_tx_descriptor *tx = &req->tx;
-
-	WARN_ON(tx->cookie < 0);
-
-	if (tx->cookie > 0) {
-		dma_cookie_complete(tx);
-
-		/*
-		 * Call the callback (must not sleep or submit new
-		 * operations to this channel)
-		 */
-		if (tx->callback)
-			tx->callback(tx->callback_param);
-
-		dma_descriptor_unmap(tx);
-	}
-
-	/* Run dependent operations */
-	dma_run_dependencies(tx);
-
-	/* If waiting for 'ack' then move to completed list */
-	if (!async_tx_test_ack(&req->tx))
-		sba_complete_chained_requests(req);
-	else
-		sba_free_chained_requests(req);
-}
-
 static void sba_receive_message(struct mbox_client *cl, void *msg)
 {
-	unsigned long flags;
 	struct brcm_message *m = msg;
-	struct sba_request *req = m->ctx, *req1;
+	struct sba_request *req = m->ctx;
 	struct sba_device *sba = req->sba;
 
 	/* Error count if message has error */
@@ -1423,36 +1464,11 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
 		dev_err(sba->dev, "%s got message with error %d",
 			dma_chan_name(&sba->dma_chan), m->error);
 
-	/* Mark request as received */
-	sba_received_request(req);
-
-	/* Wait for all chained requests to be completed */
-	if (atomic_dec_return(&req->first->next_pending_count))
-		goto done;
-
-	/* Point to first request */
-	req = req->first;
-
-	/* Update request */
-	if (req->flags & SBA_REQUEST_STATE_RECEIVED)
-		sba_dma_tx_actions(req);
-	else
-		sba_free_chained_requests(req);
-
-	spin_lock_irqsave(&sba->reqs_lock, flags);
-
-	/* Re-check all completed request waiting for 'ack' */
-	list_for_each_entry_safe(req, req1, &sba->reqs_completed_list, node) {
-		spin_unlock_irqrestore(&sba->reqs_lock, flags);
-		sba_dma_tx_actions(req);
-		spin_lock_irqsave(&sba->reqs_lock, flags);
-	}
-
-	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+	/* Process received request */
+	sba_process_received_request(sba, req);
 
-done:
-	/* Try to submit pending request */
-	sba_issue_pending(&sba->dma_chan);
+	/* Process deferred requests */
+	sba_process_deferred_requests(sba);
 }
 
 /* ====== Platform driver routines ===== */
-- 
2.7.4

Re: [PATCH v2 09/16] dmaengine: bcm-sba-raid: Improve sba_issue_pending() run duration

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:53PM +0530, Anup Patel wrote:
> The pending sba_request list can become very long in real-life usage
> (e.g. setting up RAID array) which can cause sba_issue_pending() to
> run for long duration.

that raises the warning flags.. Even if we have a long pending list why
would issue_pending run for long. The purpose of the issue_pending() is to
submit a txn if idle and return. The interrupt and tasklet shall push the
subsequent txn to hardware...


> 
> This patch adds common sba_process_deferred_requests() to process
> few completed and pending requests so that it finishes in short
> duration. We use this common sba_process_deferred_requests() in
> both sba_issue_pending() and sba_receive_message().
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 234 ++++++++++++++++++++++++---------------------
>  1 file changed, 125 insertions(+), 109 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index af6cc8e..f6616da 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -277,38 +277,28 @@ static void _sba_free_request(struct sba_device *sba,
>  		sba->reqs_fence = false;
>  }
>  
> -static void sba_received_request(struct sba_request *req)
> +/* Note: Must be called with sba->reqs_lock held */
> +static void _sba_complete_request(struct sba_device *sba,
> +				  struct sba_request *req)
>  {
> -	unsigned long flags;
> -	struct sba_device *sba = req->sba;
> -
> -	spin_lock_irqsave(&sba->reqs_lock, flags);
> +	lockdep_assert_held(&sba->reqs_lock);
>  	req->flags &= ~SBA_REQUEST_STATE_MASK;
> -	req->flags |= SBA_REQUEST_STATE_RECEIVED;
> -	list_move_tail(&req->node, &sba->reqs_received_list);
> -	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +	req->flags |= SBA_REQUEST_STATE_COMPLETED;
> +	list_move_tail(&req->node, &sba->reqs_completed_list);
> +	if (list_empty(&sba->reqs_active_list))
> +		sba->reqs_fence = false;
>  }
>  
> -static void sba_complete_chained_requests(struct sba_request *req)
> +/* Note: Must be called with sba->reqs_lock held */
> +static void _sba_received_request(struct sba_device *sba,
> +				  struct sba_request *req)
>  {
> -	unsigned long flags;
> -	struct sba_request *nreq;
> -	struct sba_device *sba = req->sba;
> -
> -	spin_lock_irqsave(&sba->reqs_lock, flags);
> -
> +	lockdep_assert_held(&sba->reqs_lock);
>  	req->flags &= ~SBA_REQUEST_STATE_MASK;
> -	req->flags |= SBA_REQUEST_STATE_COMPLETED;
> -	list_move_tail(&req->node, &sba->reqs_completed_list);
> -	list_for_each_entry(nreq, &req->next, next) {
> -		nreq->flags &= ~SBA_REQUEST_STATE_MASK;
> -		nreq->flags |= SBA_REQUEST_STATE_COMPLETED;
> -		list_move_tail(&nreq->node, &sba->reqs_completed_list);
> -	}
> +	req->flags |= SBA_REQUEST_STATE_RECEIVED;
> +	list_move_tail(&req->node, &sba->reqs_received_list);
>  	if (list_empty(&sba->reqs_active_list))
>  		sba->reqs_fence = false;
> -
> -	spin_unlock_irqrestore(&sba->reqs_lock, flags);
>  }
>  
>  static void sba_free_chained_requests(struct sba_request *req)
> @@ -386,26 +376,6 @@ static void sba_cleanup_pending_requests(struct sba_device *sba)
>  	spin_unlock_irqrestore(&sba->reqs_lock, flags);
>  }
>  
> -/* ====== DMAENGINE callbacks ===== */
> -
> -static void sba_free_chan_resources(struct dma_chan *dchan)
> -{
> -	/*
> -	 * Channel resources are pre-alloced so we just free-up
> -	 * whatever we can so that we can re-use pre-alloced
> -	 * channel resources next time.
> -	 */
> -	sba_cleanup_nonpending_requests(to_sba_device(dchan));
> -}
> -
> -static int sba_device_terminate_all(struct dma_chan *dchan)
> -{
> -	/* Cleanup all pending requests */
> -	sba_cleanup_pending_requests(to_sba_device(dchan));
> -
> -	return 0;
> -}
> -
>  static int sba_send_mbox_request(struct sba_device *sba,
>  				 struct sba_request *req)
>  {
> @@ -431,17 +401,27 @@ static int sba_send_mbox_request(struct sba_device *sba,
>  	return 0;
>  }
>  
> -static void sba_issue_pending(struct dma_chan *dchan)
> +static void sba_process_deferred_requests(struct sba_device *sba)
>  {
>  	int ret;
> +	u32 count;
>  	unsigned long flags;
> -	struct sba_request *req, *req1;
> -	struct sba_device *sba = to_sba_device(dchan);
> +	struct sba_request *req;
> +	struct dma_async_tx_descriptor *tx;
>  
>  	spin_lock_irqsave(&sba->reqs_lock, flags);
>  
> -	/* Process all pending request */
> -	list_for_each_entry_safe(req, req1, &sba->reqs_pending_list, node) {
> +	/* Count pending requests */
> +	count = 0;
> +	list_for_each_entry(req, &sba->reqs_pending_list, node)
> +		count++;
> +
> +	/* Process pending requests */
> +	while (!list_empty(&sba->reqs_pending_list) && count) {
> +		/* Get the first pending request */
> +		req = list_first_entry(&sba->reqs_pending_list,
> +				       struct sba_request, node);
> +
>  		/* Try to make request active */
>  		if (!_sba_active_request(sba, req))
>  			break;
> @@ -456,11 +436,102 @@ static void sba_issue_pending(struct dma_chan *dchan)
>  			_sba_pending_request(sba, req);
>  			break;
>  		}
> +
> +		count--;
> +	}
> +
> +	/* Count completed requests */
> +	count = 0;
> +	list_for_each_entry(req, &sba->reqs_completed_list, node)
> +		count++;
> +
> +	/* Process completed requests */
> +	while (!list_empty(&sba->reqs_completed_list) && count) {
> +		req = list_first_entry(&sba->reqs_completed_list,
> +					struct sba_request, node);
> +		list_del_init(&req->node);
> +		tx = &req->tx;
> +
> +		spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +
> +		WARN_ON(tx->cookie < 0);
> +		if (tx->cookie > 0) {
> +			dma_cookie_complete(tx);
> +			dmaengine_desc_get_callback_invoke(tx, NULL);
> +			dma_descriptor_unmap(tx);
> +			tx->callback = NULL;
> +			tx->callback_result = NULL;
> +		}
> +
> +		dma_run_dependencies(tx);
> +
> +		spin_lock_irqsave(&sba->reqs_lock, flags);
> +
> +		/* If waiting for 'ack' then move to completed list */
> +		if (!async_tx_test_ack(&req->tx))
> +			_sba_complete_request(sba, req);
> +		else
> +			_sba_free_request(sba, req);
> +
> +		count--;
>  	}
>  
> +	/* Re-check pending and completed work */
> +	count = 0;
> +	if (!list_empty(&sba->reqs_pending_list) ||
> +	    !list_empty(&sba->reqs_completed_list))
> +		count = 1;
> +
>  	spin_unlock_irqrestore(&sba->reqs_lock, flags);
>  }
>  
> +static void sba_process_received_request(struct sba_device *sba,
> +					 struct sba_request *req)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&sba->reqs_lock, flags);
> +
> +	/* Mark request as received */
> +	_sba_received_request(sba, req);
> +
> +	/* Update request */
> +	if (!atomic_dec_return(&req->first->next_pending_count))
> +		_sba_complete_request(sba, req->first);
> +	if (req->first != req)
> +		_sba_free_request(sba, req);
> +
> +	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +}
> +
> +/* ====== DMAENGINE callbacks ===== */
> +
> +static void sba_free_chan_resources(struct dma_chan *dchan)
> +{
> +	/*
> +	 * Channel resources are pre-alloced so we just free-up
> +	 * whatever we can so that we can re-use pre-alloced
> +	 * channel resources next time.
> +	 */
> +	sba_cleanup_nonpending_requests(to_sba_device(dchan));
> +}
> +
> +static int sba_device_terminate_all(struct dma_chan *dchan)
> +{
> +	/* Cleanup all pending requests */
> +	sba_cleanup_pending_requests(to_sba_device(dchan));
> +
> +	return 0;
> +}
> +
> +static void sba_issue_pending(struct dma_chan *dchan)
> +{
> +	struct sba_device *sba = to_sba_device(dchan);
> +
> +	/* Process deferred requests */
> +	sba_process_deferred_requests(sba);
> +}
> +
>  static dma_cookie_t sba_tx_submit(struct dma_async_tx_descriptor *tx)
>  {
>  	unsigned long flags;
> @@ -1382,40 +1453,10 @@ sba_prep_dma_pq(struct dma_chan *dchan, dma_addr_t *dst, dma_addr_t *src,
>  
>  /* ====== Mailbox callbacks ===== */
>  
> -static void sba_dma_tx_actions(struct sba_request *req)
> -{
> -	struct dma_async_tx_descriptor *tx = &req->tx;
> -
> -	WARN_ON(tx->cookie < 0);
> -
> -	if (tx->cookie > 0) {
> -		dma_cookie_complete(tx);
> -
> -		/*
> -		 * Call the callback (must not sleep or submit new
> -		 * operations to this channel)
> -		 */
> -		if (tx->callback)
> -			tx->callback(tx->callback_param);
> -
> -		dma_descriptor_unmap(tx);
> -	}
> -
> -	/* Run dependent operations */
> -	dma_run_dependencies(tx);
> -
> -	/* If waiting for 'ack' then move to completed list */
> -	if (!async_tx_test_ack(&req->tx))
> -		sba_complete_chained_requests(req);
> -	else
> -		sba_free_chained_requests(req);
> -}
> -
>  static void sba_receive_message(struct mbox_client *cl, void *msg)
>  {
> -	unsigned long flags;
>  	struct brcm_message *m = msg;
> -	struct sba_request *req = m->ctx, *req1;
> +	struct sba_request *req = m->ctx;
>  	struct sba_device *sba = req->sba;
>  
>  	/* Error count if message has error */
> @@ -1423,36 +1464,11 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
>  		dev_err(sba->dev, "%s got message with error %d",
>  			dma_chan_name(&sba->dma_chan), m->error);
>  
> -	/* Mark request as received */
> -	sba_received_request(req);
> -
> -	/* Wait for all chained requests to be completed */
> -	if (atomic_dec_return(&req->first->next_pending_count))
> -		goto done;
> -
> -	/* Point to first request */
> -	req = req->first;
> -
> -	/* Update request */
> -	if (req->flags & SBA_REQUEST_STATE_RECEIVED)
> -		sba_dma_tx_actions(req);
> -	else
> -		sba_free_chained_requests(req);
> -
> -	spin_lock_irqsave(&sba->reqs_lock, flags);
> -
> -	/* Re-check all completed request waiting for 'ack' */
> -	list_for_each_entry_safe(req, req1, &sba->reqs_completed_list, node) {
> -		spin_unlock_irqrestore(&sba->reqs_lock, flags);
> -		sba_dma_tx_actions(req);
> -		spin_lock_irqsave(&sba->reqs_lock, flags);
> -	}
> -
> -	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +	/* Process received request */
> +	sba_process_received_request(sba, req);
>  
> -done:
> -	/* Try to submit pending request */
> -	sba_issue_pending(&sba->dma_chan);
> +	/* Process deferred requests */
> +	sba_process_deferred_requests(sba);
>  }
>  
>  /* ====== Platform driver routines ===== */
> -- 
> 2.7.4
> 

-- 
~Vinod

Re: [PATCH v2 09/16] dmaengine: bcm-sba-raid: Improve sba_issue_pending() run duration

[Anup Patel]

On Thu, Aug 17, 2017 at 12:06 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:53PM +0530, Anup Patel wrote:
>> The pending sba_request list can become very long in real-life usage
>> (e.g. setting up RAID array) which can cause sba_issue_pending() to
>> run for long duration.
>
> that raises the warning flags.. Even if we have a long pending list why
> would issue_pending run for long. The purpose of the issue_pending() is to
> submit a txn if idle and return. The interrupt and tasklet shall push the
> subsequent txn to hardware...

Yes, we are doing very similar thing in PATCH13 by further
simplifying sba_process_deferred_requests()

Regards,
Anup

[PATCH v2 10/16] dmaengine: bcm-sba-raid: Alloc resources before registering DMA device

[Anup Patel]

We should allocate DMA channel resources before registering the
DMA device in sba_probe() because we can get DMA request soon
after registering the DMA device. If DMA channel resources are
not allocated before first DMA request then SBA-RAID driver will
crash.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index f6616da..f14ed0a 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -1478,13 +1478,13 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 	int i, j, ret = 0;
 	struct sba_request *req = NULL;
 
-	sba->resp_base = dma_alloc_coherent(sba->dma_dev.dev,
+	sba->resp_base = dma_alloc_coherent(sba->mbox_dev,
 					    sba->max_resp_pool_size,
 					    &sba->resp_dma_base, GFP_KERNEL);
 	if (!sba->resp_base)
 		return -ENOMEM;
 
-	sba->cmds_base = dma_alloc_coherent(sba->dma_dev.dev,
+	sba->cmds_base = dma_alloc_coherent(sba->mbox_dev,
 					    sba->max_cmds_pool_size,
 					    &sba->cmds_dma_base, GFP_KERNEL);
 	if (!sba->cmds_base) {
@@ -1534,11 +1534,11 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 	return 0;
 
 fail_free_cmds_pool:
-	dma_free_coherent(sba->dma_dev.dev,
+	dma_free_coherent(sba->mbox_dev,
 			  sba->max_cmds_pool_size,
 			  sba->cmds_base, sba->cmds_dma_base);
 fail_free_resp_pool:
-	dma_free_coherent(sba->dma_dev.dev,
+	dma_free_coherent(sba->mbox_dev,
 			  sba->max_resp_pool_size,
 			  sba->resp_base, sba->resp_dma_base);
 	return ret;
@@ -1547,9 +1547,9 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 static void sba_freeup_channel_resources(struct sba_device *sba)
 {
 	dmaengine_terminate_all(&sba->dma_chan);
-	dma_free_coherent(sba->dma_dev.dev, sba->max_cmds_pool_size,
+	dma_free_coherent(sba->mbox_dev, sba->max_cmds_pool_size,
 			  sba->cmds_base, sba->cmds_dma_base);
-	dma_free_coherent(sba->dma_dev.dev, sba->max_resp_pool_size,
+	dma_free_coherent(sba->mbox_dev, sba->max_resp_pool_size,
 			  sba->resp_base, sba->resp_dma_base);
 	sba->resp_base = NULL;
 	sba->resp_dma_base = 0;
@@ -1737,15 +1737,15 @@ static int sba_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* Register DMA device with linux async framework */
-	ret = sba_async_register(sba);
+	/* Prealloc channel resource */
+	ret = sba_prealloc_channel_resources(sba);
 	if (ret)
 		goto fail_free_mchans;
 
-	/* Prealloc channel resource */
-	ret = sba_prealloc_channel_resources(sba);
+	/* Register DMA device with Linux async framework */
+	ret = sba_async_register(sba);
 	if (ret)
-		goto fail_async_dev_unreg;
+		goto fail_free_resources;
 
 	/* Print device info */
 	dev_info(sba->dev, "%s using SBAv%d and %d mailbox channels",
@@ -1754,8 +1754,8 @@ static int sba_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail_async_dev_unreg:
-	dma_async_device_unregister(&sba->dma_dev);
+fail_free_resources:
+	sba_freeup_channel_resources(sba);
 fail_free_mchans:
 	for (i = 0; i < sba->mchans_count; i++)
 		mbox_free_channel(sba->mchans[i]);
@@ -1767,10 +1767,10 @@ static int sba_remove(struct platform_device *pdev)
 	int i;
 	struct sba_device *sba = platform_get_drvdata(pdev);
 
-	sba_freeup_channel_resources(sba);
-
 	dma_async_device_unregister(&sba->dma_dev);
 
+	sba_freeup_channel_resources(sba);
+
 	for (i = 0; i < sba->mchans_count; i++)
 		mbox_free_channel(sba->mchans[i]);
 
-- 
2.7.4

Re: [PATCH v2 10/16] dmaengine: bcm-sba-raid: Alloc resources before registering DMA device

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:54PM +0530, Anup Patel wrote:
> We should allocate DMA channel resources before registering the
> DMA device in sba_probe() because we can get DMA request soon
> after registering the DMA device. If DMA channel resources are
> not allocated before first DMA request then SBA-RAID driver will
> crash.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index f6616da..f14ed0a 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -1478,13 +1478,13 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
>  	int i, j, ret = 0;
>  	struct sba_request *req = NULL;
>  
> -	sba->resp_base = dma_alloc_coherent(sba->dma_dev.dev,
> +	sba->resp_base = dma_alloc_coherent(sba->mbox_dev,

how does this qualify as move before registering, you seem to be using
different device now

>  					    sba->max_resp_pool_size,
>  					    &sba->resp_dma_base, GFP_KERNEL);
>  	if (!sba->resp_base)
>  		return -ENOMEM;
>  
> -	sba->cmds_base = dma_alloc_coherent(sba->dma_dev.dev,
> +	sba->cmds_base = dma_alloc_coherent(sba->mbox_dev,
>  					    sba->max_cmds_pool_size,
>  					    &sba->cmds_dma_base, GFP_KERNEL);
>  	if (!sba->cmds_base) {
> @@ -1534,11 +1534,11 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
>  	return 0;
>  
>  fail_free_cmds_pool:
> -	dma_free_coherent(sba->dma_dev.dev,
> +	dma_free_coherent(sba->mbox_dev,
>  			  sba->max_cmds_pool_size,
>  			  sba->cmds_base, sba->cmds_dma_base);
>  fail_free_resp_pool:
> -	dma_free_coherent(sba->dma_dev.dev,
> +	dma_free_coherent(sba->mbox_dev,
>  			  sba->max_resp_pool_size,
>  			  sba->resp_base, sba->resp_dma_base);
>  	return ret;
> @@ -1547,9 +1547,9 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
>  static void sba_freeup_channel_resources(struct sba_device *sba)
>  {
>  	dmaengine_terminate_all(&sba->dma_chan);
> -	dma_free_coherent(sba->dma_dev.dev, sba->max_cmds_pool_size,
> +	dma_free_coherent(sba->mbox_dev, sba->max_cmds_pool_size,
>  			  sba->cmds_base, sba->cmds_dma_base);
> -	dma_free_coherent(sba->dma_dev.dev, sba->max_resp_pool_size,
> +	dma_free_coherent(sba->mbox_dev, sba->max_resp_pool_size,
>  			  sba->resp_base, sba->resp_dma_base);
>  	sba->resp_base = NULL;
>  	sba->resp_dma_base = 0;
> @@ -1737,15 +1737,15 @@ static int sba_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/* Register DMA device with linux async framework */
> -	ret = sba_async_register(sba);
> +	/* Prealloc channel resource */
> +	ret = sba_prealloc_channel_resources(sba);
>  	if (ret)
>  		goto fail_free_mchans;
>  
> -	/* Prealloc channel resource */
> -	ret = sba_prealloc_channel_resources(sba);
> +	/* Register DMA device with Linux async framework */
> +	ret = sba_async_register(sba);
>  	if (ret)
> -		goto fail_async_dev_unreg;
> +		goto fail_free_resources;
>  
>  	/* Print device info */
>  	dev_info(sba->dev, "%s using SBAv%d and %d mailbox channels",
> @@ -1754,8 +1754,8 @@ static int sba_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -fail_async_dev_unreg:
> -	dma_async_device_unregister(&sba->dma_dev);
> +fail_free_resources:
> +	sba_freeup_channel_resources(sba);
>  fail_free_mchans:
>  	for (i = 0; i < sba->mchans_count; i++)
>  		mbox_free_channel(sba->mchans[i]);
> @@ -1767,10 +1767,10 @@ static int sba_remove(struct platform_device *pdev)
>  	int i;
>  	struct sba_device *sba = platform_get_drvdata(pdev);
>  
> -	sba_freeup_channel_resources(sba);
> -
>  	dma_async_device_unregister(&sba->dma_dev);
>  
> +	sba_freeup_channel_resources(sba);
> +
>  	for (i = 0; i < sba->mchans_count; i++)
>  		mbox_free_channel(sba->mchans[i]);
>  
> -- 
> 2.7.4
> 

-- 
~Vinod

Re: [PATCH v2 10/16] dmaengine: bcm-sba-raid: Alloc resources before registering DMA device

[Anup Patel]

On Thu, Aug 17, 2017 at 12:08 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:54PM +0530, Anup Patel wrote:
>> We should allocate DMA channel resources before registering the
>> DMA device in sba_probe() because we can get DMA request soon
>> after registering the DMA device. If DMA channel resources are
>> not allocated before first DMA request then SBA-RAID driver will
>> crash.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> ---
>>  drivers/dma/bcm-sba-raid.c | 30 +++++++++++++++---------------
>>  1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index f6616da..f14ed0a 100644
>> --- a/drivers/dma/bcm-sba-raid.c
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -1478,13 +1478,13 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
>>       int i, j, ret = 0;
>>       struct sba_request *req = NULL;
>>
>> -     sba->resp_base = dma_alloc_coherent(sba->dma_dev.dev,
>> +     sba->resp_base = dma_alloc_coherent(sba->mbox_dev,
>
> how does this qualify as move before registering, you seem to be using
> different device now

The sba->dma_dev.dev is assigned in sba_async_register().

Now, if we are calling sba_async_register() after
sba_prealloc_channel_resources() then we cannot
use sba->dma_dev.dev in sba_prealloc_channel_resources()

Regards,
Anup

[PATCH v2 11/16] dmaengine: bcm-sba-raid: Peek mbox when we have no free requests

[Anup Patel]

When setting up RAID array on several NVMe disks we observed that
sba_alloc_request() start failing (due to no free requests left)
and RAID array setup becomes very slow.

To improve performance, we do mbox channel peek when we have
no free requests. This improves performance of RAID array setup
because mbox requests that were completed but not processed by
mbox completion worker will be processed immediately by mbox
channel peek.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index f14ed0a..399250e 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -200,6 +200,14 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 b1, u32 b0)
 
 /* ====== General helper routines ===== */
 
+static void sba_peek_mchans(struct sba_device *sba)
+{
+	int mchan_idx;
+
+	for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
+		mbox_client_peek_data(sba->mchans[mchan_idx]);
+}
+
 static struct sba_request *sba_alloc_request(struct sba_device *sba)
 {
 	unsigned long flags;
@@ -211,8 +219,17 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	if (req)
 		list_move_tail(&req->node, &sba->reqs_alloc_list);
 	spin_unlock_irqrestore(&sba->reqs_lock, flags);
-	if (!req)
+
+	if (!req) {
+		/*
+		 * We have no more free requests so, we peek
+		 * mailbox channels hoping few active requests
+		 * would have completed which will create more
+		 * room for new requests.
+		 */
+		sba_peek_mchans(sba);
 		return NULL;
+	}
 
 	req->flags = SBA_REQUEST_STATE_ALLOCED;
 	req->first = req;
@@ -560,17 +577,15 @@ static enum dma_status sba_tx_status(struct dma_chan *dchan,
 				     dma_cookie_t cookie,
 				     struct dma_tx_state *txstate)
 {
-	int mchan_idx;
 	enum dma_status ret;
 	struct sba_device *sba = to_sba_device(dchan);
 
-	for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
-		mbox_client_peek_data(sba->mchans[mchan_idx]);
-
 	ret = dma_cookie_status(dchan, cookie, txstate);
 	if (ret == DMA_COMPLETE)
 		return ret;
 
+	sba_peek_mchans(sba);
+
 	return dma_cookie_status(dchan, cookie, txstate);
 }
 
-- 
2.7.4

Re: [PATCH v2 11/16] dmaengine: bcm-sba-raid: Peek mbox when we have no free requests

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:55PM +0530, Anup Patel wrote:
> When setting up RAID array on several NVMe disks we observed that
> sba_alloc_request() start failing (due to no free requests left)
> and RAID array setup becomes very slow.
> 
> To improve performance, we do mbox channel peek when we have
> no free requests. This improves performance of RAID array setup
> because mbox requests that were completed but not processed by
> mbox completion worker will be processed immediately by mbox
> channel peek.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index f14ed0a..399250e 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -200,6 +200,14 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 b1, u32 b0)
>  
>  /* ====== General helper routines ===== */
>  
> +static void sba_peek_mchans(struct sba_device *sba)
> +{
> +	int mchan_idx;
> +
> +	for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
> +		mbox_client_peek_data(sba->mchans[mchan_idx]);
> +}
> +
>  static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  {
>  	unsigned long flags;
> @@ -211,8 +219,17 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
>  	if (req)
>  		list_move_tail(&req->node, &sba->reqs_alloc_list);
>  	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> -	if (!req)
> +
> +	if (!req) {
> +		/*
> +		 * We have no more free requests so, we peek
> +		 * mailbox channels hoping few active requests
> +		 * would have completed which will create more
> +		 * room for new requests.
> +		 */
> +		sba_peek_mchans(sba);
>  		return NULL;
> +	}
>  
>  	req->flags = SBA_REQUEST_STATE_ALLOCED;
>  	req->first = req;
> @@ -560,17 +577,15 @@ static enum dma_status sba_tx_status(struct dma_chan *dchan,
>  				     dma_cookie_t cookie,
>  				     struct dma_tx_state *txstate)
>  {
> -	int mchan_idx;
>  	enum dma_status ret;
>  	struct sba_device *sba = to_sba_device(dchan);
>  
> -	for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
> -		mbox_client_peek_data(sba->mchans[mchan_idx]);
> -
>  	ret = dma_cookie_status(dchan, cookie, txstate);
>  	if (ret == DMA_COMPLETE)
>  		return ret;
>  
> +	sba_peek_mchans(sba);

why do you want to do this while checking status..?

-- 
~Vinod

Re: [PATCH v2 11/16] dmaengine: bcm-sba-raid: Peek mbox when we have no free requests

[Anup Patel]

On Thu, Aug 17, 2017 at 12:10 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:55PM +0530, Anup Patel wrote:
>> When setting up RAID array on several NVMe disks we observed that
>> sba_alloc_request() start failing (due to no free requests left)
>> and RAID array setup becomes very slow.
>>
>> To improve performance, we do mbox channel peek when we have
>> no free requests. This improves performance of RAID array setup
>> because mbox requests that were completed but not processed by
>> mbox completion worker will be processed immediately by mbox
>> channel peek.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>  drivers/dma/bcm-sba-raid.c | 25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index f14ed0a..399250e 100644
>> --- a/drivers/dma/bcm-sba-raid.c
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -200,6 +200,14 @@ static inline u32 __pure sba_cmd_pq_c_mdata(u32 d, u32 b1, u32 b0)
>>
>>  /* ====== General helper routines ===== */
>>
>> +static void sba_peek_mchans(struct sba_device *sba)
>> +{
>> +     int mchan_idx;
>> +
>> +     for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
>> +             mbox_client_peek_data(sba->mchans[mchan_idx]);
>> +}
>> +
>>  static struct sba_request *sba_alloc_request(struct sba_device *sba)
>>  {
>>       unsigned long flags;
>> @@ -211,8 +219,17 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
>>       if (req)
>>               list_move_tail(&req->node, &sba->reqs_alloc_list);
>>       spin_unlock_irqrestore(&sba->reqs_lock, flags);
>> -     if (!req)
>> +
>> +     if (!req) {
>> +             /*
>> +              * We have no more free requests so, we peek
>> +              * mailbox channels hoping few active requests
>> +              * would have completed which will create more
>> +              * room for new requests.
>> +              */
>> +             sba_peek_mchans(sba);
>>               return NULL;
>> +     }
>>
>>       req->flags = SBA_REQUEST_STATE_ALLOCED;
>>       req->first = req;
>> @@ -560,17 +577,15 @@ static enum dma_status sba_tx_status(struct dma_chan *dchan,
>>                                    dma_cookie_t cookie,
>>                                    struct dma_tx_state *txstate)
>>  {
>> -     int mchan_idx;
>>       enum dma_status ret;
>>       struct sba_device *sba = to_sba_device(dchan);
>>
>> -     for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
>> -             mbox_client_peek_data(sba->mchans[mchan_idx]);
>> -
>>       ret = dma_cookie_status(dchan, cookie, txstate);
>>       if (ret == DMA_COMPLETE)
>>               return ret;
>>
>> +     sba_peek_mchans(sba);
>
> why do you want to do this while checking status..?

The dma_tx_state is only updated via sba_receive_message()
which in-turn is called by mailbox framework upon completion
of a request.

Placing the sba_peek_mchans() here helps polling based
DMA client by not waiting for IRQ worker to schedule and
process the completions.

Regards,
Anup

[PATCH v2 12/16] dmaengine: bcm-sba-raid: Pre-ack async tx descriptor

[Anup Patel]

We should pre-ack async tx descriptor at time of
allocating sba_request (just like other RAID drivers).

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 399250e..2f444cc 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -237,6 +237,7 @@ static struct sba_request *sba_alloc_request(struct sba_device *sba)
 	atomic_set(&req->next_pending_count, 1);
 
 	dma_async_tx_descriptor_init(&req->tx, &sba->dma_chan);
+	async_tx_ack(&req->tx);
 
 	return req;
 }
-- 
2.7.4

[PATCH v2 13/16] dmaengine: bcm-sba-raid: Re-factor sba_process_deferred_requests()

[Anup Patel]

Currently, sba_process_deferred_requests() handles both pending
and completed sba_request which is unnecessary overhead for
sba_issue_pending() because completed sba_request handling is
not required in sba_issue_pending().

This patch breaks sba_process_deferred_requests() into two parts
sba_process_received_request() and _sba_process_pending_requests().

The sba_issue_pending() will only process pending sba_request
by calling _sba_process_pending_requests(). This will improve
sba_issue_pending().

The sba_receive_message() will only process received sba_request
by calling sba_process_received_request() for each received
sba_request. The sba_process_received_request() will also call
_sba_process_pending_requests() after handling received sba_request
because we might have pending sba_request not submitted by previous
call to sba_issue_pending().

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 109 +++++++++++++++++++--------------------------
 1 file changed, 47 insertions(+), 62 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index 2f444cc..cb6b2e2 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -419,22 +419,20 @@ static int sba_send_mbox_request(struct sba_device *sba,
 	return 0;
 }
 
-static void sba_process_deferred_requests(struct sba_device *sba)
+/* Note: Must be called with sba->reqs_lock held */
+static void _sba_process_pending_requests(struct sba_device *sba)
 {
 	int ret;
 	u32 count;
-	unsigned long flags;
 	struct sba_request *req;
-	struct dma_async_tx_descriptor *tx;
-
-	spin_lock_irqsave(&sba->reqs_lock, flags);
-
-	/* Count pending requests */
-	count = 0;
-	list_for_each_entry(req, &sba->reqs_pending_list, node)
-		count++;
 
-	/* Process pending requests */
+	/*
+	 * Process few pending requests
+	 *
+	 * For now, we process (<number_of_mailbox_channels> * 8)
+	 * number of requests at a time.
+	 */
+	count = sba->mchans_count * 8;
 	while (!list_empty(&sba->reqs_pending_list) && count) {
 		/* Get the first pending request */
 		req = list_first_entry(&sba->reqs_pending_list,
@@ -445,11 +443,7 @@ static void sba_process_deferred_requests(struct sba_device *sba)
 			break;
 
 		/* Send request to mailbox channel */
-		spin_unlock_irqrestore(&sba->reqs_lock, flags);
 		ret = sba_send_mbox_request(sba, req);
-		spin_lock_irqsave(&sba->reqs_lock, flags);
-
-		/* If something went wrong then keep request pending */
 		if (ret < 0) {
 			_sba_pending_request(sba, req);
 			break;
@@ -457,20 +451,18 @@ static void sba_process_deferred_requests(struct sba_device *sba)
 
 		count--;
 	}
+}
 
-	/* Count completed requests */
-	count = 0;
-	list_for_each_entry(req, &sba->reqs_completed_list, node)
-		count++;
-
-	/* Process completed requests */
-	while (!list_empty(&sba->reqs_completed_list) && count) {
-		req = list_first_entry(&sba->reqs_completed_list,
-					struct sba_request, node);
-		list_del_init(&req->node);
-		tx = &req->tx;
+static void sba_process_received_request(struct sba_device *sba,
+					 struct sba_request *req)
+{
+	unsigned long flags;
+	struct dma_async_tx_descriptor *tx;
+	struct sba_request *nreq, *first = req->first;
 
-		spin_unlock_irqrestore(&sba->reqs_lock, flags);
+	/* Process only after all chained requests are received */
+	if (!atomic_dec_return(&first->next_pending_count)) {
+		tx = &first->tx;
 
 		WARN_ON(tx->cookie < 0);
 		if (tx->cookie > 0) {
@@ -485,41 +477,34 @@ static void sba_process_deferred_requests(struct sba_device *sba)
 
 		spin_lock_irqsave(&sba->reqs_lock, flags);
 
-		/* If waiting for 'ack' then move to completed list */
-		if (!async_tx_test_ack(&req->tx))
-			_sba_complete_request(sba, req);
-		else
-			_sba_free_request(sba, req);
+		/* Free all requests chained to first request */
+		list_for_each_entry(nreq, &first->next, next)
+			_sba_free_request(sba, nreq);
+		INIT_LIST_HEAD(&first->next);
 
-		count--;
-	}
+		/* Mark request as received */
+		_sba_received_request(sba, first);
 
-	/* Re-check pending and completed work */
-	count = 0;
-	if (!list_empty(&sba->reqs_pending_list) ||
-	    !list_empty(&sba->reqs_completed_list))
-		count = 1;
-
-	spin_unlock_irqrestore(&sba->reqs_lock, flags);
-}
-
-static void sba_process_received_request(struct sba_device *sba,
-					 struct sba_request *req)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&sba->reqs_lock, flags);
+		/* The client is allowed to attach dependent operations
+		 * until 'ack' is set
+		 */
+		if (!async_tx_test_ack(tx))
+			_sba_complete_request(sba, first);
+		else
+			_sba_free_request(sba, first);
 
-	/* Mark request as received */
-	_sba_received_request(sba, req);
+		/* Cleanup completed requests */
+		list_for_each_entry_safe(req, nreq,
+					 &sba->reqs_completed_list, node) {
+			if (async_tx_test_ack(&req->tx))
+				_sba_free_request(sba, req);
+		}
 
-	/* Update request */
-	if (!atomic_dec_return(&req->first->next_pending_count))
-		_sba_complete_request(sba, req->first);
-	if (req->first != req)
-		_sba_free_request(sba, req);
+		/* Process pending requests */
+		_sba_process_pending_requests(sba);
 
-	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+		spin_unlock_irqrestore(&sba->reqs_lock, flags);
+	}
 }
 
 /* ====== DMAENGINE callbacks ===== */
@@ -544,10 +529,13 @@ static int sba_device_terminate_all(struct dma_chan *dchan)
 
 static void sba_issue_pending(struct dma_chan *dchan)
 {
+	unsigned long flags;
 	struct sba_device *sba = to_sba_device(dchan);
 
-	/* Process deferred requests */
-	sba_process_deferred_requests(sba);
+	/* Process pending requests */
+	spin_lock_irqsave(&sba->reqs_lock, flags);
+	_sba_process_pending_requests(sba);
+	spin_unlock_irqrestore(&sba->reqs_lock, flags);
 }
 
 static dma_cookie_t sba_tx_submit(struct dma_async_tx_descriptor *tx)
@@ -1482,9 +1470,6 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
 
 	/* Process received request */
 	sba_process_received_request(sba, req);
-
-	/* Process deferred requests */
-	sba_process_deferred_requests(sba);
 }
 
 /* ====== Platform driver routines ===== */
-- 
2.7.4

[PATCH v2 14/16] dmaengine: bcm-sba-raid: Remove redundant SBA_REQUEST_STATE_RECEIVED

[Anup Patel]

The SBA_REQUEST_STATE_RECEIVED state is now redundant because
received sba_request are immediately freed or moved to completed
list in sba_process_received_request().

This patch removes redundant SBA_REQUEST_STATE_RECEIVED state.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index cb6b2e2..f0a0e80 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -98,9 +98,8 @@ enum sba_request_flags {
 	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_COMPLETED	= 0x010,
+	SBA_REQUEST_STATE_ABORTED	= 0x020,
 	SBA_REQUEST_STATE_MASK		= 0x0ff,
 	SBA_REQUEST_FENCE		= 0x100,
 };
@@ -160,7 +159,6 @@ struct sba_device {
 	struct list_head reqs_alloc_list;
 	struct list_head reqs_pending_list;
 	struct list_head reqs_active_list;
-	struct list_head reqs_received_list;
 	struct list_head reqs_completed_list;
 	struct list_head reqs_aborted_list;
 	struct list_head reqs_free_list;
@@ -307,18 +305,6 @@ static void _sba_complete_request(struct sba_device *sba,
 		sba->reqs_fence = false;
 }
 
-/* Note: Must be called with sba->reqs_lock held */
-static void _sba_received_request(struct sba_device *sba,
-				  struct sba_request *req)
-{
-	lockdep_assert_held(&sba->reqs_lock);
-	req->flags &= ~SBA_REQUEST_STATE_MASK;
-	req->flags |= SBA_REQUEST_STATE_RECEIVED;
-	list_move_tail(&req->node, &sba->reqs_received_list);
-	if (list_empty(&sba->reqs_active_list))
-		sba->reqs_fence = false;
-}
-
 static void sba_free_chained_requests(struct sba_request *req)
 {
 	unsigned long flags;
@@ -360,10 +346,6 @@ static void sba_cleanup_nonpending_requests(struct sba_device *sba)
 	list_for_each_entry_safe(req, req1, &sba->reqs_alloc_list, node)
 		_sba_free_request(sba, req);
 
-	/* Freeup all received request */
-	list_for_each_entry_safe(req, req1, &sba->reqs_received_list, node)
-		_sba_free_request(sba, req);
-
 	/* Freeup all completed request */
 	list_for_each_entry_safe(req, req1, &sba->reqs_completed_list, node)
 		_sba_free_request(sba, req);
@@ -482,9 +464,6 @@ static void sba_process_received_request(struct sba_device *sba,
 			_sba_free_request(sba, nreq);
 		INIT_LIST_HEAD(&first->next);
 
-		/* Mark request as received */
-		_sba_received_request(sba, first);
-
 		/* The client is allowed to attach dependent operations
 		 * until 'ack' is set
 		 */
@@ -1498,7 +1477,6 @@ static int sba_prealloc_channel_resources(struct sba_device *sba)
 	INIT_LIST_HEAD(&sba->reqs_alloc_list);
 	INIT_LIST_HEAD(&sba->reqs_pending_list);
 	INIT_LIST_HEAD(&sba->reqs_active_list);
-	INIT_LIST_HEAD(&sba->reqs_received_list);
 	INIT_LIST_HEAD(&sba->reqs_completed_list);
 	INIT_LIST_HEAD(&sba->reqs_aborted_list);
 	INIT_LIST_HEAD(&sba->reqs_free_list);
-- 
2.7.4

[PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Anup Patel]

This patch adds debugfs support to report stats via debugfs
which in-turn will help debug hang or error situations.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index f0a0e80..f9d110c 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -36,6 +36,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/debugfs.h>
 #include <linux/dma-mapping.h>
 #include <linux/dmaengine.h>
 #include <linux/list.h>
@@ -162,6 +163,9 @@ struct sba_device {
 	struct list_head reqs_completed_list;
 	struct list_head reqs_aborted_list;
 	struct list_head reqs_free_list;
+	/* DebugFS directory entries */
+	struct dentry *root;
+	struct dentry *stats;
 };
 
 /* ====== Command helper routines ===== */
@@ -486,6 +490,45 @@ static void sba_process_received_request(struct sba_device *sba,
 	}
 }
 
+static void sba_write_stats_in_seqfile(struct sba_device *sba,
+				       struct seq_file *file)
+{
+	unsigned long flags;
+	struct sba_request *req;
+	u32 free_count = 0, alloced_count = 0, pending_count = 0;
+	u32 active_count = 0, aborted_count = 0, completed_count = 0;
+
+	spin_lock_irqsave(&sba->reqs_lock, flags);
+
+	list_for_each_entry(req, &sba->reqs_free_list, node)
+		free_count++;
+
+	list_for_each_entry(req, &sba->reqs_alloc_list, node)
+		alloced_count++;
+
+	list_for_each_entry(req, &sba->reqs_pending_list, node)
+		pending_count++;
+
+	list_for_each_entry(req, &sba->reqs_active_list, node)
+		active_count++;
+
+	list_for_each_entry(req, &sba->reqs_aborted_list, node)
+		aborted_count++;
+
+	list_for_each_entry(req, &sba->reqs_completed_list, node)
+		completed_count++;
+
+	spin_unlock_irqrestore(&sba->reqs_lock, flags);
+
+	seq_printf(file, "maximum requests   = %d\n", sba->max_req);
+	seq_printf(file, "free requests      = %d\n", free_count);
+	seq_printf(file, "alloced requests   = %d\n", alloced_count);
+	seq_printf(file, "pending requests   = %d\n", pending_count);
+	seq_printf(file, "active requests    = %d\n", active_count);
+	seq_printf(file, "aborted requests   = %d\n", aborted_count);
+	seq_printf(file, "completed requests = %d\n", completed_count);
+}
+
 /* ====== DMAENGINE callbacks ===== */
 
 static void sba_free_chan_resources(struct dma_chan *dchan)
@@ -1451,6 +1494,19 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
 	sba_process_received_request(sba, req);
 }
 
+/* ====== Debugfs callbacks ====== */
+
+static int sba_debugfs_stats_show(struct seq_file *file, void *offset)
+{
+	struct platform_device *pdev = to_platform_device(file->private);
+	struct sba_device *sba = platform_get_drvdata(pdev);
+
+	/* Write stats in file */
+	sba_write_stats_in_seqfile(sba, file);
+
+	return 0;
+}
+
 /* ====== Platform driver routines ===== */
 
 static int sba_prealloc_channel_resources(struct sba_device *sba)
@@ -1721,10 +1777,30 @@ static int sba_probe(struct platform_device *pdev)
 	if (ret)
 		goto fail_free_mchans;
 
+	/* Check availability of debugfs */
+	if (!debugfs_initialized())
+		goto skip_debugfs;
+
+	/* Create debugfs root entry */
+	sba->root = debugfs_create_dir(dev_name(sba->dev), NULL);
+	if (IS_ERR_OR_NULL(sba->root)) {
+		ret = PTR_ERR_OR_ZERO(sba->root);
+		goto fail_free_resources;
+	}
+
+	/* Create debugfs stats entry */
+	sba->stats = debugfs_create_devm_seqfile(sba->dev, "stats", sba->root,
+						 sba_debugfs_stats_show);
+	if (IS_ERR_OR_NULL(sba->stats)) {
+		ret = PTR_ERR_OR_ZERO(sba->stats);
+		goto fail_free_debugfs_root;
+	}
+skip_debugfs:
+
 	/* Register DMA device with Linux async framework */
 	ret = sba_async_register(sba);
 	if (ret)
-		goto fail_free_resources;
+		goto fail_free_debugfs_root;
 
 	/* Print device info */
 	dev_info(sba->dev, "%s using SBAv%d and %d mailbox channels",
@@ -1733,6 +1809,8 @@ static int sba_probe(struct platform_device *pdev)
 
 	return 0;
 
+fail_free_debugfs_root:
+	debugfs_remove_recursive(sba->root);
 fail_free_resources:
 	sba_freeup_channel_resources(sba);
 fail_free_mchans:
@@ -1748,6 +1826,8 @@ static int sba_remove(struct platform_device *pdev)
 
 	dma_async_device_unregister(&sba->dma_dev);
 
+	debugfs_remove_recursive(sba->root);
+
 	sba_freeup_channel_resources(sba);
 
 	for (i = 0; i < sba->mchans_count; i++)
-- 
2.7.4

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Vinod Koul]

On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
> This patch adds debugfs support to report stats via debugfs
> which in-turn will help debug hang or error situations.
> 
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  drivers/dma/bcm-sba-raid.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> index f0a0e80..f9d110c 100644
> --- a/drivers/dma/bcm-sba-raid.c
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -36,6 +36,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/debugfs.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/dmaengine.h>
>  #include <linux/list.h>
> @@ -162,6 +163,9 @@ struct sba_device {
>  	struct list_head reqs_completed_list;
>  	struct list_head reqs_aborted_list;
>  	struct list_head reqs_free_list;
> +	/* DebugFS directory entries */
> +	struct dentry *root;
> +	struct dentry *stats;
>  };
>  
>  /* ====== Command helper routines ===== */
> @@ -486,6 +490,45 @@ static void sba_process_received_request(struct sba_device *sba,
>  	}
>  }
>  
> +static void sba_write_stats_in_seqfile(struct sba_device *sba,
> +				       struct seq_file *file)
> +{
> +	unsigned long flags;
> +	struct sba_request *req;
> +	u32 free_count = 0, alloced_count = 0, pending_count = 0;
> +	u32 active_count = 0, aborted_count = 0, completed_count = 0;
> +
> +	spin_lock_irqsave(&sba->reqs_lock, flags);
> +
> +	list_for_each_entry(req, &sba->reqs_free_list, node)
> +		free_count++;
> +
> +	list_for_each_entry(req, &sba->reqs_alloc_list, node)
> +		alloced_count++;
> +
> +	list_for_each_entry(req, &sba->reqs_pending_list, node)
> +		pending_count++;
> +
> +	list_for_each_entry(req, &sba->reqs_active_list, node)
> +		active_count++;
> +
> +	list_for_each_entry(req, &sba->reqs_aborted_list, node)
> +		aborted_count++;
> +
> +	list_for_each_entry(req, &sba->reqs_completed_list, node)
> +		completed_count++;
> +
> +	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +
> +	seq_printf(file, "maximum requests   = %d\n", sba->max_req);
> +	seq_printf(file, "free requests      = %d\n", free_count);
> +	seq_printf(file, "alloced requests   = %d\n", alloced_count);
> +	seq_printf(file, "pending requests   = %d\n", pending_count);
> +	seq_printf(file, "active requests    = %d\n", active_count);
> +	seq_printf(file, "aborted requests   = %d\n", aborted_count);
> +	seq_printf(file, "completed requests = %d\n", completed_count);
> +}
> +
>  /* ====== DMAENGINE callbacks ===== */
>  
>  static void sba_free_chan_resources(struct dma_chan *dchan)
> @@ -1451,6 +1494,19 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
>  	sba_process_received_request(sba, req);
>  }
>  
> +/* ====== Debugfs callbacks ====== */
> +
> +static int sba_debugfs_stats_show(struct seq_file *file, void *offset)
> +{
> +	struct platform_device *pdev = to_platform_device(file->private);
> +	struct sba_device *sba = platform_get_drvdata(pdev);
> +
> +	/* Write stats in file */
> +	sba_write_stats_in_seqfile(sba, file);
> +
> +	return 0;
> +}
> +
>  /* ====== Platform driver routines ===== */
>  
>  static int sba_prealloc_channel_resources(struct sba_device *sba)
> @@ -1721,10 +1777,30 @@ static int sba_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto fail_free_mchans;
>  
> +	/* Check availability of debugfs */
> +	if (!debugfs_initialized())
> +		goto skip_debugfs;
> +
> +	/* Create debugfs root entry */
> +	sba->root = debugfs_create_dir(dev_name(sba->dev), NULL);
> +	if (IS_ERR_OR_NULL(sba->root)) {
> +		ret = PTR_ERR_OR_ZERO(sba->root);
> +		goto fail_free_resources;

why fail, debugfs should be an optional thingy, why would you want to fail here?

-- 
~Vinod

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Anup Patel]

On Thu, Aug 17, 2017 at 1:31 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
>> This patch adds debugfs support to report stats via debugfs
>> which in-turn will help debug hang or error situations.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> ---
>>  drivers/dma/bcm-sba-raid.c | 82 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> index f0a0e80..f9d110c 100644
>> --- a/drivers/dma/bcm-sba-raid.c
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -36,6 +36,7 @@
>>   */
>>
>>  #include <linux/bitops.h>
>> +#include <linux/debugfs.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/dmaengine.h>
>>  #include <linux/list.h>
>> @@ -162,6 +163,9 @@ struct sba_device {
>>       struct list_head reqs_completed_list;
>>       struct list_head reqs_aborted_list;
>>       struct list_head reqs_free_list;
>> +     /* DebugFS directory entries */
>> +     struct dentry *root;
>> +     struct dentry *stats;
>>  };
>>
>>  /* ====== Command helper routines ===== */
>> @@ -486,6 +490,45 @@ static void sba_process_received_request(struct sba_device *sba,
>>       }
>>  }
>>
>> +static void sba_write_stats_in_seqfile(struct sba_device *sba,
>> +                                    struct seq_file *file)
>> +{
>> +     unsigned long flags;
>> +     struct sba_request *req;
>> +     u32 free_count = 0, alloced_count = 0, pending_count = 0;
>> +     u32 active_count = 0, aborted_count = 0, completed_count = 0;
>> +
>> +     spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> +     list_for_each_entry(req, &sba->reqs_free_list, node)
>> +             free_count++;
>> +
>> +     list_for_each_entry(req, &sba->reqs_alloc_list, node)
>> +             alloced_count++;
>> +
>> +     list_for_each_entry(req, &sba->reqs_pending_list, node)
>> +             pending_count++;
>> +
>> +     list_for_each_entry(req, &sba->reqs_active_list, node)
>> +             active_count++;
>> +
>> +     list_for_each_entry(req, &sba->reqs_aborted_list, node)
>> +             aborted_count++;
>> +
>> +     list_for_each_entry(req, &sba->reqs_completed_list, node)
>> +             completed_count++;
>> +
>> +     spin_unlock_irqrestore(&sba->reqs_lock, flags);
>> +
>> +     seq_printf(file, "maximum requests   = %d\n", sba->max_req);
>> +     seq_printf(file, "free requests      = %d\n", free_count);
>> +     seq_printf(file, "alloced requests   = %d\n", alloced_count);
>> +     seq_printf(file, "pending requests   = %d\n", pending_count);
>> +     seq_printf(file, "active requests    = %d\n", active_count);
>> +     seq_printf(file, "aborted requests   = %d\n", aborted_count);
>> +     seq_printf(file, "completed requests = %d\n", completed_count);
>> +}
>> +
>>  /* ====== DMAENGINE callbacks ===== */
>>
>>  static void sba_free_chan_resources(struct dma_chan *dchan)
>> @@ -1451,6 +1494,19 @@ static void sba_receive_message(struct mbox_client *cl, void *msg)
>>       sba_process_received_request(sba, req);
>>  }
>>
>> +/* ====== Debugfs callbacks ====== */
>> +
>> +static int sba_debugfs_stats_show(struct seq_file *file, void *offset)
>> +{
>> +     struct platform_device *pdev = to_platform_device(file->private);
>> +     struct sba_device *sba = platform_get_drvdata(pdev);
>> +
>> +     /* Write stats in file */
>> +     sba_write_stats_in_seqfile(sba, file);
>> +
>> +     return 0;
>> +}
>> +
>>  /* ====== Platform driver routines ===== */
>>
>>  static int sba_prealloc_channel_resources(struct sba_device *sba)
>> @@ -1721,10 +1777,30 @@ static int sba_probe(struct platform_device *pdev)
>>       if (ret)
>>               goto fail_free_mchans;
>>
>> +     /* Check availability of debugfs */
>> +     if (!debugfs_initialized())
>> +             goto skip_debugfs;
>> +
>> +     /* Create debugfs root entry */
>> +     sba->root = debugfs_create_dir(dev_name(sba->dev), NULL);
>> +     if (IS_ERR_OR_NULL(sba->root)) {
>> +             ret = PTR_ERR_OR_ZERO(sba->root);
>> +             goto fail_free_resources;
>
> why fail, debugfs should be an optional thingy, why would you want to fail here?

Yes, we are handling the case when debugfs is not available
and skipping debugfs gracefully.

If debugfs is available then failure of debugfs_create_dir()
should be reported.

Regards,
Anup

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Vinod Koul]

On Fri, Aug 18, 2017 at 10:33:54AM +0530, Anup Patel wrote:
> On Thu, Aug 17, 2017 at 1:31 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
> > why fail, debugfs should be an optional thingy, why would you want to fail here?
> 
> Yes, we are handling the case when debugfs is not available
> and skipping debugfs gracefully.
> 
> If debugfs is available then failure of debugfs_create_dir()
> should be reported.

reported yes, bailing out on that err no..

-- 
~Vinod

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Anup Patel]

On Fri, Aug 18, 2017 at 10:56 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Fri, Aug 18, 2017 at 10:33:54AM +0530, Anup Patel wrote:
>> On Thu, Aug 17, 2017 at 1:31 PM, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
>> > why fail, debugfs should be an optional thingy, why would you want to fail here?
>>
>> Yes, we are handling the case when debugfs is not available
>> and skipping debugfs gracefully.
>>
>> If debugfs is available then failure of debugfs_create_dir()
>> should be reported.
>
> reported yes, bailing out on that err no..

OK, I will put dev_err() instead of bailing out.

Regards,
Anup

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Greg KH]

On Fri, Aug 18, 2017 at 10:56:13AM +0530, Vinod Koul wrote:
> On Fri, Aug 18, 2017 at 10:33:54AM +0530, Anup Patel wrote:
> > On Thu, Aug 17, 2017 at 1:31 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> > > On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
> > > why fail, debugfs should be an optional thingy, why would you want to fail here?
> > 
> > Yes, we are handling the case when debugfs is not available
> > and skipping debugfs gracefully.
> > 
> > If debugfs is available then failure of debugfs_create_dir()
> > should be reported.
> 
> reported yes, bailing out on that err no..

Reported, no.  You should _never_ care about the return value of a
debugfs call.  Never check it, just move on ad keep on going.  It
doesn't matter.

thanks,

greg k-h

Re: [PATCH v2 15/16] dmaengine: bcm-sba-raid: Add debugfs support

[Vinod Koul]

On Thu, Sep 07, 2017 at 09:37:32PM +0200, Greg KH wrote:
> On Fri, Aug 18, 2017 at 10:56:13AM +0530, Vinod Koul wrote:
> > On Fri, Aug 18, 2017 at 10:33:54AM +0530, Anup Patel wrote:
> > > On Thu, Aug 17, 2017 at 1:31 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> > > > On Tue, Aug 01, 2017 at 04:07:59PM +0530, Anup Patel wrote:
> > > > why fail, debugfs should be an optional thingy, why would you want to fail here?
> > > 
> > > Yes, we are handling the case when debugfs is not available
> > > and skipping debugfs gracefully.
> > > 
> > > If debugfs is available then failure of debugfs_create_dir()
> > > should be reported.
> > 
> > reported yes, bailing out on that err no..
> 
> Reported, no.  You should _never_ care about the return value of a
> debugfs call.  Never check it, just move on ad keep on going.  It
> doesn't matter.

Agreed that makes more sense. The driver was checking and bailing out, I
advised against that :)

-- 
~Vinod

[PATCH v2 16/16] dmaengine: bcm-sba-raid: Explicitly ACK mailbox message after sending

[Anup Patel]

We should explicitly ACK mailbox message because after
sending message we can know the send status via error
attribute of brcm_message.

This will also help SBA-RAID to use "txdone_ack" method
whenever mailbox controller supports it.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/dma/bcm-sba-raid.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
index f9d110c..730e1a0 100644
--- a/drivers/dma/bcm-sba-raid.c
+++ b/drivers/dma/bcm-sba-raid.c
@@ -396,13 +396,17 @@ static int sba_send_mbox_request(struct sba_device *sba,
 		dev_err(sba->dev, "send message failed with error %d", ret);
 		return ret;
 	}
+
+	/* Check error returned by mailbox controller */
 	ret = req->msg.error;
 	if (ret < 0) {
 		dev_err(sba->dev, "message error %d", ret);
-		return ret;
 	}
 
-	return 0;
+	/* Signal txdone for mailbox channel */
+	mbox_client_txdone(sba->mchans[mchans_idx], ret);
+
+	return ret;
 }
 
 /* Note: Must be called with sba->reqs_lock held */
@@ -1724,7 +1728,7 @@ static int sba_probe(struct platform_device *pdev)
 	sba->client.dev			= &pdev->dev;
 	sba->client.rx_callback		= sba_receive_message;
 	sba->client.tx_block		= false;
-	sba->client.knows_txdone	= false;
+	sba->client.knows_txdone	= true;
 	sba->client.tx_tout		= 0;
 
 	/* Allocate mailbox channel array */
-- 
2.7.4