Re: [bug] spi-nor not unlocking on Ubiquiti XW and WA

["Michael Walle" <mwalle@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 7566 bytes --]

Hi,

> I'm reporting this as an user of OpenWRT 23.05 on an Ubiquiti 
> Nanostation AC, who hopes to upgrade to 24.10. Currently, upgrading 
> results in a device where no configuration can be saved. Recovery 
> requires physical presence, on devices often installed outdoor.

thanks for the bug report.

> The issue has an explanation and a patch (for kernel 6.6) has been 
> available since late 2024 [1, attached below][2, which merges multiple 
> fixes]. It has been used by others [3] and myself. However, its author 
> believes that it doesn't have the correct approach to be upstreamed [4]. 
> OpenWRT maintainers are waiting for an ACK or better from upstream 
> before applying [5].
>
> Is there a way to "unlock" this (pun intended) ?
>
> For reference, OpenWRT configures those devices with 
> CONFIG_MTD_SPI_NOR_SWP_DISABLE [6].
>
> I'm available to test, but:
> - the only device I can use is a production one
> - OpenWRT is still working on upgrading ath79 from 6.6 to 6.12 [7], so 
> I'd be limited to testing on 6.6
>
> Let me know how I can help.
>
> Thanks!
>
> Jean-Marc
>
>
> [1] 
> https://github.com/openwrt/openwrt/pull/17287/commits/c98a55f95268f109911c5fddf5a153cfe3565b74
> [2] https://github.com/aredn/aredn/blob/main/patches/006-flash-fixes.patch
> [3] https://github.com/openwrt/openwrt/issues/17285#issuecomment-2946978832
> [4] https://github.com/openwrt/openwrt/pull/17287#issuecomment-2558569582
> [5] https://github.com/openwrt/openwrt/pull/17287#issuecomment-2558502454
> [6] 
> https://github.com/openwrt/openwrt/blob/bb59922007043c0a0813d62b0d9f6e801caff9df/target/linux/ath79/generic/config-default
> [7] https://github.com/openwrt/openwrt/issues/16569
>
>
>  From c98a55f95268f109911c5fddf5a153cfe3565b74 Mon Sep 17 00:00:00 2001
> From: Tim Wilkinson <tim.j.wilkinson@gmail.com>
> Date: Mon, 16 Dec 2024 09:37:34 -0800
> Subject: [PATCH] kernel: Fix setup of flash chips which must be unlocked
>   before use.
>
> Setup the mtd information for spi nor flash before calling running
> the nor setup.
>
> The current code assumes the nor setup doesnt make reference to the mtd
> information. There's even a comment to this effect. However, at least
> when flash chips must be unlocked, this isn't true.  Consequently, the
> unlock code will fail because it makes reference to uninitialized mtd
> information. This failure is silent because the bad mtd information
> claims the flash is 0 bytes long, and so there is nothing to unlock.
>
> This patch moves the mtd initialization before the nor setup, so the
> mtd info is available.
>
> This fix has been tested on two different Ubiquiti devices - the
> Rocket AC Lite and the Rocket M5 XW. These use the mx25l12805d and
> mx25l6405d Macronix flash chips respectively.  Previously these devices
> had failed to operate correctly as it was not possible for any
> persistent changes to be made once the factory build had been installed.
> With this change these devices behave correctly.
>
> Signed-off-by: Tim Wilkinson <tim.j.wilkinson@gmail.com>
> ---
>   .../436-mtd-spi-earlier-mtd-setup.patch       | 20 +++++++++++++++++++
>   1 file changed, 20 insertions(+)
>   create mode 100644 
> target/linux/generic/pending-6.6/436-mtd-spi-earlier-mtd-setup.patch
>
> diff --git 
> a/target/linux/generic/pending-6.6/436-mtd-spi-earlier-mtd-setup.patch 
> b/target/linux/generic/pending-6.6/436-mtd-spi-earlier-mtd-setup.patch
> new file mode 100644
> index 00000000000000..da75e9f7abfe96
> --- /dev/null
> +++ b/target/linux/generic/pending-6.6/436-mtd-spi-earlier-mtd-setup.patch
> @@ -0,0 +1,20 @@
> +--- a/drivers/mtd/spi-nor/core.c
> ++++ b/drivers/mtd/spi-nor/core.c
> +@@ -3540,14 +3540,14 @@
> + 	if (ret)
> + 		return ret;
> +
> ++	/* No mtd_info fields should be used up to this point. */
> ++	spi_nor_set_mtd_info(nor);
> ++
> + 	/* Send all the required SPI flash commands to initialize device */
> + 	ret = spi_nor_init(nor);
> + 	if (ret)
> + 		return ret;
> +
> +-	/* No mtd_info fields should be used up to this point. */
> +-	spi_nor_set_mtd_info(nor);
> +-

