[PATCH v2 01/10] scsi: ufs: Allowing power mode change

[PATCH v2 01/10] scsi: ufs: Allowing power mode change

[Asutosh Das]

From: Yaniv Gardi <ygardi@codeaurora.org>

Due to M-PHY issues, moving from HS to any other mode or gear or
even Hibern8 may cause some un-predicted behavior
of the device.
This patch adds provides a quirk to address that.

Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 8 +++++++-
 drivers/scsi/ufs/ufshcd.h | 7 +++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5bc9dc1..f3083fe 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4162,9 +4162,15 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 			goto out;
 	} while (ret && retries--);
 
-	if (ret)
+	if (ret) {
 		/* failed to get the link up... retire */
 		goto out;
+	}
+
+	if (hba->quirks & UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE) {
+		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 0);
+		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
+	}
 
 	if (link_startup_again) {
 		link_startup_again = false;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index cbe46f6..bb4ecfb 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -591,6 +591,13 @@ struct ufs_hba {
 	 */
 	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			UFS_BIT(7)
 
+	/*
+	 * Needs to be enabled if moving from HS to any other gear/mode or even
+	 * hibern8 causes unpredicted behavior of device.
+	 * If this quirk is enabled, standard UFS driver will disable/enable
+	 * TX_LCC.
+	 */
+	#define UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE		UFS_BIT(8)
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
 	/* Device deviations from standard UFS device spec. */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 02/10] scsi: ufs-qcom: Enable UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE quirk

[Asutosh Das]

Enable UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE quirk to avoid failures
in seen on some UFS devices.

Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 4563d2e..d9edef8 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1105,7 +1105,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
 
 	if (host->hw_ver.major >= 0x2) {
 		hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
-
+		hba->quirks |= UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE;
 		if (!ufs_qcom_cap_qunipro(host))
 			/* Legacy UniPro mode still need following quirks */
 			hba->quirks |= (UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 03/10] scsi: ufs: Add LCC quirk for host and device

[Asutosh Das]

LCC (Line Control Command) is being used for communication between
UFS host and UFS device. But some hosts might have the issue with
issuing the LCC commands to UFS device and in this case LCC could be
explicitly disabled.

But there could be a need where we don't want to disable the LCC
on both host & device; hence this change splits the quirk in 2 parts
one for host and one for device.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++
 drivers/scsi/ufs/ufshcd.h | 12 ++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f3083fe..6dabce8 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4116,6 +4116,11 @@ static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
 	return err;
 }
 
+static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba)
+{
+	return ufshcd_disable_tx_lcc(hba, false);
+}
+
 static inline int ufshcd_disable_device_tx_lcc(struct ufs_hba *hba)
 {
 	return ufshcd_disable_tx_lcc(hba, true);
@@ -4172,6 +4177,17 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
 	}
 
+	if (hba->quirks & UFSHCD_BROKEN_LCC_PROCESSING_ON_HOST) {
+		ret = ufshcd_disable_device_tx_lcc(hba);
+		if (ret)
+			goto out;
+	}
+
+	if (hba->quirks & UFSHCD_BROKEN_LCC_PROCESSING_ON_DEVICE) {
+		ret = ufshcd_disable_host_tx_lcc(hba);
+		if (ret)
+			goto out;
+	}
 	if (link_startup_again) {
 		link_startup_again = false;
 		retries = DME_LINKSTARTUP_RETRIES;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index bb4ecfb..0417c42 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -598,6 +598,18 @@ struct ufs_hba {
 	 * TX_LCC.
 	 */
 	#define UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE		UFS_BIT(8)
+
+	/*
+	 * If UFS device is having issue in processing LCC (Line Control
+	 * Command) coming from UFS host controller then enable this quirk.
+	 * When this quirk is enabled, host controller driver should disable
+	 * the LCC transmission on UFS host controller (by clearing
+	 * TX_LCC_ENABLE attribute of host to 0).
+	 */
+	#define UFSHCD_BROKEN_LCC_PROCESSING_ON_DEVICE		0x100
+
+	#define UFSHCD_BROKEN_LCC_PROCESSING_ON_HOST		0x200
+
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
 	/* Device deviations from standard UFS device spec. */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 04/10] scsi: ufs: fix exception event handling

[Asutosh Das]

From: Maya Erez <merez@codeaurora.org>

The device can set the exception event bit in one of the response UPIU,
for example to notify the need for urgent BKOPs operation.
In such a case the host driver calls ufshcd_exception_event_handler to
handle this notification.
When trying to check the exception event status (for finding the cause for
the exception event), the device may be busy with additional SCSI commands
handling and may not respond within the 100ms timeout.

To prevent that, we need to block SCSI commands during handling of
exception events and allow retransmissions of the query requests,
in case of timeout.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Maya Erez <merez@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6dabce8..838ba8f0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4967,6 +4967,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 	hba = container_of(work, struct ufs_hba, eeh_work);
 
 	pm_runtime_get_sync(hba->dev);
+	scsi_block_requests(hba->host);
 	err = ufshcd_get_ee_status(hba, &status);
 	if (err) {
 		dev_err(hba->dev, "%s: failed to get exception status %d\n",
@@ -4980,6 +4981,7 @@ static void ufshcd_exception_event_handler(struct work_struct *work)
 		ufshcd_bkops_exception_event_handler(hba);
 
 out:
+	scsi_unblock_requests(hba->host);
 	pm_runtime_put_sync(hba->dev);
 	return;
 }
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 05/10] scsi: ufshcd: fix possible unclocked register access

[Asutosh Das]

From: Subhash Jadavani <subhashj@codeaurora.org>

vendor specific setup_clocks ops may depend on clocks managed by ufshcd
driver so if the vendor specific setup_clocks callback is called when
the required clocks are turned off, it results into unclocked register
access.

This change make sure that required clocks are enabled before vendor
specific setup_clocks callback is called.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 838ba8f0..dfeb194 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6780,9 +6780,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 	if (list_empty(head))
 		goto out;
 
-	ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
-	if (ret)
-		return ret;
+	/*
+	 * vendor specific setup_clocks ops may depend on clocks managed by
+	 * this standard driver hence call the vendor specific setup_clocks
+	 * before disabling the clocks managed here.
+	 */
+	if (!on) {
+		ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
+		if (ret)
+			return ret;
+	}
 
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
@@ -6806,9 +6813,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 		}
 	}
 
-	ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
-	if (ret)
-		return ret;
+	/*
+	 * vendor specific setup_clocks ops may depend on clocks managed by
+	 * this standard driver hence call the vendor specific setup_clocks
+	 * after enabling the clocks managed here.
+	 */
+	if (on) {
+		ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
+		if (ret)
+			return ret;
+	}
 
 out:
 	if (ret) {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 06/10] scsi: ufs: add reference counting for scsi block requests

