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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874ix74yrh.fsf@bootlin.com>
Date: Mon, 26 May 2025 09:28:02 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Álvaro Fernández Rojas <noltari@...il.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

On 23/05/2025 at 20:08:56 +02, Álvaro Fernández Rojas <noltari@...il.com> wrote:

> 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.

Well, you are restoring an old behavior so I won't ask for a better
support, but you should normally allow software ECC engines (and even no
engine at all) and in this case the core will require a write path. I
honestly think it is not very complex to implement but if someone is
lacking this feature it can be added later.

Please just fix the braces in the for loop that was reported, but no
hurry, I'll only take this after -rc1.

Thanks,
Miquèl