All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH 2/2] Make cmdq_en attribute writeable
@ 2021-02-15  0:32 Luca Porzio
  2021-02-15 13:49   ` Dan Carpenter
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Luca Porzio @ 2021-02-15  0:32 UTC (permalink / raw)
  To: linux-mmc; +Cc: Zhan Liu, Luca Porzio

cmdq_en attribute in sysfs now can now be written.
When 0 is written:
  CMDQ is disabled and kept disabled across device reboots.
When 1 is written:
  CMDQ mode is instantly reneabled (if supported).

Signed-off-by: Luca Porzio <lporzio@micron.com>
Signed-off-by: Zhan Liu <zliua@micron.com>
---
 drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++---------
 include/linux/mmc/card.h |   1 +
 2 files changed, 118 insertions(+), 35 deletions(-)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 0d80b72ddde8..5c7d5bac5c00 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported, "%#x\n",
 MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
 MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
 MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
-MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
+
+
+/* Setup command queue mode and CQE if underling hw supports it
+ * and assuming force_disable_cmdq has not been set.
+ */
+static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
+{
+	int err;
+
+	/* Check HW support */
+	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
+		card->force_disable_cmdq = true;
+
+	/* Enable/Disable  CMDQ mode */
+	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
+		err = mmc_cmdq_enable(card);
+		if (err && err != -EBADMSG)
+			return err;
+		if (err) {
+			pr_warn("%s: Enabling CMDQ failed\n",
+			    mmc_hostname(card->host));
+			card->ext_csd.cmdq_support = false;
+			card->ext_csd.cmdq_depth = 0;
+		}
+
+	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
+		err = mmc_cmdq_disable(card);
+		if (err) {
+			pr_warn("%s: Disabling CMDQ failed, error %d\n",
+			    mmc_hostname(card->host), err);
+			err = 0;
+		}
+	}
+
+	/*
+	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
+	 * disabled for a time, so a flag is needed to indicate to re-enable the
+	 * Command Queue.
+	 */
+	card->reenable_cmdq = card->ext_csd.cmdq_en;
+
+	/* Enable/Disable Host CQE */
+	if (!card->force_disable_cmdq) {
+
+		if (host->cqe_ops && !host->cqe_enabled) {
+			err = host->cqe_ops->cqe_enable(host, card);
+			if (!err) {
+				host->cqe_enabled = true;
+
+				if (card->ext_csd.cmdq_en) {
+					pr_info("%s: Command Queue Engine enabled\n",
+					    mmc_hostname(host));
+				} else {
+					host->hsq_enabled = true;
+					pr_info("%s: Host Software Queue enabled\n",
+					    mmc_hostname(host));
+				}
+			}
+		}
+
+	} else {
+
+		if (host->cqe_enabled) {
+			host->cqe_ops->cqe_disable(host);
+			host->cqe_enabled = false;
+			pr_info("%s: Command Queue Engine disabled\n",
+			    mmc_hostname(host));
+		}
+
+		host->hsq_enabled = false;
+		err = 0;
+	}
+
+	return err;
+}
+
+
+static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct mmc_card *card = mmc_dev_to_card(dev);
+
+	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
+}
+
+static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct mmc_card *card = mmc_dev_to_card(dev);
+	struct mmc_host *host = card->host;
+	unsigned long enable;
+	int err;
+
+	if (!card || kstrtoul(buf, 0, &enable))
+		return -EINVAL;
+	if (!card->ext_csd.cmdq_support)
+		return -EOPNOTSUPP;
+
+	enable = !!enable;
+	if (enable == card->ext_csd.cmdq_en)
+		return count;
+
+	mmc_get_card(card, NULL);
+	card->force_disable_cmdq = !enable;
+	err = mmc_cmdq_setup(host, card);
+	mmc_put_card(card, NULL);
+
+	if (err)
+		return err;
+	else
+		return count;
+}
+
+static DEVICE_ATTR_RW(cmdq_en);
+
 
 static ssize_t mmc_fwrev_show(struct device *dev,
 			      struct device_attribute *attr,
@@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
 	 * Enable Command Queue if supported. Note that Packed Commands cannot
 	 * be used with Command Queue.
 	 */
-	card->ext_csd.cmdq_en = false;
-	if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) {
-		err = mmc_cmdq_enable(card);
-		if (err && err != -EBADMSG)
-			goto free_card;
-		if (err) {
-			pr_warn("%s: Enabling CMDQ failed\n",
-				mmc_hostname(card->host));
-			card->ext_csd.cmdq_support = false;
-			card->ext_csd.cmdq_depth = 0;
-		}
-	}
-	/*
-	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
-	 * disabled for a time, so a flag is needed to indicate to re-enable the
-	 * Command Queue.
-	 */
-	card->reenable_cmdq = card->ext_csd.cmdq_en;
-
-	if (host->cqe_ops && !host->cqe_enabled) {
-		err = host->cqe_ops->cqe_enable(host, card);
-		if (!err) {
-			host->cqe_enabled = true;
-
-			if (card->ext_csd.cmdq_en) {
-				pr_info("%s: Command Queue Engine enabled\n",
-					mmc_hostname(host));
-			} else {
-				host->hsq_enabled = true;
-				pr_info("%s: Host Software Queue enabled\n",
-					mmc_hostname(host));
-			}
-		}
-	}
+	err = mmc_cmdq_setup(host, card);
+	if (err)
+		goto free_card;
 
 	if (host->caps2 & MMC_CAP2_AVOID_3_3V &&
 	    host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index f9ad35dd6012..e554bb0cf722 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -272,6 +272,7 @@ struct mmc_card {
 #define MMC_QUIRK_BROKEN_HPI	(1<<13)		/* Disable broken HPI support */
 
 	bool			reenable_cmdq;	/* Re-enable Command Queue */
+	bool			force_disable_cmdq; /* Keep Command Queue disabled */
 
 	unsigned int		erase_size;	/* erase size in sectors */
  	unsigned int		erase_shift;	/* if erase unit is power 2 */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-02-15  0:32 [RFC PATCH 2/2] Make cmdq_en attribute writeable Luca Porzio
@ 2021-02-15 13:49   ` Dan Carpenter
  2021-02-17 13:04 ` kernel test robot
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2021-02-15 13:49 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 8357 bytes --]

Hi Luca,

url:    https://github.com/0day-ci/linux/commits/Luca-Porzio/Support-temporarily-disable-of-the-CMDQ-mode/20210215-083730
base:    07f7e57c63aaa2afb4ea31edef05e08699a63a00
config: i386-randconfig-m021-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/mmc/core/mmc.c:870 mmc_cmdq_setup() error: uninitialized symbol 'err'.
drivers/mmc/core/mmc.c:889 cmdq_en_store() warn: variable dereferenced before check 'card' (see line 885)

vim +/err +870 drivers/mmc/core/mmc.c

07a9ce0e702520 Luca Porzio 2021-02-15  802  static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
07a9ce0e702520 Luca Porzio 2021-02-15  803  {
07a9ce0e702520 Luca Porzio 2021-02-15  804  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  805  
07a9ce0e702520 Luca Porzio 2021-02-15  806  	/* Check HW support */
07a9ce0e702520 Luca Porzio 2021-02-15  807  	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
07a9ce0e702520 Luca Porzio 2021-02-15  808  		card->force_disable_cmdq = true;
07a9ce0e702520 Luca Porzio 2021-02-15  809  
07a9ce0e702520 Luca Porzio 2021-02-15  810  	/* Enable/Disable  CMDQ mode */
07a9ce0e702520 Luca Porzio 2021-02-15  811  	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  812  		err = mmc_cmdq_enable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  813  		if (err && err != -EBADMSG)
07a9ce0e702520 Luca Porzio 2021-02-15  814  			return err;
07a9ce0e702520 Luca Porzio 2021-02-15  815  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  816  			pr_warn("%s: Enabling CMDQ failed\n",
07a9ce0e702520 Luca Porzio 2021-02-15  817  			    mmc_hostname(card->host));
07a9ce0e702520 Luca Porzio 2021-02-15  818  			card->ext_csd.cmdq_support = false;
07a9ce0e702520 Luca Porzio 2021-02-15  819  			card->ext_csd.cmdq_depth = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  820  		}
07a9ce0e702520 Luca Porzio 2021-02-15  821  
07a9ce0e702520 Luca Porzio 2021-02-15  822  	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  823  		err = mmc_cmdq_disable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  824  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  825  			pr_warn("%s: Disabling CMDQ failed, error %d\n",
07a9ce0e702520 Luca Porzio 2021-02-15  826  			    mmc_hostname(card->host), err);
07a9ce0e702520 Luca Porzio 2021-02-15  827  			err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  828  		}
07a9ce0e702520 Luca Porzio 2021-02-15  829  	}

"err not set on else path"

07a9ce0e702520 Luca Porzio 2021-02-15  830  
07a9ce0e702520 Luca Porzio 2021-02-15  831  	/*
07a9ce0e702520 Luca Porzio 2021-02-15  832  	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
07a9ce0e702520 Luca Porzio 2021-02-15  833  	 * disabled for a time, so a flag is needed to indicate to re-enable the
07a9ce0e702520 Luca Porzio 2021-02-15  834  	 * Command Queue.
07a9ce0e702520 Luca Porzio 2021-02-15  835  	 */
07a9ce0e702520 Luca Porzio 2021-02-15  836  	card->reenable_cmdq = card->ext_csd.cmdq_en;
07a9ce0e702520 Luca Porzio 2021-02-15  837  
07a9ce0e702520 Luca Porzio 2021-02-15  838  	/* Enable/Disable Host CQE */
07a9ce0e702520 Luca Porzio 2021-02-15  839  	if (!card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  840  
07a9ce0e702520 Luca Porzio 2021-02-15  841  		if (host->cqe_ops && !host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  842  			err = host->cqe_ops->cqe_enable(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  843  			if (!err) {
07a9ce0e702520 Luca Porzio 2021-02-15  844  				host->cqe_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  845  
07a9ce0e702520 Luca Porzio 2021-02-15  846  				if (card->ext_csd.cmdq_en) {
07a9ce0e702520 Luca Porzio 2021-02-15  847  					pr_info("%s: Command Queue Engine enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  848  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  849  				} else {
07a9ce0e702520 Luca Porzio 2021-02-15  850  					host->hsq_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  851  					pr_info("%s: Host Software Queue enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  852  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  853  				}
07a9ce0e702520 Luca Porzio 2021-02-15  854  			}
07a9ce0e702520 Luca Porzio 2021-02-15  855  		}

"err" not set on this else path either.

07a9ce0e702520 Luca Porzio 2021-02-15  856  
07a9ce0e702520 Luca Porzio 2021-02-15  857  	} else {
07a9ce0e702520 Luca Porzio 2021-02-15  858  
07a9ce0e702520 Luca Porzio 2021-02-15  859  		if (host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  860  			host->cqe_ops->cqe_disable(host);
07a9ce0e702520 Luca Porzio 2021-02-15  861  			host->cqe_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  862  			pr_info("%s: Command Queue Engine disabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  863  			    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  864  		}
07a9ce0e702520 Luca Porzio 2021-02-15  865  
07a9ce0e702520 Luca Porzio 2021-02-15  866  		host->hsq_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  867  		err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  868  	}
07a9ce0e702520 Luca Porzio 2021-02-15  869  
07a9ce0e702520 Luca Porzio 2021-02-15 @870  	return err;
07a9ce0e702520 Luca Porzio 2021-02-15  871  }
07a9ce0e702520 Luca Porzio 2021-02-15  872  
07a9ce0e702520 Luca Porzio 2021-02-15  873  
07a9ce0e702520 Luca Porzio 2021-02-15  874  static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
07a9ce0e702520 Luca Porzio 2021-02-15  875  {
07a9ce0e702520 Luca Porzio 2021-02-15  876  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15  877  
07a9ce0e702520 Luca Porzio 2021-02-15  878  	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
07a9ce0e702520 Luca Porzio 2021-02-15  879  }
07a9ce0e702520 Luca Porzio 2021-02-15  880  
07a9ce0e702520 Luca Porzio 2021-02-15  881  static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
07a9ce0e702520 Luca Porzio 2021-02-15  882  				 const char *buf, size_t count)
07a9ce0e702520 Luca Porzio 2021-02-15  883  {
07a9ce0e702520 Luca Porzio 2021-02-15  884  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15 @885  	struct mmc_host *host = card->host;
                                                                        ^^^^^^^^^^
Dereferences "card"

07a9ce0e702520 Luca Porzio 2021-02-15  886  	unsigned long enable;
07a9ce0e702520 Luca Porzio 2021-02-15  887  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  888  
07a9ce0e702520 Luca Porzio 2021-02-15 @889  	if (!card || kstrtoul(buf, 0, &enable))
                                                    ^^^^^

Checked too late.

07a9ce0e702520 Luca Porzio 2021-02-15  890  		return -EINVAL;
07a9ce0e702520 Luca Porzio 2021-02-15  891  	if (!card->ext_csd.cmdq_support)
07a9ce0e702520 Luca Porzio 2021-02-15  892  		return -EOPNOTSUPP;
07a9ce0e702520 Luca Porzio 2021-02-15  893  
07a9ce0e702520 Luca Porzio 2021-02-15  894  	enable = !!enable;
07a9ce0e702520 Luca Porzio 2021-02-15  895  	if (enable == card->ext_csd.cmdq_en)
07a9ce0e702520 Luca Porzio 2021-02-15  896  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  897  
07a9ce0e702520 Luca Porzio 2021-02-15  898  	mmc_get_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  899  	card->force_disable_cmdq = !enable;
07a9ce0e702520 Luca Porzio 2021-02-15  900  	err = mmc_cmdq_setup(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  901  	mmc_put_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  902  
07a9ce0e702520 Luca Porzio 2021-02-15  903  	if (err)
07a9ce0e702520 Luca Porzio 2021-02-15  904  		return err;
07a9ce0e702520 Luca Porzio 2021-02-15  905  	else
07a9ce0e702520 Luca Porzio 2021-02-15  906  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  907  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38219 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
@ 2021-02-15 13:49   ` Dan Carpenter
  0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2021-02-15 13:49 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 8357 bytes --]

