Re: [PATCH v2 3/3] mtd: spi-nor: add locking support for is25xxxxx device

[Paul Walmsley]

On Sun, 28 Apr 2019, Sagar Shrikant Kadam wrote:

> The locking scheme for ISSI devices is based on stm_lock mechanism.
> The is25xxxxx  devices have 4 bits for selecting the range of blocks to
> be locked for write.
> 
> The current implementation, blocks entire 512 blocks of flash memory.
> 
> Signed-off-by: Sagar Shrikant Kadam <sagar.kadam@xxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 60 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 81c7b3e..2dba7e9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1459,6 +1459,65 @@ static int macronix_quad_enable(struct spi_nor *nor)
>  
>  	return 0;
>  }
> +/**

The above sequence indicates a kerneldoc-style comment, but the format of 
the comment is not in kerneldoc format.  Please fix this comment to 
conform with

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/kernel-docs.rst


> + * Lock a region of the flash.Implementation is based on stm_lock
> + * Supports the block protection bits BP{0,1,2,3} in the status register
> + * Returns negative on errors, 0 on success.
> + */
> +static int issi_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{

Almost all of this function is copied and pasted from stm_lock().  Please 
don't do this: it adds bloat, makes the code hard to maintain, and 
increases the risk that fixes will only target one function rather than 
both.  Instead please pull the common code out into a separate static 
function.

> +	struct mtd_info *mtd = &nor->mtd;
> +	int status_old, status_new;
> +	u8 mask = SR_BP3 | SR_BP2 | SR_BP1 | SR_BP0;
> +	u8 shift = ffs(mask) - 1, pow, val = 0;
> +	loff_t lock_len;
> +	bool use_top = true;
> +
> +	status_old = read_sr(nor);
> +
> +	if (status_old < 0)
> +		return status_old;
> +
> +	/* lock_len: length of region that should end up locked */
> +	if (use_top)
> +		lock_len = mtd->size - ofs;
> +	else
> +		lock_len = ofs + len;
> +
> +	/*
> +	 * Need smallest pow such that:
> +	 *
> +	 *   1 / (2^pow) <= (len / size)
> +	 *
> +	 * so (assuming power-of-2 size) we do:
> +	 *
> +	 *   pow = ceil(log2(size / len)) = log2(size) - floor(log2(len))
> +	 */
> +	pow = ilog2(mtd->size) - ilog2(lock_len);
> +	val = mask - (pow << shift);
> +
> +	if (val & ~mask)
> +		return -EINVAL;
> +
> +	/* Don't "lock" with no region! */
> +	if (!(val & mask))
> +		return -EINVAL;
> +
> +	status_new = (status_old & ~mask & ~SR_TB) | val;
> +
> +	/* Disallow further writes if WP pin is asserted */
> +	status_new |= SR_SRWD;
> +
> +	/* Don't bother if they're the same */
> +	if (status_new == status_old)
> +		return 0;
> +
> +	/* Only modify protection if it will not unlock other areas */
> +	if ((status_new & mask) < (status_old & mask))
> +		return -EINVAL;
> +
> +	return write_sr_and_check(nor, status_new, mask);
> +}
>  
>  /**
>   * issi_unlock() - clear BP[0123] write-protection.
> @@ -4121,6 +4180,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	/* NOR protection support for ISSI chips */
>  	if (JEDEC_MFR(info) == SNOR_MFR_ISSI ||
>  	    info->flags & SPI_NOR_HAS_LOCK) {
> +		nor->flash_lock = issi_lock;
>  		nor->flash_unlock = issi_unlock;
>  
>  	}
> -- 
> 1.9.1
> 
>