linux-kernel - Re: [PATCH v5] mtd: rawnand: brcmnand: legacy exec

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: <CAKR-sGeUGpUFBf_Zvg=7ro0EpGKy0dQVF58mAQt27YX+79qv1A@mail.gmail.com>
Date: Fri, 23 May 2025 20:08:56 +0200
From: Álvaro Fernández Rojas <noltari@...il.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: linux-mtd@...ts.infradead.org, dregan@...adcom.com, 
	bcm-kernel-feedback-list@...adcom.com, florian.fainelli@...adcom.com, 
	rafal@...ecki.pl, computersforpeace@...il.com, kamal.dasu@...adcom.com, 
	dan.beygelman@...adcom.com, william.zhang@...adcom.com, 
	frieder.schrempf@...tron.de, linux-kernel@...r.kernel.org, vigneshr@...com, 
	richard@....at, bbrezillon@...nel.org, kdasu.kdev@...il.com, 
	jaimeliao.tw@...il.com, kilobyte@...band.pl, jonas.gorski@...il.com, 
	dgcbueu@...il.com
Subject: Re: [PATCH v5] mtd: rawnand: brcmnand: legacy exec_op implementation

Hi Miquèl,

El vie, 23 may 2025 a las 16:42, Miquel Raynal
(<miquel.raynal@...tlin.com>) escribió:
>
> On 21/05/2025 at 10:03:25 +02, Álvaro Fernández Rojas <noltari@...il.com> wrote:
>
> > Commit 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > removed legacy interface functions, breaking < v5.0 controllers support.
> > In order to fix older controllers we need to add an alternative exec_op
> > implementation which doesn't rely on low level registers.
> >
> > Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> > Signed-off-by: Álvaro Fernández Rojas <noltari@...il.com>
> > Reviewed-by: David Regan <dregan@...adcom.com>
> > ---
> >  drivers/mtd/nand/raw/brcmnand/brcmnand.c | 222 ++++++++++++++++++++++-
> >  1 file changed, 215 insertions(+), 7 deletions(-)
> >
> >  v5: add changes requested by Miquèl Raynal:
> >   - Mention and explain legacy in native_cmd_conv.
> >   - EOPNOTSUPP instead of EINVAL for instr->type else.
> >   - Implement missing check_only functionality.
> >
> >  v4: add changes requested by Jonas Gorski:
> >   - Add missing breaks in brcmnand_exec_instructions_legacy.
> >   - Restore missing ret assignment in brcmnand_exec_op.
> >
> >  v3: add changes requested by Florian and other improvements:
> >   - Add associative array for native command conversion.
> >   - Add function pointer to brcmnand_controller for exec_instr
> >     functionality.
> >   - Fix CMD_BLOCK_ERASE address.
> >   - Drop NAND_CMD_READOOB support.
> >
> >  v2: multiple improvements:
> >   - Use proper native commands for checks.
> >   - Fix NAND_CMD_PARAM/NAND_CMD_RNDOUT addr calculation.
> >   - Remove host->last_addr usage.
> >   - Remove sector_size_1k since it only applies to v5.0+ controllers.
> >   - Remove brcmnand_wp since it doesn't exist for < v5.0 controllers.
> >   - Use j instead of i for flash_cache loop.
> >
>
> ...
>
> > +static int brcmnand_check_instructions_legacy(struct nand_chip *chip,
> > +                                           const struct nand_operation *op)
> > +{
> > +     const struct nand_op_instr *instr;
> > +     unsigned int i;
> > +     u8 cmd;
> > +
> > +     for (i = 0; i < op->ninstrs; i++) {
> > +             instr = &op->instrs[i];
> > +
> > +             switch (instr->type) {
> > +             case NAND_OP_CMD_INSTR:
> > +                     cmd = native_cmd_conv[instr->ctx.cmd.opcode];
> > +                     if (cmd == CMD_NOT_SUPPORTED)
> > +                             return -EOPNOTSUPP;
> > +                     break;
> > +             case NAND_OP_ADDR_INSTR:
> > +             case NAND_OP_DATA_IN_INSTR:
>
> No NAND_OP_DATA_OUT_INSTR?

AFAIK, the legacy functions were only using it for
NAND_CMD_SET_FEATURES, which we don't support:
https://github.com/torvalds/linux/blob/c86b63b82fde4f96ee94dde827a5f28ff5adeb57/drivers/mtd/nand/raw/brcmnand/brcmnand.c#L1922-L1938

The other uses I could find are already covered by our
chip->ecc.read/write functions.

In any case I've tested the patch for reading, erasing and writing the
NAND and so far I haven't found any unsupported error apart from
NAND_CMD_GET_FEATURES with a Macronix NAND in the Sercom H500-s
(BCM63268).
I believe it's used for unlocking the NAND, which isn't needed in that device.

>
> > +             case NAND_OP_WAITRDY_INSTR:
> > +                     break;
> > +             default:
> > +                     return -EOPNOTSUPP;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
> Rest lgtm.
>
> Thanks,
> Miquèl

Best regards,
Álvaro.