Re: [PATCH 5/8] mtd: rawnand: onfi: Avoid doing a copy of the parameter page

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Michal Simek <monstr@monstr.eu>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Naga Sureshkumar Relli <nagasure@xilinx.com>
Subject: Re: [PATCH 5/8] mtd: rawnand: onfi: Avoid doing a copy of the parameter page
Date: Wed, 22 Apr 2020 08:54:48 +0200	[thread overview]
Message-ID: <20200422085448.7e2fb95f@collabora.com> (raw)
In-Reply-To: <20200421164637.8086-6-miquel.raynal@bootlin.com>

On Tue, 21 Apr 2020 18:46:34 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There is not need for copying the parameter page, playing with

	    ^no

> pointers works the same.

	   ^does the trick?

> 
> There is not functional change.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/mtd/nand/raw/nand_onfi.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_onfi.c b/drivers/mtd/nand/raw/nand_onfi.c
> index 7d9a3130443a..d6124180b47b 100644
> --- a/drivers/mtd/nand/raw/nand_onfi.c
> +++ b/drivers/mtd/nand/raw/nand_onfi.c
> @@ -141,7 +141,7 @@ int nand_onfi_detect(struct nand_chip *chip)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	struct nand_memory_organization *memorg;
> -	struct nand_onfi_params *p;
> +	struct nand_onfi_params *p = NULL, *pbuf;
>  	struct onfi_params *onfi;
>  	int onfi_version = 0;
>  	char id[4];
> @@ -156,8 +156,8 @@ int nand_onfi_detect(struct nand_chip *chip)
>  		return 0;
>  
>  	/* ONFI chip: allocate a buffer to hold its parameter page */
> -	p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> -	if (!p)
> +	pbuf = kzalloc((sizeof(*pbuf) * 3), GFP_KERNEL);
> +	if (!pbuf)
>  		return -ENOMEM;
>  
>  	ret = nand_read_param_page_op(chip, 0, NULL, 0);
> @@ -167,32 +167,31 @@ int nand_onfi_detect(struct nand_chip *chip)
>  	}
>  
>  	for (i = 0; i < 3; i++) {
> -		ret = nand_read_data_op(chip, &p[i], sizeof(*p), true);
> +		ret = nand_read_data_op(chip, &pbuf[i], sizeof(*pbuf), true);
>  		if (ret) {
>  			ret = 0;
>  			goto free_onfi_param_page;
>  		}
>  
> -		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254);
> -		if (crc == le16_to_cpu(p[i].crc)) {
> -			if (i)
> -				memcpy(p, &p[i], sizeof(*p));
> +		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)&pbuf[i], 254);
> +		if (crc == le16_to_cpu(pbuf[i].crc)) {
> +			p = &pbuf[i];
>  			break;
>  		}
>  	}
>  
>  	if (i == 3) {
> -		const void *srcbufs[3] = {p, p + 1, p + 2};
> -
> +		const void *srcbufs[3] = {pbuf, pbuf + 1, pbuf + 2};
>  		pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> -		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> -				       sizeof(*p));
> +		nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), pbuf,
> +				       sizeof(*pbuf));
>  
> -		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254);
> -		if (crc != le16_to_cpu(p->crc)) {
> +		crc = onfi_crc16(ONFI_CRC_BASE, (u8 *)pbuf, 254);
> +		if (crc != le16_to_cpu(pbuf->crc)) {
>  			pr_err("ONFI parameter recovery failed, aborting\n");
>  			goto free_onfi_param_page;
>  		}
> +		p = pbuf;
>  	}
>  
>  	if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
> @@ -300,7 +299,7 @@ int nand_onfi_detect(struct nand_chip *chip)
>  	chip->parameters.onfi = onfi;
>  
>  	/* Identification done, free the full ONFI parameter page and exit */
> -	kfree(p);
> +	kfree(pbuf);
>  
>  	return 1;
>  


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/