[PATCH] mtd: spi-nor: use _lock_and_prep() in _read_sfdp()

[Cyrille Pitchen]

Le 22/12/2017 à 13:49, Cyrille Pitchen a écrit :
> Hi Yogesh,
> 
> Le 22/12/2017 à 09:35, Yogesh Gaur a écrit :
>> spi_nor_read_sfdp() calls nor->read() to read the SFDP data.
>> But first needs to call nor->prepare() to prepare the READ command before
>> calling nor->read().
>>
> 
> Currently, I don't agree with this: spi_nor_read_sfdp() is called only from
> spi_nor_scan(). Before the SFDP patch, spi_nor_scan() has already been
> calling for years nor->read_reg() and nor->write_reg() without calling
> spi_nor_lock_and_prep() first.
> 
> Indeed, still being in spi_nor_scan(), the SPI NOR memory should have not
> been registered yet to the above MTD layer. So there should be no need to
> lock nor->lock at this point.
> 
> Then, about calling nor->prepare(), IMHO, it's look a little bit odd that

s/it's look/it looks/

:p

> neither nor->read_reg() nor nor->write_reg() require nor->prepare() to be
> called first whereas nor->read() does need it.
> 
> You didn't describe what issue or bug you actually noticed so now I assume
> it deals with the fsl-quadspi.c driver. So let's have a look at its
> implementation of nor->prepare() / fsl_qspi_prep():
> 
> +static int fsl_qspi_prep(struct spi_nor *nor, enum spi_nor_ops ops)
> +{
> +	struct fsl_qspi *q = nor->priv;
> +	int ret;
> +
> +	mutex_lock(&q->lock);
> 
> It was not needed before by fsl_qspi_read_reg() or fls_qspi_write_reg(),
> why would it be needed now by fls_qspi_read()?
> 
> Then I guess, this is not the issue.
> 
> Anyway, if actually needed or safer, the mutex can be locked just after
> mutex_init() and before for_each_available_child_of_node() in
> fsl_qspi_probe().
> 
> +
> +	ret = fsl_qspi_clk_prep_enable(q);
> 
> Already called from fls_qpsi_probe() before calling spi_nor_scan():
> should not be the issue neither.
> 
> +	if (ret)
> +		goto err_mutex;
> +
> +	fsl_qspi_set_base_addr(q, nor);
> 
> Also called from fsl_qspi_probe() right before calling spi_nor_scan():
> 
> 		/* set the chip address for READID */
> 		fsl_qspi_set_base_addr(q, nor);
> 
> 		ret = spi_nor_scan(nor, NULL, &hwcaps);
> 		if (ret)
> 			goto mutex_failed;
> 
> 		ret = mtd_device_register(mtd, NULL, 0);
> 		if (ret)
> 			goto mutex_failed;
> 
> 		/* Set the correct NOR size now. */
> 		if (q->nor_size == 0) {
> 			q->nor_size = mtd->size;
> 
> 			/* Map the SPI NOR to accessiable address */
> 			fsl_qspi_set_map_addr(q);
> 		}
> 
> q->nor_size is only updated after spi_nor_scan() hence I don't see why
> calling nor->prepare() hence fsl_qpsi_set_base_addr() one more time now
> during spi_nor_scan() would actually change anything.
> 
> Please note that all I said was based on the assumption that you are trying
> to fix an issue in the fsl-quadspi.c driver (based on your email address:
> @nxp.com) as the nxp-spifi.c driver doesn't provide any implementation for
> the optional nor->prepare().
> 
> Then could you please describe a little bit more details about the actual
> issue? What does this patch fix? Which drivers are involved? 
> 
> Indeed, you would like to have a precise understanding of the issue and
> also avoid modifying spi-nor.c rather than the single fsl-quadspi.c driver
> or whatever driver is concerned since calling nor->preprare() from
> spi_nor_scan() has never been done before and may have unwanted side effects
> on other SPI flash controller drivers.
> 
> Actually, this is the reason why spi_nor_read_sfdp() directly calls
> nor->read() instead of spi_nor_read().
> 
> Best regards,
> 
> Cyrille
> 
> +	return 0;
> +
> +err_mutex:
> +	mutex_unlock(&q->lock);
> +	return ret;
> +}
> 
>  
>> Patch adds call to spi_nor_lock_and_prep() to prepare the READ command
>> and add subsequent call to spi_nor_unlock_and_unprep() to release lock
>> and call nor->unprepare().
>>
>> Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur at nxp.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 2203d6ef..9fb85ae 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -1807,6 +1807,10 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>>  	nor->addr_width = 3;
>>  	nor->read_dummy = 8;
>>  
>> +	ret = spi_nor_lock_and_prep(nor, SPI_NOR_OPS_READ);
>> +	if (ret)
>> +		goto lock_err;
>> +
>>  	while (len) {
>>  		ret = nor->read(nor, addr, len, (u8 *)buf);
>>  		if (!ret || ret > len) {
>> @@ -1822,6 +1826,8 @@ static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
>>  	}
>>  	ret = 0;
>>  
>> +	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
>> +lock_err:
>>  read_err:
>>  	nor->read_opcode = read_opcode;
>>  	nor->addr_width = addr_width;
>>
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>