| 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
| ||
|
Message-ID: <dbb33973-bb6f-9a01-b821-693387aff98a@microchip.com>
Date: Thu, 25 Jul 2019 13:17:07 +0000
From: <Tudor.Ambarus@...rochip.com>
To: <boris.brezillon@...labora.com>
CC: <vigneshr@...com>, <miquel.raynal@...tlin.com>, <richard@....at>,
<marek.vasut@...il.com>, <bbrezillon@...nel.org>,
<yogeshnarayan.gaur@....com>, <linux-kernel@...r.kernel.org>,
<linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH v2 1/2] mtd: spi-nor: Move m25p80 code in spi-nor.c
Hi, Boris,
On 07/25/2019 03:37 PM, Boris Brezillon wrote:
> External E-Mail
>
>
> On Thu, 25 Jul 2019 11:19:06 +0000
> <Tudor.Ambarus@...rochip.com> wrote:
>
>>> + */
>>> +static int spi_nor_exec_op(struct spi_nor *nor, struct spi_mem_op *op,
>>> + u64 *addr, void *buf, size_t len)
>>> +{
>>> + int ret;
>>> + bool usebouncebuf = false;
>>
>> I don't think we need a bounce buffer for regs. What is the maximum size that we
>> read/write regs, SPI_NOR_MAX_CMD_SIZE(8)?
>>
>> In spi-nor.c the maximum length that we pass to nor->read_reg()/write_reg() is
>> SPI_NOR_MAX_ID_LEN(6).
>>
>> I can provide a patch to always use nor->cmd_buf when reading/writing regs so
>> you respin the series on top of it, if you feel the same.
>>
>> With nor->cmd_buf this function will be reduced to the following:
>>
>> static int spi_nor_spimem_xfer_reg(struct spi_nor *nor, struct spi_mem_op *op)
>> {
>> if (!op || (op->data.nbytes && !nor->cmd_buf))
>> return -EINVAL;
>>
>> return spi_mem_exec_op(nor->spimem, op);
>> }
>
> Well, I don't think that's a good idea. ->cmd_buf is an array in the
> middle of the spi_nor struct, which means it won't be aligned on a
> cache line and you'll have to be extra careful not to touch the spi_nor
> fields when calling spi_mem_exec_op(). Might work, but I wouldn't take
> the risk if I were you.
>
u8 cmd_buf[SPI_NOR_MAX_CMD_SIZE] ____cacheline_aligned;
Does this help?
> Another option would be to allocate ->cmd_buf with kmalloc() instead of
> having it defined as a static array.
>
>>
>> spi_nor_exec_op() always received a NULL addr, let's get rid of it. We won't
>> need buf anymore and you can retrieve the length from op->data.nbytes. Now that
>> we trimmed the arguments, I think I would get rid of the
>> spi_nor_data/nodata_op() wrappers and use spi_nor_spimem_xfer_reg() directly.
>
> I think I added the addr param for a good reason (probably to support
> Octo mode cmds that take an address parameter). This being said, I
> agree with you, we should just pass everything through the op parameter
> (including the address if we ever need to add one).
>
>
>>> +
>>> +/**
>>> + * spi_nor_spimem_xfer_data() - helper function to read/write data to
>>> + * flash's memory region
>>> + * @nor: pointer to 'struct spi_nor'
>>> + * @op: pointer to 'struct spi_mem_op' template for transfer
>>> + * @proto: protocol to be used for transfer
>>> + *
>>> + * Return: number of bytes transferred on success, -errno otherwise
>>> + */
>>> +static ssize_t spi_nor_spimem_xfer_data(struct spi_nor *nor,
>>> + struct spi_mem_op *op,
>>> + enum spi_nor_protocol proto)
>>> +{
>>> + bool usebouncebuf = false;
>>
>> declare bool at the end to avoid stack padding.
>
> But it breaks the reverse-xmas-tree formatting :-).
>
>>
>>> + void *rdbuf = NULL;
>>> + const void *buf;
>>
>> you can get rid of rdbuf and buf if you pass buf as argument.
>
> Hm, passing the buffer to send data from/receive data into is already
> part of the spi_mem_op definition process (which is done in the caller
> of this func) so why bother passing an extra arg to the function.
> Note that you had the exact opposite argument for the
> spi_nor_spimem_xfer_reg() prototype you suggested above (which I
> agree with BTW) :P.
In order to avoid if clauses like "if (op->data.dir == SPI_MEM_DATA_IN)". You
can't use op->data.buf directly, the *out const qualifier can be discarded.
pointer to buf was not needed in spi_nor_spimem_xfer_reg(), we could use
nor->cmd_buf.
>
>>
>>> + int ret;
>>> +
>>> + /* get transfer protocols. */
>>> + op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
>>> + op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
>>> + op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
>>> +
>>> + if (op->data.dir == SPI_MEM_DATA_IN)
>>> + buf = op->data.buf.in;
>>> + else
>>> + buf = op->data.buf.out;
>>> +
>>> + if (object_is_on_stack(buf) || !virt_addr_valid(buf))
>>> + usebouncebuf = true;
>>> +
>>> + if (usebouncebuf) {
>>> + if (op->data.nbytes > nor->bouncebuf_size)
>>> + op->data.nbytes = nor->bouncebuf_size;
>>> +
>>> + if (op->data.dir == SPI_MEM_DATA_IN) {
>>> + rdbuf = op->data.buf.in;
>>> + op->data.buf.in = nor->bouncebuf;
>>> + } else {
>>> + op->data.buf.out = nor->bouncebuf;
>>> + memcpy(nor->bouncebuf, buf,
>>> + op->data.nbytes);
>>> + }
>>> + }
>>> +
>>> + ret = spi_mem_adjust_op_size(nor->spimem, op);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = spi_mem_exec_op(nor->spimem, op);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (usebouncebuf && op->data.dir == SPI_MEM_DATA_IN)
>>> + memcpy(rdbuf, nor->bouncebuf, op->data.nbytes);
>>> +
>>> + return op->data.nbytes;
>>> +}
>>> +
>>> +/**
>>> + * spi_nor_spimem_read_data() - read data from flash's memory region via
>>> + * spi-mem
>>> + * @nor: pointer to 'struct spi_nor'
>>> + * @ofs: offset to read from
>>> + * @len: number of bytes to read
>>> + * @buf: pointer to dst buffer
>>> + *
>>> + * Return: number of bytes read successfully, -errno otherwise
>>> + */
>>> +static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t ofs,
>>
>> s/ofs/from? both flash and buf may have offsets, "from" better indicates that
>> the offset is associated with the flash.
>
> The semantic is well documented in the doc just above the function, but
> I have the feeling that you're in 'nitpick' mode, so I'll just let you
> pick the one you prefer :).
Not my intention. struct mtd_info and struct spi_nor use "from" when referring
to the offset from where to read in the read() calls. Just consistency reasons.
Cheers,
ta
Powered by blists - more mailing lists