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

Groups > linux.kernel > #1578315 > unrolled thread

[PATCH v3 0/4] Broadcom SBA RAID support

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/4] Broadcom SBA RAID support Anup Patel <anup.patel@broadcom.com> - 2017-02-10 10:10 +0100
    [PATCH v3 4/4] dt-bindings: Add DT bindings document for Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-10 10:10 +0100
    [PATCH v3 2/4] async_tx: Fix DMA_PREP_FENCE usage in do_async_gen_syndrome() Anup Patel <anup.patel@broadcom.com> - 2017-02-10 10:10 +0100
    [PATCH v3 1/4] lib/raid6: Add log-of-2 table for RAID6 HW requiring disk position Anup Patel <anup.patel@broadcom.com> - 2017-02-10 10:20 +0100
    Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver Dan Williams <dan.j.williams@intel.com> - 2017-02-10 19:00 +0100
      Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-13 10:20 +0100
        Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-14 05:10 +0100
      Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver Anup Patel <anup.patel@broadcom.com> - 2017-02-14 06:30 +0100

#1578315 — [PATCH v3 0/4] 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).

The Broadcom SBA RAID hardware requires PQ disk position instead
of PQ disk coefficient. To address this, we have added raid_gflog
table which will help driver to convert PQ disk coefficient to PQ
disk position.

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

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

Changes since v2:
 - Droped patch to handle DMA devices having support for fewer
   PQ coefficients in Linux Async Tx
 - Added work-around in bcm-sba-raid driver to handle unsupported
   PQ coefficients using multiple SBA requests

Changes since v1:
 - Droped patch to add mbox_channel_device() API
 - Used GENMASK and BIT macros wherever possible in bcm-sba-raid driver
 - Replaced C_MDATA macros with static inline functions in
   bcm-sba-raid driver
 - Removed sba_alloc_chan_resources() callback in bcm-sba-raid driver
 - Used dev_err() instead of dev_info() wherever applicable
 - Removed call to sba_issue_pending() from sba_tx_submit() in
   bcm-sba-raid driver
 - Implemented SBA request chaning for handling (len > sba->req_size)
   in bcm-sba-raid driver
 - Implemented device_terminate_all() callback in bcm-sba-raid driver

Anup Patel (4):
  lib/raid6: Add log-of-2 table for RAID6 HW requiring disk position
  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                         |    5 +-
 drivers/dma/Kconfig                                |   13 +
 drivers/dma/Makefile                               |    1 +
 drivers/dma/bcm-sba-raid.c                         | 1711 ++++++++++++++++++++
 include/linux/raid/pq.h                            |    1 +
 lib/raid6/mktables.c                               |   20 +
 7 files changed, 1777 insertions(+), 3 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]

#1578316 — [PATCH v3 4/4] 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]

#1578317 — [PATCH v3 2/4] 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 f83de99..56bd612 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]

#1578322 — [PATCH v3 1/4] lib/raid6: Add log-of-2 table for RAID6 HW requiring disk position

The raid6_gfexp table represents {2}^n values for 0 <= n < 256. The
Linux async_tx framework pass values from raid6_gfexp as coefficients
for each source to prep_dma_pq() callback of DMA channel with PQ
capability. This creates problem for RAID6 offload engines (such as
Broadcom SBA) which take disk position (i.e. log of {2}) instead of
multiplicative cofficients from raid6_gfexp table.

This patch adds raid6_gflog table having log-of-2 value for any given
x such that 0 <= x < 256. For any given disk coefficient x, the
corresponding disk position is given by raid6_gflog[x]. The RAID6
offload engine driver can use this newly added raid6_gflog table to
get disk position from multiplicative coefficient.

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>
---
 include/linux/raid/pq.h |  1 +
 lib/raid6/mktables.c    | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/raid/pq.h b/include/linux/raid/pq.h
index 4d57bba..30f9453 100644
--- a/include/linux/raid/pq.h
+++ b/include/linux/raid/pq.h
@@ -142,6 +142,7 @@ int raid6_select_algo(void);
 extern const u8 raid6_gfmul[256][256] __attribute__((aligned(256)));
 extern const u8 raid6_vgfmul[256][32] __attribute__((aligned(256)));
 extern const u8 raid6_gfexp[256]      __attribute__((aligned(256)));
