mmc: sdhci: Tidy-up write-protect handling

PATCH In review Adrian Hunter 2 participants 5 messages Jun 14, 2024 – Jun 20, 2024

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Adrian Hunter

author maintainer

2024-06-14 08:01 UTC #1 lore raw

PATCH 1/3 mmc: sdhci: Do not invert write-protect twice

mmc_of_parse() reads device property "wp-inverted" and sets MMC_CAP2_RO_ACTIVE_HIGH if it is true. MMC_CAP2_RO_ACTIVE_HIGH is used to invert a write-protect (AKA read-only) GPIO value.

sdhci_get_property() also reads "wp-inverted" and sets SDHCI_QUIRK_INVERTED_WRITE_PROTECT which is used to invert the write-protect value as well but also acts upon a value read out from the SDHCI_PRESENT_STATE register.

Many drivers call both mmc_of_parse() and sdhci_get_property(), so that both MMC_CAP2_RO_ACTIVE_HIGH and SDHCI_QUIRK_INVERTED_WRITE_PROTECT will be set if the controller has device property "wp-inverted".

Amend the logic in sdhci_check_ro() to allow for that possibility, so that the write-protect value is not inverted twice.

Also do not invert the value if it is a negative error value. Note that callers treat an error the same as not-write-protected, so the result is functionally the same in that case.

Also do not invert the value if sdhci host operation ->get_ro() is used. None of the users of that callback set SDHCI_QUIRK_INVERTED_WRITE_PROTECT directly or indirectly, but two do call mmc_gpio_get_ro(), so leave it to them to deal with that if they ever set SDHCI_QUIRK_INVERTED_WRITE_PROTECT in the future.

Fixes 6d5cd068ee59 ("mmc: sdhci: use WP GPIO in sdhci_check_ro()")

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

drivers/mmc/host/sdhci.c +15 -7

index 746f4cf7ab03..81b81d7bb3d8 100644
@@ -2515,26 +2515,34 @@ EXPORT_SYMBOL_GPL(sdhci_get_cd_nogpio);
 
 static int sdhci_check_ro(struct sdhci_host *host)
 {
+	bool allow_invert = false;
 	unsigned long flags;
 	int is_readonly;
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (host->flags & SDHCI_DEVICE_DEAD)
+	if (host->flags & SDHCI_DEVICE_DEAD) {
 		is_readonly = 0;
-	else if (host->ops->get_ro)
+	} else if (host->ops->get_ro) {
 		is_readonly = host->ops->get_ro(host);
-	else if (mmc_can_gpio_ro(host->mmc))
+	} else if (mmc_can_gpio_ro(host->mmc)) {
 		is_readonly = mmc_gpio_get_ro(host->mmc);
-	else
+		/* Do not invert twice */
+		allow_invert = !(host->mmc->caps2 & MMC_CAP2_RO_ACTIVE_HIGH);
+	} else {
 		is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE)
 				& SDHCI_WRITE_PROTECT);
+		allow_invert = true;
+	}
 
 	spin_unlock_irqrestore(&host->lock, flags);
 
-	/* This quirk needs to be replaced by a callback-function later */
-	return host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT ?
-		!is_readonly : is_readonly;
+	if (is_readonly >= 0 &&
+	    allow_invert &&
+	    (host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT))
+		is_readonly = !is_readonly;
+
+	return is_readonly;
 }
 
 #define SAMPLE_COUNT	5

Signature · 1 line

2.34.1

Adrian Hunter

author maintainer

2024-06-14 08:01 UTC #2 lore raw

PATCH 2/3 mmc: sdhci: Do not lock spinlock around mmc_gpio_get_ro()

In reply to Adrian Hunter

sdhci_check_ro() can call mmc_gpio_get_ro() while holding the sdhci host->lock spinlock. That would be a problem if the GPIO access done by mmc_gpio_get_ro() needed to sleep.

However, host->lock is not needed anyway. The mmc core ensures that host operations do not race with each other, and asynchronous callbacks like the interrupt handler, software timeouts, completion work etc, cannot affect sdhci_check_ro().

So remove the locking.

Fixes 6d5cd068ee59 ("mmc: sdhci: use WP GPIO in sdhci_check_ro()")

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci.c | 5 -----
 1 file changed, 5 deletions(-)

