linux-kernel - Re: [PATCH 2/4] mtd: rawnand: fsmc: Force to use 8 bits access when expected

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: <20211112163859.23a2487a@xps13>
Date:   Fri, 12 Nov 2021 16:38:59 +0100
From:   Miquel Raynal <miquel.raynal@...tlin.com>
To:     Herve Codina <herve.codina@...tlin.com>
Cc:     Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/4] mtd: rawnand: fsmc: Force to use 8 bits access when
 expected

Hi Hervé,

herve.codina@...tlin.com wrote on Fri, 12 Nov 2021 15:38:53 +0100:

> Some data transfers are expected on 8 bits by the nand core.
> The fsmc driver did not check this constraint and these transfers
> can be done on 32 bits depending on buffer alignment and transfers
> data size.
> 
> This patch ensures that these transfers will be 8bits transfers in
> all cases.

I believe there is a misunderstanding here: NAND buses -between the
NAND controller and the NAND chip- are either 8-bit or 16-bit wide and
the amount of bytes that you will retrieve per register read is not
related to it.

When the controller supports 16-bit accesses, there are certain
operations that must be performed using only the lowest 8 bits of the
NAND bus, such as reading a status [1]. In this case, the controller
must have a way to disable the 16-bit mode temporarily. See [2] and [3]
for an example. Reading with readb() or readl() will IMHO not impact the
amount of data lines used for the operation.

> Signed-off-by: Herve Codina <herve.codina@...tlin.com>

[1] https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/nand_base.c#L673
[2] Marvell NAND controller can change the used width of the bus
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L1777
[3] ... while still doing 32-bit accesses
https://elixir.bootlin.com/linux/latest/source/drivers/mtd/nand/raw/marvell_nand.c#L906

Thanks,
Miquèl

> ---
>  drivers/mtd/nand/raw/fsmc_nand.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/fsmc_nand.c b/drivers/mtd/nand/raw/fsmc_nand.c
> index 658f0cbe7ce8..7f057cfee6c4 100644
> --- a/drivers/mtd/nand/raw/fsmc_nand.c
> +++ b/drivers/mtd/nand/raw/fsmc_nand.c
> @@ -540,12 +540,12 @@ static int dma_xfer(struct fsmc_nand_data *host, void *buffer, int len,
>   * @len:	number of bytes to write
>   */
>  static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
> -			   int len)
> +			   int len, bool force_8bit)
>  {
>  	int i;
>  
>  	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
> -	    IS_ALIGNED(len, sizeof(u32))) {
> +	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
>  		u32 *p = (u32 *)buf;
>  
>  		len = len >> 2;
> @@ -563,12 +563,13 @@ static void fsmc_write_buf(struct fsmc_nand_data *host, const u8 *buf,
>   * @buf:	buffer to store date
>   * @len:	number of bytes to read
>   */
> -static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len)
> +static void fsmc_read_buf(struct fsmc_nand_data *host, u8 *buf, int len,
> +			  bool force_8bit)
>  {
>  	int i;
>  
>  	if (IS_ALIGNED((uintptr_t)buf, sizeof(u32)) &&
> -	    IS_ALIGNED(len, sizeof(u32))) {
> +	    IS_ALIGNED(len, sizeof(u32)) && !force_8bit) {
>  		u32 *p = (u32 *)buf;
>  
>  		len = len >> 2;
> @@ -646,7 +647,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
>  						  instr->ctx.data.len);
>  			else
>  				fsmc_read_buf(host, instr->ctx.data.buf.in,
> -					      instr->ctx.data.len);
> +					      instr->ctx.data.len,
> +					      instr->ctx.data.force_8bit);
>  			break;
>  
>  		case NAND_OP_DATA_OUT_INSTR:
> @@ -656,7 +658,8 @@ static int fsmc_exec_op(struct nand_chip *chip, const struct nand_operation *op,
>  						   instr->ctx.data.len);
>  			else
>  				fsmc_write_buf(host, instr->ctx.data.buf.out,
> -					       instr->ctx.data.len);
> +					       instr->ctx.data.len,
> +					       instr->ctx.data.force_8bit);
>  			break;
>  
>  		case NAND_OP_WAITRDY_INSTR: