Re: [PATCH 1/4] scsi: ufs: add quirk to fix mishandling utrlclr/utmrlclr

[Asutosh Das (asd)]

On 5/6/2018 3:44 PM, Alim Akhtar wrote:

In the right behavior, setting the bit to '0' indicates clear and
'1' indicates no change. If host controller handles this the other way,
UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR can be used.

Signed-off-by: Seungwon Jeon 
Signed-off-by: Alim Akhtar 
---
  drivers/scsi/ufs/ufshcd.c | 21 +++--
  drivers/scsi/ufs/ufshcd.h |  5 +
  2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e7905..9898ce5 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -675,7 +675,24 @@ static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, 
int slot)
   */
  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
  {
-   ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+   if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
+   ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+   else
+   ufshcd_writel(hba, ~(1 << pos),
+   REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+}
+
+/**
+ * ufshcd_utmrl_clear - Clear a bit in UTRMLCLR register
+ * @hba: per adapter instance
+ * @pos: position of the bit to be cleared
+ */
+static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
+{
+   if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
+   ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
+   else
+   ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
  }
  
  /**

@@ -5398,7 +5415,7 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int 
tag)
goto out;
  
  	spin_lock_irqsave(hba->host->host_lock, flags);

-   ufshcd_writel(hba, ~(1 << tag), REG_UTP_TASK_REQ_LIST_CLEAR);
+   ufshcd_utmrl_clear(hba, tag);
spin_unlock_irqrestore(hba->host->host_lock, flags);
  
  	/* poll for max. 1 sec to clear door bell register by h/w */

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 8110dcd..43035f8 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -595,6 +595,11 @@ struct ufs_hba {
 */
#define UFSHCD_QUIRK_PRDT_BYTE_GRAN 0x80
  
+	/*

+* Cleaer handling for transfer/task request list is just opposite.
+*/

Spell check - should be 'Clear'

+   #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR0x100
+
unsigned int quirks;/* Deviations from standard UFSHCI spec. */
  
  	/* Device deviations from standard UFS device spec. */




Looks good to me, except the spell-check.

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

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

[Asutosh Das (asd)]

On 2/24/2018 5:27 AM, Miguel Ojeda wrote:

On Wed, Feb 21, 2018 at 5:56 AM, Asutosh Das  wrote:

From: Vijay Viswanath 

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__RECLAIM flag enabled.Such a workqueue will have


WQ_MEM_RECLAIM here. Also space after dot.


a rescue thread which will be called when the above deadlock
condition is possible.

Signed-off-by: Vijay Viswanath 
Signed-off-by: Can Guo 
Signed-off-by: Asutosh Das 
---
  drivers/scsi/ufs/ufshcd.c | 10 +-
  drivers/scsi/ufs/ufshcd.h |  1 +
  2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 6541e1d..bb3382a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1503,7 +1503,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(>clk_gating.ungate_work);
+   queue_work(hba->clk_gating.clk_gating_workq,
+  >clk_gating.ungate_work);
 /*
  * fall through to check if we should wait for this
  * work to be done or not.
@@ -1689,6 +1690,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;

@@ -1696,6 +1699,10 @@ static void ufshcd_init_clk_gating(struct ufs_hba *hba)
 INIT_DELAYED_WORK(>clk_gating.gate_work, ufshcd_gate_work);
 INIT_WORK(>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 = 
create_singlethread_workqueue(wq_name);


Shouldn't this be alloc_ordered_workqueue() with WQ_MEM_RECLAIM then?
(create_singlethread_workqueue() and friends are deprecated).

Cheers,
Miguel


+
 hba->clk_gating.is_enabled = true;

 hba->clk_gating.delay_attr.show = ufshcd_clkgate_delay_show;
@@ -1723,6 +1730,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba)
 device_remove_file(hba->dev, >clk_gating.enable_attr);
 cancel_work_sync(>clk_gating.ungate_work);
 cancel_delayed_work_sync(>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 4385741..570c33e 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.



Hi Miguel
Thanks for the review.

I'll check this and put up the changes in v2.


-asd

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

Re: [PATCH 1/9] scsi: ufs: Allowing power mode change

[Asutosh Das (asd)]

On 2/23/2018 10:40 AM, Kyuho Choi wrote:

Hi Asutosh,

I've simple question in below.

On 2/21/18, Asutosh Das  wrote:

From: Yaniv Gardi 

Due to M-PHY issues, moving from HS to any other mode or gear or
even Hibern8 causes some un-predicted behavior of the device.
This patch fixes this issues.

Signed-off-by: Yaniv Gardi 
Signed-off-by: Subhash Jadavani 
Signed-off-by: Can Guo 
Signed-off-by: Asutosh Das 
---
  drivers/scsi/ufs/ufshcd.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 011c336..d74d529 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4167,9 +4167,13 @@ 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;
+   } else {
+   ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 0);
+   ufshcd_dme_set(hba, UIC_ARG_MIB(TX_LCC_ENABLE), 1);
+   }



Every ufs host has same issue and affected?.


if (link_startup_again) {
link_startup_again = 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.



Hi Choi
Thanks for the review.

No - I can't say if every host has the same issue. However, I get your 
point. It could be done with a quirk.


I'll fix this in v2 after collating all the comments from the rest of 
the patches.


-asd

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

Re: [PATCH 5/9] scsi: ufs: add reference counting for scsi block requests

[Asutosh Das (asd)]

On 2/21/2018 6:48 PM, Stanislav Nijnikov wrote:




-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Asutosh Das
Sent: Wednesday, February 21, 2018 6:57 AM
To: subha...@codeaurora.org; c...@codeaurora.org;
vivek.gau...@codeaurora.org; rna...@codeaurora.org;
vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com
Cc: linux-scsi@vger.kernel.org; Asutosh Das ;
open list 
Subject: [PATCH 5/9] scsi: ufs: add reference counting for scsi block requests

From: Subhash Jadavani 

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 
Signed-off-by: Can Guo 
Signed-off-by: Asutosh Das 
---
  drivers/scsi/ufs/ufshcd.c | 44
+---
  drivers/scsi/ufs/ufshcd.h |  5 +
  2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
7a4df95..987b81b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -264,6 +264,36 @@ static inline void ufshcd_disable_irq(struct ufs_hba
*hba)
}
  }

+void ufshcd_scsi_unblock_requests(struct ufs_hba *hba) {
+   unsigned long flags;
+   bool unblock = false;
+
+   spin_lock_irqsave(hba->host->host_lock, flags);
+   hba->scsi_block_reqs_cnt--;
+   unblock = !hba->scsi_block_reqs_cnt;
+   spin_unlock_irqrestore(hba->host->host_lock, flags);
+   if (unblock)
+   scsi_unblock_requests(hba->host);
+}
+EXPORT_SYMBOL(ufshcd_scsi_unblock_requests);
+
+static inline void __ufshcd_scsi_block_requests(struct ufs_hba *hba) {
+   if (!hba->scsi_block_reqs_cnt++)
+   scsi_block_requests(hba->host);
+}
+
+void ufshcd_scsi_block_requests(struct ufs_hba *hba) {
+   unsigned long flags;
+
+   spin_lock_irqsave(hba->host->host_lock, flags);
+   __ufshcd_scsi_block_requests(hba);
+   spin_unlock_irqrestore(hba->host->host_lock, flags); }
+EXPORT_SYMBOL(ufshcd_scsi_block_requests);
+
  /* replace non-printable or non-ASCII characters with spaces */  static inline
void ufshcd_remove_non_printable(char *val)  { @@ -1079,12 +1109,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(>clk_scaling_lock);
if (ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
ret = -EBUSY;
up_write(>clk_scaling_lock);
-   scsi_unblock_requests(hba->host);
+   ufshcd_scsi_unblock_requests(hba);
}

return ret;
@@ -1093,7 +1123,7 @@ static int ufshcd_clock_scaling_prepare(struct
ufs_hba *hba)  static void ufshcd_clock_scaling_unprepare(struct ufs_hba
*hba)  {
up_write(>clk_scaling_lock);
-   scsi_unblock_requests(hba->host);
+   ufshcd_scsi_unblock_requests(hba);
  }

  /**
@@ -1413,7 +1443,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);
  }

  /**
@@ -1469,7 +1499,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);
@@ -5197,7 +5227,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);
  }
@@ -5299,7 +5329,7 @@ static void ufshcd_check_errors(struct ufs_hba
*hba)

Re: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

[Asutosh Das (asd)]

On 2/2/2018 8:53 AM, Asutosh Das (asd) wrote:

On 1/31/2018 1:09 PM, Avri Altman wrote:

Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?

Thanks,
Avri


-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: subha...@codeaurora.org; c...@codeaurora.org;
vivek.gau...@codeaurora.org; rna...@codeaurora.org;
vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com
Cc: linux-scsi@vger.kernel.org; Venkat Gopalakrishnan
<venk...@codeaurora.org>; Asutosh Das <asuto...@codeaurora.org>; open
list <linux-ker...@vger.kernel.org>
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

From: Venkat Gopalakrishnan <venk...@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 <venk...@codeaurora.org>
Signed-off-by: Asutosh Das <asuto...@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
8af2af3..58d81de 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5357,19 +5357,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.




Hi
yes - interrupt aggregation makes sense here. But there were some 
performance concerns with it; well, I don't have the data to back that 
up now though.

However, I can code it up and check it.
Will post it in some time.

-asd


Hi Avri,
I went through the UFS HCI - v2.1 spec. Specifically, in sec 7.2.3 it 
explicitly mentions that the software should determine if new TRs were 
completed since the interrupt status was last read/cleared. This step is 
independent of aggregation.


So I think the above implementation makes sense. Please let me know if I 
understood your concern correctly.


-asd

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

Re: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

[Asutosh Das (asd)]

On 1/31/2018 1:09 PM, Avri Altman wrote:

Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?

Thanks,
Avri


-Original Message-
From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
ow...@vger.kernel.org] On Behalf Of Asutosh Das
Sent: Tuesday, January 30, 2018 6:54 AM
To: subha...@codeaurora.org; c...@codeaurora.org;
vivek.gau...@codeaurora.org; rna...@codeaurora.org;
vinholika...@gmail.com; j...@linux.vnet.ibm.com;
martin.peter...@oracle.com
Cc: linux-scsi@vger.kernel.org; Venkat Gopalakrishnan
; Asutosh Das ; open
list 
Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed

From: Venkat Gopalakrishnan 

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 
Signed-off-by: Asutosh Das 
---
  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
8af2af3..58d81de 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5357,19 +5357,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.




Hi
yes - interrupt aggregation makes sense here. But there were some 
performance concerns with it; well, I don't have the data to back that 
up now though.

However, I can code it up and check it.
Will post it in some time.

-asd

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

Re: [PATCH 1/1] scsi: ufs-qcom: remove broken hci version quirk

[Asutosh Das (asd)]

On 1/30/2018 11:33 AM, Vivek Gautam wrote:

Hi Asutosh,


On 1/30/2018 10:11 AM, Asutosh Das wrote:

From: Subhash Jadavani 

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 
Signed-off-by: Asutosh Das 
---


This patch and all other ufs patches that you have posted recently,
do they all fall under one 'ufs-qcom fixes' patch series for fixes that
we would want to do?
If it is so, then please club them together in a series, so that
it's easy for reviewers and maintainers to review, and keep track
of all the patches that can get-in after the reviews.
If they belong to two or more separate patch-series then please
create such patch series.
It's difficult to read through a lot of [PATCH 1/1] ... patch.

Sure Vivek - Makes sense. Will resend it.



Regards
Vivek


  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 2b38db2..221820a 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1098,7 +1098,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;
  if (!ufs_qcom_cap_qunipro(host))





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

Re: [PATCH 0/5] qcom-ufs: phy/hcd: Refactor phy initialization code

[Asutosh Das (asd)]

Vivek,

On 8/11/2017 12:42 PM, Vivek Gautam wrote:

On Fri, Aug 11, 2017 at 5:33 AM, Martin K. Petersen
<martin.peter...@oracle.com> wrote:


Vivek,


Can you kindly review this patch series (for UFS controller changes)
and consider giving your Ack so that Kishon can pull in the series
through phy tree.


SCSI piece looks OK.


Thank you Martin for your review.


Would still like Subhash to review the rest.


Subhash is on vacation with limited access to emails. I will ask
his team to take a look.


Looks good to me.

regards
Vivek



--
Martin K. Petersen  Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html






--
Asutosh Das (asd)
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project

Re: [ufs]: [scsi]: BUG: spinlock recursion on CPU#4

[Asutosh Das (asd)]

On 6/1/2017 7:32 PM, Bart Van Assche wrote:

On Thu, 2017-06-01 at 12:28 +0530, Asutosh Das (asd) wrote:

Please can you check if this is actually a bug and my understanding is
correct.


Hello Asutosh,

Spinlock recursion is always a bug. With what kernel version did you encounter
this? Was it with a kernel from kernel.org, a distro kernel or an Android 
kernel?
Have you already tried to reproduce this with kernel debugging enabled? Enabling
CONFIG_DEBUG_SPINLOCK and CONFIG_PROVE_LOCKING should provide more detailed
information.

Bart.



Hello Bart,
Thanks.

It's on 4.4 and its an Android kernel.

No - I haven't tried it out yet. I could get some clues from the 
call-stack itself, like I explained before. I can try these configs 
though. While I do that, I'd like to know your thoughts on my analysis. 
Do you think with the current data, it makes sense?


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

[ufs]: [scsi]: BUG: spinlock recursion on CPU#4

[Asutosh Das (asd)]

Hi All,

Recently, I came across an issue with the below call stack.

-000|arch_counter_get_cntvct(inline)
-000|__delay()
-001|__const_udelay(?)
-002|msm_trigger_wdog_bite()
-003|spin_dump(inline)
-003|spin_bug(lock = ?, ?)
-004|current_thread_info(inline)
-004|debug_spin_lock_before(inline)
-004|do_raw_spin_lock()
-005|raw_spin_lock_irqsave(lock = ?)
-006|blk_end_bidi_request(inline)
-006|blk_end_request_all(rq = ?, error = 0) <-- this tries to acquire 
the lock acquired by blk_delay_work (-024) and spinbug recursion occurs


-007|dm_end_request(clone = ?, error = 0)
-008|dm_done(inline)
-008|dm_softirq_done()
-009|blk_done_softirq()
-010|__read_once_size(inline)
-010|static_key_count(inline)
-010|static_key_false(inline)
-010|trace_softirq_exit(inline)
-010|__do_softirq()
-011|do_softirq_own_stack(inline)
-011|invoke_softirq(inline) <-- softirq is triggered because 
scsi_request_fn (-016) enabled interrupts on this cpu


-011|irq_exit()
-012|handle_IPI()
-013|gic_handle_irq()
-014|el1_irq(asm)
 -->|exception
-015|__raw_spin_unlock_irq(inline)
-015|raw_spin_unlock_irq(lock = ?)
-016|scsi_request_fn() <-- Unlocks the queue using spin_unlock, doesn't 
restore the flags, thus enabling the interrupts


-017|__blk_run_queue_uncond(inline)
-017|__blk_run_queue(q = ?)
-018|__elv_add_request()
-019|blk_insert_cloned_request() <-- acquires the queue lock & saves the 
flags


-020|dm_dispatch_clone_request(clone = ?, rq = ?)
-021|map_request()
-022|dm_request_fn()
-023|__blk_run_queue_uncond(inline)
-023|__blk_run_queue
-024|spin_unlock_irq(inline)
-024|blk_delay_work(?) <-- also acquires a queue lock, but this is a 
different queue, blk_end_request_all will reference this queue


-025|__read_once_size(inline)
-025|static_key_count(inline)
-025|static_key_false(inline)
-025|trace_workqueue_execute_end(inline)
-025|process_one_work()
-026|worker_thread()
-027|kthread()
-028|ret_from_fork(asm)
 ---|end of frame

Please can you check if this is actually a bug and my understanding is 
correct.

If so, I can put up a patch for the same.

--
Asutosh Das (asd)

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