This seems to be due to the use of the uninitalized "mtd->size".
Could you try the following patch which is based on the latest
next kernel. It replaces mtd->size with nor->params->size, so you
could backport it to 6.6, but maybe it will apply anyway.

---snip---
diff --git a/drivers/mtd/spi-nor/swp.c b/drivers/mtd/spi-nor/swp.c
index 9c9328478d8a..9b07f83aeac7 100644
--- a/drivers/mtd/spi-nor/swp.c
+++ b/drivers/mtd/spi-nor/swp.c
@@ -56,7 +56,6 @@ static u64 spi_nor_get_min_prot_length_sr(struct spi_nor *nor)
 static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 					u64 *len)
 {
-	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
 	u8 tb_mask = spi_nor_get_sr_tb_mask(nor);
@@ -77,13 +76,13 @@ static void spi_nor_get_locked_range_sr(struct spi_nor *nor, u8 sr, loff_t *ofs,
 	min_prot_len = spi_nor_get_min_prot_length_sr(nor);
 	*len = min_prot_len << (bp - 1);
 
-	if (*len > mtd->size)
-		*len = mtd->size;
+	if (*len > nor->params->size)
+		*len = nor->params->size;
 
 	if (nor->flags & SNOR_F_HAS_SR_TB && sr & tb_mask)
 		*ofs = 0;
 	else
-		*ofs = mtd->size - *len;
+		*ofs = nor->params->size - *len;
 }
 
 /*
@@ -158,7 +157,6 @@ static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, u64 len,
  */
 static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
-	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
@@ -183,7 +181,7 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 		can_be_bottom = false;
 
 	/* If anything above us is unlocked, we can't use 'top' protection */
-	if (!spi_nor_is_locked_sr(nor, ofs + len, mtd->size - (ofs + len),
+	if (!spi_nor_is_locked_sr(nor, ofs + len, nor->params->size - (ofs + len),
 				  status_old))
 		can_be_top = false;
 
@@ -195,11 +193,11 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
 
 	/* lock_len: length of region that should end up locked */
 	if (use_top)
-		lock_len = mtd->size - ofs;
+		lock_len = nor->params->size - ofs;
 	else
 		lock_len = ofs + len;
 
-	if (lock_len == mtd->size) {
+	if (lock_len == nor->params->size) {
 		val = mask;
 	} else {
 		min_prot_len = spi_nor_get_min_prot_length_sr(nor);
@@ -248,7 +246,6 @@ static int spi_nor_sr_lock(struct spi_nor *nor, loff_t ofs, u64 len)
  */
 static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 {
-	struct mtd_info *mtd = &nor->mtd;
 	u64 min_prot_len;
 	int ret, status_old, status_new;
 	u8 mask = spi_nor_get_sr_bp_mask(nor);
@@ -273,7 +270,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 		can_be_top = false;
 
 	/* If anything above us is locked, we can't use 'bottom' protection */
-	if (!spi_nor_is_unlocked_sr(nor, ofs + len, mtd->size - (ofs + len),
+	if (!spi_nor_is_unlocked_sr(nor, ofs + len, nor->params->size - (ofs + len),
 				    status_old))
 		can_be_bottom = false;
 
@@ -285,7 +282,7 @@ static int spi_nor_sr_unlock(struct spi_nor *nor, loff_t ofs, u64 len)
 
 	/* lock_len: length of region that should remain locked */
 	if (use_top)
-		lock_len = mtd->size - (ofs + len);
+		lock_len = nor->params->size - (ofs + len);
 	else
 		lock_len = ofs;
 
---snip---

-michael

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/