[Asutosh Das]

From: Subhash Jadavani <subhashj@codeaurora.org>

Currently we call the scsi_block_requests()/scsi_unblock_requests()
whenever we want to block/unblock scsi requests but as there is no
reference counting, nesting of these calls could leave us in undesired
state sometime. Consider following call flow sequence:
1. func1() calls scsi_block_requests() but calls func2() before
   calling scsi_unblock_requests()
2. func2() calls scsi_block_requests()
3. func2() calls scsi_unblock_requests()
4. func1() calls scsi_unblock_requests()

As there is no reference counting, we will have scsi requests unblocked
after #3 instead of it to be unblocked only after #4. Though we may not
have failures seen with this, we might run into some failures in future.
Better solution would be to fix this by adding reference counting.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 28 ++++++++++++++++++++--------
 drivers/scsi/ufs/ufshcd.h |  2 ++
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index dfeb194..c35a076 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -264,6 +264,18 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba)
 	}
 }
 
+static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
+{
+	if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
+		scsi_unblock_requests(hba->host);
+}
+
+static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
+{
+	if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
+		scsi_block_requests(hba->host);
+}
+
 /* replace non-printable or non-ASCII characters with spaces */
 static inline void ufshcd_remove_non_printable(char *val)
 {
@@ -1077,12 +1089,12 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 	 * make sure that there are no outstanding requests when
 	 * clock scaling is in progress
 	 */
-	scsi_block_requests(hba->host);
+	ufshcd_scsi_block_requests(hba);
 	down_write(&hba->clk_scaling_lock);
 	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
 		ret = -EBUSY;
 		up_write(&hba->clk_scaling_lock);
-		scsi_unblock_requests(hba->host);
+		ufshcd_scsi_unblock_requests(hba);
 	}
 
 	return ret;
@@ -1091,7 +1103,7 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
 static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
 {
 	up_write(&hba->clk_scaling_lock);
-	scsi_unblock_requests(hba->host);
+	ufshcd_scsi_unblock_requests(hba);
 }
 
 /**
@@ -1411,7 +1423,7 @@ static void ufshcd_ungate_work(struct work_struct *work)
 		hba->clk_gating.is_suspended = false;
 	}
 unblock_reqs:
-	scsi_unblock_requests(hba->host);
+	ufshcd_scsi_unblock_requests(hba);
 }
 
 /**
@@ -1467,7 +1479,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		 * work and to enable clocks.
 		 */
 	case CLKS_OFF:
-		scsi_block_requests(hba->host);
+		ufshcd_scsi_block_requests(hba);
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
@@ -5192,7 +5204,7 @@ static void ufshcd_err_handler(struct work_struct *work)
 
 out:
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	scsi_unblock_requests(hba->host);
+	ufshcd_scsi_unblock_requests(hba);
 	ufshcd_release(hba);
 	pm_runtime_put_sync(hba->dev);
 }
@@ -5294,7 +5306,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 		/* handle fatal errors only when link is functional */
 		if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
 			/* block commands from scsi mid-layer */
-			scsi_block_requests(hba->host);
+			ufshcd_scsi_block_requests(hba);
 
 			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED;
 
@@ -8017,7 +8029,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 
 	/* Hold auto suspend until async scan completes */
 	pm_runtime_get_sync(dev);
-
+	atomic_set(&hba->scsi_block_reqs_cnt, 0);
 	/*
 	 * We are assuming that device wasn't put in sleep/power-down
 	 * state exclusively during the boot stage before kernel.
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 0417c42..76c31d5 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -498,6 +498,7 @@ struct ufs_stats {
  * @urgent_bkops_lvl: keeps track of urgent bkops level for device
  * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
  *  device is known or not.
+ * @scsi_block_reqs_cnt: reference counting for scsi block requests
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
@@ -698,6 +699,7 @@ struct ufs_hba {
 
 	struct rw_semaphore clk_scaling_lock;
 	struct ufs_desc_size desc_size;
+	atomic_t scsi_block_reqs_cnt;
 };
 
 /* Returns true if clocks can be gated. Otherwise false */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 07/10] scsi: ufs-qcom: remove broken hci version quirk

[Asutosh Das]

From: Subhash Jadavani <subhashj@codeaurora.org>

UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION is only applicable for QCOM UFS host
controller version 2.x.y and this has been fixed from version 3.x.y
onwards, hence this change removes this quirk for version 3.x.y onwards.

Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index d9edef8..27be327 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1103,7 +1103,7 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
 		hba->quirks |= UFSHCD_QUIRK_BROKEN_LCC;
 	}
 
-	if (host->hw_ver.major >= 0x2) {
+	if (host->hw_ver.major == 0x2) {
 		hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
 		hba->quirks |= UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE;
 		if (!ufs_qcom_cap_qunipro(host))
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 08/10] scsi: ufs: make sure all interrupts are processed

[Asutosh Das]

From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

As multiple requests are submitted to the ufs host controller in
parallel there could be instances where the command completion
interrupt arrives later for a request that is already processed
earlier as the corresponding doorbell was cleared when handling
the previous interrupt. Read the interrupt status in a loop after
processing the received interrupt to catch such interrupts and
handle it.

Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c35a076..09b7a3f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5383,19 +5383,30 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 	u32 intr_status, enabled_intr_status;
 	irqreturn_t retval = IRQ_NONE;
 	struct ufs_hba *hba = __hba;
+	int retries = hba->nutrs;
 
 	spin_lock(hba->host->host_lock);
 	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
-	enabled_intr_status =
-		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (intr_status)
-		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+	/*
+	 * There could be max of hba->nutrs reqs in flight and in worst case
+	 * if the reqs get finished 1 by 1 after the interrupt status is
+	 * read, make sure we handle them by checking the interrupt status
+	 * again in a loop until we process all of the reqs before returning.
+	 */
+	do {
+		enabled_intr_status =
+			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
+		if (intr_status)
+			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
+		if (enabled_intr_status) {
+			ufshcd_sl_intr(hba, enabled_intr_status);
+			retval = IRQ_HANDLED;
+		}
+
+		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
+	} while (intr_status && --retries);
 
-	if (enabled_intr_status) {
-		ufshcd_sl_intr(hba, enabled_intr_status);
-		retval = IRQ_HANDLED;
-	}
 	spin_unlock(hba->host->host_lock);
 	return retval;
 }
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 09/10] scsi: ufs: fix irq return code

[Asutosh Das]

From: Venkat Gopalakrishnan <venkatg@codeaurora.org>

Return IRQ_HANDLED only if the irq is really handled, this will
help in catching spurious interrupts that go unhandled.

Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 137 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 101 insertions(+), 36 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 09b7a3f..557d538 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -211,7 +211,7 @@ enum {
 	END_FIX
 };
 
