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]

Date:   Mon, 31 Jul 2017 23:35:05 +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 07/31/2017 10:54 PM, Alexandru Gagniuc wrote:
> Hi Marek,
> 
> Me again!
> 
> On 07/29/2017 02:34 AM, Marek Vasut wrote:
>> On 07/29/2017 12:07 AM, Alexandru Gagniuc wrote:
>>> +static void aspi_drain_fifo(struct anarion_qspi *aspi, uint8_t *buf,
>>> size_t len)
>>> +{
>>> +    uint32_t data;
>>
>> Is this stuff below something like ioread32_rep() ?
>>
> [snip]
>>> +    aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>>> +    while (len >= 4) {
>>> +        memcpy(&data, buf, sizeof(data));
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>
>> iowrite32_rep ?
>>
>>> +        buf += 4;
>>> +        len -= 4;
>>> +    }
> 
> I looked at using io(read|write)32_rep in these two places, and I've run
> into some issues.
> 
> First, I'm seeing unaligned MMIO accesses, which are not supported on
> ARC. Note that 'buf' has an alignment of 1, while the register requires
> an alignment of 4. The memcpy() in-between takes care of that, which was
> the original intent.
> 
> Other than that, we still need to break off the tail because we need to
> update ASPI_REG_BYTE_COUNT before writing/reading any more data from the
> FIFO. We have to keep track of the remainder, so we're not really saving
> any SLOC.
> 
> I'd like to keep the original version as I find it to be much more
> symmetrical and readable.

Look at the other drivers, AFAIR the io*_rep() issue was solved over and
over again, maybe you can factor that code out.

> Thanks,
> Alex
> 
>>> +
>>> +    if (len) {
>>> +        aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>>> +        memcpy(&data, buf, len);
>>> +        aspi_write_reg(spi, ASPI_REG_DATA1, data);
>>> +    }
>>> +}
> 
> 
> Exhibit A: aspi_seed_fifo with writesl()
> 
> static void aspi_seed_fifo(struct anarion_qspi *spi,
>                const uint8_t *buf, size_t len)
> {
>     uint32_t data;
>     void __iomem *data_reg = (void *)(spi->regbase + ASPI_REG_DATA1);
> 
>     aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, sizeof(uint32_t));
>     writesl(data_reg, buf, len / 4);
>     buf += len & ~0x03;
>     len &= 0x03;
> 
>     if (len) {
>         aspi_write_reg(spi, ASPI_REG_BYTE_COUNT, len);
>         memcpy(&data, buf, len);
>         aspi_write_reg(spi, ASPI_REG_DATA1, data);
>     }
> }


-- 
Best regards,
Marek Vasut