linux-kernel - Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI controller

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1da97744-bc02-420e-d4e9-5ebf331475e8@gmail.com>
Date:   Tue, 1 Aug 2017 00:43:41 +0200
From:   Marek Vasut <marek.vasut@...il.com>
To:     Alexandru Gagniuc <alex.g@...ptrum.com>,
        linux-snps-arc@...ts.infradead.org, linux-kernel@...r.kernel.org
Cc:     David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Richard Weinberger <richard@....at>,
        Cyrille Pitchen <cyrille.pitchen@...ev4u.fr>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-mtd@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 4/5] mtd: spi-nor: Add driver for Adaptrum Anarion QSPI
 controller

On 08/01/2017 12:20 AM, Alexandru Gagniuc wrote:
> On 07/31/2017 02:33 PM, Marek Vasut wrote:
>> On 07/31/2017 07:17 PM, Alexandru Gagniuc wrote:
> 
> Hi Marek,
> 
> Thank you again for your feedback. I've applied a majority of your
> suggestions, and I am very happy with the result. I should have v2
> posted within a day or so.

No. You should have v2 out in about a week or so after people have time
to review v1 some more.

> [snip]
>>>>> +/*
>>>>> + * This mask does not match reality. Get over it:
>>>>
>>>> What is this about ?
>>>
>>> Each stage of the QSPI chain has two registers. The second register has
>>> a bitfield which takes in the length of the stage. For example, for
>>> DATA2, we can set the length up to 0x4000, but for ADDR2, we can only
>>> set a max of 4 bytes. I wrote this comment as a reminder to myself to be
>>> careful about using this mask. I'll rephrase the comment for [v2]
>>
>> Please do.
>>
> Staged for [PATCH v2]
> 
>>>>> + * DATA2:    0x3fff
>>>>> + * CMD2:    0x0003
>>>>> + * ADDR2:    0x0007
>>>>> + * PERF2:    0x0000
>>>>> + * HI_Z:    0x003f
>>>>> + * BCNT:    0x0007
>>>>> + */
>>>>> +#define CHAIN_LEN(x)        ((x - 1) & ASPI_DATA_LEN_MASK)

btw parenthesis around (x) missing, although this is like GEN_MASK() or
something here ...

>>>>> +struct anarion_qspi {
>>>>> +    struct        spi_nor nor;
>>>>> +    struct        device *dev;
>>>>> +    uintptr_t    regbase;
>>>>
>>>> Should be void __iomem * I guess ?
>>>
>>> I chose uintptr_t as opposed to void *, because arithmetic on void * is
>>> not valid in C. What is the right answer hen, without risking undefined
>>> behavior?
>>
>> What sort of arithmetic ? It's perfectly valid in general ...
> 
> ISO/IEC 9899:201x, Section 6.5.6, constraint(2) is not met when the one
> of the operands to addition is a void pointer.
> Section 6.2.5 (19) defines void to be an incomplete type.

Is that something new in C 201x draft ? Anyway, this would mean half of
the drivers are broken, so I'm not convinced.

> [snip]
> 
>>>> Is this stuff below something like ioread32_rep() ?
>>>>
>>>>> +    aspi_write_reg(aspi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>>>> +    while (len >= 4) {
>>>>> +        data = aspi_read_reg(aspi, ASPI_REG_DATA1);
>>>>> +        memcpy(buf, &data, sizeof(data));
>>>>> +        buf += 4;
>>>>> +        len -= 4;
>>>>> +    }
>>>
>>> That is very similar to ioread32_rep, yes. I kept this as for the
>>> reasons outlined above, but changing this to _rep() seems innocent
>>> enough.
>>
>> What reason ?
> 
> Being able to share the code between the different codebases where it is
> used.

Yes, that argument isn't gonna work, it'd make things impossible to
maintain in the kernel.

-- 
Best regards,
Marek Vasut