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-s...@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 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-s...@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)
/* handle fatal errors only when link is functional */
if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
   

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

[Stanislav Nijnikov]

> -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-s...@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);
>   

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

[Stanislav Nijnikov]

> -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-s...@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)
>   /* handle fatal errors only when link is