Re: [PATCH v2] dmatest: masking tests for channel capabilities

[Dan Williams]

On Fri, Jun 21, 2013 at 4:46 PM, Jubin Mehta <jubin.mehta@xxxxxxxxx> wrote:
> The current dmatest module tests all the hardware capabilities (MEMCPY, XOR
> and PQ) supported by a particular DMA channel and these tests are performed
> concurrently by default. This patch allows the user to enable or disable the
> test performed for any particular capability. The mask bits for enabling the
> tests are set using the debugfs.
>
> Signed-off-by: Jubin Mehta <jubin.mehta@xxxxxxxxx>
[..]
> diff --git a/drivers/dma/dmatest.c b/drivers/dma/dmatest.c
> index d8ce4ec..c152755 100644
> --- a/drivers/dma/dmatest.c
> +++ b/drivers/dma/dmatest.c
> @@ -23,6 +23,7 @@
>  #include <linux/debugfs.h>
>  #include <linux/uaccess.h>
>  #include <linux/seq_file.h>
> +#include <linux/string.h>
>
>  static unsigned int test_buf_size = 16384;
>  module_param(test_buf_size, uint, S_IRUGO);
> @@ -66,6 +67,11 @@ module_param(timeout, uint, S_IRUGO);
>  MODULE_PARM_DESC(timeout, "Transfer Timeout in msec (default: 3000), "
>                  "Pass -1 for infinite timeout");
>
> +static char test_cap_mask[20] = "copy,xor,pq";
> +module_param_string(cap_mask, test_cap_mask, sizeof(test_cap_mask), S_IRUGO);

Let's just make this module parameter writable so we don't need to add
a new debugfs file.

The 'get' routine can also display the available testable operation types.

> +MODULE_PARM_DESC(cap_mask, "Mask bits for different capability test "
> +                "(default: copy,xor,pq)");
> +
>  /* Maximum amount of mismatched bytes in buffer to print */
>  #define MAX_ERROR_COUNT                32
>
> @@ -164,6 +170,7 @@ struct dmatest_chan {
>   * @xor_sources:       number of xor source buffers
>   * @pq_sources:                number of p+q source buffers
>   * @timeout:           transfer timeout in msec, -1 for infinite timeout
> + * @cap_mask:          mask for different hardware capabilities tests
>   */
>  struct dmatest_params {
>         unsigned int    buf_size;
> @@ -175,6 +182,7 @@ struct dmatest_params {
>         unsigned int    xor_sources;
>         unsigned int    pq_sources;
>         int             timeout;
> +       char            cap_mask[128];

This can be the capability mask directly... see below:

>  };
>
>  /**
> @@ -883,11 +891,44 @@ static int dmatest_add_threads(struct dmatest_info *info,
>         return i;
>  }
>
> +static int dmatest_cap_check(struct dmatest_info *info,
> +               dma_cap_mask_t *test_cap_mask)
> +{
> +       struct dmatest_params   *params;
> +       char                    *cap;
> +       unsigned char c;
> +       int i;
> +
> +       params = &info->params;
> +       cap = (params->cap_mask);
> +
> +       for (i = 0; i < 3; i++) {
> +               if (!strncmp(cap, "copy", 4)) {
> +                       dma_cap_set(DMA_MEMCPY, *test_cap_mask);
> +                       cap += 4;
> +               } else if (!strncmp(cap, "xor", 3)) {
> +                       dma_cap_set(DMA_XOR, *test_cap_mask);
> +                       cap += 3;
> +               } else if (!strncmp(cap, "pq", 2)) {
> +                       dma_cap_set(DMA_PQ, *test_cap_mask);
> +                       cap += 2;
> +               } else {
> +                       return -1;
> +               }
> +               c = *cap++;
> +               if (c != ',')
> +                       break;
> +       }
> +       return 0;
> +}

We can run this routine when the capabilities are set to return error
on invalid capability masks directly.  Then we can set the
test_cap_mask before dmatest_add_channel().

> +
>  static int dmatest_add_channel(struct dmatest_info *info,
>                 struct dma_chan *chan)
>  {
>         struct dmatest_chan     *dtc;
>         struct dma_device       *dma_dev = chan->device;
> +       struct dmatest_params   *params;
> +       dma_cap_mask_t          test_cap_mask;
>         unsigned int            thread_count = 0;
>         int cnt;
>
> @@ -900,15 +941,28 @@ static int dmatest_add_channel(struct dmatest_info *info,
>         dtc->chan = chan;
>         INIT_LIST_HEAD(&dtc->threads);
>
> -       if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)) {
> +       params = &info->params;
> +
> +       dma_cap_zero(test_cap_mask);
> +       cnt = dmatest_cap_check(info, &test_cap_mask);
> +       if (cnt) {
> +               pr_warn("dmatest: Invalid Capability Mask Paramters\n");
> +               return -EINVAL;
> +       }

As above I think this too late to be validating the parameters.

> +
> +       /* Run dmatest threads for the supported and enabled capabilities */
> +       if (dma_has_cap(DMA_MEMCPY, dma_dev->cap_mask)
> +           && dma_has_cap(DMA_MEMCPY, test_cap_mask)) {
>                 cnt = dmatest_add_threads(info, dtc, DMA_MEMCPY);
>                 thread_count += cnt > 0 ? cnt : 0;
>         }
> -       if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)) {
> +       if (dma_has_cap(DMA_XOR, dma_dev->cap_mask)
> +           && dma_has_cap(DMA_XOR, test_cap_mask)) {
>                 cnt = dmatest_add_threads(info, dtc, DMA_XOR);
>                 thread_count += cnt > 0 ? cnt : 0;
>         }
> -       if (dma_has_cap(DMA_PQ, dma_dev->cap_mask)) {
> +       if (dma_has_cap(DMA_PQ, dma_dev->cap_mask)
> +           && dma_has_cap(DMA_PQ, test_cap_mask)) {
>                 cnt = dmatest_add_threads(info, dtc, DMA_PQ);
>                 thread_count += cnt > 0 ? cnt : 0;
>         }

Could above be written simpler with one bitmap_and(test_cap_mask,
dma_dev->cap_mask, test_cap_mask) at the start and then
for_each_set_bit(test_cap_mask)?


--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/