linux-kernel - Re: [PATCH v2 1/2] mtd: spi-nor: Move m25p80 code in spi-nor.c

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: <20190725160025.2d8e24f8@collabora.com>
Date:   Thu, 25 Jul 2019 16:00:25 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     <Tudor.Ambarus@...rochip.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

On Thu, 25 Jul 2019 13:17:07 +0000
<Tudor.Ambarus@...rochip.com> wrote:

> 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?

I guess you'll also need one on the following field to guarantee that
cmd_buf is covering the whole cache line. TBH, I really prefer the
option of allocating ->cmd_buf.

> 
> > 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.

Not entirely sure why you think this is important to avoid that
test (looks like a micro-optimization to me), but if you really want to
have a non-const buffer, just use the one pointed by op->data.buf.in
(buf is a union so both in and out point to the same thing). Note that
we'd need a comment explaining why this is safe to do that, because
bypassing constness constraints is usually a bad thing.

> 
> pointer to buf was not needed in spi_nor_spimem_xfer_reg(), we could use
> nor->cmd_buf.

Do you mean that callers of spi_nor_spimem_xfer_data() should put data
into/read from ->cmd_buf and let spi_nor_spimem_xfer_data() assign
op->data.buf.{in,out} to ->cmd_buf?