linux-kernel - Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for register read/writes

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: <d057db47-64a6-01ca-2ccb-d57c532213d4@ti.com>
Date:   Thu, 1 Aug 2019 12:15:30 +0530
From:   Vignesh Raghavendra <vigneshr@...com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
CC:     Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Boris Brezillon <bbrezillon@...nel.org>,
        <linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v3 1/3] mtd: spi-nor: always use bounce buffer for
 register read/writes

Hi Boris

On 01/08/19 11:16 AM, Boris Brezillon wrote:
> On Thu, 1 Aug 2019 10:00:50 +0530
> Vignesh Raghavendra <vigneshr@...com> wrote:
> 
>> spi-mem layer expects all buffers passed to it to be DMA'able. But
>> spi-nor layer mostly allocates buffers on stack for reading/writing to
>> registers and therefore are not DMA'able. Introduce bounce buffer to be
>> used to read/write to registers. This ensures that buffer passed to
>> spi-mem layer during register read/writes is DMA'able. With this change
>> nor->cmd-buf is no longer used, so drop it.
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
>> ---
>> v3: new patch
>>

[...]

>> @@ -2180,11 +2179,13 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
>>  	u8			id[SPI_NOR_MAX_ID_LEN];
>>  	const struct flash_info	*info;
>>  
>> -	tmp = nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN);
>> +	tmp = nor->read_reg(nor, SPINOR_OP_RDID, nor->bouncebuf,
>> +			    SPI_NOR_MAX_ID_LEN);
>>  	if (tmp < 0) {
>>  		dev_err(nor->dev, "error %d reading JEDEC ID\n", tmp);
>>  		return ERR_PTR(tmp);
>>  	}
>> +	memcpy(id, nor->bouncebuf, SPI_NOR_MAX_ID_LEN);
> 
> Why not directly including the change you have in patch 2 (id is a
> pointer that points directly to ->bouncebuf) so you can get rid of this
> memcpy() here?
>

Ok will do that in next version.


>>  
>>  	for (tmp = 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) {
>>  		info = &spi_nor_ids[tmp];
>> @@ -4121,6 +4122,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	nor->read_proto = SNOR_PROTO_1_1_1;
>>  	nor->write_proto = SNOR_PROTO_1_1_1;
>>  
>> +	/*
>> +	 * We need the bounce buffer early to read/write registers when going
>> +	 * through the spi-mem layer (buffers have to be DMA-able).
> 
> You should probably extend this comment in patch 2 to explain why 4k
> should be enough for regular read/write operations.
> 

Will update

> The patch looks good otherwise.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@...labora.com>
> 

Thanks for the review!

-- 
Regards
Vignesh