linux-kernel - Re: [PATCH 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]

Date:   Wed, 28 Jun 2017 21:54:14 -0400
From:   Sinan Kaya <okaya@...eaurora.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
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 2/2] dmaengine: dmatest: add support for memset test

Hi Andy,


On 6/28/2017 4:58 AM, Andy Shevchenko wrote:
> On Wed, Jun 28, 2017 at 3:46 AM, Sinan Kaya <okaya@...eaurora.org> wrote:
>>
>> 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
> 
> Good someone is developing tests!
> My comments below.

Yeah, no problem. 

> 
>>  static void dmatest_init_srcs(u8 **bufs, unsigned int start, unsigned int len,
>> +               unsigned int buf_size, bool is_memset)
>>  {
>>         unsigned int i;
>>         u8 *buf;
>>
>>         for (; (buf = *bufs); bufs++) {
>> +               for (i = 0; i < start; i++) {
> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> +                       buf[i] = PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>>                         buf[i] = PATTERN_SRC | PATTERN_COPY
>> +                               | (~val & PATTERN_COUNT_MASK);
> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> +                       buf[i] = PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
> 
> Looks like a candidate now for a helper function
> 
> static inline u8 gen_src_value(u8 index, bool is_memset)
> {
>     u8 val = is_memset ? PATTERN_MEMSET_IDX : i;
>     return PATTERN_SRC | (~val & PATTERN_COUNT_MASK);
> }
> 
> buf[i] = gen_src_value(i, is_memset);
> buf[i] = gen_src_value(i, is_memset) | PATTERN_COPY;
> buf[i] = gen_src_value(i, is_memset);

will change as you recommended.

> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> +                       buf[i] = PATTERN_DST | (~val & PATTERN_COUNT_MASK);
> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>>                         buf[i] = PATTERN_DST | PATTERN_OVERWRITE
>> +                               | (~val & PATTERN_COUNT_MASK);
> 
>> +                       unsigned int val = is_memset ? PATTERN_MEMSET_IDX : i;
>> +
>> +                       buf[i] = PATTERN_DST | (~val & PATTERN_COUNT_MASK);
> 
> Ditto for DST.

done.

> 
>> +               unsigned int counter, bool is_srcbuf, bool is_memset)
> 
> This is a signal to replace it with
> 
> unsigned int flags
> 
> #define DMATEST_FL_SRCBUF BIT(0)
> #define DMATEST_FL_MEMSET BIT(1)
> 
> or alike.
> 
> I'm not insisting, just consider an opportunity.

OK. I'm not touching it for the moment. Maybe, we can have another patch later
to get rid of flags.

> 
>> +               bool is_srcbuf, bool is_memset)
> 
> Ditto.
> 
>>                 start = ktime_get();
>> +
>>                 pr_debug("%s: verifying source buffer...\n", current->comm);
> 
> The change doesn't belong to the patch.
> 
>> +
>>                 error_count += dmatest_verify(thread->dsts, dst_off + len,
> 
> Ditto.
> 

Posted V2 with the change now.

Sinan

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.