linux-kernel - Re: [PATCH] mtd: spi-nor: macronix: Add block protection support to mx25u6435f

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: <CAATdQgCYoeEKfvQPNUxzMA+o4Xwtt80SoMmfweTAB1MCijyC6A@mail.gmail.com>
Date:   Wed, 14 Apr 2021 14:53:15 +0800
From:   Ikjoon Jang <ikjn@...omium.org>
To:     Michael Walle <michael@...le.cc>
Cc:     linux-mtd@...ts.infradead.org,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Pratyush Yadav <p.yadav@...com>,
        Richard Weinberger <richard@....at>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Vignesh Raghavendra <vigneshr@...com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: macronix: Add block protection support to mx25u6435f

HI Michael, thanks for the review.

On Tue, Apr 13, 2021 at 8:26 PM Michael Walle <michael@...le.cc> wrote:
>
> Hi Ikjoon,
>
> Am 2021-04-13 14:02, schrieb Ikjoon Jang:
> > This patch adds block protection support to Macronix mx25u6432f and
> > mx25u6435f. Two different chips share the same JEDEC ID while only
> > mx25u6423f support section protections. And two chips have slightly
> > different definitions of BP bits than generic (ST Micro)
> > implementation.
>
> What is different compared to the current implementation? Could you give
> an example?

Two chips have different definitions on BP and  T/B bits.
- 35f has 4 BPs without T/B, BP3 behaves like T/B bit.
- 32f has 4 BPs plus T/B on CR.

# MX25U6435F

 BPs | BP3=0 | BP3=1
---------------------
 000 | None  | 1/2
 001 | 1/128 | 3/4
 010 | 1/64  | 7/8
 011 | 1/32  | 15/16
 100 | 1/16  | 31/32
 101 | 1/8   | 63/64
 110 | 1/4   | 127/128
 111 | 1/2   | All

# MX25U6432F

  BPs | T/B=0 | T/B=1
---------------------
 0000 | None  | None
 0001 | 1/128 | 1/128
 0010 | 1/64  | 1/64
 0011 | 1/32  | 1/32
 0100 | 1/16  | 1/16
 0101 | 1/8   | 1/8
 0110 | 1/4   | 1/4
 0111 | 1/2   | 1/2
 1xxx | All   | All

It seems that 35f has a unique definition on bottom protections than others.
Assuming there's no way to distinguish between mx25u6435f and 32f,
This patch simply takes the common parts only - BP[2:0]
without using T/B or BP3 at all.

But the current swp implementation implies that "BP with all ones"
is to be 'all' protection while in this approach it's 1/2,
A hidden BP3 should be involved for 'all' protection.

>
> > So this patch defines a new spi_nor_locking_ops only for macronix
> > until this could be merged into a generic swp implementation.
>
> TBH, I don't really like the code duplication and I'd presume that it
> won't ever be merged with the generic code.

Agree, I hope I could make a more generalized version into swp.c.

Honestly I expected that I just needed to add one line of SPI_NOR_HAS_LOCK
to flash_info to support mx256432f (this was the main purpose of my patch)
before I read the datasheets. :)

>
> You also assume that both the WPSEL and T/B bit are 0, which might not
> be true. Please note that both are write-once, thus should only be read.

yep, that also should be considered,
I'm thinking just not to support WPSEL=1 case for now.

>
> See also:
> https://lore.kernel.org/linux-mtd/346332bf6ab0dd92b9ffd9e126b6b97c@walle.cc/
>

Thanks, let me try it.

> -michael