Hi Luca,

url:    https://github.com/0day-ci/linux/commits/Luca-Porzio/Support-temporarily-disable-of-the-CMDQ-mode/20210215-083730
base:    07f7e57c63aaa2afb4ea31edef05e08699a63a00
config: i386-randconfig-m021-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/mmc/core/mmc.c:870 mmc_cmdq_setup() error: uninitialized symbol 'err'.
drivers/mmc/core/mmc.c:889 cmdq_en_store() warn: variable dereferenced before check 'card' (see line 885)

vim +/err +870 drivers/mmc/core/mmc.c

07a9ce0e702520 Luca Porzio 2021-02-15  802  static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
07a9ce0e702520 Luca Porzio 2021-02-15  803  {
07a9ce0e702520 Luca Porzio 2021-02-15  804  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  805  
07a9ce0e702520 Luca Porzio 2021-02-15  806  	/* Check HW support */
07a9ce0e702520 Luca Porzio 2021-02-15  807  	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
07a9ce0e702520 Luca Porzio 2021-02-15  808  		card->force_disable_cmdq = true;
07a9ce0e702520 Luca Porzio 2021-02-15  809  
07a9ce0e702520 Luca Porzio 2021-02-15  810  	/* Enable/Disable  CMDQ mode */
07a9ce0e702520 Luca Porzio 2021-02-15  811  	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  812  		err = mmc_cmdq_enable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  813  		if (err && err != -EBADMSG)
07a9ce0e702520 Luca Porzio 2021-02-15  814  			return err;
07a9ce0e702520 Luca Porzio 2021-02-15  815  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  816  			pr_warn("%s: Enabling CMDQ failed\n",
07a9ce0e702520 Luca Porzio 2021-02-15  817  			    mmc_hostname(card->host));
07a9ce0e702520 Luca Porzio 2021-02-15  818  			card->ext_csd.cmdq_support = false;
07a9ce0e702520 Luca Porzio 2021-02-15  819  			card->ext_csd.cmdq_depth = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  820  		}
07a9ce0e702520 Luca Porzio 2021-02-15  821  
07a9ce0e702520 Luca Porzio 2021-02-15  822  	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  823  		err = mmc_cmdq_disable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  824  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  825  			pr_warn("%s: Disabling CMDQ failed, error %d\n",
07a9ce0e702520 Luca Porzio 2021-02-15  826  			    mmc_hostname(card->host), err);
07a9ce0e702520 Luca Porzio 2021-02-15  827  			err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  828  		}
07a9ce0e702520 Luca Porzio 2021-02-15  829  	}

"err not set on else path"

