Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1572139 > unrolled thread

[PATCH 0/6] Broadcom SBA RAID support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] Broadcom SBA RAID support Anup Patel <anup.patel@broadcom.com> - 2017-02-02 05:50 +0100
    [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients Anup Patel <anup.patel@broadcom.com> - 2017-02-02 05:50 +0100
      Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer  PQ coefficients Dan Williams <dan.j.williams@intel.com> - 2017-02-02 07:10 +0100
        Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer  PQ coefficients Anup Patel <anup.patel@broadcom.com> - 2017-02-03 12:10 +0100
        Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer  PQ coefficients Dan Williams <dan.j.williams@intel.com> - 2017-02-03 19:50 +0100
          Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer  PQ coefficients Anup Patel <anup.patel@broadcom.com> - 2017-02-06 05:00 +0100
    [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients Anup Patel <anup.patel@broadcom.com> - 2017-02-02 05:50 +0100
      Re: [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients Jassi Brar <jassisinghbrar@gmail.com> - 2017-02-03 13:10 +0100
        Re: [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients Anup Patel <anup.patel@broadcom.com> - 2017-02-06 05:30 +0100
    [PATCH 4/6] async_tx: Fix DMA_PREP_FENCE usage in do_async_gen_syndrome() Anup Patel <anup.patel@broadcom.com> - 2017-02-02 05:50 +0100
    [PATCH 6/6] dt-bindings: Add DT bindings document for Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-02 05:50 +0100
    Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver Vinod Koul <vinod.koul@intel.com> - 2017-02-05 07:10 +0100
      Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-06 13:10 +0100
        Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver Vinod Koul <vinod.koul@intel.com> - 2017-02-06 18:00 +0100
          Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-07 07:10 +0100

#1572139 — [PATCH 0/6] Broadcom SBA RAID support

The Broadcom SBA RAID is a stream-based device which provides
RAID5/6 offload.

It requires a SoC specific ring manager (such as Broadcom FlexRM
ring manager) to provide ring-based programming interface. Due to
this, the Broadcom SBA RAID driver (mailbox client) implements
DMA device having one DMA channel using a set of mailbox channels
provided by Broadcom SoC specific ring manager driver (mailbox
controller).

Important limitations of Broadcom SBA RAID hardware are:
1. Requires disk position instead of disk coefficient 
2. Supports only 30 PQ disk coefficients

To address limitation #1, we have added raid_gflog table which
will help driver convert disk coefficient to disk position. To
address limitation #2, we have extended Linux Async Tx APIs to
check for available PQ coefficients before doing PQ offload.

This patchset is based on Linux-4.10-rc6 and depends on patchset
"[PATCH v4 0/2] Broadcom FlexRM ring manager support"

It is also available at sba-raid-v1 branch of
https://github.com/Broadcom/arm64-linux.git

Anup Patel (6):
  mailbox: Add new API mbox_channel_device() for clients
  lib/raid6: Add log-of-2 table for RAID6 HW requiring disk position
  async_tx: Handle DMA devices having support for fewer PQ coefficients
  async_tx: Fix DMA_PREP_FENCE usage in do_async_gen_syndrome()
  dmaengine: Add Broadcom SBA RAID driver
  dt-bindings: Add DT bindings document for Broadcom SBA RAID driver

 .../devicetree/bindings/dma/brcm,iproc-sba.txt     |   29 +
 crypto/async_tx/async_pq.c                         |    8 +-
 crypto/async_tx/async_raid6_recov.c                |   12 +-
 drivers/dma/Kconfig                                |   13 +
 drivers/dma/Makefile                               |    1 +
 drivers/dma/bcm-sba-raid.c                         | 1309 ++++++++++++++++++++
 drivers/mailbox/mailbox.c                          |   21 +
 include/linux/dmaengine.h                          |   19 +
 include/linux/mailbox_client.h                     |    1 +
 include/linux/raid/pq.h                            |    4 +
 lib/raid6/mktables.c                               |   20 +
 11 files changed, 1432 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/dma/brcm,iproc-sba.txt
 create mode 100644 drivers/dma/bcm-sba-raid.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1572141 — [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients

The DMAENGINE framework assumes that if PQ offload is supported by a
DMA device then all 256 PQ coefficients are supported. This assumption
does not hold anymore because we now have BCM-SBA-RAID offload engine
which supports PQ offload with limited number of PQ coefficients.

This patch extends async_tx APIs to handle DMA devices with support
for fewer PQ coefficients.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
---
 crypto/async_tx/async_pq.c          |  3 +++
 crypto/async_tx/async_raid6_recov.c | 12 ++++++++++--
 include/linux/dmaengine.h           | 19 +++++++++++++++++++
 include/linux/raid/pq.h             |  3 +++
 4 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
index f83de99..16c6526 100644
--- a/crypto/async_tx/async_pq.c
+++ b/crypto/async_tx/async_pq.c
@@ -187,6 +187,9 @@ async_gen_syndrome(struct page **blocks, unsigned int offset, int disks,
 
 	BUG_ON(disks > 255 || !(P(blocks, disks) || Q(blocks, disks)));
 
+	if (device && dma_maxpqcoef(device) < src_cnt)
+		device = NULL;
+
 	if (device)
 		unmap = dmaengine_get_unmap_data(device->dev, disks, GFP_NOWAIT);
 
diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 8fab627..2916f95 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -352,6 +352,7 @@ async_raid6_2data_recov(int disks, size_t bytes, int faila, int failb,
 {
 	void *scribble = submit->scribble;
 	int non_zero_srcs, i;
+	struct dma_chan *chan = async_dma_find_channel(DMA_PQ);
 
 	BUG_ON(faila == failb);
 	if (failb < faila)
@@ -359,12 +360,15 @@ async_raid6_2data_recov(int disks, size_t bytes, int faila, int failb,
 
 	pr_debug("%s: disks: %d len: %zu\n", __func__, disks, bytes);
 
+	if (chan && dma_maxpqcoef(chan->device) < RAID6_PQ_MAX_COEF)
+		chan = NULL;
+
 	/* if a dma resource is not available or a scribble buffer is not
 	 * available punt to the synchronous path.  In the 'dma not
 	 * available' case be sure to use the scribble buffer to
 	 * preserve the content of 'blocks' as the caller intended.
 	 */
-	if (!async_dma_find_channel(DMA_PQ) || !scribble) {
+	if (!chan || !scribble) {
 		void **ptrs = scribble ? scribble : (void **) blocks;
 
 		async_tx_quiesce(&submit->depend_tx);
@@ -432,15 +436,19 @@ async_raid6_datap_recov(int disks, size_t bytes, int faila,
 	void *scribble = submit->scribble;
 	int good_srcs, good, i;
 	struct page *srcs[2];
+	struct dma_chan *chan = async_dma_find_channel(DMA_PQ);
 
 	pr_debug("%s: disks: %d len: %zu\n", __func__, disks, bytes);
 
+	if (chan && dma_maxpqcoef(chan->device) < RAID6_PQ_MAX_COEF)
+		chan = NULL;
+
 	/* if a dma resource is not available or a scribble buffer is not
 	 * available punt to the synchronous path.  In the 'dma not
 	 * available' case be sure to use the scribble buffer to
 	 * preserve the content of 'blocks' as the caller intended.
 	 */
-	if (!async_dma_find_channel(DMA_PQ) || !scribble) {
+	if (!chan || !scribble) {
 		void **ptrs = scribble ? scribble : (void **) blocks;
 
 		async_tx_quiesce(&submit->depend_tx);
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index feee6ec..d938a8b 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -24,6 +24,7 @@
 #include <linux/scatterlist.h>
 #include <linux/bitmap.h>
 #include <linux/types.h>
+#include <linux/raid/pq.h>
 #include <asm/page.h>
 
 /**
@@ -668,6 +669,7 @@ struct dma_filter {
  * @cap_mask: one or more dma_capability flags
  * @max_xor: maximum number of xor sources, 0 if no capability
  * @max_pq: maximum number of PQ sources and PQ-continue capability
+ * @max_pqcoef: maximum number of PQ coefficients, 0 if all supported
  * @copy_align: alignment shift for memcpy operations
  * @xor_align: alignment shift for xor operations
  * @pq_align: alignment shift for pq operations
@@ -727,11 +729,13 @@ struct dma_device {
 	dma_cap_mask_t  cap_mask;
 	unsigned short max_xor;
 	unsigned short max_pq;
+	unsigned short max_pqcoef;
 	enum dmaengine_alignment copy_align;
 	enum dmaengine_alignment xor_align;
 	enum dmaengine_alignment pq_align;
 	enum dmaengine_alignment fill_align;
 	#define DMA_HAS_PQ_CONTINUE (1 << 15)
+	#define DMA_HAS_FEWER_PQ_COEF (1 << 15)
 
 	int dev_id;
 	struct device *dev;
@@ -1122,6 +1126,21 @@ static inline int dma_maxpq(struct dma_device *dma, enum dma_ctrl_flags flags)
 	BUG();
 }
 
+static inline void dma_set_maxpqcoef(struct dma_device *dma,
+				     unsigned short max_pqcoef)
+{
+	if (max_pqcoef < RAID6_PQ_MAX_COEF) {
+		dma->max_pqcoef = max_pqcoef;
+		dma->max_pqcoef |= DMA_HAS_FEWER_PQ_COEF;
+	}
+}
+
+static inline unsigned short dma_maxpqcoef(struct dma_device *dma)
+{
+	return (dma->max_pqcoef & DMA_HAS_FEWER_PQ_COEF) ?
+		(dma->max_pqcoef & ~DMA_HAS_FEWER_PQ_COEF) : RAID6_PQ_MAX_COEF;
+}
+
 static inline size_t dmaengine_get_icg(bool inc, bool sgl, size_t icg,
 				      size_t dir_icg)
 {
diff --git a/include/linux/raid/pq.h b/include/linux/raid/pq.h
index 30f9453..f3a04bb 100644
--- a/include/linux/raid/pq.h
+++ b/include/linux/raid/pq.h
@@ -15,6 +15,9 @@
 
 #ifdef __KERNEL__
 
+/* Max number of PQ coefficients */
+#define RAID6_PQ_MAX_COEF 256
+
 /* Set to 1 to use kernel-wide empty_zero_page */
 #define RAID6_USE_EMPTY_ZERO_PAGE 0
 #include <linux/blkdev.h>
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1572154 — Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients

On Wed, Feb 1, 2017 at 8:47 PM, Anup Patel <anup.patel@broadcom.com> wrote:
> The DMAENGINE framework assumes that if PQ offload is supported by a
> DMA device then all 256 PQ coefficients are supported. This assumption
> does not hold anymore because we now have BCM-SBA-RAID offload engine
> which supports PQ offload with limited number of PQ coefficients.
>
> This patch extends async_tx APIs to handle DMA devices with support
> for fewer PQ coefficients.
>
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
>  crypto/async_tx/async_pq.c          |  3 +++
>  crypto/async_tx/async_raid6_recov.c | 12 ++++++++++--
>  include/linux/dmaengine.h           | 19 +++++++++++++++++++
>  include/linux/raid/pq.h             |  3 +++
>  4 files changed, 35 insertions(+), 2 deletions(-)

So, I hate the way async_tx does these checks on each operation, and
it's ok for me to say that because it's my fault. Really it's md that
should be validating engine offload capabilities once at the beginning
of time. I'd rather we move in that direction than continue to pile
onto a bad design.

[toc] | [prev] | [next] | [standalone]

#1573026 — Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients

On Thu, Feb 2, 2017 at 11:31 AM, Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Feb 1, 2017 at 8:47 PM, Anup Patel <anup.patel@broadcom.com> wrote:
> > The DMAENGINE framework assumes that if PQ offload is supported by a
> > DMA device then all 256 PQ coefficients are supported. This assumption
> > does not hold anymore because we now have BCM-SBA-RAID offload engine
> > which supports PQ offload with limited number of PQ coefficients.
> >
> > This patch extends async_tx APIs to handle DMA devices with support
> > for fewer PQ coefficients.
> >
> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> > ---
> >  crypto/async_tx/async_pq.c          |  3 +++
> >  crypto/async_tx/async_raid6_recov.c | 12 ++++++++++--
> >  include/linux/dmaengine.h           | 19 +++++++++++++++++++
> >  include/linux/raid/pq.h             |  3 +++
> >  4 files changed, 35 insertions(+), 2 deletions(-)
>
> So, I hate the way async_tx does these checks on each operation, and
> it's ok for me to say that because it's my fault. Really it's md that
> should be validating engine offload capabilities once at the beginning
> of time. I'd rather we move in that direction than continue to pile
> onto a bad design.

Yes, indeed. All async_tx APIs have lot of checks and for high throughput
RAID offload engine these checks can add some overhead.

I think doing checks in Linux md would be certainly better but this would
mean lot of changes in Linux md as well as remove checks in async_tx.

Also, async_tx APIs should not find DMA channel on its own instead it
should rely on Linux md to provide DMA channel pointer as parameter.

It's better to do checks cleanup in async_tx as separate patchset and
keep this patchset simple.

Regards,
Anup

[toc] | [prev] | [next] | [standalone]

#1573370 — Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients

On Fri, Feb 3, 2017 at 2:59 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>
>
> On Thu, Feb 2, 2017 at 11:31 AM, Dan Williams <dan.j.williams@intel.com>
> wrote:
>>
>> On Wed, Feb 1, 2017 at 8:47 PM, Anup Patel <anup.patel@broadcom.com>
>> wrote:
>> > The DMAENGINE framework assumes that if PQ offload is supported by a
>> > DMA device then all 256 PQ coefficients are supported. This assumption
>> > does not hold anymore because we now have BCM-SBA-RAID offload engine
>> > which supports PQ offload with limited number of PQ coefficients.
>> >
>> > This patch extends async_tx APIs to handle DMA devices with support
>> > for fewer PQ coefficients.
>> >
>> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>> > ---
>> >  crypto/async_tx/async_pq.c          |  3 +++
>> >  crypto/async_tx/async_raid6_recov.c | 12 ++++++++++--
>> >  include/linux/dmaengine.h           | 19 +++++++++++++++++++
>> >  include/linux/raid/pq.h             |  3 +++
>> >  4 files changed, 35 insertions(+), 2 deletions(-)
>>
>> So, I hate the way async_tx does these checks on each operation, and
>> it's ok for me to say that because it's my fault. Really it's md that
>> should be validating engine offload capabilities once at the beginning
>> of time. I'd rather we move in that direction than continue to pile
>> onto a bad design.
>
>
> Yes, indeed. All async_tx APIs have lot of checks and for high throughput
> RAID offload engine these checks can add some overhead.
>
> I think doing checks in Linux md would be certainly better but this would
> mean lot of changes in Linux md as well as remove checks in async_tx.
>
> Also, async_tx APIs should not find DMA channel on its own instead it
> should rely on Linux md to provide DMA channel pointer as parameter.
>
> It's better to do checks cleanup in async_tx as separate patchset and
> keep this patchset simple.

That's been the problem with async_tx being broken like this for
years. Once you get this "small / simple" patch upstream, that
arguably makes async_tx a little bit worse, there is no longer any
motivation to fix the underlying issues. If you care about the long
term health of raid offload and are enabling new hardware support you
should first tackle the known problems with it before adding new
features.

[toc] | [prev] | [next] | [standalone]

#1574350 — Re: [PATCH 3/6] async_tx: Handle DMA devices having support for fewer PQ coefficients

On Sat, Feb 4, 2017 at 12:12 AM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Feb 3, 2017 at 2:59 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>
>>
>> On Thu, Feb 2, 2017 at 11:31 AM, Dan Williams <dan.j.williams@intel.com>
>> wrote:
>>>
>>> On Wed, Feb 1, 2017 at 8:47 PM, Anup Patel <anup.patel@broadcom.com>
>>> wrote:
>>> > The DMAENGINE framework assumes that if PQ offload is supported by a
>>> > DMA device then all 256 PQ coefficients are supported. This assumption
>>> > does not hold anymore because we now have BCM-SBA-RAID offload engine
>>> > which supports PQ offload with limited number of PQ coefficients.
>>> >
>>> > This patch extends async_tx APIs to handle DMA devices with support
>>> > for fewer PQ coefficients.
>>> >
>>> > Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>>> > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
>>> > ---
>>> >  crypto/async_tx/async_pq.c          |  3 +++
>>> >  crypto/async_tx/async_raid6_recov.c | 12 ++++++++++--
>>> >  include/linux/dmaengine.h           | 19 +++++++++++++++++++
>>> >  include/linux/raid/pq.h             |  3 +++
>>> >  4 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>> So, I hate the way async_tx does these checks on each operation, and
>>> it's ok for me to say that because it's my fault. Really it's md that
>>> should be validating engine offload capabilities once at the beginning
>>> of time. I'd rather we move in that direction than continue to pile
>>> onto a bad design.
>>
>>
>> Yes, indeed. All async_tx APIs have lot of checks and for high throughput
>> RAID offload engine these checks can add some overhead.
>>
>> I think doing checks in Linux md would be certainly better but this would
>> mean lot of changes in Linux md as well as remove checks in async_tx.
>>
>> Also, async_tx APIs should not find DMA channel on its own instead it
>> should rely on Linux md to provide DMA channel pointer as parameter.
>>
>> It's better to do checks cleanup in async_tx as separate patchset and
>> keep this patchset simple.
>
> That's been the problem with async_tx being broken like this for
> years. Once you get this "small / simple" patch upstream, that
> arguably makes async_tx a little bit worse, there is no longer any
> motivation to fix the underlying issues. If you care about the long
> term health of raid offload and are enabling new hardware support you
> should first tackle the known problems with it before adding new
> features.

Apart from the checks related issue you pointed there are other
issues with async_tx APIs such as:

1. The mechanism to do update PQ (or RAID6 update) operation
in current async_tx APIs is to call async_gen_syndrome() twice
with ASYNC_TX_PQ_XOR_DST flag set. Also, async_gen_syndrome()
will always prefer SW approach when ASYNC_TX_PQ_XOR_DST flag
is set. This means async_tx API is forcing SW approach for update
PQ operation and in-addition we require two async_gen_syndrome()
calls to achieve update PQ. This limitations of async_gen_syndrome()
reduces performance of async_tx APIs. Instead of this we should
have a dedicated async_update_pq() API which will allow RAID
offload engine drivers (such as BCM-FS4-RAID) to implement
update PQ using HW offload and this new API will fall-back to
SW approach using async_gen_syndrome() if no DMA channel
provides update PQ HW offload.

2. In our stress testing, we have observed that dma_map_page()
and dma_unmap_page() used in various async_tx APIs are the
major cause of overhead. If we directly call DMA channel callbacks
with pre-DMA-mapped pages then we get very high throughput.
The async_tx APIs should provide a way for pre-DMA-mapped
pages so that Linux MD can exploit this fact for better performance.

3. We really don't have a test module to stress/benchmark all
async_tx APIs using multi-threading and batching large number
of request in each thread. This kind of test module is very much
required for performance benchmarking and stressing high
throughput (hundreds of Gbps) RAID offload engines (such as
BCM-FS4-RAID).

From the above, we already have async_tx_test module to
address point3. We also plan to address point1 above but
this would also require changes in Linux MD to use new
async_update_pq() API.

As you can see, this patchset is not end of story of us if we
want best possible utilization of BCM-FS4-RAID.

Regards,
Anup

[toc] | [prev] | [next] | [standalone]

#1572142 — [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients

The remote processor can have DMAENGINE capabilities and client
can pass data to be processed via main memory. In such cases,
the client will require DMAble memory for remote processor.

This patch adds new API mbox_channel_device() which can be
used by clients to get struct device pointer of underlying
mailbox controller. This struct device pointer of mailbox
controller can be used by clients to allocate DMAble memory
for remote processor.

Signed-off-by: Anup Patel <anup.patel@broadcom.com>
Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/mailbox/mailbox.c      | 21 +++++++++++++++++++++
 include/linux/mailbox_client.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index 4671f8a..d4380fc 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -281,6 +281,27 @@ int mbox_send_message(struct mbox_chan *chan, void *mssg)
 EXPORT_SYMBOL_GPL(mbox_send_message);
 
 /**
+ * mbox_channel_device - Get device pointer of a mailbox channel.
+ * @chan: Mailbox channel assigned to this client.
+ *
+ * The remote processor can have DMAENGINE capabilities and client
+ * can pass data to be processed via main memory. In such cases,
+ * the client will require struct device pointer of the mailbox
+ * channel to map/unmap/allocate/free DMAble memory.
+ *
+ * Return: Pointer to the struct device of mailbox channel.
+ *	   ERR_PTR on failure.
+ */
+struct device *mbox_channel_device(struct mbox_chan *chan)
+{
+	if (!chan || !chan->cl)
+		return ERR_PTR(-EINVAL);
+
+	return chan->mbox->dev;
+}
+EXPORT_SYMBOL_GPL(mbox_channel_device);
+
+/**
  * mbox_request_channel - Request a mailbox channel.
  * @cl: Identity of the client requesting the channel.
  * @index: Index of mailbox specifier in 'mboxes' property.
diff --git a/include/linux/mailbox_client.h b/include/linux/mailbox_client.h
index 4434871..3daffad 100644
--- a/include/linux/mailbox_client.h
+++ b/include/linux/mailbox_client.h
@@ -40,6 +40,7 @@ struct mbox_client {
 	void (*tx_done)(struct mbox_client *cl, void *mssg, int r);
 };
 
+struct device *mbox_channel_device(struct mbox_chan *chan);
 struct mbox_chan *mbox_request_channel_byname(struct mbox_client *cl,
 					      const char *name);
 struct mbox_chan *mbox_request_channel(struct mbox_client *cl, int index);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1573048 — Re: [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients

On Thu, Feb 2, 2017 at 10:17 AM, Anup Patel <anup.patel@broadcom.com> wrote:
> The remote processor can have DMAENGINE capabilities and client
> can pass data to be processed via main memory. In such cases,
> the client will require DMAble memory for remote processor.
>
> This patch adds new API mbox_channel_device() which can be
> used by clients to get struct device pointer of underlying
> mailbox controller. This struct device pointer of mailbox
> controller can be used by clients to allocate DMAble memory
> for remote processor.
>
IIUC, DT already provides a way for what you need.

[toc] | [prev] | [next] | [standalone]

#1574354 — Re: [PATCH 1/6] mailbox: Add new API mbox_channel_device() for clients

On Fri, Feb 3, 2017 at 5:35 PM, Jassi Brar <jassisinghbrar@gmail.com> wrote:
> On Thu, Feb 2, 2017 at 10:17 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> The remote processor can have DMAENGINE capabilities and client
>> can pass data to be processed via main memory. In such cases,
>> the client will require DMAble memory for remote processor.
>>
>> This patch adds new API mbox_channel_device() which can be
>> used by clients to get struct device pointer of underlying
>> mailbox controller. This struct device pointer of mailbox
>> controller can be used by clients to allocate DMAble memory
>> for remote processor.
>>
> IIUC, DT already provides a way for what you need.

Thanks for the suggestion. I will explore in this direction and
try to avoid this patch in next revision.

Can you please have a look at FlexRM driver which I
had submitted previously?
https://lkml.org/lkml/2017/1/5/291
https://lkml.org/lkml/2017/1/5/293

Regards,
Anup

[toc] | [prev] | [next] | [standalone]

#1572143 — [PATCH 4/6] async_tx: Fix DMA_PREP_FENCE usage in do_async_gen_syndrome()

The DMA_PREP_FENCE is to be used when preparing Tx descriptor if output
of Tx descriptor is to be used by next/dependent Tx descriptor.

The DMA_PREP_FENSE will not be set correctly in do_async_gen_syndrome()
when calling dma->device_prep_dma_pq() under following conditions:
1. ASYNC_TX_FENCE not set in submit->flags
2. DMA_PREP_FENCE not set in dma_flags
3. src_cnt (= (disks - 2)) is greater than dma_maxpq(dma, dma_flags)

This patch fixes DMA_PREP_FENCE usage in do_async_gen_syndrome() taking
inspiration from do_async_xor() implementation.

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>
---
 crypto/async_tx/async_pq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/crypto/async_tx/async_pq.c b/crypto/async_tx/async_pq.c
index 16c6526..947cf35 100644
--- a/crypto/async_tx/async_pq.c
+++ b/crypto/async_tx/async_pq.c
@@ -62,9 +62,6 @@ do_async_gen_syndrome(struct dma_chan *chan,
 	dma_addr_t dma_dest[2];
 	int src_off = 0;
 
-	if (submit->flags & ASYNC_TX_FENCE)
-		dma_flags |= DMA_PREP_FENCE;
-
 	while (src_cnt > 0) {
 		submit->flags = flags_orig;
 		pq_src_cnt = min(src_cnt, dma_maxpq(dma, dma_flags));
@@ -83,6 +80,8 @@ do_async_gen_syndrome(struct dma_chan *chan,
 			if (cb_fn_orig)
 				dma_flags |= DMA_PREP_INTERRUPT;
 		}
+		if (submit->flags & ASYNC_TX_FENCE)
+			dma_flags |= DMA_PREP_FENCE;
 
 		/* Drivers force forward progress in case they can not provide
 		 * a descriptor
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1572144 — [PATCH 6/6] dt-bindings: Add DT bindings document for Broadcom SBA RAID driver

This patch adds the DT bindings document for newly added Broadcom
SBA RAID driver.

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>
---
 .../devicetree/bindings/dma/brcm,iproc-sba.txt     | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/brcm,iproc-sba.txt

diff --git a/Documentation/devicetree/bindings/dma/brcm,iproc-sba.txt b/Documentation/devicetree/bindings/dma/brcm,iproc-sba.txt
new file mode 100644
index 0000000..092913a
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/brcm,iproc-sba.txt
@@ -0,0 +1,29 @@
+* Broadcom SBA RAID engine
+
+Required properties:
+- compatible: Should be one of the following
+	      "brcm,iproc-sba"
+	      "brcm,iproc-sba-v2"
+  The "brcm,iproc-sba" has support for only 6 PQ coefficients
+  The "brcm,iproc-sba-v2" has support for only 30 PQ coefficients
+- mboxes: List of phandle and mailbox channel specifiers
+
+Example:
+
+raid_mbox: mbox@67400000 {
+	...
+	#mbox-cells = <3>;
+	...
+};
+
+raid0 {
+	compatible = "brcm,iproc-sba-v2";
+	mboxes = <&raid_mbox 0 0x1 0xffff>,
+		 <&raid_mbox 1 0x1 0xffff>,
+		 <&raid_mbox 2 0x1 0xffff>,
+		 <&raid_mbox 3 0x1 0xffff>,
+		 <&raid_mbox 4 0x1 0xffff>,
+		 <&raid_mbox 5 0x1 0xffff>,
+		 <&raid_mbox 6 0x1 0xffff>,
+		 <&raid_mbox 7 0x1 0xffff>;
+};
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1573816 — Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver

On Thu, Feb 02, 2017 at 10:17:15AM +0530, Anup Patel wrote:
> +config BCM_SBA_RAID
> +        tristate "Broadcom SBA RAID engine support"
> +        depends on (ARM64 && MAILBOX && RAID6_PQ) || COMPILE_TEST
> +        select DMA_ENGINE
> +        select DMA_ENGINE_RAID
> +	select ASYNC_TX_ENABLE_CHANNEL_SWITCH
> +	default ARCH_BCM_IPROC

whats with the funny alignement?

> +/* SBA command related defines */
> +#define SBA_TYPE_SHIFT					48
> +#define SBA_TYPE_MASK					0x3
> +#define SBA_TYPE_A					0x0
> +#define SBA_TYPE_B					0x2
> +#define SBA_TYPE_C					0x3
> +#define SBA_USER_DEF_SHIFT				32
> +#define SBA_USER_DEF_MASK				0xffff
> +#define SBA_R_MDATA_SHIFT				24
> +#define SBA_R_MDATA_MASK				0xff
> +#define SBA_C_MDATA_MS_SHIFT				18
> +#define SBA_C_MDATA_MS_MASK				0x3
> +#define SBA_INT_SHIFT					17
> +#define SBA_INT_MASK					0x1
> +#define SBA_RESP_SHIFT					16
> +#define SBA_RESP_MASK					0x1
> +#define SBA_C_MDATA_SHIFT				8
> +#define SBA_C_MDATA_MASK				0xff
> +#define SBA_CMD_SHIFT					0
> +#define SBA_CMD_MASK					0xf
> +#define SBA_CMD_ZERO_ALL_BUFFERS			0x8
> +#define SBA_CMD_LOAD_BUFFER				0x9
> +#define SBA_CMD_XOR					0xa
> +#define SBA_CMD_GALOIS_XOR				0xb
> +#define SBA_CMD_ZERO_BUFFER				0x4
> +#define SBA_CMD_WRITE_BUFFER				0xc

Try using BIT and GENMAST for hardware descriptions

> +
> +/* SBA C_MDATA helper macros */
> +#define SBA_C_MDATA_LOAD_VAL(__bnum0)		((__bnum0) & 0x3)
> +#define SBA_C_MDATA_WRITE_VAL(__bnum0)		((__bnum0) & 0x3)
> +#define SBA_C_MDATA_XOR_VAL(__bnum1, __bnum0)			\
> +			({	u32 __v = ((__bnum0) & 0x3);	\
> +				__v |= ((__bnum1) & 0x3) << 2;	\
> +				__v;				\
> +			})
> +#define SBA_C_MDATA_PQ_VAL(__dnum, __bnum1, __bnum0)		\
> +			({	u32 __v = ((__bnum0) & 0x3);	\
> +				__v |= ((__bnum1) & 0x3) << 2;	\
> +				__v |= ((__dnum) & 0x1f) << 5;	\
> +				__v;				\
> +			})

ah why are we usig complex macros, why can't these be simple functions..

> +#define SBA_C_MDATA_LS(__c_mdata_val)	((__c_mdata_val) & 0xff)
> +#define SBA_C_MDATA_MS(__c_mdata_val)	(((__c_mdata_val) >> 8) & 0x3)
> +
> +/* Driver helper macros */
> +#define to_sba_request(tx)		\
> +	container_of(tx, struct sba_request, tx)
> +#define to_sba_device(dchan)		\
> +	container_of(dchan, struct sba_device, dma_chan)
> +
> +enum sba_request_state {
> +	SBA_REQUEST_STATE_FREE = 1,
> +	SBA_REQUEST_STATE_ALLOCED = 2,
> +	SBA_REQUEST_STATE_PENDING = 3,
> +	SBA_REQUEST_STATE_ACTIVE = 4,
> +	SBA_REQUEST_STATE_COMPLETED = 5,
> +	SBA_REQUEST_STATE_ABORTED = 6,

whats up with a very funny indentation setting, we use 8 chars.

Please re-read the Documentation/process/coding-style.rst

> +static int sba_alloc_chan_resources(struct dma_chan *dchan)
> +{
> +	/*
> +	 * We only have one channel so we have pre-alloced
> +	 * channel resources. Over here we just return number
> +	 * of free request.
> +	 */
> +	return sba_free_request_count(to_sba_device(dchan));
> +}

essentially you are not doing much, so you can skip it. Its an optional
call.

> +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_inflight_requests(to_sba_device(dchan));

well this one checks for pending requests as well, which shouldn't be there
when freeing a channel, something seems not quite right here..

> +static int sba_send_mbox_request(struct sba_device *sba,
> +				 struct sba_request *req)
> +{
> +	int mchans_idx, ret = 0;
> +
> +	/* Select mailbox channel in round-robin fashion */
> +	mchans_idx = atomic_inc_return(&sba->mchans_current);
> +	mchans_idx = mchans_idx % sba->mchans_count;
> +
> +	/* Send batch message for the request */
> +	req->bmsg.batch.msgs_queued = 0;
> +	ret = mbox_send_message(sba->mchans[mchans_idx], &req->bmsg);
> +	if (ret < 0) {
> +		dev_info(sba->dev, "channel %d message %d (total %d)",
> +			 mchans_idx, req->bmsg.batch.msgs_queued,
> +			 req->bmsg.batch.msgs_count);

dev_err?

> +		dev_err(sba->dev, "send message failed with error %d", ret);
> +		return ret;
> +	}
> +	ret = req->bmsg.error;
> +	if (ret < 0) {
> +		dev_info(sba->dev,
> +			 "mbox channel %d message %d (total %d)",
> +			 mchans_idx, req->bmsg.batch.msgs_queued,
> +			 req->bmsg.batch.msgs_count);

same here

> +static dma_cookie_t sba_tx_submit(struct dma_async_tx_descriptor *tx)
> +{
> +	unsigned long flags;
> +	dma_cookie_t cookie;
> +	struct sba_request *req;
> +	struct sba_device *sba;
> +
> +	if (unlikely(!tx))
> +		return -EINVAL;
> +
> +	sba = to_sba_device(tx->chan);
> +	req = to_sba_request(tx);
> +
> +	/* Assign cookie and mark request pending */
> +	spin_lock_irqsave(&sba->reqs_lock, flags);
> +	cookie = dma_cookie_assign(tx);
> +	_sba_pending_request(sba, req);
> +	spin_unlock_irqrestore(&sba->reqs_lock, flags);
> +
> +	/* Try to submit pending request */
> +	sba_issue_pending(&sba->dma_chan);

Nope, thats wrong, caller needs to call .issue_pending for that

> +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);
> +
> +	ret = dma_cookie_status(dchan, cookie, txstate);
> +	if (ret == DMA_COMPLETE)
> +		return ret;
> +
> +	for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
> +		mbox_client_peek_data(sba->mchans[mchan_idx]);

what is this achieving?

> +static struct dma_async_tx_descriptor *
> +sba_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst, dma_addr_t src,
> +		    size_t len, unsigned long flags)
> +{
> +	size_t msg_len;
> +	dma_addr_t msg_offset = 0;
> +	unsigned int msgs_count = 0, cmds_count, cmds_idx = 0;
> +	struct sba_device *sba = to_sba_device(dchan);
> +	struct sba_request *req = NULL;
> +
> +	/* Sanity checks */
> +	if (unlikely(len > sba->req_size))
> +		return NULL;

why is that an error, you can create multiple txn of max length

> +static int sba_async_register(struct sba_device *sba)
> +{
> +	int ret;
> +	struct dma_device *dma_dev = &sba->dma_dev;
> +
> +	/* Initialize DMA channel cookie */
> +	sba->dma_chan.device = dma_dev;
> +	dma_cookie_init(&sba->dma_chan);
> +
> +	/* Initialize DMA device capability mask */
> +	dma_cap_zero(dma_dev->cap_mask);
> +	dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> +	dma_cap_set(DMA_XOR, dma_dev->cap_mask);
> +	dma_cap_set(DMA_PQ, dma_dev->cap_mask);
> +
> +	/*
> +	 * Set mailbox channel device as the base device of
> +	 * our dma_device because the actual memory accesses
> +	 * will be done by mailbox controller
> +	 */
> +	dma_dev->dev = sba->mbox_dev;
> +
> +	/* Set base prep routines */
> +	dma_dev->device_alloc_chan_resources = sba_alloc_chan_resources;
> +	dma_dev->device_free_chan_resources = sba_free_chan_resources;
> +	dma_dev->device_issue_pending = sba_issue_pending;
> +	dma_dev->device_tx_status = sba_tx_status;

Please add terminate callback support, also add the capabilities, we need to
advertise that and use in clients

Also you can simplify bunch of code by using virt-chan support for managing
channels and descriptors

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1574594 — Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver

On Sun, Feb 5, 2017 at 11:36 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Thu, Feb 02, 2017 at 10:17:15AM +0530, Anup Patel wrote:
>> +config BCM_SBA_RAID
>> +        tristate "Broadcom SBA RAID engine support"
>> +        depends on (ARM64 && MAILBOX && RAID6_PQ) || COMPILE_TEST
>> +        select DMA_ENGINE
>> +        select DMA_ENGINE_RAID
>> +     select ASYNC_TX_ENABLE_CHANNEL_SWITCH
>> +     default ARCH_BCM_IPROC
>
> whats with the funny alignement?

Sure, I will use tabs here.

>
>> +/* SBA command related defines */
>> +#define SBA_TYPE_SHIFT                                       48
>> +#define SBA_TYPE_MASK                                        0x3
>> +#define SBA_TYPE_A                                   0x0
>> +#define SBA_TYPE_B                                   0x2
>> +#define SBA_TYPE_C                                   0x3
>> +#define SBA_USER_DEF_SHIFT                           32
>> +#define SBA_USER_DEF_MASK                            0xffff
>> +#define SBA_R_MDATA_SHIFT                            24
>> +#define SBA_R_MDATA_MASK                             0xff
>> +#define SBA_C_MDATA_MS_SHIFT                         18
>> +#define SBA_C_MDATA_MS_MASK                          0x3
>> +#define SBA_INT_SHIFT                                        17
>> +#define SBA_INT_MASK                                 0x1
>> +#define SBA_RESP_SHIFT                                       16
>> +#define SBA_RESP_MASK                                        0x1
>> +#define SBA_C_MDATA_SHIFT                            8
>> +#define SBA_C_MDATA_MASK                             0xff
>> +#define SBA_CMD_SHIFT                                        0
>> +#define SBA_CMD_MASK                                 0xf
>> +#define SBA_CMD_ZERO_ALL_BUFFERS                     0x8
>> +#define SBA_CMD_LOAD_BUFFER                          0x9
>> +#define SBA_CMD_XOR                                  0xa
>> +#define SBA_CMD_GALOIS_XOR                           0xb
>> +#define SBA_CMD_ZERO_BUFFER                          0x4
>> +#define SBA_CMD_WRITE_BUFFER                         0xc
>
> Try using BIT and GENMAST for hardware descriptions

Sure, will do.

>
>> +
>> +/* SBA C_MDATA helper macros */
>> +#define SBA_C_MDATA_LOAD_VAL(__bnum0)                ((__bnum0) & 0x3)
>> +#define SBA_C_MDATA_WRITE_VAL(__bnum0)               ((__bnum0) & 0x3)
>> +#define SBA_C_MDATA_XOR_VAL(__bnum1, __bnum0)                        \
>> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
>> +                             __v |= ((__bnum1) & 0x3) << 2;  \
>> +                             __v;                            \
>> +                     })
>> +#define SBA_C_MDATA_PQ_VAL(__dnum, __bnum1, __bnum0)         \
>> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
>> +                             __v |= ((__bnum1) & 0x3) << 2;  \
>> +                             __v |= ((__dnum) & 0x1f) << 5;  \
>> +                             __v;                            \
>> +                     })
>
> ah why are we usig complex macros, why can't these be simple functions..

"static inline functions" seemed too complicated here because most of
these macros are two lines of c-code.

Do you still insist on using "static inline functions"?

>
>> +#define SBA_C_MDATA_LS(__c_mdata_val)        ((__c_mdata_val) & 0xff)
>> +#define SBA_C_MDATA_MS(__c_mdata_val)        (((__c_mdata_val) >> 8) & 0x3)
>> +
>> +/* Driver helper macros */
>> +#define to_sba_request(tx)           \
>> +     container_of(tx, struct sba_request, tx)
>> +#define to_sba_device(dchan)         \
>> +     container_of(dchan, struct sba_device, dma_chan)
>> +
>> +enum sba_request_state {
>> +     SBA_REQUEST_STATE_FREE = 1,
>> +     SBA_REQUEST_STATE_ALLOCED = 2,
>> +     SBA_REQUEST_STATE_PENDING = 3,
>> +     SBA_REQUEST_STATE_ACTIVE = 4,
>> +     SBA_REQUEST_STATE_COMPLETED = 5,
>> +     SBA_REQUEST_STATE_ABORTED = 6,
>
> whats up with a very funny indentation setting, we use 8 chars.
>
> Please re-read the Documentation/process/coding-style.rst

I have double checked this enum. The indentation is fine
and as-per coding style. Am I missing anything else?

>
>> +static int sba_alloc_chan_resources(struct dma_chan *dchan)
>> +{
>> +     /*
>> +      * We only have one channel so we have pre-alloced
>> +      * channel resources. Over here we just return number
>> +      * of free request.
>> +      */
>> +     return sba_free_request_count(to_sba_device(dchan));
>> +}
>
> essentially you are not doing much, so you can skip it. Its an optional
> call.

Sure, will do.

>
>> +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_inflight_requests(to_sba_device(dchan));
>
> well this one checks for pending requests as well, which shouldn't be there
> when freeing a channel, something seems not quite right here..
>
>> +static int sba_send_mbox_request(struct sba_device *sba,
>> +                              struct sba_request *req)
>> +{
>> +     int mchans_idx, ret = 0;
>> +
>> +     /* Select mailbox channel in round-robin fashion */
>> +     mchans_idx = atomic_inc_return(&sba->mchans_current);
>> +     mchans_idx = mchans_idx % sba->mchans_count;
>> +
>> +     /* Send batch message for the request */
>> +     req->bmsg.batch.msgs_queued = 0;
>> +     ret = mbox_send_message(sba->mchans[mchans_idx], &req->bmsg);
>> +     if (ret < 0) {
>> +             dev_info(sba->dev, "channel %d message %d (total %d)",
>> +                      mchans_idx, req->bmsg.batch.msgs_queued,
>> +                      req->bmsg.batch.msgs_count);
>
> dev_err?

Sure, will use dev_err.

>
>> +             dev_err(sba->dev, "send message failed with error %d", ret);
>> +             return ret;
>> +     }
>> +     ret = req->bmsg.error;
>> +     if (ret < 0) {
>> +             dev_info(sba->dev,
>> +                      "mbox channel %d message %d (total %d)",
>> +                      mchans_idx, req->bmsg.batch.msgs_queued,
>> +                      req->bmsg.batch.msgs_count);
>
> same here

OK.

>
>> +static dma_cookie_t sba_tx_submit(struct dma_async_tx_descriptor *tx)
>> +{
>> +     unsigned long flags;
>> +     dma_cookie_t cookie;
>> +     struct sba_request *req;
>> +     struct sba_device *sba;
>> +
>> +     if (unlikely(!tx))
>> +             return -EINVAL;
>> +
>> +     sba = to_sba_device(tx->chan);
>> +     req = to_sba_request(tx);
>> +
>> +     /* Assign cookie and mark request pending */
>> +     spin_lock_irqsave(&sba->reqs_lock, flags);
>> +     cookie = dma_cookie_assign(tx);
>> +     _sba_pending_request(sba, req);
>> +     spin_unlock_irqrestore(&sba->reqs_lock, flags);
>> +
>> +     /* Try to submit pending request */
>> +     sba_issue_pending(&sba->dma_chan);
>
> Nope, thats wrong, caller needs to call .issue_pending for that

This was giving minor performance improvement but I will
remove this since its against API usage.

>
>> +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);
>> +
>> +     ret = dma_cookie_status(dchan, cookie, txstate);
>> +     if (ret == DMA_COMPLETE)
>> +             return ret;
>> +
>> +     for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
>> +             mbox_client_peek_data(sba->mchans[mchan_idx]);
>
> what is this achieving?

The mbox_client_peek_data() is a hint to mailbox controller driver
to check for available messages.

This gives good performance improvement when some DMA client
code is polling using tx_status() callback.

>
>> +static struct dma_async_tx_descriptor *
>> +sba_prep_dma_memcpy(struct dma_chan *dchan, dma_addr_t dst, dma_addr_t src,
>> +                 size_t len, unsigned long flags)
>> +{
>> +     size_t msg_len;
>> +     dma_addr_t msg_offset = 0;
>> +     unsigned int msgs_count = 0, cmds_count, cmds_idx = 0;
>> +     struct sba_device *sba = to_sba_device(dchan);
>> +     struct sba_request *req = NULL;
>> +
>> +     /* Sanity checks */
>> +     if (unlikely(len > sba->req_size))
>> +             return NULL;
>
> why is that an error, you can create multiple txn of max length

Sure, I will extend driver to create multiple txn when
"len > req->size"

>
>> +static int sba_async_register(struct sba_device *sba)
>> +{
>> +     int ret;
>> +     struct dma_device *dma_dev = &sba->dma_dev;
>> +
>> +     /* Initialize DMA channel cookie */
>> +     sba->dma_chan.device = dma_dev;
>> +     dma_cookie_init(&sba->dma_chan);
>> +
>> +     /* Initialize DMA device capability mask */
>> +     dma_cap_zero(dma_dev->cap_mask);
>> +     dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_XOR, dma_dev->cap_mask);
>> +     dma_cap_set(DMA_PQ, dma_dev->cap_mask);
>> +
>> +     /*
>> +      * Set mailbox channel device as the base device of
>> +      * our dma_device because the actual memory accesses
>> +      * will be done by mailbox controller
>> +      */
>> +     dma_dev->dev = sba->mbox_dev;
>> +
>> +     /* Set base prep routines */
>> +     dma_dev->device_alloc_chan_resources = sba_alloc_chan_resources;
>> +     dma_dev->device_free_chan_resources = sba_free_chan_resources;
>> +     dma_dev->device_issue_pending = sba_issue_pending;
>> +     dma_dev->device_tx_status = sba_tx_status;
>
> Please add terminate callback support, also add the capabilities, we need to
> advertise that and use in clients

OK, I will add terminate callback.

>
> Also you can simplify bunch of code by using virt-chan support for managing
> channels and descriptors

OK, I will surely explore virt-chan.

Regards,
Anup

[toc] | [prev] | [next] | [standalone]

#1574994 — Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver

On Mon, Feb 06, 2017 at 05:31:15PM +0530, Anup Patel wrote:

> >> +
> >> +/* SBA C_MDATA helper macros */
> >> +#define SBA_C_MDATA_LOAD_VAL(__bnum0)                ((__bnum0) & 0x3)
> >> +#define SBA_C_MDATA_WRITE_VAL(__bnum0)               ((__bnum0) & 0x3)
> >> +#define SBA_C_MDATA_XOR_VAL(__bnum1, __bnum0)                        \
> >> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
> >> +                             __v |= ((__bnum1) & 0x3) << 2;  \
> >> +                             __v;                            \
> >> +                     })
> >> +#define SBA_C_MDATA_PQ_VAL(__dnum, __bnum1, __bnum0)         \
> >> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
> >> +                             __v |= ((__bnum1) & 0x3) << 2;  \
> >> +                             __v |= ((__dnum) & 0x1f) << 5;  \
> >> +                             __v;                            \
> >> +                     })
> >
> > ah why are we usig complex macros, why can't these be simple functions..
> 
> "static inline functions" seemed too complicated here because most of
> these macros are two lines of c-code.

and thats where I have an issue with this. Macros for simple things is fine
but not for couple of line of logic!

> 
> Do you still insist on using "static inline functions"?

Yes

> 
> >
> >> +#define SBA_C_MDATA_LS(__c_mdata_val)        ((__c_mdata_val) & 0xff)
> >> +#define SBA_C_MDATA_MS(__c_mdata_val)        (((__c_mdata_val) >> 8) & 0x3)
> >> +
> >> +/* Driver helper macros */
> >> +#define to_sba_request(tx)           \
> >> +     container_of(tx, struct sba_request, tx)
> >> +#define to_sba_device(dchan)         \
> >> +     container_of(dchan, struct sba_device, dma_chan)
> >> +
> >> +enum sba_request_state {
> >> +     SBA_REQUEST_STATE_FREE = 1,
> >> +     SBA_REQUEST_STATE_ALLOCED = 2,
> >> +     SBA_REQUEST_STATE_PENDING = 3,
> >> +     SBA_REQUEST_STATE_ACTIVE = 4,
> >> +     SBA_REQUEST_STATE_COMPLETED = 5,
> >> +     SBA_REQUEST_STATE_ABORTED = 6,
> >
> > whats up with a very funny indentation setting, we use 8 chars.
> >
> > Please re-read the Documentation/process/coding-style.rst
> 
> I have double checked this enum. The indentation is fine
> and as-per coding style. Am I missing anything else?

Somehow the initial indent doesnt seem to be 8 chars to me.

> >> +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);
> >> +
> >> +     ret = dma_cookie_status(dchan, cookie, txstate);
> >> +     if (ret == DMA_COMPLETE)
> >> +             return ret;
> >> +
> >> +     for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
> >> +             mbox_client_peek_data(sba->mchans[mchan_idx]);
> >
> > what is this achieving?
> 
> The mbox_client_peek_data() is a hint to mailbox controller driver
> to check for available messages.
> 
> This gives good performance improvement when some DMA client
> code is polling using tx_status() callback.

Then why do it before and then check status.

-- 
~Vinod

[toc] | [prev] | [next] | [standalone]

#1575398 — Re: [PATCH 5/6] dmaengine: Add Broadcom SBA RAID driver

On Mon, Feb 6, 2017 at 10:24 PM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Mon, Feb 06, 2017 at 05:31:15PM +0530, Anup Patel wrote:
>
>> >> +
>> >> +/* SBA C_MDATA helper macros */
>> >> +#define SBA_C_MDATA_LOAD_VAL(__bnum0)                ((__bnum0) & 0x3)
>> >> +#define SBA_C_MDATA_WRITE_VAL(__bnum0)               ((__bnum0) & 0x3)
>> >> +#define SBA_C_MDATA_XOR_VAL(__bnum1, __bnum0)                        \
>> >> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
>> >> +                             __v |= ((__bnum1) & 0x3) << 2;  \
>> >> +                             __v;                            \
>> >> +                     })
>> >> +#define SBA_C_MDATA_PQ_VAL(__dnum, __bnum1, __bnum0)         \
>> >> +                     ({      u32 __v = ((__bnum0) & 0x3);    \
>> >> +                             __v |= ((__bnum1) & 0x3) << 2;  \
>> >> +                             __v |= ((__dnum) & 0x1f) << 5;  \
>> >> +                             __v;                            \
>> >> +                     })
>> >
>> > ah why are we usig complex macros, why can't these be simple functions..
>>
>> "static inline functions" seemed too complicated here because most of
>> these macros are two lines of c-code.
>
> and thats where I have an issue with this. Macros for simple things is fine
> but not for couple of line of logic!
>
>>
>> Do you still insist on using "static inline functions"?
>
> Yes

Sure, will use "static inline functions" instead these macros.

>
>>
>> >
>> >> +#define SBA_C_MDATA_LS(__c_mdata_val)        ((__c_mdata_val) & 0xff)
>> >> +#define SBA_C_MDATA_MS(__c_mdata_val)        (((__c_mdata_val) >> 8) & 0x3)
>> >> +
>> >> +/* Driver helper macros */
>> >> +#define to_sba_request(tx)           \
>> >> +     container_of(tx, struct sba_request, tx)
>> >> +#define to_sba_device(dchan)         \
>> >> +     container_of(dchan, struct sba_device, dma_chan)
>> >> +
>> >> +enum sba_request_state {
>> >> +     SBA_REQUEST_STATE_FREE = 1,
>> >> +     SBA_REQUEST_STATE_ALLOCED = 2,
>> >> +     SBA_REQUEST_STATE_PENDING = 3,
>> >> +     SBA_REQUEST_STATE_ACTIVE = 4,
>> >> +     SBA_REQUEST_STATE_COMPLETED = 5,
>> >> +     SBA_REQUEST_STATE_ABORTED = 6,
>> >
>> > whats up with a very funny indentation setting, we use 8 chars.
>> >
>> > Please re-read the Documentation/process/coding-style.rst
>>
>> I have double checked this enum. The indentation is fine
>> and as-per coding style. Am I missing anything else?
>
> Somehow the initial indent doesnt seem to be 8 chars to me.
>
>> >> +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);
>> >> +
>> >> +     ret = dma_cookie_status(dchan, cookie, txstate);
>> >> +     if (ret == DMA_COMPLETE)
>> >> +             return ret;
>> >> +
>> >> +     for (mchan_idx = 0; mchan_idx < sba->mchans_count; mchan_idx++)
>> >> +             mbox_client_peek_data(sba->mchans[mchan_idx]);
>> >
>> > what is this achieving?
>>
>> The mbox_client_peek_data() is a hint to mailbox controller driver
>> to check for available messages.
>>
>> This gives good performance improvement when some DMA client
>> code is polling using tx_status() callback.
>
> Then why do it before and then check status.

If there was a work completed when mbox_client_peek_data()
is called then sba_receive_message() will be called immediately
by mailbox controller driver.

We are doing dma_cookie_complete() in sba_receive_message()
so if mbox_client_peek_data() is called before dma_cookie_status()
then dma_cookie_status() will see correct state of cookie.

Also, I explored virt-dma APIs for BCM-SBA-RAID driver. The virt-dma
implements tasklet based bottom-half for each virt-dma-channel. This
bottom-half is not required for BCM-FS4-RAID driver because its a
mailbox client driver and the mailbox controller driver already implements
bottom-half for each mailbox channel.

If we still go ahead and use virt-dma in BCM-FS4-RAID driver then we
will have two bottom-halfs in-action one in mailbox controller driver and
another in BCM-FS4-RAID driver which in-turn will add bottom-half
scheduling overhead thereby reducing performance of BCM-FS4-RAID
driver.

Regards,
Anup

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web