drivers/mmc/host/sdhci.c +0 -5

index 81b81d7bb3d8..112584aa0772 100644
@@ -2516,11 +2516,8 @@ EXPORT_SYMBOL_GPL(sdhci_get_cd_nogpio);
 static int sdhci_check_ro(struct sdhci_host *host)
 {
 	bool allow_invert = false;
-	unsigned long flags;
 	int is_readonly;
 
-	spin_lock_irqsave(&host->lock, flags);
-
 	if (host->flags & SDHCI_DEVICE_DEAD) {
 		is_readonly = 0;
 	} else if (host->ops->get_ro) {

@@ -2535,8 +2532,6 @@ static int sdhci_check_ro(struct sdhci_host *host)
 		allow_invert = true;
 	}
 
-	spin_unlock_irqrestore(&host->lock, flags);
-
 	if (is_readonly >= 0 &&
 	    allow_invert &&
 	    (host->quirks & SDHCI_QUIRK_INVERTED_WRITE_PROTECT))

Signature · 1 line

2.34.1

Adrian Hunter

author maintainer

2024-06-14 08:01 UTC #3 lore raw

PATCH 3/3 mmc: sdhci: Eliminate SDHCI_QUIRK_UNSTABLE_RO_DETECT

In reply to Adrian Hunter

SDHCI_QUIRK_UNSTABLE_RO_DETECT is used by only one driver variant. It was added in 2011 by commit 82b0e23a295c ("mmc: sdhci: Fix read-only detection with JMicron 388 chip").

Simplify sdhci by moving the logic to the only place it is used.

Signed-off-by Adrian Hunter <[email protected]>

---
 drivers/mmc/host/sdhci-pci-core.c | 27 ++++++++++++++++++++++-----
 drivers/mmc/host/sdhci.c          | 31 ++++++-------------------------
 drivers/mmc/host/sdhci.h          |  3 +--
 3 files changed, 29 insertions(+), 32 deletions(-)

drivers/mmc/host/sdhci-pci-core.c +22 -5

index 23e6ba70144c..ed45ed0bdafd 100644
@@ -1319,6 +1319,23 @@ static const struct sdhci_pci_fixes sdhci_intel_mrfld_mmc = {
 	.probe_slot	= intel_mrfld_mmc_probe_slot,
 };
 
+#define JMB388_SAMPLE_COUNT	5
+
+static int jmicron_jmb388_get_ro(struct mmc_host *mmc)
+{
+	int i, ro_count;
+
+	ro_count = 0;
+	for (i = 0; i < JMB388_SAMPLE_COUNT; i++) {
+		if (sdhci_get_ro(mmc) > 0) {
+			if (++ro_count > JMB388_SAMPLE_COUNT / 2)
+				return 1;
+		}
+		msleep(30);
+	}
+	return 0;
+}
+
 static int jmicron_pmos(struct sdhci_pci_chip *chip, int on)
 {
 	u8 scratch;

@@ -1403,11 +1420,6 @@ static int jmicron_probe(struct sdhci_pci_chip *chip)
 		return ret;
 	}
 
-	/* quirk for unsable RO-detection on JM388 chips */
-	if (chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_SD ||
-	    chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_ESD)
-		chip->quirks |= SDHCI_QUIRK_UNSTABLE_RO_DETECT;
-
 	return 0;
 }

@@ -1462,6 +1474,11 @@ static int jmicron_probe_slot(struct sdhci_pci_slot *slot)
 
 	slot->host->mmc->caps |= MMC_CAP_BUS_WIDTH_TEST;
 
+	/* Handle unstable RO-detection on JM388 chips */
+	if (slot->chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_SD ||
+	    slot->chip->pdev->device == PCI_DEVICE_ID_JMICRON_JMB388_ESD)
+		slot->host->mmc_host_ops.get_ro = jmicron_jmb388_get_ro;
+
 	return 0;
 }

drivers/mmc/host/sdhci.c +6 ~~-25~~

index 112584aa0772..a20df9383b20 100644
@@ -2513,8 +2513,9 @@ int sdhci_get_cd_nogpio(struct mmc_host *mmc)
 }
 EXPORT_SYMBOL_GPL(sdhci_get_cd_nogpio);
 