07a9ce0e702520 Luca Porzio 2021-02-15  830  
07a9ce0e702520 Luca Porzio 2021-02-15  831  	/*
07a9ce0e702520 Luca Porzio 2021-02-15  832  	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
07a9ce0e702520 Luca Porzio 2021-02-15  833  	 * disabled for a time, so a flag is needed to indicate to re-enable the
07a9ce0e702520 Luca Porzio 2021-02-15  834  	 * Command Queue.
07a9ce0e702520 Luca Porzio 2021-02-15  835  	 */
07a9ce0e702520 Luca Porzio 2021-02-15  836  	card->reenable_cmdq = card->ext_csd.cmdq_en;
07a9ce0e702520 Luca Porzio 2021-02-15  837  
07a9ce0e702520 Luca Porzio 2021-02-15  838  	/* Enable/Disable Host CQE */
07a9ce0e702520 Luca Porzio 2021-02-15  839  	if (!card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  840  
07a9ce0e702520 Luca Porzio 2021-02-15  841  		if (host->cqe_ops && !host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  842  			err = host->cqe_ops->cqe_enable(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  843  			if (!err) {
07a9ce0e702520 Luca Porzio 2021-02-15  844  				host->cqe_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  845  
07a9ce0e702520 Luca Porzio 2021-02-15  846  				if (card->ext_csd.cmdq_en) {
07a9ce0e702520 Luca Porzio 2021-02-15  847  					pr_info("%s: Command Queue Engine enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  848  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  849  				} else {
07a9ce0e702520 Luca Porzio 2021-02-15  850  					host->hsq_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  851  					pr_info("%s: Host Software Queue enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  852  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  853  				}
07a9ce0e702520 Luca Porzio 2021-02-15  854  			}
07a9ce0e702520 Luca Porzio 2021-02-15  855  		}

"err" not set on this else path either.

07a9ce0e702520 Luca Porzio 2021-02-15  856  
07a9ce0e702520 Luca Porzio 2021-02-15  857  	} else {
07a9ce0e702520 Luca Porzio 2021-02-15  858  
07a9ce0e702520 Luca Porzio 2021-02-15  859  		if (host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  860  			host->cqe_ops->cqe_disable(host);
07a9ce0e702520 Luca Porzio 2021-02-15  861  			host->cqe_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  862  			pr_info("%s: Command Queue Engine disabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  863  			    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  864  		}
07a9ce0e702520 Luca Porzio 2021-02-15  865  
07a9ce0e702520 Luca Porzio 2021-02-15  866  		host->hsq_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  867  		err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  868  	}
07a9ce0e702520 Luca Porzio 2021-02-15  869  
07a9ce0e702520 Luca Porzio 2021-02-15 @870  	return err;
07a9ce0e702520 Luca Porzio 2021-02-15  871  }
07a9ce0e702520 Luca Porzio 2021-02-15  872  
07a9ce0e702520 Luca Porzio 2021-02-15  873  
07a9ce0e702520 Luca Porzio 2021-02-15  874  static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
07a9ce0e702520 Luca Porzio 2021-02-15  875  {
07a9ce0e702520 Luca Porzio 2021-02-15  876  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15  877  
07a9ce0e702520 Luca Porzio 2021-02-15  878  	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
07a9ce0e702520 Luca Porzio 2021-02-15  879  }
07a9ce0e702520 Luca Porzio 2021-02-15  880  
07a9ce0e702520 Luca Porzio 2021-02-15  881  static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
07a9ce0e702520 Luca Porzio 2021-02-15  882  				 const char *buf, size_t count)
07a9ce0e702520 Luca Porzio 2021-02-15  883  {
07a9ce0e702520 Luca Porzio 2021-02-15  884  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15 @885  	struct mmc_host *host = card->host;
                                                                        ^^^^^^^^^^
Dereferences "card"

07a9ce0e702520 Luca Porzio 2021-02-15  886  	unsigned long enable;
07a9ce0e702520 Luca Porzio 2021-02-15  887  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  888  
07a9ce0e702520 Luca Porzio 2021-02-15 @889  	if (!card || kstrtoul(buf, 0, &enable))
                                                    ^^^^^

Checked too late.

07a9ce0e702520 Luca Porzio 2021-02-15  890  		return -EINVAL;
07a9ce0e702520 Luca Porzio 2021-02-15  891  	if (!card->ext_csd.cmdq_support)
07a9ce0e702520 Luca Porzio 2021-02-15  892  		return -EOPNOTSUPP;
07a9ce0e702520 Luca Porzio 2021-02-15  893  
07a9ce0e702520 Luca Porzio 2021-02-15  894  	enable = !!enable;
07a9ce0e702520 Luca Porzio 2021-02-15  895  	if (enable == card->ext_csd.cmdq_en)
07a9ce0e702520 Luca Porzio 2021-02-15  896  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  897  
07a9ce0e702520 Luca Porzio 2021-02-15  898  	mmc_get_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  899  	card->force_disable_cmdq = !enable;
07a9ce0e702520 Luca Porzio 2021-02-15  900  	err = mmc_cmdq_setup(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  901  	mmc_put_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  902  
07a9ce0e702520 Luca Porzio 2021-02-15  903  	if (err)
07a9ce0e702520 Luca Porzio 2021-02-15  904  		return err;
07a9ce0e702520 Luca Porzio 2021-02-15  905  	else
07a9ce0e702520 Luca Porzio 2021-02-15  906  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  907  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38219 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-02-15  0:32 [RFC PATCH 2/2] Make cmdq_en attribute writeable Luca Porzio
  2021-02-15 13:49   ` Dan Carpenter
@ 2021-02-17 13:04 ` kernel test robot
  2021-03-02 10:46 ` Ulf Hansson
  2021-03-08 17:05 ` Ulf Hansson
  3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-02-17 13:04 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 9291 bytes --]

Hi Luca,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on next-20210212]
[cannot apply to linus/master mmc/mmc-next v5.11-rc7 v5.11-rc6 v5.11-rc5 v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luca-Porzio/Support-temporarily-disable-of-the-CMDQ-mode/20210215-083730
base:    07f7e57c63aaa2afb4ea31edef05e08699a63a00
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"cppcheck warnings: (new ones prefixed by >>)"
>> drivers/mmc/core/mmc.c:870:9: warning: Uninitialized variable: err [uninitvar]
    return err;
           ^

cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

>> drivers/mmc/core/mmc.c:885:26: warning: Either the condition '!card' is redundant or there is possible null pointer dereference: card. [nullPointerRedundantCheck]
    struct mmc_host *host = card->host;
                            ^
   drivers/mmc/core/mmc.c:889:6: note: Assuming that condition '!card' is not redundant
    if (!card || kstrtoul(buf, 0, &enable))
        ^
   drivers/mmc/core/mmc.c:885:26: note: Null pointer dereference
    struct mmc_host *host = card->host;
                            ^

vim +885 drivers/mmc/core/mmc.c

07a9ce0e702520 Luca Porzio 2021-02-15  797  
07a9ce0e702520 Luca Porzio 2021-02-15  798  
07a9ce0e702520 Luca Porzio 2021-02-15  799  /* Setup command queue mode and CQE if underling hw supports it
07a9ce0e702520 Luca Porzio 2021-02-15  800   * and assuming force_disable_cmdq has not been set.
07a9ce0e702520 Luca Porzio 2021-02-15  801   */
07a9ce0e702520 Luca Porzio 2021-02-15  802  static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
07a9ce0e702520 Luca Porzio 2021-02-15  803  {
07a9ce0e702520 Luca Porzio 2021-02-15  804  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  805  
07a9ce0e702520 Luca Porzio 2021-02-15  806  	/* Check HW support */
07a9ce0e702520 Luca Porzio 2021-02-15  807  	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
07a9ce0e702520 Luca Porzio 2021-02-15  808  		card->force_disable_cmdq = true;
07a9ce0e702520 Luca Porzio 2021-02-15  809  
07a9ce0e702520 Luca Porzio 2021-02-15  810  	/* Enable/Disable  CMDQ mode */
07a9ce0e702520 Luca Porzio 2021-02-15  811  	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  812  		err = mmc_cmdq_enable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  813  		if (err && err != -EBADMSG)
07a9ce0e702520 Luca Porzio 2021-02-15  814  			return err;
07a9ce0e702520 Luca Porzio 2021-02-15  815  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  816  			pr_warn("%s: Enabling CMDQ failed\n",
07a9ce0e702520 Luca Porzio 2021-02-15  817  			    mmc_hostname(card->host));
07a9ce0e702520 Luca Porzio 2021-02-15  818  			card->ext_csd.cmdq_support = false;
07a9ce0e702520 Luca Porzio 2021-02-15  819  			card->ext_csd.cmdq_depth = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  820  		}
07a9ce0e702520 Luca Porzio 2021-02-15  821  
07a9ce0e702520 Luca Porzio 2021-02-15  822  	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  823  		err = mmc_cmdq_disable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  824  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  825  			pr_warn("%s: Disabling CMDQ failed, error %d\n",
07a9ce0e702520 Luca Porzio 2021-02-15  826  			    mmc_hostname(card->host), err);
07a9ce0e702520 Luca Porzio 2021-02-15  827  			err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  828  		}
07a9ce0e702520 Luca Porzio 2021-02-15  829  	}
07a9ce0e702520 Luca Porzio 2021-02-15  830  
07a9ce0e702520 Luca Porzio 2021-02-15  831  	/*
07a9ce0e702520 Luca Porzio 2021-02-15  832  	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
07a9ce0e702520 Luca Porzio 2021-02-15  833  	 * disabled for a time, so a flag is needed to indicate to re-enable the
07a9ce0e702520 Luca Porzio 2021-02-15  834  	 * Command Queue.
07a9ce0e702520 Luca Porzio 2021-02-15  835  	 */
07a9ce0e702520 Luca Porzio 2021-02-15  836  	card->reenable_cmdq = card->ext_csd.cmdq_en;
07a9ce0e702520 Luca Porzio 2021-02-15  837  
07a9ce0e702520 Luca Porzio 2021-02-15  838  	/* Enable/Disable Host CQE */
07a9ce0e702520 Luca Porzio 2021-02-15  839  	if (!card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  840  
07a9ce0e702520 Luca Porzio 2021-02-15  841  		if (host->cqe_ops && !host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  842  			err = host->cqe_ops->cqe_enable(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  843  			if (!err) {
07a9ce0e702520 Luca Porzio 2021-02-15  844  				host->cqe_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  845  
07a9ce0e702520 Luca Porzio 2021-02-15  846  				if (card->ext_csd.cmdq_en) {
07a9ce0e702520 Luca Porzio 2021-02-15  847  					pr_info("%s: Command Queue Engine enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  848  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  849  				} else {
07a9ce0e702520 Luca Porzio 2021-02-15  850  					host->hsq_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  851  					pr_info("%s: Host Software Queue enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  852  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  853  				}
07a9ce0e702520 Luca Porzio 2021-02-15  854  			}
07a9ce0e702520 Luca Porzio 2021-02-15  855  		}
07a9ce0e702520 Luca Porzio 2021-02-15  856  
07a9ce0e702520 Luca Porzio 2021-02-15  857  	} else {
07a9ce0e702520 Luca Porzio 2021-02-15  858  
07a9ce0e702520 Luca Porzio 2021-02-15  859  		if (host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  860  			host->cqe_ops->cqe_disable(host);
07a9ce0e702520 Luca Porzio 2021-02-15  861  			host->cqe_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  862  			pr_info("%s: Command Queue Engine disabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  863  			    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  864  		}
07a9ce0e702520 Luca Porzio 2021-02-15  865  
07a9ce0e702520 Luca Porzio 2021-02-15  866  		host->hsq_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  867  		err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  868  	}
07a9ce0e702520 Luca Porzio 2021-02-15  869  
07a9ce0e702520 Luca Porzio 2021-02-15 @870  	return err;
07a9ce0e702520 Luca Porzio 2021-02-15  871  }
07a9ce0e702520 Luca Porzio 2021-02-15  872  
07a9ce0e702520 Luca Porzio 2021-02-15  873  
07a9ce0e702520 Luca Porzio 2021-02-15  874  static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
07a9ce0e702520 Luca Porzio 2021-02-15  875  {
07a9ce0e702520 Luca Porzio 2021-02-15  876  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15  877  
07a9ce0e702520 Luca Porzio 2021-02-15  878  	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
07a9ce0e702520 Luca Porzio 2021-02-15  879  }
07a9ce0e702520 Luca Porzio 2021-02-15  880  
07a9ce0e702520 Luca Porzio 2021-02-15  881  static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
07a9ce0e702520 Luca Porzio 2021-02-15  882  				 const char *buf, size_t count)
07a9ce0e702520 Luca Porzio 2021-02-15  883  {
07a9ce0e702520 Luca Porzio 2021-02-15  884  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15 @885  	struct mmc_host *host = card->host;
07a9ce0e702520 Luca Porzio 2021-02-15  886  	unsigned long enable;
07a9ce0e702520 Luca Porzio 2021-02-15  887  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  888  
07a9ce0e702520 Luca Porzio 2021-02-15  889  	if (!card || kstrtoul(buf, 0, &enable))
07a9ce0e702520 Luca Porzio 2021-02-15  890  		return -EINVAL;
07a9ce0e702520 Luca Porzio 2021-02-15  891  	if (!card->ext_csd.cmdq_support)
07a9ce0e702520 Luca Porzio 2021-02-15  892  		return -EOPNOTSUPP;
07a9ce0e702520 Luca Porzio 2021-02-15  893  
07a9ce0e702520 Luca Porzio 2021-02-15  894  	enable = !!enable;
07a9ce0e702520 Luca Porzio 2021-02-15  895  	if (enable == card->ext_csd.cmdq_en)
07a9ce0e702520 Luca Porzio 2021-02-15  896  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  897  
07a9ce0e702520 Luca Porzio 2021-02-15  898  	mmc_get_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  899  	card->force_disable_cmdq = !enable;
07a9ce0e702520 Luca Porzio 2021-02-15  900  	err = mmc_cmdq_setup(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  901  	mmc_put_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  902  
07a9ce0e702520 Luca Porzio 2021-02-15  903  	if (err)
07a9ce0e702520 Luca Porzio 2021-02-15  904  		return err;
07a9ce0e702520 Luca Porzio 2021-02-15  905  	else
07a9ce0e702520 Luca Porzio 2021-02-15  906  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  907  }
07a9ce0e702520 Luca Porzio 2021-02-15  908  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-02-15  0:32 [RFC PATCH 2/2] Make cmdq_en attribute writeable Luca Porzio
  2021-02-15 13:49   ` Dan Carpenter
  2021-02-17 13:04 ` kernel test robot
@ 2021-03-02 10:46 ` Ulf Hansson
  2021-03-10  8:54   ` Adrian Hunter
  2021-03-08 17:05 ` Ulf Hansson
  3 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2021-03-02 10:46 UTC (permalink / raw)
  To: Luca Porzio, Adrian Hunter; +Cc: linux-mmc, Zhan Liu, Luca Porzio

+ Adrian

On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> wrote:
>
> cmdq_en attribute in sysfs now can now be written.
> When 0 is written:
>   CMDQ is disabled and kept disabled across device reboots.
> When 1 is written:
>   CMDQ mode is instantly reneabled (if supported).
>
> Signed-off-by: Luca Porzio <lporzio@micron.com>
> Signed-off-by: Zhan Liu <zliua@micron.com>

Luca, thanks for your patch! I am about to start to review this.

I have also looped in Adrian to get his opinions.

Get back to you soon!

Kind regards
Uffe

> ---
>  drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++---------
>  include/linux/mmc/card.h |   1 +
>  2 files changed, 118 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> index 0d80b72ddde8..5c7d5bac5c00 100644
> --- a/drivers/mmc/core/mmc.c
> +++ b/drivers/mmc/core/mmc.c
> @@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported, "%#x\n",
>  MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
>  MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
> -MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
> +
> +
> +/* Setup command queue mode and CQE if underling hw supports it
> + * and assuming force_disable_cmdq has not been set.
> + */
> +static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
> +{
> +       int err;
> +
> +       /* Check HW support */
> +       if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
> +               card->force_disable_cmdq = true;
> +
> +       /* Enable/Disable  CMDQ mode */
> +       if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
> +               err = mmc_cmdq_enable(card);
> +               if (err && err != -EBADMSG)
> +                       return err;
> +               if (err) {
> +                       pr_warn("%s: Enabling CMDQ failed\n",
> +                           mmc_hostname(card->host));
> +                       card->ext_csd.cmdq_support = false;
> +                       card->ext_csd.cmdq_depth = 0;
> +               }
> +
> +       } else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
> +               err = mmc_cmdq_disable(card);
> +               if (err) {
> +                       pr_warn("%s: Disabling CMDQ failed, error %d\n",
> +                           mmc_hostname(card->host), err);
> +                       err = 0;
> +               }
> +       }
> +
> +       /*
> +        * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
> +        * disabled for a time, so a flag is needed to indicate to re-enable the
> +        * Command Queue.
> +        */
> +       card->reenable_cmdq = card->ext_csd.cmdq_en;
> +
> +       /* Enable/Disable Host CQE */
> +       if (!card->force_disable_cmdq) {
> +
> +               if (host->cqe_ops && !host->cqe_enabled) {
> +                       err = host->cqe_ops->cqe_enable(host, card);
> +                       if (!err) {
> +                               host->cqe_enabled = true;
> +
> +                               if (card->ext_csd.cmdq_en) {
> +                                       pr_info("%s: Command Queue Engine enabled\n",
> +                                           mmc_hostname(host));
> +                               } else {
> +                                       host->hsq_enabled = true;
> +                                       pr_info("%s: Host Software Queue enabled\n",
> +                                           mmc_hostname(host));
> +                               }
> +                       }
> +               }
> +
> +       } else {
> +
> +               if (host->cqe_enabled) {
> +                       host->cqe_ops->cqe_disable(host);
> +                       host->cqe_enabled = false;
> +                       pr_info("%s: Command Queue Engine disabled\n",
> +                           mmc_hostname(host));
> +               }
> +
> +               host->hsq_enabled = false;
> +               err = 0;
> +       }
> +
> +       return err;
> +}
> +
> +
> +static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       struct mmc_card *card = mmc_dev_to_card(dev);
> +
> +       return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
> +}
> +
> +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +       struct mmc_card *card = mmc_dev_to_card(dev);
> +       struct mmc_host *host = card->host;
> +       unsigned long enable;
> +       int err;
> +
> +       if (!card || kstrtoul(buf, 0, &enable))
> +               return -EINVAL;
> +       if (!card->ext_csd.cmdq_support)
> +               return -EOPNOTSUPP;
> +
> +       enable = !!enable;
> +       if (enable == card->ext_csd.cmdq_en)
> +               return count;
> +
> +       mmc_get_card(card, NULL);
> +       card->force_disable_cmdq = !enable;
> +       err = mmc_cmdq_setup(host, card);
> +       mmc_put_card(card, NULL);
> +
> +       if (err)
> +               return err;
> +       else
> +               return count;
> +}
> +
> +static DEVICE_ATTR_RW(cmdq_en);
> +
>
>  static ssize_t mmc_fwrev_show(struct device *dev,
>                               struct device_attribute *attr,
> @@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>          * Enable Command Queue if supported. Note that Packed Commands cannot
>          * be used with Command Queue.
>          */
> -       card->ext_csd.cmdq_en = false;
> -       if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) {
> -               err = mmc_cmdq_enable(card);
> -               if (err && err != -EBADMSG)
> -                       goto free_card;
> -               if (err) {
> -                       pr_warn("%s: Enabling CMDQ failed\n",
> -                               mmc_hostname(card->host));
> -                       card->ext_csd.cmdq_support = false;
> -                       card->ext_csd.cmdq_depth = 0;
> -               }
> -       }
> -       /*
> -        * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
> -        * disabled for a time, so a flag is needed to indicate to re-enable the
> -        * Command Queue.
> -        */
> -       card->reenable_cmdq = card->ext_csd.cmdq_en;
> -
> -       if (host->cqe_ops && !host->cqe_enabled) {
> -               err = host->cqe_ops->cqe_enable(host, card);
> -               if (!err) {
> -                       host->cqe_enabled = true;
> -
> -                       if (card->ext_csd.cmdq_en) {
> -                               pr_info("%s: Command Queue Engine enabled\n",
> -                                       mmc_hostname(host));
> -                       } else {
> -                               host->hsq_enabled = true;
> -                               pr_info("%s: Host Software Queue enabled\n",
> -                                       mmc_hostname(host));
> -                       }
> -               }
> -       }
> +       err = mmc_cmdq_setup(host, card);
> +       if (err)
> +               goto free_card;
>
>         if (host->caps2 & MMC_CAP2_AVOID_3_3V &&
>             host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> index f9ad35dd6012..e554bb0cf722 100644
> --- a/include/linux/mmc/card.h
> +++ b/include/linux/mmc/card.h
> @@ -272,6 +272,7 @@ struct mmc_card {
>  #define MMC_QUIRK_BROKEN_HPI   (1<<13)         /* Disable broken HPI support */
>
>         bool                    reenable_cmdq;  /* Re-enable Command Queue */
> +       bool                    force_disable_cmdq; /* Keep Command Queue disabled */
>
>         unsigned int            erase_size;     /* erase size in sectors */
>         unsigned int            erase_shift;    /* if erase unit is power 2 */
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-02-15  0:32 [RFC PATCH 2/2] Make cmdq_en attribute writeable Luca Porzio
                   ` (2 preceding siblings ...)
  2021-03-02 10:46 ` Ulf Hansson
@ 2021-03-08 17:05 ` Ulf Hansson
       [not found]   ` <CABhGgDPpUXPHJ49E_ku5N-fO=GWZKTdQQUGugAruG6y2=J1YgA@mail.gmail.com>
  3 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2021-03-08 17:05 UTC (permalink / raw)
  To: Luca Porzio; +Cc: linux-mmc, Zhan Liu, Luca Porzio

On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> wrote:
>
> cmdq_en attribute in sysfs now can now be written.
> When 0 is written:
>   CMDQ is disabled and kept disabled across device reboots.
> When 1 is written:
>   CMDQ mode is instantly reneabled (if supported).
>
> Signed-off-by: Luca Porzio <lporzio@micron.com>
> Signed-off-by: Zhan Liu <zliua@micron.com>
> ---
>  drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++---------
>  include/linux/mmc/card.h |   1 +
>  2 files changed, 118 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c

[...]

> +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +       struct mmc_card *card = mmc_dev_to_card(dev);
> +       struct mmc_host *host = card->host;
> +       unsigned long enable;
> +       int err;
> +
> +       if (!card || kstrtoul(buf, 0, &enable))
> +               return -EINVAL;
> +       if (!card->ext_csd.cmdq_support)
> +               return -EOPNOTSUPP;
> +
> +       enable = !!enable;
> +       if (enable == card->ext_csd.cmdq_en)
> +               return count;
> +
> +       mmc_get_card(card, NULL);

This means adding a new path for when the host needs to get locked
(claimed), which is the opposite direction of what we have been
working on for SD/eMMC during the last couple of years.

Please have a look at mmc_blk_issue_drv_op(), where you can find how
these kinds of requests are being funneled through the mmc block
device layer instead. This is the preferred option.

That said, I am actually wondering if we perhaps could manage the
enable/disable of CQE "automagically" for FFU, along the lines of what
we do for RPMB already. In fact, maybe the best/easiest approach is to
*always* disable CQE when there is a request being received through
the mmc ioctl interface. Of course, then if we disabled CQE we should
also re-enable it when the ioctl request has been completed.

What do you think?

> +       card->force_disable_cmdq = !enable;
> +       err = mmc_cmdq_setup(host, card);
> +       mmc_put_card(card, NULL);
> +
> +       if (err)
> +               return err;
> +       else
> +               return count;
> +}
> +
> +static DEVICE_ATTR_RW(cmdq_en);
> +
>

[...]

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
       [not found]   ` <CABhGgDPpUXPHJ49E_ku5N-fO=GWZKTdQQUGugAruG6y2=J1YgA@mail.gmail.com>
@ 2021-03-09 15:47     ` Ulf Hansson
  2021-03-14 22:26       ` [EXT] " Luca Porzio (lporzio)
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2021-03-09 15:47 UTC (permalink / raw)
  To: Luca Porzio; +Cc: linux-mmc, Zhan Liu, Luca Porzio

On Tue, 9 Mar 2021 at 14:18, Luca Porzio <porzio@gmail.com> wrote:
>
>
>>
>> This means adding a new path for when the host needs to get locked
>> (claimed), which is the opposite direction of what we have been
>> working on for SD/eMMC during the last couple of years.
>>
>> Please have a look at mmc_blk_issue_drv_op(), where you can find how
>> these kinds of requests are being funneled through the mmc block
>> device layer instead. This is the preferred option.
>>
>> That said, I am actually wondering if we perhaps could manage the
>> enable/disable of CQE "automagically" for FFU, along the lines of what
>> we do for RPMB already. In fact, maybe the best/easiest approach is to
>> *always* disable CQE when there is a request being received through
>> the mmc ioctl interface. Of course, then if we disabled CQE we should
>> also re-enable it when the ioctl request has been completed.
>>
>> What do you think?
>>
>>
>
> Thanks a lot for your feedback!
>
> The reason why this is an RFC patch and not a clear patch is exactly what you
> say here.
> During the FFU (as well as other sequence of commands issued through
> multi cmd() ioctl feature, is that sometimes it may require an emmc reboot or
> some other legacy command subsequent sequence (= which needs to be
> sent with two ioctl() multi_cmd sequences) and the legacy mode need to
> survive across reboots or partition changes.

The reboot is kind of a separate problem, even if it's related, isn't it?

What you propose is to add a sysfs node to let userspace
enable/disable CQE. I was under the impression that this was solely to
allow the FFU process to be completed, no?

Are you saying that we may want CQE to stay disabled beyond the FFU
process, to allow the reboot to be completed safely?

> Also we need to claim the host to make sure all the pending commands
> in the queue are completed successfully before disabling.

Yes, of course. It sounds like you may have misinterpreted my proposals.

The problem is not that we need to claim the host, but that you add an
entirely new path to do it.

*If* we conclude that we should add a sysfs node to control CQE, we
should create a mmc blk request for it (which will manage the claim
for us as well). I suggest you have a closer look at
power_ro_lock_store(), which should be the equivalent to what we need
to implement here.

>
> I can rethink the patch to implement a specific iotcl() request which disables
> CMDQ if you think that is a better implementation but in the end it will still
> require the host claim.
>
> Any feedback or suggestion is appreciated.

To be clear, I am not proposing to add a new ioctl for mmc. Those we
have today, should be sufficient I think.

However, rather than adding a new sysfs node to control CQE, perhaps
we can parse the received ioctl data structure, to find out if the
command/request is for FFU and then take specific actions. Along the
lines of what we do for mmc_sanitize(), for example.

Another option, rather than parsing ioctl data for the FFU
command/request, is to always temporarily disable CQE while processing
an mmc ioctl request.

Would this work?

Kind regards
Uffe

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-03-02 10:46 ` Ulf Hansson
@ 2021-03-10  8:54   ` Adrian Hunter
  2021-03-14 22:33     ` [EXT] " Luca Porzio (lporzio)
  0 siblings, 1 reply; 13+ messages in thread
From: Adrian Hunter @ 2021-03-10  8:54 UTC (permalink / raw)
  To: Ulf Hansson, Luca Porzio; +Cc: linux-mmc, Zhan Liu, Luca Porzio

On 2/03/21 12:46 pm, Ulf Hansson wrote:
> + Adrian
> 
> On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> wrote:
>>
>> cmdq_en attribute in sysfs now can now be written.
>> When 0 is written:
>>   CMDQ is disabled and kept disabled across device reboots.
>> When 1 is written:
>>   CMDQ mode is instantly reneabled (if supported).
>>
>> Signed-off-by: Luca Porzio <lporzio@micron.com>
>> Signed-off-by: Zhan Liu <zliua@micron.com>
> 
> Luca, thanks for your patch! I am about to start to review this.
> 
> I have also looped in Adrian to get his opinions.
> 
> Get back to you soon!
> 
> Kind regards
> Uffe
> 
>> ---
>>  drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++---------
>>  include/linux/mmc/card.h |   1 +
>>  2 files changed, 118 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
>> index 0d80b72ddde8..5c7d5bac5c00 100644
>> --- a/drivers/mmc/core/mmc.c
>> +++ b/drivers/mmc/core/mmc.c
>> @@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported, "%#x\n",
>>  MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>>  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
>>  MMC_DEV_ATTR(rca, "0x%04x\n", card->rca);
>> -MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);
>> +
>> +
>> +/* Setup command queue mode and CQE if underling hw supports it
>> + * and assuming force_disable_cmdq has not been set.
>> + */
>> +static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
>> +{
>> +       int err;
>> +
>> +       /* Check HW support */
>> +       if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
>> +               card->force_disable_cmdq = true;
>> +
>> +       /* Enable/Disable  CMDQ mode */
>> +       if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
>> +               err = mmc_cmdq_enable(card);
>> +               if (err && err != -EBADMSG)
>> +                       return err;
>> +               if (err) {
>> +                       pr_warn("%s: Enabling CMDQ failed\n",
>> +                           mmc_hostname(card->host));
>> +                       card->ext_csd.cmdq_support = false;
>> +                       card->ext_csd.cmdq_depth = 0;
>> +               }
>> +
>> +       } else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
>> +               err = mmc_cmdq_disable(card);
>> +               if (err) {
>> +                       pr_warn("%s: Disabling CMDQ failed, error %d\n",
>> +                           mmc_hostname(card->host), err);
>> +                       err = 0;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
>> +        * disabled for a time, so a flag is needed to indicate to re-enable the
>> +        * Command Queue.
>> +        */
>> +       card->reenable_cmdq = card->ext_csd.cmdq_en;
>> +
>> +       /* Enable/Disable Host CQE */
>> +       if (!card->force_disable_cmdq) {
>> +
>> +               if (host->cqe_ops && !host->cqe_enabled) {
>> +                       err = host->cqe_ops->cqe_enable(host, card);
>> +                       if (!err) {
>> +                               host->cqe_enabled = true;

Re-initializing the card is also a recovery path for the block driver.
Changing host->cqe_enabled during a recovery reset, creates an unexpected
dependency for the block driver.  That should not be necessary, and given
that cqhci does memory allocation as part of enabling, it is better not to
disable / re-enable if it can be helped.

From a design point of view, it is really the block driver that should
control the use of command queuing rather than expecting it to cope with
changes from a lower level.

>> +
>> +                               if (card->ext_csd.cmdq_en) {
>> +                                       pr_info("%s: Command Queue Engine enabled\n",
>> +                                           mmc_hostname(host));
>> +                               } else {
>> +                                       host->hsq_enabled = true;
>> +                                       pr_info("%s: Host Software Queue enabled\n",
>> +                                           mmc_hostname(host));
>> +                               }
>> +                       }
>> +               }
>> +
>> +       } else {
>> +
>> +               if (host->cqe_enabled) {
>> +                       host->cqe_ops->cqe_disable(host);
>> +                       host->cqe_enabled = false;
>> +                       pr_info("%s: Command Queue Engine disabled\n",
>> +                           mmc_hostname(host));
>> +               }
>> +
>> +               host->hsq_enabled = false;

This looks quite wrong for hsq which is currently not used with
cmdq.

>> +               err = 0;
>> +       }
>> +
>> +       return err;
>> +}
>> +
>> +
>> +static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +       struct mmc_card *card = mmc_dev_to_card(dev);
>> +
>> +       return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
>> +}
>> +
>> +static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
>> +                                const char *buf, size_t count)
>> +{
>> +       struct mmc_card *card = mmc_dev_to_card(dev);
>> +       struct mmc_host *host = card->host;
>> +       unsigned long enable;
>> +       int err;
>> +
>> +       if (!card || kstrtoul(buf, 0, &enable))
>> +               return -EINVAL;
>> +       if (!card->ext_csd.cmdq_support)
>> +               return -EOPNOTSUPP;
>> +
>> +       enable = !!enable;
>> +       if (enable == card->ext_csd.cmdq_en)
>> +               return count;
>> +
>> +       mmc_get_card(card, NULL);
>> +       card->force_disable_cmdq = !enable;
>> +       err = mmc_cmdq_setup(host, card);
>> +       mmc_put_card(card, NULL);
>> +
>> +       if (err)
>> +               return err;
>> +       else
>> +               return count;
>> +}
>> +
>> +static DEVICE_ATTR_RW(cmdq_en);
>> +
>>
>>  static ssize_t mmc_fwrev_show(struct device *dev,
>>                               struct device_attribute *attr,
>> @@ -1838,40 +1951,9 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
>>          * Enable Command Queue if supported. Note that Packed Commands cannot
>>          * be used with Command Queue.
>>          */
>> -       card->ext_csd.cmdq_en = false;
>> -       if (card->ext_csd.cmdq_support && host->caps2 & MMC_CAP2_CQE) {
>> -               err = mmc_cmdq_enable(card);
>> -               if (err && err != -EBADMSG)
>> -                       goto free_card;
>> -               if (err) {
>> -                       pr_warn("%s: Enabling CMDQ failed\n",
>> -                               mmc_hostname(card->host));
>> -                       card->ext_csd.cmdq_support = false;
>> -                       card->ext_csd.cmdq_depth = 0;
>> -               }
>> -       }
>> -       /*
>> -        * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
>> -        * disabled for a time, so a flag is needed to indicate to re-enable the
>> -        * Command Queue.
>> -        */
>> -       card->reenable_cmdq = card->ext_csd.cmdq_en;
>> -
>> -       if (host->cqe_ops && !host->cqe_enabled) {
>> -               err = host->cqe_ops->cqe_enable(host, card);
>> -               if (!err) {
>> -                       host->cqe_enabled = true;
>> -
>> -                       if (card->ext_csd.cmdq_en) {
>> -                               pr_info("%s: Command Queue Engine enabled\n",
>> -                                       mmc_hostname(host));
>> -                       } else {
>> -                               host->hsq_enabled = true;
>> -                               pr_info("%s: Host Software Queue enabled\n",
>> -                                       mmc_hostname(host));
>> -                       }
>> -               }
>> -       }
>> +       err = mmc_cmdq_setup(host, card);
>> +       if (err)
>> +               goto free_card;
>>
>>         if (host->caps2 & MMC_CAP2_AVOID_3_3V &&
>>             host->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330) {
>> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
>> index f9ad35dd6012..e554bb0cf722 100644
>> --- a/include/linux/mmc/card.h
>> +++ b/include/linux/mmc/card.h
>> @@ -272,6 +272,7 @@ struct mmc_card {
>>  #define MMC_QUIRK_BROKEN_HPI   (1<<13)         /* Disable broken HPI support */
>>
>>         bool                    reenable_cmdq;  /* Re-enable Command Queue */
>> +       bool                    force_disable_cmdq; /* Keep Command Queue disabled */
>>
>>         unsigned int            erase_size;     /* erase size in sectors */
>>         unsigned int            erase_shift;    /* if erase unit is power 2 */
>> --
>> 2.17.1
>>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-03-09 15:47     ` Ulf Hansson
@ 2021-03-14 22:26       ` Luca Porzio (lporzio)
  0 siblings, 0 replies; 13+ messages in thread
From: Luca Porzio (lporzio) @ 2021-03-14 22:26 UTC (permalink / raw)
  To: Ulf Hansson, Luca Porzio; +Cc: linux-mmc, Zhan Liu (zliua)

Micron Confidential

> 
> Are you saying that we may want CQE to stay disabled beyond the FFU
> process, to allow the reboot to be completed safely?
> 

Yes, it is. Also there are use cases where reboot is part of the sequence 
e.g. the force_disable_cmdq flag for the tests is a good example.

> > Also we need to claim the host to make sure all the pending commands
> > in the queue are completed successfully before disabling.
> 
> Yes, of course. It sounds like you may have misinterpreted my proposals.
> 
> The problem is not that we need to claim the host, but that you add an
> entirely new path to do it.
> 
> *If* we conclude that we should add a sysfs node to control CQE, we should
> create a mmc blk request for it (which will manage the claim for us as well). I
> suggest you have a closer look at power_ro_lock_store(), which should be
> the equivalent to what we need to implement here.
> 

I understand what you mean and try to rethink the patch taking inspiration 
from that.

> >
> > I can rethink the patch to implement a specific iotcl() request which
> > disables CMDQ if you think that is a better implementation but in the
> > end it will still require the host claim.
> >
> > Any feedback or suggestion is appreciated.
> 
> To be clear, I am not proposing to add a new ioctl for mmc. Those we have
> today, should be sufficient I think.
> 
> However, rather than adding a new sysfs node to control CQE, perhaps we
> can parse the received ioctl data structure, to find out if the
> command/request is for FFU and then take specific actions. Along the lines of
> what we do for mmc_sanitize(), for example.
> 
> Another option, rather than parsing ioctl data for the FFU command/request,
> is to always temporarily disable CQE while processing an mmc ioctl request.
> 
> Would this work?
>

I think you are right: we should always disable CMDQ for all ioctl requests 
as of today I do not know a case where CMDQ on is useful for multi cmd lists.

What do you suggest:
1 - mimic the power_ro_lock_store and let CMDQ be off across reboots
2 - disable cmdq for multi cmd lists

I would prefer 1 because it covers most cases but 2 could be an option I 
can investigate.
 
> Kind regards
> Uffe

Cheers!
   Luca

Micron Confidential

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-03-10  8:54   ` Adrian Hunter
@ 2021-03-14 22:33     ` Luca Porzio (lporzio)
  2021-03-15  7:11       ` Adrian Hunter
  0 siblings, 1 reply; 13+ messages in thread
From: Luca Porzio (lporzio) @ 2021-03-14 22:33 UTC (permalink / raw)
  To: Adrian Hunter, Ulf Hansson, Luca Porzio; +Cc: linux-mmc, Zhan Liu (zliua)

Micron Confidential



> -----Original Message-----
> From: Adrian Hunter <adrian.hunter@intel.com>
> Sent: Wednesday, March 10, 2021 9:54 AM
> To: Ulf Hansson <ulf.hansson@linaro.org>; Luca Porzio <porzio@gmail.com>
> Cc: linux-mmc@vger.kernel.org; Zhan Liu (zliua) <zliua@micron.com>; Luca
> Porzio (lporzio) <lporzio@micron.com>
> Subject: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
> 
> CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless
> you recognize the sender and were expecting this message.
> 
> 
> On 2/03/21 12:46 pm, Ulf Hansson wrote:
> > + Adrian
> >
> > On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> wrote:
> >>
> >> cmdq_en attribute in sysfs now can now be written.
> >> When 0 is written:
> >>   CMDQ is disabled and kept disabled across device reboots.
> >> When 1 is written:
> >>   CMDQ mode is instantly reneabled (if supported).
> >>
> >> Signed-off-by: Luca Porzio <lporzio@micron.com>
> >> Signed-off-by: Zhan Liu <zliua@micron.com>
> >
> > Luca, thanks for your patch! I am about to start to review this.
> >
> > I have also looped in Adrian to get his opinions.
> >
> > Get back to you soon!
> >
> > Kind regards
> > Uffe
> >
> >> ---
> >>  drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++-
> --------
> >>  include/linux/mmc/card.h |   1 +
> >>  2 files changed, 118 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
> >> 0d80b72ddde8..5c7d5bac5c00 100644
> >> --- a/drivers/mmc/core/mmc.c
> >> +++ b/drivers/mmc/core/mmc.c
> >> @@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported,
> "%#x\n",
> >> MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
> >> MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);  MMC_DEV_ATTR(rca,
> >> "0x%04x\n", card->rca); -MMC_DEV_ATTR(cmdq_en, "%d\n",
> >> card->ext_csd.cmdq_en);
> >> +
> >> +
> >> +/* Setup command queue mode and CQE if underling hw supports it
> >> + * and assuming force_disable_cmdq has not been set.
> >> + */
> >> +static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card
> >> +*card) {
> >> +       int err;
> >> +
> >> +       /* Check HW support */
> >> +       if (!card->ext_csd.cmdq_support || !(host->caps2 &
> MMC_CAP2_CQE))
> >> +               card->force_disable_cmdq = true;
> >> +
> >> +       /* Enable/Disable  CMDQ mode */
> >> +       if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
> >> +               err = mmc_cmdq_enable(card);
> >> +               if (err && err != -EBADMSG)
> >> +                       return err;
> >> +               if (err) {
> >> +                       pr_warn("%s: Enabling CMDQ failed\n",
> >> +                           mmc_hostname(card->host));
> >> +                       card->ext_csd.cmdq_support = false;
> >> +                       card->ext_csd.cmdq_depth = 0;
> >> +               }
> >> +
> >> +       } else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
> >> +               err = mmc_cmdq_disable(card);
> >> +               if (err) {
> >> +                       pr_warn("%s: Disabling CMDQ failed, error %d\n",
> >> +                           mmc_hostname(card->host), err);
> >> +                       err = 0;
> >> +               }
> >> +       }
> >> +
> >> +       /*
> >> +        * In some cases (e.g. RPMB or mmc_test), the Command Queue
> must be
> >> +        * disabled for a time, so a flag is needed to indicate to re-enable the
> >> +        * Command Queue.
> >> +        */
> >> +       card->reenable_cmdq = card->ext_csd.cmdq_en;
> >> +
> >> +       /* Enable/Disable Host CQE */
> >> +       if (!card->force_disable_cmdq) {
> >> +
> >> +               if (host->cqe_ops && !host->cqe_enabled) {
> >> +                       err = host->cqe_ops->cqe_enable(host, card);
> >> +                       if (!err) {
> >> +                               host->cqe_enabled = true;
> 
> Re-initializing the card is also a recovery path for the block driver.
> Changing host->cqe_enabled during a recovery reset, creates an unexpected
> dependency for the block driver.  That should not be necessary, and given
> that cqhci does memory allocation as part of enabling, it is better not to
> disable / re-enable if it can be helped.
>

Adrian,

This patch does nothing if the CMDQ maintains the same status (even 
across reboots). It only take decision if CMDQ state is to be changed, thus
the occurrence you mention should not happen.
 
> From a design point of view, it is really the block driver that should control
> the use of command queuing rather than expecting it to cope with changes
> from a lower level.
> 

I see this is also related with comment from Ulf. I will propose a new patch 
which addresses this one. Thanks for your comment here.

> >> +
> >> +                               if (card->ext_csd.cmdq_en) {
> >> +                                       pr_info("%s: Command Queue Engine enabled\n",
> >> +                                           mmc_hostname(host));
> >> +                               } else {
> >> +                                       host->hsq_enabled = true;
> >> +                                       pr_info("%s: Host Software Queue enabled\n",
> >> +                                           mmc_hostname(host));
> >> +                               }
> >> +                       }
> >> +               }
> >> +
> >> +       } else {
> >> +
> >> +               if (host->cqe_enabled) {
> >> +                       host->cqe_ops->cqe_disable(host);
> >> +                       host->cqe_enabled = false;
> >> +                       pr_info("%s: Command Queue Engine disabled\n",
> >> +                           mmc_hostname(host));
> >> +               }
> >> +
> >> +               host->hsq_enabled = false;
> 
> This looks quite wrong for hsq which is currently not used with cmdq.
> 

HSQ is really trying to speed up command adoption but in my use case
(ie. multi cmd list with as low as possible interference from other sources)
HSQ could mix commands and make problems, so I decided to stay safe 
and prefer stability against speed of execution.

Do you agree? Any suggestion?

Thanks,
   Luca

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
  2021-03-14 22:33     ` [EXT] " Luca Porzio (lporzio)
@ 2021-03-15  7:11       ` Adrian Hunter
  0 siblings, 0 replies; 13+ messages in thread
From: Adrian Hunter @ 2021-03-15  7:11 UTC (permalink / raw)
  To: Luca Porzio (lporzio), Ulf Hansson, Luca Porzio
  Cc: linux-mmc, Zhan Liu (zliua)

On 15/03/21 12:33 am, Luca Porzio (lporzio) wrote:
> Micron Confidential
> 
> 
> 
>> -----Original Message-----
>> From: Adrian Hunter <adrian.hunter@intel.com>
>> Sent: Wednesday, March 10, 2021 9:54 AM
>> To: Ulf Hansson <ulf.hansson@linaro.org>; Luca Porzio <porzio@gmail.com>
>> Cc: linux-mmc@vger.kernel.org; Zhan Liu (zliua) <zliua@micron.com>; Luca
>> Porzio (lporzio) <lporzio@micron.com>
>> Subject: [EXT] Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
>>
>> CAUTION: EXTERNAL EMAIL. Do not click links or open attachments unless
>> you recognize the sender and were expecting this message.
>>
>>
>> On 2/03/21 12:46 pm, Ulf Hansson wrote:
>>> + Adrian
>>>
>>> On Mon, 15 Feb 2021 at 01:33, Luca Porzio <porzio@gmail.com> wrote:
>>>>
>>>> cmdq_en attribute in sysfs now can now be written.
>>>> When 0 is written:
>>>>   CMDQ is disabled and kept disabled across device reboots.
>>>> When 1 is written:
>>>>   CMDQ mode is instantly reneabled (if supported).
>>>>
>>>> Signed-off-by: Luca Porzio <lporzio@micron.com>
>>>> Signed-off-by: Zhan Liu <zliua@micron.com>
>>>
>>> Luca, thanks for your patch! I am about to start to review this.
>>>
>>> I have also looped in Adrian to get his opinions.
>>>
>>> Get back to you soon!
>>>
>>> Kind regards
>>> Uffe
>>>
>>>> ---
>>>>  drivers/mmc/core/mmc.c   | 152 ++++++++++++++++++++++++++++++-
>> --------
>>>>  include/linux/mmc/card.h |   1 +
>>>>  2 files changed, 118 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index
>>>> 0d80b72ddde8..5c7d5bac5c00 100644
>>>> --- a/drivers/mmc/core/mmc.c
>>>> +++ b/drivers/mmc/core/mmc.c
>>>> @@ -794,7 +794,120 @@ MMC_DEV_ATTR(enhanced_rpmb_supported,
>> "%#x\n",
>>>> MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
>>>> MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);  MMC_DEV_ATTR(rca,
>>>> "0x%04x\n", card->rca); -MMC_DEV_ATTR(cmdq_en, "%d\n",
>>>> card->ext_csd.cmdq_en);
>>>> +
>>>> +
>>>> +/* Setup command queue mode and CQE if underling hw supports it
>>>> + * and assuming force_disable_cmdq has not been set.
>>>> + */
>>>> +static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card
>>>> +*card) {
>>>> +       int err;
>>>> +
>>>> +       /* Check HW support */
>>>> +       if (!card->ext_csd.cmdq_support || !(host->caps2 &
>> MMC_CAP2_CQE))
>>>> +               card->force_disable_cmdq = true;
>>>> +
>>>> +       /* Enable/Disable  CMDQ mode */
>>>> +       if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
>>>> +               err = mmc_cmdq_enable(card);
>>>> +               if (err && err != -EBADMSG)
>>>> +                       return err;
>>>> +               if (err) {
>>>> +                       pr_warn("%s: Enabling CMDQ failed\n",
>>>> +                           mmc_hostname(card->host));
>>>> +                       card->ext_csd.cmdq_support = false;
>>>> +                       card->ext_csd.cmdq_depth = 0;
>>>> +               }
>>>> +
>>>> +       } else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
>>>> +               err = mmc_cmdq_disable(card);
>>>> +               if (err) {
>>>> +                       pr_warn("%s: Disabling CMDQ failed, error %d\n",
>>>> +                           mmc_hostname(card->host), err);
>>>> +                       err = 0;
>>>> +               }
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * In some cases (e.g. RPMB or mmc_test), the Command Queue
>> must be
>>>> +        * disabled for a time, so a flag is needed to indicate to re-enable the
>>>> +        * Command Queue.
>>>> +        */
>>>> +       card->reenable_cmdq = card->ext_csd.cmdq_en;
>>>> +
>>>> +       /* Enable/Disable Host CQE */
>>>> +       if (!card->force_disable_cmdq) {
>>>> +
>>>> +               if (host->cqe_ops && !host->cqe_enabled) {
>>>> +                       err = host->cqe_ops->cqe_enable(host, card);
>>>> +                       if (!err) {
>>>> +                               host->cqe_enabled = true;
>>
>> Re-initializing the card is also a recovery path for the block driver.
>> Changing host->cqe_enabled during a recovery reset, creates an unexpected
>> dependency for the block driver.  That should not be necessary, and given
>> that cqhci does memory allocation as part of enabling, it is better not to
>> disable / re-enable if it can be helped.
>>
> 
> Adrian,
> 
> This patch does nothing if the CMDQ maintains the same status (even 
> across reboots). It only take decision if CMDQ state is to be changed, thus
> the occurrence you mention should not happen.

It does if there are errors.

>  
>> From a design point of view, it is really the block driver that should control
>> the use of command queuing rather than expecting it to cope with changes
>> from a lower level.
>>
> 
> I see this is also related with comment from Ulf. I will propose a new patch 
> which addresses this one. Thanks for your comment here.
> 
>>>> +
>>>> +                               if (card->ext_csd.cmdq_en) {
>>>> +                                       pr_info("%s: Command Queue Engine enabled\n",
>>>> +                                           mmc_hostname(host));
>>>> +                               } else {
>>>> +                                       host->hsq_enabled = true;
>>>> +                                       pr_info("%s: Host Software Queue enabled\n",
>>>> +                                           mmc_hostname(host));
>>>> +                               }
>>>> +                       }
>>>> +               }
>>>> +
>>>> +       } else {
>>>> +
>>>> +               if (host->cqe_enabled) {
>>>> +                       host->cqe_ops->cqe_disable(host);
>>>> +                       host->cqe_enabled = false;
>>>> +                       pr_info("%s: Command Queue Engine disabled\n",
>>>> +                           mmc_hostname(host));
>>>> +               }
>>>> +
>>>> +               host->hsq_enabled = false;
>>
>> This looks quite wrong for hsq which is currently not used with cmdq.
>>
> 
> HSQ is really trying to speed up command adoption but in my use case
> (ie. multi cmd list with as low as possible interference from other sources)
> HSQ could mix commands and make problems, so I decided to stay safe 
> and prefer stability against speed of execution.
> 
> Do you agree? Any suggestion?

I just meant, do not break HSQ.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
@ 2021-02-15  3:09 kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-02-15  3:09 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 9527 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210215003249.GA12303@lupo-laptop>
References: <20210215003249.GA12303@lupo-laptop>
TO: Luca Porzio <porzio@gmail.com>

Hi Luca,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on next-20210212]
[cannot apply to linus/master mmc/mmc-next v5.11-rc7 v5.11-rc6 v5.11-rc5 v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luca-Porzio/Support-temporarily-disable-of-the-CMDQ-mode/20210215-083730
base:    07f7e57c63aaa2afb4ea31edef05e08699a63a00
:::::: branch date: 3 hours ago
:::::: commit date: 3 hours ago
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"cppcheck warnings: (new ones prefixed by >>)"
>> drivers/mmc/core/mmc.c:870:9: warning: Uninitialized variable: err [uninitvar]
    return err;
           ^

cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

>> drivers/mmc/core/mmc.c:885:26: warning: Either the condition '!card' is redundant or there is possible null pointer dereference: card. [nullPointerRedundantCheck]
    struct mmc_host *host = card->host;
                            ^
   drivers/mmc/core/mmc.c:889:6: note: Assuming that condition '!card' is not redundant
    if (!card || kstrtoul(buf, 0, &enable))
        ^
   drivers/mmc/core/mmc.c:885:26: note: Null pointer dereference
    struct mmc_host *host = card->host;
                            ^

vim +885 drivers/mmc/core/mmc.c

07a9ce0e702520 Luca Porzio 2021-02-15  797  
07a9ce0e702520 Luca Porzio 2021-02-15  798  
07a9ce0e702520 Luca Porzio 2021-02-15  799  /* Setup command queue mode and CQE if underling hw supports it
07a9ce0e702520 Luca Porzio 2021-02-15  800   * and assuming force_disable_cmdq has not been set.
07a9ce0e702520 Luca Porzio 2021-02-15  801   */
07a9ce0e702520 Luca Porzio 2021-02-15  802  static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
07a9ce0e702520 Luca Porzio 2021-02-15  803  {
07a9ce0e702520 Luca Porzio 2021-02-15  804  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  805  
07a9ce0e702520 Luca Porzio 2021-02-15  806  	/* Check HW support */
07a9ce0e702520 Luca Porzio 2021-02-15  807  	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
07a9ce0e702520 Luca Porzio 2021-02-15  808  		card->force_disable_cmdq = true;
07a9ce0e702520 Luca Porzio 2021-02-15  809  
07a9ce0e702520 Luca Porzio 2021-02-15  810  	/* Enable/Disable  CMDQ mode */
07a9ce0e702520 Luca Porzio 2021-02-15  811  	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  812  		err = mmc_cmdq_enable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  813  		if (err && err != -EBADMSG)
07a9ce0e702520 Luca Porzio 2021-02-15  814  			return err;
07a9ce0e702520 Luca Porzio 2021-02-15  815  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  816  			pr_warn("%s: Enabling CMDQ failed\n",
07a9ce0e702520 Luca Porzio 2021-02-15  817  			    mmc_hostname(card->host));
07a9ce0e702520 Luca Porzio 2021-02-15  818  			card->ext_csd.cmdq_support = false;
07a9ce0e702520 Luca Porzio 2021-02-15  819  			card->ext_csd.cmdq_depth = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  820  		}
07a9ce0e702520 Luca Porzio 2021-02-15  821  
07a9ce0e702520 Luca Porzio 2021-02-15  822  	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  823  		err = mmc_cmdq_disable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  824  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  825  			pr_warn("%s: Disabling CMDQ failed, error %d\n",
07a9ce0e702520 Luca Porzio 2021-02-15  826  			    mmc_hostname(card->host), err);
07a9ce0e702520 Luca Porzio 2021-02-15  827  			err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  828  		}
07a9ce0e702520 Luca Porzio 2021-02-15  829  	}
07a9ce0e702520 Luca Porzio 2021-02-15  830  
07a9ce0e702520 Luca Porzio 2021-02-15  831  	/*
07a9ce0e702520 Luca Porzio 2021-02-15  832  	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
07a9ce0e702520 Luca Porzio 2021-02-15  833  	 * disabled for a time, so a flag is needed to indicate to re-enable the
07a9ce0e702520 Luca Porzio 2021-02-15  834  	 * Command Queue.
07a9ce0e702520 Luca Porzio 2021-02-15  835  	 */
07a9ce0e702520 Luca Porzio 2021-02-15  836  	card->reenable_cmdq = card->ext_csd.cmdq_en;
07a9ce0e702520 Luca Porzio 2021-02-15  837  
07a9ce0e702520 Luca Porzio 2021-02-15  838  	/* Enable/Disable Host CQE */
07a9ce0e702520 Luca Porzio 2021-02-15  839  	if (!card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  840  
07a9ce0e702520 Luca Porzio 2021-02-15  841  		if (host->cqe_ops && !host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  842  			err = host->cqe_ops->cqe_enable(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  843  			if (!err) {
07a9ce0e702520 Luca Porzio 2021-02-15  844  				host->cqe_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  845  
07a9ce0e702520 Luca Porzio 2021-02-15  846  				if (card->ext_csd.cmdq_en) {
07a9ce0e702520 Luca Porzio 2021-02-15  847  					pr_info("%s: Command Queue Engine enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  848  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  849  				} else {
07a9ce0e702520 Luca Porzio 2021-02-15  850  					host->hsq_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  851  					pr_info("%s: Host Software Queue enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  852  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  853  				}
07a9ce0e702520 Luca Porzio 2021-02-15  854  			}
07a9ce0e702520 Luca Porzio 2021-02-15  855  		}
07a9ce0e702520 Luca Porzio 2021-02-15  856  
07a9ce0e702520 Luca Porzio 2021-02-15  857  	} else {
07a9ce0e702520 Luca Porzio 2021-02-15  858  
07a9ce0e702520 Luca Porzio 2021-02-15  859  		if (host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  860  			host->cqe_ops->cqe_disable(host);
07a9ce0e702520 Luca Porzio 2021-02-15  861  			host->cqe_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  862  			pr_info("%s: Command Queue Engine disabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  863  			    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  864  		}
07a9ce0e702520 Luca Porzio 2021-02-15  865  
07a9ce0e702520 Luca Porzio 2021-02-15  866  		host->hsq_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  867  		err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  868  	}
07a9ce0e702520 Luca Porzio 2021-02-15  869  
07a9ce0e702520 Luca Porzio 2021-02-15 @870  	return err;
07a9ce0e702520 Luca Porzio 2021-02-15  871  }
07a9ce0e702520 Luca Porzio 2021-02-15  872  
07a9ce0e702520 Luca Porzio 2021-02-15  873  
07a9ce0e702520 Luca Porzio 2021-02-15  874  static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
07a9ce0e702520 Luca Porzio 2021-02-15  875  {
07a9ce0e702520 Luca Porzio 2021-02-15  876  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15  877  
07a9ce0e702520 Luca Porzio 2021-02-15  878  	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
07a9ce0e702520 Luca Porzio 2021-02-15  879  }
07a9ce0e702520 Luca Porzio 2021-02-15  880  
07a9ce0e702520 Luca Porzio 2021-02-15  881  static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
07a9ce0e702520 Luca Porzio 2021-02-15  882  				 const char *buf, size_t count)
07a9ce0e702520 Luca Porzio 2021-02-15  883  {
07a9ce0e702520 Luca Porzio 2021-02-15  884  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15 @885  	struct mmc_host *host = card->host;
07a9ce0e702520 Luca Porzio 2021-02-15  886  	unsigned long enable;
07a9ce0e702520 Luca Porzio 2021-02-15  887  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  888  
07a9ce0e702520 Luca Porzio 2021-02-15  889  	if (!card || kstrtoul(buf, 0, &enable))
07a9ce0e702520 Luca Porzio 2021-02-15  890  		return -EINVAL;
07a9ce0e702520 Luca Porzio 2021-02-15  891  	if (!card->ext_csd.cmdq_support)
07a9ce0e702520 Luca Porzio 2021-02-15  892  		return -EOPNOTSUPP;
07a9ce0e702520 Luca Porzio 2021-02-15  893  
07a9ce0e702520 Luca Porzio 2021-02-15  894  	enable = !!enable;
07a9ce0e702520 Luca Porzio 2021-02-15  895  	if (enable == card->ext_csd.cmdq_en)
07a9ce0e702520 Luca Porzio 2021-02-15  896  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  897  
07a9ce0e702520 Luca Porzio 2021-02-15  898  	mmc_get_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  899  	card->force_disable_cmdq = !enable;
07a9ce0e702520 Luca Porzio 2021-02-15  900  	err = mmc_cmdq_setup(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  901  	mmc_put_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  902  
07a9ce0e702520 Luca Porzio 2021-02-15  903  	if (err)
07a9ce0e702520 Luca Porzio 2021-02-15  904  		return err;
07a9ce0e702520 Luca Porzio 2021-02-15  905  	else
07a9ce0e702520 Luca Porzio 2021-02-15  906  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  907  }
07a9ce0e702520 Luca Porzio 2021-02-15  908  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH 2/2] Make cmdq_en attribute writeable
@ 2021-02-15  2:21 kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-02-15  2:21 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 9634 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210215003249.GA12303@lupo-laptop>
References: <20210215003249.GA12303@lupo-laptop>
TO: Luca Porzio <porzio@gmail.com>

Hi Luca,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on next-20210212]
[cannot apply to linus/master mmc/mmc-next v5.11-rc7 v5.11-rc6 v5.11-rc5 v5.11]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Luca-Porzio/Support-temporarily-disable-of-the-CMDQ-mode/20210215-083730
base:    07f7e57c63aaa2afb4ea31edef05e08699a63a00
:::::: branch date: 2 hours ago
:::::: commit date: 2 hours ago
config: i386-randconfig-m021-20210215 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
drivers/mmc/core/mmc.c:870 mmc_cmdq_setup() error: uninitialized symbol 'err'.
drivers/mmc/core/mmc.c:889 cmdq_en_store() warn: variable dereferenced before check 'card' (see line 885)

Old smatch warnings:
drivers/mmc/core/mmc.c:774 mmc_erase_size_show() warn: should 'card->erase_size << 9' be a 64 bit type?
drivers/mmc/core/mmc.c:775 mmc_preferred_erase_size_show() warn: should 'card->pref_erase << 9' be a 64 bit type?
drivers/mmc/core/mmc.c:1072 mmc_select_powerclass() warn: should '1 << bus_width' be a 64 bit type?
drivers/mmc/core/mmc.c:1159 mmc_select_bus_width() warn: should '1 << bus_width' be a 64 bit type?
drivers/mmc/core/mmc.c:1211 mmc_select_hs_ddr() warn: should '1 << bus_width' be a 64 bit type?

vim +/err +870 drivers/mmc/core/mmc.c

07a9ce0e702520 Luca Porzio 2021-02-15  797  
07a9ce0e702520 Luca Porzio 2021-02-15  798  
07a9ce0e702520 Luca Porzio 2021-02-15  799  /* Setup command queue mode and CQE if underling hw supports it
07a9ce0e702520 Luca Porzio 2021-02-15  800   * and assuming force_disable_cmdq has not been set.
07a9ce0e702520 Luca Porzio 2021-02-15  801   */
07a9ce0e702520 Luca Porzio 2021-02-15  802  static int mmc_cmdq_setup(struct mmc_host *host, struct mmc_card *card)
07a9ce0e702520 Luca Porzio 2021-02-15  803  {
07a9ce0e702520 Luca Porzio 2021-02-15  804  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  805  
07a9ce0e702520 Luca Porzio 2021-02-15  806  	/* Check HW support */
07a9ce0e702520 Luca Porzio 2021-02-15  807  	if (!card->ext_csd.cmdq_support || !(host->caps2 & MMC_CAP2_CQE))
07a9ce0e702520 Luca Porzio 2021-02-15  808  		card->force_disable_cmdq = true;
07a9ce0e702520 Luca Porzio 2021-02-15  809  
07a9ce0e702520 Luca Porzio 2021-02-15  810  	/* Enable/Disable  CMDQ mode */
07a9ce0e702520 Luca Porzio 2021-02-15  811  	if (!card->ext_csd.cmdq_en && !card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  812  		err = mmc_cmdq_enable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  813  		if (err && err != -EBADMSG)
07a9ce0e702520 Luca Porzio 2021-02-15  814  			return err;
07a9ce0e702520 Luca Porzio 2021-02-15  815  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  816  			pr_warn("%s: Enabling CMDQ failed\n",
07a9ce0e702520 Luca Porzio 2021-02-15  817  			    mmc_hostname(card->host));
07a9ce0e702520 Luca Porzio 2021-02-15  818  			card->ext_csd.cmdq_support = false;
07a9ce0e702520 Luca Porzio 2021-02-15  819  			card->ext_csd.cmdq_depth = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  820  		}
07a9ce0e702520 Luca Porzio 2021-02-15  821  
07a9ce0e702520 Luca Porzio 2021-02-15  822  	} else if (card->ext_csd.cmdq_en && card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  823  		err = mmc_cmdq_disable(card);
07a9ce0e702520 Luca Porzio 2021-02-15  824  		if (err) {
07a9ce0e702520 Luca Porzio 2021-02-15  825  			pr_warn("%s: Disabling CMDQ failed, error %d\n",
07a9ce0e702520 Luca Porzio 2021-02-15  826  			    mmc_hostname(card->host), err);
07a9ce0e702520 Luca Porzio 2021-02-15  827  			err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  828  		}
07a9ce0e702520 Luca Porzio 2021-02-15  829  	}
07a9ce0e702520 Luca Porzio 2021-02-15  830  
07a9ce0e702520 Luca Porzio 2021-02-15  831  	/*
07a9ce0e702520 Luca Porzio 2021-02-15  832  	 * In some cases (e.g. RPMB or mmc_test), the Command Queue must be
07a9ce0e702520 Luca Porzio 2021-02-15  833  	 * disabled for a time, so a flag is needed to indicate to re-enable the
07a9ce0e702520 Luca Porzio 2021-02-15  834  	 * Command Queue.
07a9ce0e702520 Luca Porzio 2021-02-15  835  	 */
07a9ce0e702520 Luca Porzio 2021-02-15  836  	card->reenable_cmdq = card->ext_csd.cmdq_en;
07a9ce0e702520 Luca Porzio 2021-02-15  837  
07a9ce0e702520 Luca Porzio 2021-02-15  838  	/* Enable/Disable Host CQE */
07a9ce0e702520 Luca Porzio 2021-02-15  839  	if (!card->force_disable_cmdq) {
07a9ce0e702520 Luca Porzio 2021-02-15  840  
07a9ce0e702520 Luca Porzio 2021-02-15  841  		if (host->cqe_ops && !host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  842  			err = host->cqe_ops->cqe_enable(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  843  			if (!err) {
07a9ce0e702520 Luca Porzio 2021-02-15  844  				host->cqe_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  845  
07a9ce0e702520 Luca Porzio 2021-02-15  846  				if (card->ext_csd.cmdq_en) {
07a9ce0e702520 Luca Porzio 2021-02-15  847  					pr_info("%s: Command Queue Engine enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  848  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  849  				} else {
07a9ce0e702520 Luca Porzio 2021-02-15  850  					host->hsq_enabled = true;
07a9ce0e702520 Luca Porzio 2021-02-15  851  					pr_info("%s: Host Software Queue enabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  852  					    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  853  				}
07a9ce0e702520 Luca Porzio 2021-02-15  854  			}
07a9ce0e702520 Luca Porzio 2021-02-15  855  		}
07a9ce0e702520 Luca Porzio 2021-02-15  856  
07a9ce0e702520 Luca Porzio 2021-02-15  857  	} else {
07a9ce0e702520 Luca Porzio 2021-02-15  858  
07a9ce0e702520 Luca Porzio 2021-02-15  859  		if (host->cqe_enabled) {
07a9ce0e702520 Luca Porzio 2021-02-15  860  			host->cqe_ops->cqe_disable(host);
07a9ce0e702520 Luca Porzio 2021-02-15  861  			host->cqe_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  862  			pr_info("%s: Command Queue Engine disabled\n",
07a9ce0e702520 Luca Porzio 2021-02-15  863  			    mmc_hostname(host));
07a9ce0e702520 Luca Porzio 2021-02-15  864  		}
07a9ce0e702520 Luca Porzio 2021-02-15  865  
07a9ce0e702520 Luca Porzio 2021-02-15  866  		host->hsq_enabled = false;
07a9ce0e702520 Luca Porzio 2021-02-15  867  		err = 0;
07a9ce0e702520 Luca Porzio 2021-02-15  868  	}
07a9ce0e702520 Luca Porzio 2021-02-15  869  
07a9ce0e702520 Luca Porzio 2021-02-15 @870  	return err;
07a9ce0e702520 Luca Porzio 2021-02-15  871  }
07a9ce0e702520 Luca Porzio 2021-02-15  872  
07a9ce0e702520 Luca Porzio 2021-02-15  873  
07a9ce0e702520 Luca Porzio 2021-02-15  874  static ssize_t cmdq_en_show(struct device *dev, struct device_attribute *attr, char *buf)
07a9ce0e702520 Luca Porzio 2021-02-15  875  {
07a9ce0e702520 Luca Porzio 2021-02-15  876  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15  877  
07a9ce0e702520 Luca Porzio 2021-02-15  878  	return sprintf(buf, "%d\n", card->ext_csd.cmdq_en);
07a9ce0e702520 Luca Porzio 2021-02-15  879  }
07a9ce0e702520 Luca Porzio 2021-02-15  880  
07a9ce0e702520 Luca Porzio 2021-02-15  881  static ssize_t cmdq_en_store(struct device *dev, struct device_attribute *attr,
07a9ce0e702520 Luca Porzio 2021-02-15  882  				 const char *buf, size_t count)
07a9ce0e702520 Luca Porzio 2021-02-15  883  {
07a9ce0e702520 Luca Porzio 2021-02-15  884  	struct mmc_card *card = mmc_dev_to_card(dev);
07a9ce0e702520 Luca Porzio 2021-02-15 @885  	struct mmc_host *host = card->host;
07a9ce0e702520 Luca Porzio 2021-02-15  886  	unsigned long enable;
07a9ce0e702520 Luca Porzio 2021-02-15  887  	int err;
07a9ce0e702520 Luca Porzio 2021-02-15  888  
07a9ce0e702520 Luca Porzio 2021-02-15 @889  	if (!card || kstrtoul(buf, 0, &enable))
07a9ce0e702520 Luca Porzio 2021-02-15  890  		return -EINVAL;
07a9ce0e702520 Luca Porzio 2021-02-15  891  	if (!card->ext_csd.cmdq_support)
07a9ce0e702520 Luca Porzio 2021-02-15  892  		return -EOPNOTSUPP;
07a9ce0e702520 Luca Porzio 2021-02-15  893  
07a9ce0e702520 Luca Porzio 2021-02-15  894  	enable = !!enable;
07a9ce0e702520 Luca Porzio 2021-02-15  895  	if (enable == card->ext_csd.cmdq_en)
07a9ce0e702520 Luca Porzio 2021-02-15  896  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  897  
07a9ce0e702520 Luca Porzio 2021-02-15  898  	mmc_get_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  899  	card->force_disable_cmdq = !enable;
07a9ce0e702520 Luca Porzio 2021-02-15  900  	err = mmc_cmdq_setup(host, card);
07a9ce0e702520 Luca Porzio 2021-02-15  901  	mmc_put_card(card, NULL);
07a9ce0e702520 Luca Porzio 2021-02-15  902  
07a9ce0e702520 Luca Porzio 2021-02-15  903  	if (err)
07a9ce0e702520 Luca Porzio 2021-02-15  904  		return err;
07a9ce0e702520 Luca Porzio 2021-02-15  905  	else
07a9ce0e702520 Luca Porzio 2021-02-15  906  		return count;
07a9ce0e702520 Luca Porzio 2021-02-15  907  }
07a9ce0e702520 Luca Porzio 2021-02-15  908  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38219 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-03-15  7:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-15  0:32 [RFC PATCH 2/2] Make cmdq_en attribute writeable Luca Porzio
2021-02-15 13:49 ` Dan Carpenter
2021-02-15 13:49   ` Dan Carpenter
2021-02-17 13:04 ` kernel test robot
2021-03-02 10:46 ` Ulf Hansson
2021-03-10  8:54   ` Adrian Hunter
2021-03-14 22:33     ` [EXT] " Luca Porzio (lporzio)
2021-03-15  7:11       ` Adrian Hunter
2021-03-08 17:05 ` Ulf Hansson
     [not found]   ` <CABhGgDPpUXPHJ49E_ku5N-fO=GWZKTdQQUGugAruG6y2=J1YgA@mail.gmail.com>
2021-03-09 15:47     ` Ulf Hansson
2021-03-14 22:26       ` [EXT] " Luca Porzio (lporzio)
2021-02-15  2:21 kernel test robot
2021-02-15  3:09 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.