-static void ufshcd_tmc_handler(struct ufs_hba *hba);
+static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
 static void ufshcd_async_scan(void *data, async_cookie_t cookie);
 static int ufshcd_reset_and_restore(struct ufs_hba *hba);
 static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
@@ -4609,19 +4609,29 @@ static int ufshcd_task_req_compl(struct ufs_hba *hba, u32 index, u8 *resp)
  * ufshcd_uic_cmd_compl - handle completion of uic command
  * @hba: per adapter instance
  * @intr_status: interrupt status generated by the controller
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
+static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
 {
+	irqreturn_t retval = IRQ_NONE;
+
 	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
 		hba->active_uic_cmd->argument2 |=
 			ufshcd_get_uic_cmd_result(hba);
 		hba->active_uic_cmd->argument3 =
 			ufshcd_get_dme_attr_val(hba);
 		complete(&hba->active_uic_cmd->done);
+		retval = IRQ_HANDLED;
 	}
 
-	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
+	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
 		complete(hba->uic_async_done);
+		retval = IRQ_HANDLED;
+	}
+	return retval;
 }
 
 /**
@@ -4675,8 +4685,12 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
 /**
  * ufshcd_transfer_req_compl - handle SCSI and query command completion
  * @hba: per adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
+static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 {
 	unsigned long completed_reqs;
 	u32 tr_doorbell;
@@ -4694,7 +4708,12 @@ static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
 	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
 
-	__ufshcd_transfer_req_compl(hba, completed_reqs);
+	if (completed_reqs) {
+		__ufshcd_transfer_req_compl(hba, completed_reqs);
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
@@ -5220,16 +5239,21 @@ static void ufshcd_update_uic_reg_hist(struct ufs_uic_err_reg_hist *reg_hist,
 /**
  * ufshcd_update_uic_error - check and set fatal UIC error flags.
  * @hba: per-adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_update_uic_error(struct ufs_hba *hba)
+static irqreturn_t ufshcd_update_uic_error(struct ufs_hba *hba)
 {
 	u32 reg;
+	irqreturn_t retval = IRQ_NONE;
 
 	/* PHY layer lane error */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER);
 	/* Ignore LINERESET indication, as this is not an error */
 	if ((reg & UIC_PHY_ADAPTER_LAYER_ERROR) &&
-			(reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
+			(reg & UIC_PHY_ADAPTER_LAYER_ERROR_CODE_MASK)) {
 		/*
 		 * To know whether this error is fatal or not, DB timeout
 		 * must be checked but this error is handled separately.
@@ -5240,57 +5264,73 @@ static void ufshcd_update_uic_error(struct ufs_hba *hba)
 
 	/* PA_INIT_ERROR is fatal and needs UIC reset */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
-	if (reg)
+	if ((reg & UIC_DATA_LINK_LAYER_ERROR) &&
+	    (reg & UIC_DATA_LINK_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_uic_reg_hist(&hba->ufs_stats.dl_err, reg);
-
-	if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
-		hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
-	else if (hba->dev_quirks &
-		   UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
-		if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
-			hba->uic_error |=
-				UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
-		else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
-			hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
+		if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT) {
+			hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
+		} else if (hba->dev_quirks &
+			   UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
+			if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
+				hba->uic_error |=
+					UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
+			else if (reg &
+				UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
+				hba->uic_error |=
+					UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
+		}
+		retval |= IRQ_HANDLED;
 	}
 
 	/* UIC NL/TL/DME errors needs software retry */
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
-	if (reg) {
+	if ((reg & UIC_NETWORK_LAYER_ERROR) &&
+	    (reg & UIC_NETWORK_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_uic_reg_hist(&hba->ufs_stats.nl_err, reg);
 		hba->uic_error |= UFSHCD_UIC_NL_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
-	if (reg) {
+	if ((reg & UIC_TRANSPORT_LAYER_ERROR) &&
+	    (reg & UIC_TRANSPORT_LAYER_ERROR_CODE_MASK)) {
 		ufshcd_update_uic_reg_hist(&hba->ufs_stats.tl_err, reg);
 		hba->uic_error |= UFSHCD_UIC_TL_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
-	if (reg) {
+	if ((reg & UIC_DME_ERROR) &&
+	    (reg & UIC_DME_ERROR_CODE_MASK)) {
 		ufshcd_update_uic_reg_hist(&hba->ufs_stats.dme_err, reg);
 		hba->uic_error |= UFSHCD_UIC_DME_ERROR;
+		retval |= IRQ_HANDLED;
 	}
 
 	dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
 			__func__, hba->uic_error);
+	return retval;
 }
 
 /**
  * ufshcd_check_errors - Check for errors that need s/w attention
  * @hba: per-adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_check_errors(struct ufs_hba *hba)
+static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
 {
 	bool queue_eh_work = false;
+	irqreturn_t retval = IRQ_NONE;
 
 	if (hba->errors & INT_FATAL_ERRORS)
 		queue_eh_work = true;
 
 	if (hba->errors & UIC_ERROR) {
 		hba->uic_error = 0;
-		ufshcd_update_uic_error(hba);
+		retval = ufshcd_update_uic_error(hba);
 		if (hba->uic_error)
 			queue_eh_work = true;
 	}
@@ -5327,6 +5367,7 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 			}
 			schedule_work(&hba->eh_work);
 		}
+		retval |= IRQ_HANDLED;
 	}
 	/*
 	 * if (!queue_eh_work) -
@@ -5334,40 +5375,58 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
 	 * itself without s/w intervention or errors that will be
 	 * handled by the SCSI core layer.
 	 */
+	return retval;
 }
 
 /**
  * ufshcd_tmc_handler - handle task management function completion
  * @hba: per adapter instance
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_tmc_handler(struct ufs_hba *hba)
+static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
 {
 	u32 tm_doorbell;
 
 	tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
 	hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
-	wake_up(&hba->tm_wq);
+	if (hba->tm_condition) {
+		wake_up(&hba->tm_wq);
+		return IRQ_HANDLED;
+	} else {
+		return IRQ_NONE;
+	}
 }
 
 /**
  * ufshcd_sl_intr - Interrupt service routine
  * @hba: per adapter instance
  * @intr_status: contains interrupts generated by the controller
+ *
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
-static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
+static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
 {
+	irqreturn_t retval = IRQ_NONE;
+
 	hba->errors = UFSHCD_ERROR_MASK & intr_status;
 	if (hba->errors)
-		ufshcd_check_errors(hba);
+		retval |= ufshcd_check_errors(hba);
 
 	if (intr_status & UFSHCD_UIC_MASK)
-		ufshcd_uic_cmd_compl(hba, intr_status);
+		retval |= ufshcd_uic_cmd_compl(hba, intr_status);
 
 	if (intr_status & UTP_TASK_REQ_COMPL)
-		ufshcd_tmc_handler(hba);
+		retval |= ufshcd_tmc_handler(hba);
 
 	if (intr_status & UTP_TRANSFER_REQ_COMPL)
-		ufshcd_transfer_req_compl(hba);
+		retval |= ufshcd_transfer_req_compl(hba);
+
+	return retval;
 }
 
 /**
@@ -5375,8 +5434,9 @@ static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
  * @irq: irq number
  * @__hba: pointer to adapter instance
  *
- * Returns IRQ_HANDLED - If interrupt is valid
- *		IRQ_NONE - If invalid interrupt
+ * Returns
+ *  IRQ_HANDLED - If interrupt is valid
+ *  IRQ_NONE    - If invalid interrupt
  */
 static irqreturn_t ufshcd_intr(int irq, void *__hba)
 {
@@ -5399,14 +5459,19 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
 			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 		if (intr_status)
 			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
-		if (enabled_intr_status) {
-			ufshcd_sl_intr(hba, enabled_intr_status);
-			retval = IRQ_HANDLED;
-		}
+		if (enabled_intr_status)
+			retval |= ufshcd_sl_intr(hba, enabled_intr_status);
 
 		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
 	} while (intr_status && --retries);
 
+	if (retval == IRQ_NONE) {
+		dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x\n",
+			__func__, intr_status);
+		ufshcd_hex_dump("host regs: ", hba->mmio_base,
+				UFSHCI_REG_SPACE_SIZE);
+	}
+
 	spin_unlock(hba->host->host_lock);
 	return retval;
 }
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 10/10] scsi: ufs: Add clock ungating to a separate workqueue

[Asutosh Das]

From: Vijay Viswanath <vviswana@codeaurora.org>

UFS driver can receive a request during memory reclaim by kswapd.
So when ufs driver puts the ungate work in queue, and if there are no
idle workers, kthreadd is invoked to create a new kworker. Since
kswapd task holds a mutex which kthreadd also needs, this can cause
a deadlock situation. So ungate work must be done in a separate
work queue with WQ_MEM_RECLAIM flag enabled.
Such a workqueue will have a rescue thread which will be called
when the above deadlock condition is possible.

Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
Signed-off-by: Can Guo <cang@codeaurora.org>
Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
 drivers/scsi/ufs/ufshcd.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 557d538..3be61b7 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1483,7 +1483,8 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
 		hba->clk_gating.state = REQ_CLKS_ON;
 		trace_ufshcd_clk_gating(dev_name(hba->dev),
 					hba->clk_gating.state);
-		schedule_work(&hba->clk_gating.ungate_work);
+		queue_work(hba->clk_gating.clk_gating_workq,
+			   &hba->clk_gating.ungate_work);
 		/*
 		 * fall through to check if we should wait for this
 		 * work to be done or not.
@@ -1669,6 +1670,8 @@ static ssize_t ufshcd_clkgate_enable_store(struct device *dev,
 
 static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 {
+	char wq_name[sizeof("ufs_clk_gating_00")];
+
 	if (!ufshcd_is_clkgating_allowed(hba))
 		return;
 
@@ -1676,6 +1679,11 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 	INIT_DELAYED_WORK(&hba->clk_gating.gate_work, ufshcd_gate_work);
 	INIT_WORK(&hba->clk_gating.ungate_work, ufshcd_ungate_work);
 
+	snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
+		 hba->host->host_no);
+	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
+							   WQ_MEM_RECLAIM);
+
 	hba->clk_gating.is_enabled = true;
 
 	hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
@@ -1703,6 +1711,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 	device_remove_file(hba->dev, &hba->clk_gating.enable_attr);
 	cancel_work_sync(&hba->clk_gating.ungate_work);
 	cancel_delayed_work_sync(&hba->clk_gating.gate_work);
+	destroy_workqueue(hba->clk_gating.clk_gating_workq);
 }
 
 /* Must be called with host lock acquired */
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 76c31d5..2e6bdc0 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -361,6 +361,7 @@ struct ufs_clk_gating {
 	struct device_attribute enable_attr;
 	bool is_enabled;
 	int active_reqs;
+	struct workqueue_struct *clk_gating_workq;
 };
 
 struct ufs_saved_pwr_info {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 01/10] scsi: ufs: Allowing power mode change

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Yaniv Gardi <ygardi@codeaurora.org>
> 
> Due to M-PHY issues, moving from HS to any other mode or gear or
> even Hibern8 may cause some un-predicted behavior
> of the device.
> This patch adds provides a quirk to address that.
> 
> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org>
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 +++++++-
>  drivers/scsi/ufs/ufshcd.h | 7 +++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 5bc9dc1..f3083fe 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4162,9 +4162,15 @@ static int ufshcd_link_startup(struct ufs_hba 
> *hba)
>  			goto out;
>  	} while (ret && retries--);
> 
> -	if (ret)
> +	if (ret) {
>  		/* failed to get the link up... retire */
>  		goto out;
> +	}
> +
> +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE) {
> +		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 0);
> +		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
> +	}
> 
>  	if (link_startup_again) {
>  		link_startup_again = false;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index cbe46f6..bb4ecfb 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -591,6 +591,13 @@ struct ufs_hba {
>  	 */
>  	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			UFS_BIT(7)
> 
> +	/*
> +	 * Needs to be enabled if moving from HS to any other gear/mode or 
> even
> +	 * hibern8 causes unpredicted behavior of device.
> +	 * If this quirk is enabled, standard UFS driver will disable/enable
> +	 * TX_LCC.
> +	 */
> +	#define UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE		UFS_BIT(8)
>  	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
> 
>  	/* Device deviations from standard UFS device spec. */

This was probably needed in early generation of UFS devices (non 
commercial) and may not be really needed now. I believe we can skip this 
patch.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 02/10] scsi: ufs-qcom: Enable UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE quirk

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> Enable UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE quirk to avoid failures
> in seen on some UFS devices.
> 
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 4563d2e..d9edef8 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1105,7 +1105,7 @@ static void ufs_qcom_advertise_quirks(struct 
> ufs_hba *hba)
> 
>  	if (host->hw_ver.major >= 0x2) {
>  		hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
> -
> +		hba->quirks |= UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE;
>  		if (!ufs_qcom_cap_qunipro(host))
>  			/* Legacy UniPro mode still need following quirks */
>  			hba->quirks |= (UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS

We may not need this.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 03/10] scsi: ufs: Add LCC quirk for host and device

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> LCC (Line Control Command) is being used for communication between
> UFS host and UFS device. But some hosts might have the issue with
> issuing the LCC commands to UFS device and in this case LCC could be
> explicitly disabled.
> 
> But there could be a need where we don't want to disable the LCC
> on both host & device; hence this change splits the quirk in 2 parts
> one for host and one for device.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++
>  drivers/scsi/ufs/ufshcd.h | 12 ++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index f3083fe..6dabce8 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4116,6 +4116,11 @@ static int ufshcd_disable_tx_lcc(struct ufs_hba
> *hba, bool peer)
>  	return err;
>  }
> 
> +static inline int ufshcd_disable_host_tx_lcc(struct ufs_hba *hba)
> +{
> +	return ufshcd_disable_tx_lcc(hba, false);
> +}
> +
>  static inline int ufshcd_disable_device_tx_lcc(struct ufs_hba *hba)
>  {
>  	return ufshcd_disable_tx_lcc(hba, true);
> @@ -4172,6 +4177,17 @@ static int ufshcd_link_startup(struct ufs_hba 
> *hba)
>  		ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
>  	}
> 
> +	if (hba->quirks & UFSHCD_BROKEN_LCC_PROCESSING_ON_HOST) {
> +		ret = ufshcd_disable_device_tx_lcc(hba);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	if (hba->quirks & UFSHCD_BROKEN_LCC_PROCESSING_ON_DEVICE) {
> +		ret = ufshcd_disable_host_tx_lcc(hba);
> +		if (ret)
> +			goto out;
> +	}
>  	if (link_startup_again) {
>  		link_startup_again = false;
>  		retries = DME_LINKSTARTUP_RETRIES;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index bb4ecfb..0417c42 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -598,6 +598,18 @@ struct ufs_hba {
>  	 * TX_LCC.
>  	 */
>  	#define UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE		UFS_BIT(8)
> +
> +	/*
> +	 * If UFS device is having issue in processing LCC (Line Control
> +	 * Command) coming from UFS host controller then enable this quirk.
> +	 * When this quirk is enabled, host controller driver should disable
> +	 * the LCC transmission on UFS host controller (by clearing
> +	 * TX_LCC_ENABLE attribute of host to 0).
> +	 */
> +	#define UFSHCD_BROKEN_LCC_PROCESSING_ON_DEVICE		0x100
> +
> +	#define UFSHCD_BROKEN_LCC_PROCESSING_ON_HOST		0x200
> +
>  	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
> 
>  	/* Device deviations from standard UFS device spec. */

Please check if these quirks are really needed for commercial version of 
UFS host controllers.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 04/10] scsi: ufs: fix exception event handling

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Maya Erez <merez@codeaurora.org>
> 
> The device can set the exception event bit in one of the response UPIU,
> for example to notify the need for urgent BKOPs operation.
> In such a case the host driver calls ufshcd_exception_event_handler to
> handle this notification.
> When trying to check the exception event status (for finding the cause 
> for
> the exception event), the device may be busy with additional SCSI 
> commands
> handling and may not respond within the 100ms timeout.
> 
> To prevent that, we need to block SCSI commands during handling of
> exception events and allow retransmissions of the query requests,
> in case of timeout.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Maya Erez <merez@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 6dabce8..838ba8f0 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4967,6 +4967,7 @@ static void
> ufshcd_exception_event_handler(struct work_struct *work)
>  	hba = container_of(work, struct ufs_hba, eeh_work);
> 
>  	pm_runtime_get_sync(hba->dev);
> +	scsi_block_requests(hba->host);
>  	err = ufshcd_get_ee_status(hba, &status);
>  	if (err) {
>  		dev_err(hba->dev, "%s: failed to get exception status %d\n",
> @@ -4980,6 +4981,7 @@ static void
> ufshcd_exception_event_handler(struct work_struct *work)
>  		ufshcd_bkops_exception_event_handler(hba);
> 
>  out:
> +	scsi_unblock_requests(hba->host);
>  	pm_runtime_put_sync(hba->dev);
>  	return;
>  }

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 05/10] scsi: ufshcd: fix possible unclocked register access

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> vendor specific setup_clocks ops may depend on clocks managed by ufshcd
> driver so if the vendor specific setup_clocks callback is called when
> the required clocks are turned off, it results into unclocked register
> access.
> 
> This change make sure that required clocks are enabled before vendor
> specific setup_clocks callback is called.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 838ba8f0..dfeb194 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6780,9 +6780,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  	if (list_empty(head))
>  		goto out;
> 
> -	ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * vendor specific setup_clocks ops may depend on clocks managed by
> +	 * this standard driver hence call the vendor specific setup_clocks
> +	 * before disabling the clocks managed here.
> +	 */
> +	if (!on) {
> +		ret = ufshcd_vops_setup_clocks(hba, on, PRE_CHANGE);
> +		if (ret)
> +			return ret;
> +	}
> 
>  	list_for_each_entry(clki, head, list) {
>  		if (!IS_ERR_OR_NULL(clki->clk)) {
> @@ -6806,9 +6813,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  		}
>  	}
> 
> -	ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
> -	if (ret)
> -		return ret;
> +	/*
> +	 * vendor specific setup_clocks ops may depend on clocks managed by
> +	 * this standard driver hence call the vendor specific setup_clocks
> +	 * after enabling the clocks managed here.
> +	 */
> +	if (on) {
> +		ret = ufshcd_vops_setup_clocks(hba, on, POST_CHANGE);
> +		if (ret)
> +			return ret;
> +	}
> 
>  out:
>  	if (ret) {

Looks good to me.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 06/10] scsi: ufs: add reference counting for scsi block requests

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> Currently we call the scsi_block_requests()/scsi_unblock_requests()
> whenever we want to block/unblock scsi requests but as there is no
> reference counting, nesting of these calls could leave us in undesired
> state sometime. Consider following call flow sequence:
> 1. func1() calls scsi_block_requests() but calls func2() before
>    calling scsi_unblock_requests()
> 2. func2() calls scsi_block_requests()
> 3. func2() calls scsi_unblock_requests()
> 4. func1() calls scsi_unblock_requests()
> 
> As there is no reference counting, we will have scsi requests unblocked
> after #3 instead of it to be unblocked only after #4. Though we may not
> have failures seen with this, we might run into some failures in 
> future.
> Better solution would be to fix this by adding reference counting.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 28 ++++++++++++++++++++--------
>  drivers/scsi/ufs/ufshcd.h |  2 ++
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index dfeb194..c35a076 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -264,6 +264,18 @@ static inline void ufshcd_disable_irq(struct 
> ufs_hba *hba)
>  	}
>  }
> 
> +static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
> +{
> +	if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
> +		scsi_unblock_requests(hba->host);
> +}
> +
> +static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
> +{
> +	if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
> +		scsi_block_requests(hba->host);
> +}
> +
>  /* replace non-printable or non-ASCII characters with spaces */
>  static inline void ufshcd_remove_non_printable(char *val)
>  {
> @@ -1077,12 +1089,12 @@ static int ufshcd_clock_scaling_prepare(struct
> ufs_hba *hba)
>  	 * make sure that there are no outstanding requests when
>  	 * clock scaling is in progress
>  	 */
> -	scsi_block_requests(hba->host);
> +	ufshcd_scsi_block_requests(hba);
>  	down_write(&hba->clk_scaling_lock);
>  	if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
>  		ret = -EBUSY;
>  		up_write(&hba->clk_scaling_lock);
> -		scsi_unblock_requests(hba->host);
> +		ufshcd_scsi_unblock_requests(hba);
>  	}
> 
>  	return ret;
> @@ -1091,7 +1103,7 @@ static int ufshcd_clock_scaling_prepare(struct
> ufs_hba *hba)
>  static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
>  {
>  	up_write(&hba->clk_scaling_lock);
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  }
> 
>  /**
> @@ -1411,7 +1423,7 @@ static void ufshcd_ungate_work(struct work_struct 
> *work)
>  		hba->clk_gating.is_suspended = false;
>  	}
>  unblock_reqs:
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  }
> 
>  /**
> @@ -1467,7 +1479,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>  		 * work and to enable clocks.
>  		 */
>  	case CLKS_OFF:
> -		scsi_block_requests(hba->host);
> +		ufshcd_scsi_block_requests(hba);
>  		hba->clk_gating.state = REQ_CLKS_ON;
>  		trace_ufshcd_clk_gating(dev_name(hba->dev),
>  					hba->clk_gating.state);
> @@ -5192,7 +5204,7 @@ static void ufshcd_err_handler(struct work_struct 
> *work)
> 
>  out:
>  	spin_unlock_irqrestore(hba->host->host_lock, flags);
> -	scsi_unblock_requests(hba->host);
> +	ufshcd_scsi_unblock_requests(hba);
>  	ufshcd_release(hba);
>  	pm_runtime_put_sync(hba->dev);
>  }
> @@ -5294,7 +5306,7 @@ static void ufshcd_check_errors(struct ufs_hba 
> *hba)
>  		/* handle fatal errors only when link is functional */
>  		if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
>  			/* block commands from scsi mid-layer */
> -			scsi_block_requests(hba->host);
> +			ufshcd_scsi_block_requests(hba);
> 
>  			hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED;
> 
> @@ -8017,7 +8029,7 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
> 
>  	/* Hold auto suspend until async scan completes */
>  	pm_runtime_get_sync(dev);
> -
> +	atomic_set(&hba->scsi_block_reqs_cnt, 0);
>  	/*
>  	 * We are assuming that device wasn't put in sleep/power-down
>  	 * state exclusively during the boot stage before kernel.
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 0417c42..76c31d5 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -498,6 +498,7 @@ struct ufs_stats {
>   * @urgent_bkops_lvl: keeps track of urgent bkops level for device
>   * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level 
> for
>   *  device is known or not.
> + * @scsi_block_reqs_cnt: reference counting for scsi block requests
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> @@ -698,6 +699,7 @@ struct ufs_hba {
> 
>  	struct rw_semaphore clk_scaling_lock;
>  	struct ufs_desc_size desc_size;
> +	atomic_t scsi_block_reqs_cnt;
>  };
> 
>  /* Returns true if clocks can be gated. Otherwise false */

Looks good to me.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 07/10] scsi: ufs-qcom: remove broken hci version quirk

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Subhash Jadavani <subhashj@codeaurora.org>
> 
> UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION is only applicable for QCOM UFS 
> host
> controller version 2.x.y and this has been fixed from version 3.x.y
> onwards, hence this change removes this quirk for version 3.x.y 
> onwards.
> 
> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index d9edef8..27be327 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -1103,7 +1103,7 @@ static void ufs_qcom_advertise_quirks(struct 
> ufs_hba *hba)
>  		hba->quirks |= UFSHCD_QUIRK_BROKEN_LCC;
>  	}
> 
> -	if (host->hw_ver.major >= 0x2) {
> +	if (host->hw_ver.major == 0x2) {
>  		hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION;
>  		hba->quirks |= UFSHCD_QUIRK_BROKEN_PWR_MODE_CHANGE;
>  		if (!ufs_qcom_cap_qunipro(host))

Looks good to me.

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 08/10] scsi: ufs: make sure all interrupts are processed

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> 
> As multiple requests are submitted to the ufs host controller in
> parallel there could be instances where the command completion
> interrupt arrives later for a request that is already processed
> earlier as the corresponding doorbell was cleared when handling
> the previous interrupt. Read the interrupt status in a loop after
> processing the received interrupt to catch such interrupts and
> handle it.
> 
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c35a076..09b7a3f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5383,19 +5383,30 @@ static irqreturn_t ufshcd_intr(int irq, void 
> *__hba)
>  	u32 intr_status, enabled_intr_status;
>  	irqreturn_t retval = IRQ_NONE;
>  	struct ufs_hba *hba = __hba;
> +	int retries = hba->nutrs;
> 
>  	spin_lock(hba->host->host_lock);
>  	intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> -	enabled_intr_status =
> -		intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> 
> -	if (intr_status)
> -		ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> +	/*
> +	 * There could be max of hba->nutrs reqs in flight and in worst case
> +	 * if the reqs get finished 1 by 1 after the interrupt status is
> +	 * read, make sure we handle them by checking the interrupt status
> +	 * again in a loop until we process all of the reqs before returning.
> +	 */
> +	do {
> +		enabled_intr_status =
> +			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> +		if (intr_status)
> +			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> +		if (enabled_intr_status) {
> +			ufshcd_sl_intr(hba, enabled_intr_status);
> +			retval = IRQ_HANDLED;
> +		}
> +
> +		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> +	} while (intr_status && --retries);
> 
> -	if (enabled_intr_status) {
> -		ufshcd_sl_intr(hba, enabled_intr_status);
> -		retval = IRQ_HANDLED;
> -	}
>  	spin_unlock(hba->host->host_lock);
>  	return retval;
>  }

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 10/10] scsi: ufs: Add clock ungating to a separate workqueue

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Vijay Viswanath <vviswana@codeaurora.org>
> 
> UFS driver can receive a request during memory reclaim by kswapd.
> So when ufs driver puts the ungate work in queue, and if there are no
> idle workers, kthreadd is invoked to create a new kworker. Since
> kswapd task holds a mutex which kthreadd also needs, this can cause
> a deadlock situation. So ungate work must be done in a separate
> work queue with WQ_MEM_RECLAIM flag enabled.
> Such a workqueue will have a rescue thread which will be called
> when the above deadlock condition is possible.
> 
> Signed-off-by: Vijay Viswanath <vviswana@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 11 ++++++++++-
>  drivers/scsi/ufs/ufshcd.h |  1 +
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 557d538..3be61b7 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1483,7 +1483,8 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
>  		hba->clk_gating.state = REQ_CLKS_ON;
>  		trace_ufshcd_clk_gating(dev_name(hba->dev),
>  					hba->clk_gating.state);
> -		schedule_work(&hba->clk_gating.ungate_work);
> +		queue_work(hba->clk_gating.clk_gating_workq,
> +			   &hba->clk_gating.ungate_work);
>  		/*
>  		 * fall through to check if we should wait for this
>  		 * work to be done or not.
> @@ -1669,6 +1670,8 @@ static ssize_t
> ufshcd_clkgate_enable_store(struct device *dev,
> 
>  static void ufshcd_init_clk_gating(struct ufs_hba *hba)
>  {
> +	char wq_name[sizeof("ufs_clk_gating_00")];
> +
>  	if (!ufshcd_is_clkgating_allowed(hba))
>  		return;
> 
> @@ -1676,6 +1679,11 @@ static void ufshcd_init_clk_gating(struct 
> ufs_hba *hba)
>  	INIT_DELAYED_WORK(&hba->clk_gating.gate_work, ufshcd_gate_work);
>  	INIT_WORK(&hba->clk_gating.ungate_work, ufshcd_ungate_work);
> 
> +	snprintf(wq_name, ARRAY_SIZE(wq_name), "ufs_clk_gating_%d",
> +		 hba->host->host_no);
> +	hba->clk_gating.clk_gating_workq = alloc_ordered_workqueue(wq_name,
> +							   WQ_MEM_RECLAIM);
> +
>  	hba->clk_gating.is_enabled = true;
> 
>  	hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
> @@ -1703,6 +1711,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba 
> *hba)
>  	device_remove_file(hba->dev, &hba->clk_gating.enable_attr);
>  	cancel_work_sync(&hba->clk_gating.ungate_work);
>  	cancel_delayed_work_sync(&hba->clk_gating.gate_work);
> +	destroy_workqueue(hba->clk_gating.clk_gating_workq);
>  }
> 
>  /* Must be called with host lock acquired */
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 76c31d5..2e6bdc0 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -361,6 +361,7 @@ struct ufs_clk_gating {
>  	struct device_attribute enable_attr;
>  	bool is_enabled;
>  	int active_reqs;
> +	struct workqueue_struct *clk_gating_workq;
>  };
> 
>  struct ufs_saved_pwr_info {

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 09/10] scsi: ufs: fix irq return code