-static int sdhci_check_ro(struct sdhci_host *host)
+int sdhci_get_ro(struct mmc_host *mmc)
 {
+	struct sdhci_host *host = mmc_priv(mmc);
 	bool allow_invert = false;
 	int is_readonly;

@@ -2522,10 +2523,10 @@ static int sdhci_check_ro(struct sdhci_host *host)
 		is_readonly = 0;
 	} else if (host->ops->get_ro) {
 		is_readonly = host->ops->get_ro(host);
-	} else if (mmc_can_gpio_ro(host->mmc)) {
-		is_readonly = mmc_gpio_get_ro(host->mmc);
+	} else if (mmc_can_gpio_ro(mmc)) {
+		is_readonly = mmc_gpio_get_ro(mmc);
 		/* Do not invert twice */
-		allow_invert = !(host->mmc->caps2 & MMC_CAP2_RO_ACTIVE_HIGH);
+		allow_invert = !(mmc->caps2 & MMC_CAP2_RO_ACTIVE_HIGH);
 	} else {
 		is_readonly = !(sdhci_readl(host, SDHCI_PRESENT_STATE)
 				& SDHCI_WRITE_PROTECT);

@@ -2539,27 +2540,7 @@ static int sdhci_check_ro(struct sdhci_host *host)
 
 	return is_readonly;
 }
-
-#define SAMPLE_COUNT	5
-
-static int sdhci_get_ro(struct mmc_host *mmc)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-	int i, ro_count;
-
-	if (!(host->quirks & SDHCI_QUIRK_UNSTABLE_RO_DETECT))
-		return sdhci_check_ro(host);
-
-	ro_count = 0;
-	for (i = 0; i < SAMPLE_COUNT; i++) {
-		if (sdhci_check_ro(host)) {
-			if (++ro_count > SAMPLE_COUNT / 2)
-				return 1;
-		}
-		msleep(30);
-	}
-	return 0;
-}
+EXPORT_SYMBOL_GPL(sdhci_get_ro);
 
 static void sdhci_hw_reset(struct mmc_host *mmc)
 {

drivers/mmc/host/sdhci.h +1 -2

index 957c7a917ffb..f531b617f28d 100644
@@ -437,8 +437,6 @@ struct sdhci_host {
 #define SDHCI_QUIRK_NO_HISPD_BIT			(1<<29)
 /* Controller treats ADMA descriptors with length 0000h incorrectly */
 #define SDHCI_QUIRK_BROKEN_ADMA_ZEROLEN_DESC		(1<<30)
-/* The read-only detection via SDHCI_PRESENT_STATE register is unstable */
-#define SDHCI_QUIRK_UNSTABLE_RO_DETECT			(1<<31)
 
 	unsigned int quirks2;	/* More deviations from spec. */

@@ -788,6 +786,7 @@ void sdhci_set_power_and_bus_voltage(struct sdhci_host *host,
 void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
 			   unsigned short vdd);
 int sdhci_get_cd_nogpio(struct mmc_host *mmc);
+int sdhci_get_ro(struct mmc_host *mmc);
 void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
 int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
 void sdhci_set_bus_width(struct sdhci_host *host, int width);

Signature · 1 line

2.34.1

Ulf Hansson

2024-06-20 15:15 UTC #4 lore raw

In reply to Adrian Hunter

Quoted 13 lines · Adrian Hunter

Hi
There seemed to be some issues with sdhci write-protect handling, although no reports of them actually being hit. Here are 3 little patches to tidy things up a bit.
Adrian Hunter (3):
mmc: sdhci: Do not invert write-protect twice
mmc: sdhci: Do not lock spinlock around mmc_gpio_get_ro()
mmc: sdhci: Eliminate SDHCI_QUIRK_UNSTABLE_RO_DETECT

Quoted 5 lines

 drivers/mmc/host/sdhci-pci-core.c | 27 ++++++++++++++++----
 drivers/mmc/host/sdhci.c          | 54 ++++++++++++++-------------------------
 drivers/mmc/host/sdhci.h          |  3 +--
 3 files changed, 42 insertions(+), 42 deletions(-)

Patch 1 and patch2 applied for fixes, patch3 for next, thanks!

Kind regards
Uffe