linux-kernel - Re: [PATCH V2 2/2] dmaengine: dmatest: add support for memset test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75VfJ=nq4Ycd7SDiXbW4hXNZ3o_6VEiF7JM3J=vrwVcUnYg@mail.gmail.com>
Date:   Thu, 29 Jun 2017 15:05:58 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Sinan Kaya <okaya@...eaurora.org>
Cc:     dmaengine <dmaengine@...r.kernel.org>,
        Timur Tabi <timur@...eaurora.org>,
        linux-arm-msm@...r.kernel.org,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Dan Williams <dan.j.williams@...el.com>,
        Vinod Koul <vinod.koul@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 2/2] dmaengine: dmatest: add support for memset test

On Thu, Jun 29, 2017 at 4:51 AM, Sinan Kaya <okaya@...eaurora.org> wrote:
> Introducing memset test into dmatest. It allows us to test memset capable
> HW using the dmatest suite. The new dmatest value is 2 and it is
> changeable through sysfs.
>
> Memset shares the same code path as the other dmatest code. The only
> difference is that the first value inside the source buffer is used
> to fill in the destination address space.
>
> Source/destination buffers are initialized with the 1 counter value
> prior to test so that we can do pattern check against a known expected
> value.
>
> An example run us as follows:
>
> echo dma0chan0 > /sys/module/dmatest/parameters/channel
> echo 2 >  /sys/module/dmatest/parameters/dmatest
> echo 2000 >  /sys/module/dmatest/parameters/timeout
> echo 10 >  /sys/module/dmatest/parameters/iterations
> echo 1 >  /sys/module/dmatest/parameters/run
>

See my comments below.
After addressing them,
Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>

> +static inline u8 gen_inv_idx(u8 index, bool is_memset)
> +{
> +       u8 val = is_memset ? PATTERN_MEMSET_IDX : index;
> +

> +       return (~val & PATTERN_COUNT_MASK);

Redundant parens.

> +}

> +                       buf[i] = gen_src_value(i, is_memset);
> +

Please, remove this new empty lines which doesn't belong to the patch.

> +                       buf[i] = gen_src_value(i, is_memset) | PATTERN_COPY;
> +

Ditto.

> +                       buf[i] = gen_dst_value(i, is_memset);
> +

Ditto.

> +                       buf[i] = gen_dst_value(i, is_memset) |
> +                                               PATTERN_OVERWRITE;
> +

Ditoo.

> +                               0, PATTERN_DST, false, is_memset);
> +

Ditto.

> +                               PATTERN_SRC | PATTERN_COPY, false, is_memset);
> +

Ditto.

> +       if (dma_has_cap(DMA_MEMSET, dma_dev->cap_mask)) {
> +               if (dmatest == 2) {

Double if is effectively && in this case. Though it looks like the
pattern already in use in the module.
So, for now we might leave it untouched.

> +                       cnt = dmatest_add_threads(info, dtc, DMA_MEMSET);
> +                       thread_count += cnt > 0 ? cnt : 0;
> +               }
> +       }

-- 
With Best Regards,
Andy Shevchenko