+extern const u8 raid6_gflog[256]      __attribute__((aligned(256)));
 extern const u8 raid6_gfinv[256]      __attribute__((aligned(256)));
 extern const u8 raid6_gfexi[256]      __attribute__((aligned(256)));
 
diff --git a/lib/raid6/mktables.c b/lib/raid6/mktables.c
index 39787db..e824d08 100644
--- a/lib/raid6/mktables.c
+++ b/lib/raid6/mktables.c
@@ -125,6 +125,26 @@ int main(int argc, char *argv[])
 	printf("EXPORT_SYMBOL(raid6_gfexp);\n");
 	printf("#endif\n");
 
+	/* Compute log-of-2 table */
+	printf("\nconst u8 __attribute__((aligned(256)))\n"
+	       "raid6_gflog[256] =\n" "{\n");
+	for (i = 0; i < 256; i += 8) {
+		printf("\t");
+		for (j = 0; j < 8; j++) {
+			v = 255;
+			for (k = 0; k < 256; k++)
+				if (exptbl[k] == (i + j)) {
+					v = k;
+					break;
+				}
+			printf("0x%02x,%c", v, (j == 7) ? '\n' : ' ');
+		}
+	}
+	printf("};\n");
+	printf("#ifdef __KERNEL__\n");
+	printf("EXPORT_SYMBOL(raid6_gflog);\n");
+	printf("#endif\n");
+
 	/* Compute inverse table x^-1 == x^254 */
 	printf("\nconst u8 __attribute__((aligned(256)))\n"
 	       "raid6_gfinv[256] =\n" "{\n");
-- 
2.7.4

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

#1578654 — Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver

On Fri, Feb 10, 2017 at 1:07 AM, Anup Patel <anup.patel@broadcom.com> wrote:
> The Broadcom stream buffer accelerator (SBA) provides offloading
> capabilities for RAID operations. This SBA offload engine is
> accessible via Broadcom SoC specific ring manager.
>
> This patch adds Broadcom SBA RAID driver which provides one
> DMA device with RAID capabilities using one or more Broadcom
> SoC specific ring manager channels. The SBA RAID driver in its
> current shape implements memcpy, xor, and pq operations.
>
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  drivers/dma/Kconfig        |   13 +
>  drivers/dma/Makefile       |    1 +
>  drivers/dma/bcm-sba-raid.c | 1711 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1725 insertions(+)
>  create mode 100644 drivers/dma/bcm-sba-raid.c
>
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 263495d..bf8fb84 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -99,6 +99,19 @@ config AXI_DMAC
>           controller is often used in Analog Device's reference designs for FPGA
>           platforms.
>
> +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

ASYNC_TX_ENABLE_CHANNEL_SWITCH violates the DMA mapping API and
Russell has warned it's especially problematic on ARM [1].  If you
need channel switching for this offload engine to be useful then you
need to move DMA mapping and channel switching responsibilities to MD
itself.

