Re: [PATCH v7 3/3] mtd: rawnand: Add Loongson-1 NAND Controller driver

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Keguang Zhang <keguang.zhang@gmail.com>
Cc: Keguang Zhang via B4 Relay
	<devnull+keguang.zhang.gmail.com@kernel.org>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-mips@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v7 3/3] mtd: rawnand: Add Loongson-1 NAND Controller driver
Date: Mon, 12 Aug 2024 09:46:21 +0200	[thread overview]
Message-ID: <20240812094621.1f4954d4@xps-13> (raw)
In-Reply-To: <CAJhJPsUfRpkK6sNuHR0tNfr24bnv8HhJc6FQ0W+_=WeRouhzJQ@mail.gmail.com>

Hi Keguang,

> > > +
> > > +                     nfc->data->parse_address(chip, addrs, naddrs, cmd);
> > > +                     /* set NAND address */
> > > +                     nand_writel(nfc, NAND_ADDR1, nfc->addr1_reg);
> > > +                     nand_writel(nfc, NAND_ADDR2, nfc->addr2_reg);
> > > +                     break;
> > > +             case NAND_OP_DATA_IN_INSTR:
> > > +             case NAND_OP_DATA_OUT_INSTR:
> > > +                     offset = nand_subop_get_data_start_off(subop, op_id);
> > > +                     nfc->len = nand_subop_get_data_len(subop, op_id);
> > > +                     if (instr->type == NAND_OP_DATA_IN_INSTR)
> > > +                             nfc->buf =
> > > +                                 (void *)instr->ctx.data.buf.in + offset;
> > > +                     else if (instr->type == NAND_OP_DATA_OUT_INSTR)
> > > +                             nfc->buf =
> > > +                                 (void *)instr->ctx.data.buf.out + offset;  
> >
> > The buf pointer feels clunky. You don't know for how long the buffer
> > you point to will be valid, please don't do that.
> >  
> The buf pointer is used for DMA transfer afterwards.
> I referred to other drivers, and they used the same approach.
> https://elixir.bootlin.com/linux/v6.8.9/source/drivers/mtd/nand/raw/arasan-nand-controller.c#L647

nfc_op is a local variable, it doesn't live past the function call.

> https://elixir.bootlin.com/linux/v6.8.9/source/drivers/mtd/nand/raw/rockchip-nand-controller.c#L366

Thanks,
Miquèl