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

[<Tudor.Ambarus@microchip.com>]

On 07/25/2019 05:00 PM, Boris Brezillon wrote:
> External E-Mail
> 
> 
> On Thu, 25 Jul 2019 13:17:07 +0000
> <Tudor.Ambarus@microchip.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@microchip.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.

agreed.

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

No need for a buf argument, I missed that the const qualifier will be discarded
when passing the pointer. We'll keep the function as it is, with the amend that
the "enum spi_nor_protocol proto" will be removed from the arguments.

Thanks for jumping in,
ta