[Subhash Jadavani]

On 2018-05-03 04:07, Asutosh Das wrote:
> From: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> 
> Return IRQ_HANDLED only if the irq is really handled, this will
> help in catching spurious interrupts that go unhandled.
> 
> Signed-off-by: Venkat Gopalakrishnan <venkatg@codeaurora.org>
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Signed-off-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 137 
> ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 101 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 09b7a3f..557d538 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -211,7 +211,7 @@ enum {
>  	END_FIX
>  };
> 
> -static void ufshcd_tmc_handler(struct ufs_hba *hba);
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba);
>  static void ufshcd_async_scan(void *data, async_cookie_t cookie);
>  static int ufshcd_reset_and_restore(struct ufs_hba *hba);
>  static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
> @@ -4609,19 +4609,29 @@ static int ufshcd_task_req_compl(struct
> ufs_hba *hba, u32 index, u8 *resp)
>   * ufshcd_uic_cmd_compl - handle completion of uic command
>   * @hba: per adapter instance
>   * @intr_status: interrupt status generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_uic_cmd_compl(struct ufs_hba *hba, u32 
> intr_status)
>  {
> +	irqreturn_t retval = IRQ_NONE;
> +
>  	if ((intr_status & UIC_COMMAND_COMPL) && hba->active_uic_cmd) {
>  		hba->active_uic_cmd->argument2 |=
>  			ufshcd_get_uic_cmd_result(hba);
>  		hba->active_uic_cmd->argument3 =
>  			ufshcd_get_dme_attr_val(hba);
>  		complete(&hba->active_uic_cmd->done);
> +		retval = IRQ_HANDLED;
>  	}
> 
> -	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done)
> +	if ((intr_status & UFSHCD_UIC_PWR_MASK) && hba->uic_async_done) {
>  		complete(hba->uic_async_done);
> +		retval = IRQ_HANDLED;
> +	}
> +	return retval;
>  }
> 
>  /**
> @@ -4675,8 +4685,12 @@ static void __ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  /**
>   * ufshcd_transfer_req_compl - handle SCSI and query command 
> completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_transfer_req_compl(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>  {
>  	unsigned long completed_reqs;
>  	u32 tr_doorbell;
> @@ -4694,7 +4708,12 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>  	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
>  	completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
> 
> -	__ufshcd_transfer_req_compl(hba, completed_reqs);
> +	if (completed_reqs) {
> +		__ufshcd_transfer_req_compl(hba, completed_reqs);
> +		return IRQ_HANDLED;
> +	} else {
> +		return IRQ_NONE;
> +	}
>  }
> 
>  /**
> @@ -5220,16 +5239,21 @@ static void ufshcd_update_uic_reg_hist(struct
> ufs_uic_err_reg_hist *reg_hist,
>  /**
>   * ufshcd_update_uic_error - check and set fatal UIC error flags.
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_update_uic_error(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_update_uic_error(struct ufs_hba *hba)
>  {
>  	u32 reg;
> +	irqreturn_t retval = IRQ_NONE;
> 
>  	/* PHY layer lane error */
>  	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_PHY_ADAPTER_LAYER);
>  	/* Ignore LINERESET indication, as this is not an error */
>  	if ((reg & UIC_PHY_ADAPTER_LAYER_ERROR) &&
> -			(reg & UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK)) {
> +			(reg & UIC_PHY_ADAPTER_LAYER_ERROR_CODE_MASK)) {
>  		/*
>  		 * To know whether this error is fatal or not, DB timeout
>  		 * must be checked but this error is handled separately.
> @@ -5240,57 +5264,73 @@ static void ufshcd_update_uic_error(struct 
> ufs_hba *hba)
> 
>  	/* PA_INIT_ERROR is fatal and needs UIC reset */
>  	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DATA_LINK_LAYER);
> -	if (reg)
> +	if ((reg & UIC_DATA_LINK_LAYER_ERROR) &&
> +	    (reg & UIC_DATA_LINK_LAYER_ERROR_CODE_MASK)) {
>  		ufshcd_update_uic_reg_hist(&hba->ufs_stats.dl_err, reg);
> -
> -	if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT)
> -		hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> -	else if (hba->dev_quirks &
> -		   UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
> -		if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> -			hba->uic_error |=
> -				UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> -		else if (reg & UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
> -			hba->uic_error |= UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
> +		if (reg & UIC_DATA_LINK_LAYER_ERROR_PA_INIT) {
> +			hba->uic_error |= UFSHCD_UIC_DL_PA_INIT_ERROR;
> +		} else if (hba->dev_quirks &
> +			   UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS) {
> +			if (reg & UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED)
> +				hba->uic_error |=
> +					UFSHCD_UIC_DL_NAC_RECEIVED_ERROR;
> +			else if (reg &
> +				UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT)
> +				hba->uic_error |=
> +					UFSHCD_UIC_DL_TCx_REPLAY_ERROR;
> +		}
> +		retval |= IRQ_HANDLED;
>  	}
> 
>  	/* UIC NL/TL/DME errors needs software retry */
>  	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_NETWORK_LAYER);
> -	if (reg) {
> +	if ((reg & UIC_NETWORK_LAYER_ERROR) &&
> +	    (reg & UIC_NETWORK_LAYER_ERROR_CODE_MASK)) {
>  		ufshcd_update_uic_reg_hist(&hba->ufs_stats.nl_err, reg);
>  		hba->uic_error |= UFSHCD_UIC_NL_ERROR;
> +		retval |= IRQ_HANDLED;
>  	}
> 
>  	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_TRANSPORT_LAYER);
> -	if (reg) {
> +	if ((reg & UIC_TRANSPORT_LAYER_ERROR) &&
> +	    (reg & UIC_TRANSPORT_LAYER_ERROR_CODE_MASK)) {
>  		ufshcd_update_uic_reg_hist(&hba->ufs_stats.tl_err, reg);
>  		hba->uic_error |= UFSHCD_UIC_TL_ERROR;
> +		retval |= IRQ_HANDLED;
>  	}
> 
>  	reg = ufshcd_readl(hba, REG_UIC_ERROR_CODE_DME);
> -	if (reg) {
> +	if ((reg & UIC_DME_ERROR) &&
> +	    (reg & UIC_DME_ERROR_CODE_MASK)) {
>  		ufshcd_update_uic_reg_hist(&hba->ufs_stats.dme_err, reg);
>  		hba->uic_error |= UFSHCD_UIC_DME_ERROR;
> +		retval |= IRQ_HANDLED;
>  	}
> 
>  	dev_dbg(hba->dev, "%s: UIC error flags = 0x%08x\n",
>  			__func__, hba->uic_error);
> +	return retval;
>  }
> 
>  /**
>   * ufshcd_check_errors - Check for errors that need s/w attention
>   * @hba: per-adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_check_errors(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_check_errors(struct ufs_hba *hba)
>  {
>  	bool queue_eh_work = false;
> +	irqreturn_t retval = IRQ_NONE;
> 
>  	if (hba->errors & INT_FATAL_ERRORS)
>  		queue_eh_work = true;
> 
>  	if (hba->errors & UIC_ERROR) {
>  		hba->uic_error = 0;
> -		ufshcd_update_uic_error(hba);
> +		retval = ufshcd_update_uic_error(hba);
>  		if (hba->uic_error)
>  			queue_eh_work = true;
>  	}
> @@ -5327,6 +5367,7 @@ static void ufshcd_check_errors(struct ufs_hba 
> *hba)
>  			}
>  			schedule_work(&hba->eh_work);
>  		}
> +		retval |= IRQ_HANDLED;
>  	}
>  	/*
>  	 * if (!queue_eh_work) -
> @@ -5334,40 +5375,58 @@ static void ufshcd_check_errors(struct ufs_hba 
> *hba)
>  	 * itself without s/w intervention or errors that will be
>  	 * handled by the SCSI core layer.
>  	 */
> +	return retval;
>  }
> 
>  /**
>   * ufshcd_tmc_handler - handle task management function completion
>   * @hba: per adapter instance
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_tmc_handler(struct ufs_hba *hba)
> +static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
>  {
>  	u32 tm_doorbell;
> 
>  	tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
>  	hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
> -	wake_up(&hba->tm_wq);
> +	if (hba->tm_condition) {
> +		wake_up(&hba->tm_wq);
> +		return IRQ_HANDLED;
> +	} else {
> +		return IRQ_NONE;
> +	}
>  }
> 
>  /**
>   * ufshcd_sl_intr - Interrupt service routine
>   * @hba: per adapter instance
>   * @intr_status: contains interrupts generated by the controller
> + *
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
> -static void ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status)
> +static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 
> intr_status)
>  {
> +	irqreturn_t retval = IRQ_NONE;
> +
>  	hba->errors = UFSHCD_ERROR_MASK & intr_status;
>  	if (hba->errors)
> -		ufshcd_check_errors(hba);
> +		retval |= ufshcd_check_errors(hba);
> 
>  	if (intr_status & UFSHCD_UIC_MASK)
> -		ufshcd_uic_cmd_compl(hba, intr_status);
> +		retval |= ufshcd_uic_cmd_compl(hba, intr_status);
> 
>  	if (intr_status & UTP_TASK_REQ_COMPL)
> -		ufshcd_tmc_handler(hba);
> +		retval |= ufshcd_tmc_handler(hba);
> 
>  	if (intr_status & UTP_TRANSFER_REQ_COMPL)
> -		ufshcd_transfer_req_compl(hba);
> +		retval |= ufshcd_transfer_req_compl(hba);
> +
> +	return retval;
>  }
> 
>  /**
> @@ -5375,8 +5434,9 @@ static void ufshcd_sl_intr(struct ufs_hba *hba,
> u32 intr_status)
>   * @irq: irq number
>   * @__hba: pointer to adapter instance
>   *
> - * Returns IRQ_HANDLED - If interrupt is valid
> - *		IRQ_NONE - If invalid interrupt
> + * Returns
> + *  IRQ_HANDLED - If interrupt is valid
> + *  IRQ_NONE    - If invalid interrupt
>   */
>  static irqreturn_t ufshcd_intr(int irq, void *__hba)
>  {
> @@ -5399,14 +5459,19 @@ static irqreturn_t ufshcd_intr(int irq, void 
> *__hba)
>  			intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>  		if (intr_status)
>  			ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> -		if (enabled_intr_status) {
> -			ufshcd_sl_intr(hba, enabled_intr_status);
> -			retval = IRQ_HANDLED;
> -		}
> +		if (enabled_intr_status)
> +			retval |= ufshcd_sl_intr(hba, enabled_intr_status);
> 
>  		intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
>  	} while (intr_status && --retries);
> 
> +	if (retval == IRQ_NONE) {
> +		dev_err(hba->dev, "%s: Unhandled interrupt 0x%08x\n",
> +			__func__, intr_status);
> +		ufshcd_hex_dump("host regs: ", hba->mmio_base,
> +				UFSHCI_REG_SPACE_SIZE);
> +	}
> +
>  	spin_unlock(hba->host->host_lock);
>  	return retval;
>  }


Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project