[1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036753.html


[..]
> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
> new file mode 100644
> index 0000000..bab9918
> --- /dev/null
> +++ b/drivers/dma/bcm-sba-raid.c
> @@ -0,0 +1,1711 @@
> +/*
> + * Copyright (C) 2017 Broadcom
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * Broadcom SBA RAID Driver
> + *
> + * The Broadcom stream buffer accelerator (SBA) provides offloading
> + * capabilities for RAID operations. The SBA offload engine is accessible
> + * via Broadcom SoC specific ring manager. Two or more offload engines
> + * can share same Broadcom SoC specific ring manager due to this Broadcom
> + * SoC specific ring manager driver is implemented as a mailbox controller
> + * driver and offload engine drivers are implemented as mallbox clients.
> + *
> + * Typically, Broadcom SoC specific ring manager will implement larger
> + * number of hardware rings over one or more SBA hardware devices. By
> + * design, the internal buffer size of SBA hardware device is limited
> + * but all offload operations supported by SBA can be broken down into
> + * multiple small size requests and executed parallely on multiple SBA
> + * hardware devices for achieving high through-put.
> + *
> + * The Broadcom SBA RAID driver does not require any register programming
> + * except submitting request to SBA hardware device via mailbox channels.
> + * This driver implements a DMA device with one DMA channel using a set
> + * of mailbox channels provided by Broadcom SoC specific ring manager
> + * driver. To exploit parallelism (as described above), all DMA request
> + * coming to SBA RAID DMA channel are broken down to smaller requests
> + * and submitted to multiple mailbox channels in round-robin fashion.
> + * For having more SBA DMA channels, we can create more SBA device nodes
> + * in Broadcom SoC specific DTS based on number of hardware rings supported
> + * by Broadcom SoC ring manager.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/list.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/mailbox/brcm-message.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/raid/pq.h>
> +
> +#include "dmaengine.h"
> +
> +/* SBA command helper macros */
> +#define SBA_DEC(_d, _s, _m)            (((_d) >> (_s)) & (_m))
> +#define SBA_ENC(_d, _v, _s, _m)                                        \
> +               do {                                            \
> +                       (_d) &= ~((u64)(_m) << (_s));           \
> +                       (_d) |= (((u64)(_v) & (_m)) << (_s));   \
> +               } while (0)

Reusing a macro argument multiple times is problematic, consider
SBA_ENC(..., arg++, ...), and hiding assignments in a macro make this
hard to read. The compiler should inline it properly if you just make
this a function that returns a value. You could also mark it __pure.

[..]
> +
> +static struct sba_request *sba_alloc_request(struct sba_device *sba)
> +{
> +       unsigned long flags;
> +       struct sba_request *req = NULL;
> +
> +       spin_lock_irqsave(&sba->reqs_lock, flags);
> +
> +       if (!list_empty(&sba->reqs_free_list)) {
> +               req = list_first_entry(&sba->reqs_free_list,
> +                                      struct sba_request,
> +                                      node);

You could use list_first_entry_or_null() here.

[..]
> +
> +/* Note: Must be called with sba->reqs_lock held */
> +static void _sba_pending_request(struct sba_device *sba,
> +                                struct sba_request *req)
> +{

You can validate the locking assumptions here with
lockdep_assert_head(sba->reqs_lock).

[..]
> +
> +static void sba_cleanup_nonpending_requests(struct sba_device *sba)
> +{
> +       unsigned long flags;
> +       struct sba_request *req, *req1;
> +
> +       spin_lock_irqsave(&sba->reqs_lock, flags);
> +
> +       /* Freeup all alloced request */
> +       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);
> +       }
> +
> +       /* Set all active requests as aborted */
> +       list_for_each_entry_safe(req, req1, &sba->reqs_active_list, node) {
> +               _sba_abort_request(sba, req);
> +       }

In some parts of the driver you leave off unneeded braces like the for
loop in sba_prep_dma_pq(), and in some case you include them. I'd say
remove them if they're not necessary, but either way make it
consistent across the driver.

[..]
> +
> +static struct dma_async_tx_descriptor *
> +sba_prep_dma_pq(struct dma_chan *dchan, dma_addr_t *dst, dma_addr_t *src,
> +               u32 src_cnt, const u8 *scf, size_t len, unsigned long flags)
> +{
> +       u32 i, dst_q_index;
> +       size_t req_len;
> +       bool slow = false;
> +       dma_addr_t off = 0;
> +       dma_addr_t *dst_p = NULL, *dst_q = NULL;
> +       struct sba_device *sba = to_sba_device(dchan);
> +       struct sba_request *first = NULL, *req;
> +
> +       /* Sanity checks */
> +       if (unlikely(src_cnt > sba->max_pq_srcs))
> +               return NULL;
> +       for (i = 0; i < src_cnt; i++)
> +               if (sba->max_pq_coefs <= raid6_gflog[scf[i]])
> +                       slow = true;

Thanks, yes, I do think this is cleaner here than in async_tx itself.

[..]
> +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_device *sba = req->sba;
> +
> +       /* Error count if message has error */
> +       if (m->error < 0) {
> +               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->state == 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);

You've now required all callback paths to be hardirq safe whereas
previously the callbacks only assumed softirq exclusion. Have you run
this with CONFIG_PROVE_LOCKING enabled?

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

#1579541 — Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver

On Fri, Feb 10, 2017 at 11:20 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Feb 10, 2017 at 1:07 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> The Broadcom stream buffer accelerator (SBA) provides offloading
>> capabilities for RAID operations. This SBA offload engine is
>> accessible via Broadcom SoC specific ring manager.
>>
>> This patch adds Broadcom SBA RAID driver which provides one
>> DMA device with RAID capabilities using one or more Broadcom
>> SoC specific ring manager channels. The SBA RAID driver in its
>> current shape implements memcpy, xor, and pq operations.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>  drivers/dma/Kconfig        |   13 +
>>  drivers/dma/Makefile       |    1 +
>>  drivers/dma/bcm-sba-raid.c | 1711 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 1725 insertions(+)
>>  create mode 100644 drivers/dma/bcm-sba-raid.c
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 263495d..bf8fb84 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -99,6 +99,19 @@ config AXI_DMAC
>>           controller is often used in Analog Device's reference designs for FPGA
>>           platforms.
>>
>> +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
>
> ASYNC_TX_ENABLE_CHANNEL_SWITCH violates the DMA mapping API and
> Russell has warned it's especially problematic on ARM [1].  If you
> need channel switching for this offload engine to be useful then you
> need to move DMA mapping and channel switching responsibilities to MD
> itself.
>
> [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036753.html

Actually driver works fine with/without
ASYNC_TX_ENABLE_CHANNEL_SWITCH enabled
so I am fine with removing dependency on this config option.

>
>
> [..]
>> diff --git a/drivers/dma/bcm-sba-raid.c b/drivers/dma/bcm-sba-raid.c
>> new file mode 100644
>> index 0000000..bab9918
>> --- /dev/null
>> +++ b/drivers/dma/bcm-sba-raid.c
>> @@ -0,0 +1,1711 @@
>> +/*
>> + * Copyright (C) 2017 Broadcom
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +/*
>> + * Broadcom SBA RAID Driver
>> + *
>> + * The Broadcom stream buffer accelerator (SBA) provides offloading
>> + * capabilities for RAID operations. The SBA offload engine is accessible
>> + * via Broadcom SoC specific ring manager. Two or more offload engines
>> + * can share same Broadcom SoC specific ring manager due to this Broadcom
>> + * SoC specific ring manager driver is implemented as a mailbox controller
>> + * driver and offload engine drivers are implemented as mallbox clients.
>> + *
>> + * Typically, Broadcom SoC specific ring manager will implement larger
>> + * number of hardware rings over one or more SBA hardware devices. By
>> + * design, the internal buffer size of SBA hardware device is limited
>> + * but all offload operations supported by SBA can be broken down into
>> + * multiple small size requests and executed parallely on multiple SBA
>> + * hardware devices for achieving high through-put.
>> + *
>> + * The Broadcom SBA RAID driver does not require any register programming
>> + * except submitting request to SBA hardware device via mailbox channels.
>> + * This driver implements a DMA device with one DMA channel using a set
>> + * of mailbox channels provided by Broadcom SoC specific ring manager
>> + * driver. To exploit parallelism (as described above), all DMA request
>> + * coming to SBA RAID DMA channel are broken down to smaller requests
>> + * and submitted to multiple mailbox channels in round-robin fashion.
>> + * For having more SBA DMA channels, we can create more SBA device nodes
>> + * in Broadcom SoC specific DTS based on number of hardware rings supported
>> + * by Broadcom SoC ring manager.
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/dmaengine.h>
>> +#include <linux/list.h>
>> +#include <linux/mailbox_client.h>
>> +#include <linux/mailbox/brcm-message.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/raid/pq.h>
>> +
>> +#include "dmaengine.h"
>> +
>> +/* SBA command helper macros */
>> +#define SBA_DEC(_d, _s, _m)            (((_d) >> (_s)) & (_m))
>> +#define SBA_ENC(_d, _v, _s, _m)                                        \
>> +               do {                                            \
>> +                       (_d) &= ~((u64)(_m) << (_s));           \
>> +                       (_d) |= (((u64)(_v) & (_m)) << (_s));   \
>> +               } while (0)
>
> Reusing a macro argument multiple times is problematic, consider
> SBA_ENC(..., arg++, ...), and hiding assignments in a macro make this
> hard to read. The compiler should inline it properly if you just make
> this a function that returns a value. You could also mark it __pure.

OK, I will make SBA_ENC as "static inline __pure" function.

>
> [..]
>> +
>> +static struct sba_request *sba_alloc_request(struct sba_device *sba)
>> +{
>> +       unsigned long flags;
>> +       struct sba_request *req = NULL;
>> +
>> +       spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> +       if (!list_empty(&sba->reqs_free_list)) {
>> +               req = list_first_entry(&sba->reqs_free_list,
>> +                                      struct sba_request,
>> +                                      node);
>
> You could use list_first_entry_or_null() here.

OK, will use this.

>
> [..]
>> +
>> +/* Note: Must be called with sba->reqs_lock held */
>> +static void _sba_pending_request(struct sba_device *sba,
>> +                                struct sba_request *req)
>> +{
>
> You can validate the locking assumptions here with
> lockdep_assert_head(sba->reqs_lock).

OK, will try this.

>
> [..]
>> +
>> +static void sba_cleanup_nonpending_requests(struct sba_device *sba)
>> +{
>> +       unsigned long flags;
>> +       struct sba_request *req, *req1;
>> +
>> +       spin_lock_irqsave(&sba->reqs_lock, flags);
>> +
>> +       /* Freeup all alloced request */
>> +       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);
>> +       }
>> +
>> +       /* Set all active requests as aborted */
>> +       list_for_each_entry_safe(req, req1, &sba->reqs_active_list, node) {
>> +               _sba_abort_request(sba, req);
>> +       }
>
> In some parts of the driver you leave off unneeded braces like the for
> loop in sba_prep_dma_pq(), and in some case you include them. I'd say
> remove them if they're not necessary, but either way make it
> consistent across the driver.

I think I relied too much on checkpatch.pl to catch this
kind of coding-style issues.

I will fix this. Thanks for catching.

>
> [..]
>> +
>> +static struct dma_async_tx_descriptor *
>> +sba_prep_dma_pq(struct dma_chan *dchan, dma_addr_t *dst, dma_addr_t *src,
>> +               u32 src_cnt, const u8 *scf, size_t len, unsigned long flags)
>> +{
>> +       u32 i, dst_q_index;
>> +       size_t req_len;
>> +       bool slow = false;
>> +       dma_addr_t off = 0;
>> +       dma_addr_t *dst_p = NULL, *dst_q = NULL;
>> +       struct sba_device *sba = to_sba_device(dchan);
>> +       struct sba_request *first = NULL, *req;
>> +
>> +       /* Sanity checks */
>> +       if (unlikely(src_cnt > sba->max_pq_srcs))
>> +               return NULL;
>> +       for (i = 0; i < src_cnt; i++)
>> +               if (sba->max_pq_coefs <= raid6_gflog[scf[i]])
>> +                       slow = true;
>
> Thanks, yes, I do think this is cleaner here than in async_tx itself.
>
> [..]
>> +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_device *sba = req->sba;
>> +
>> +       /* Error count if message has error */
>> +       if (m->error < 0) {
>> +               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->state == 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);
>
> You've now required all callback paths to be hardirq safe whereas
> previously the callbacks only assumed softirq exclusion. Have you run
> this with CONFIG_PROVE_LOCKING enabled?

We have run stress tests on driver with multiple threads
trying to submit txn.

I will certainly try CONFIG_PROVE_LOCKING to be
double sure.

Thanks,
Anup

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

#1580287 — Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver

On Mon, Feb 13, 2017 at 2:43 PM, Anup Patel <anup.patel@broadcom.com> wrote:
> On Fri, Feb 10, 2017 at 11:20 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Fri, Feb 10, 2017 at 1:07 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>>> The Broadcom stream buffer accelerator (SBA) provides offloading
>>> capabilities for RAID operations. This SBA offload engine is
>>> accessible via Broadcom SoC specific ring manager.
>>>
>>> This patch adds Broadcom SBA RAID driver which provides one
>>> DMA device with RAID capabilities using one or more Broadcom
>>> SoC specific ring manager channels. The SBA RAID driver in its
>>> current shape implements memcpy, xor, and pq operations.
>>>
>>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>>> ---
>>>  drivers/dma/Kconfig        |   13 +
>>>  drivers/dma/Makefile       |    1 +
>>>  drivers/dma/bcm-sba-raid.c | 1711 ++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 1725 insertions(+)
>>>  create mode 100644 drivers/dma/bcm-sba-raid.c
>>>
>>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>>> index 263495d..bf8fb84 100644
>>> --- a/drivers/dma/Kconfig
>>> +++ b/drivers/dma/Kconfig
>>> @@ -99,6 +99,19 @@ config AXI_DMAC
>>>           controller is often used in Analog Device's reference designs for FPGA
>>>           platforms.
>>>
>>> +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
>>
>> ASYNC_TX_ENABLE_CHANNEL_SWITCH violates the DMA mapping API and
>> Russell has warned it's especially problematic on ARM [1].  If you
>> need channel switching for this offload engine to be useful then you
>> need to move DMA mapping and channel switching responsibilities to MD
>> itself.
>>
>> [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036753.html
>
> Actually driver works fine with/without
> ASYNC_TX_ENABLE_CHANNEL_SWITCH enabled
> so I am fine with removing dependency on this config option.

I stand corrected.

Previously, when I had tried removing
ASYNC_TX_ENABLE_CHANNEL_SWITCH
from BCM_SBA_RAID config option it worked because other
drivers such xgene-dma and mv_xor_v2 are selecting this option.

The BCM-SBA-RAID driver requires
ASYNC_TX_ENABLE_CHANNEL_SWITCH option

There is no issue reported for
ASYNC_TX_ENABLE_CHANNEL_SWITCH
with ARM64 kernel.

The issue you pointed out was with ARM kernel.

We will have to select
ASYNC_TX_ENABLE_CHANNEL_SWITCH
for BCM-SBA-RAID driver
just like other ARM64 RAID drivers such
as xgene-dma and mv_xor_v2.
(Refer, XGENE_DMA and MV_XOR_V2 options)

Regards,
Anup

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

#1580305 — Re: [PATCH v3 3/4] dmaengine: Add Broadcom SBA RAID driver

On Fri, Feb 10, 2017 at 11:20 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Fri, Feb 10, 2017 at 1:07 AM, Anup Patel <anup.patel@broadcom.com> wrote:
>> The Broadcom stream buffer accelerator (SBA) provides offloading
>> capabilities for RAID operations. This SBA offload engine is
>> accessible via Broadcom SoC specific ring manager.
>>
>> This patch adds Broadcom SBA RAID driver which provides one
>> DMA device with RAID capabilities using one or more Broadcom
>> SoC specific ring manager channels. The SBA RAID driver in its
>> current shape implements memcpy, xor, and pq operations.
>>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>> ---
>>  drivers/dma/Kconfig        |   13 +
>>  drivers/dma/Makefile       |    1 +
>>  drivers/dma/bcm-sba-raid.c | 1711 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 1725 insertions(+)
>>  create mode 100644 drivers/dma/bcm-sba-raid.c
>>
>> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>> index 263495d..bf8fb84 100644
>> --- a/drivers/dma/Kconfig
>> +++ b/drivers/dma/Kconfig
>> @@ -99,6 +99,19 @@ config AXI_DMAC
>>           controller is often used in Analog Device's reference designs for FPGA
>>           platforms.
>>
>> +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
>
> ASYNC_TX_ENABLE_CHANNEL_SWITCH violates the DMA mapping API and
> Russell has warned it's especially problematic on ARM [1].  If you
> need channel switching for this offload engine to be useful then you
> need to move DMA mapping and channel switching responsibilities to MD
> itself.
>
> [1]: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-January/036753.html

In case of BCM-SBA-RAID, the underlying "struct device" for each
DMA channel is the mailbox controller "struct device" (i.e. Ring
Manager device). This is because Ring Manager HW is the front
facing device which we program to submit work to BCM-SBA-RAID
HW.

This means DMA channels provided by BCM-SBA-RAID driver
will use same "struct device" for DMA mappings hence channel
switching between BCM-SBA-RAID DMA channels is safe.

Due to above, we can safely enable
ASYNC_TX_ENABLE_CHANNEL_SWITCH option for
BCM-SBA-RAID driver.

Regards,
Anup

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web