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

From: Asutosh Das (asd)
Date: Thu Feb 22 2018 - 21:31:57 EST

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


-----Original Message-----
From: linux-scsi-owner@xxxxxxxxxxxxxxx [mailto:linux-scsi-
owner@xxxxxxxxxxxxxxx] On Behalf Of Asutosh Das
Sent: Wednesday, February 21, 2018 6:57 AM
To: subhashj@xxxxxxxxxxxxxx; cang@xxxxxxxxxxxxxx;
vivek.gautam@xxxxxxxxxxxxxx; rnayak@xxxxxxxxxxxxxx;
vinholikatti@xxxxxxxxx; jejb@xxxxxxxxxxxxxxxxxx;
martin.petersen@xxxxxxxxxx
Cc: linux-scsi@xxxxxxxxxxxxxxx; Asutosh Das <asutoshd@xxxxxxxxxxxxxx>;
open list <linux-kernel@xxxxxxxxxxxxxxx>
Subject: [PATCH 5/9] scsi: ufs: add reference counting for scsi block requests

From: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx>
Signed-off-by: Can Guo <cang@xxxxxxxxxxxxxx>
Signed-off-by: Asutosh Das <asutoshd@xxxxxxxxxxxxxx>
---
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(&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;
@@ -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(&hba->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) {
/* block commands from scsi mid-layer */
- scsi_block_requests(hba->host);
+ __ufshcd_scsi_block_requests(hba);

hba->ufshcd_state =
UFSHCD_STATE_EH_SCHEDULED;

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
7a2dad3..4385741 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;
@@ -690,6 +691,7 @@ struct ufs_hba {

struct rw_semaphore clk_scaling_lock;
struct ufs_desc_size desc_size;
+ int scsi_block_reqs_cnt;
};

/* Returns true if clocks can be gated. Otherwise false */ @@ -862,6 +864,9
@@ int ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum
desc_idn desc_id,

u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);

+void ufshcd_scsi_block_requests(struct ufs_hba *hba); void
+ufshcd_scsi_unblock_requests(struct ufs_hba *hba);
+
/* Wrapper functions for safely calling variant operations */ static inline
const char *ufshcd_get_var_name(struct ufs_hba *hba) {
--
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.

1. The atomic variable and operations could be used for the reference counting.
This will allow to avoid usage of the locks.
2. Why are the ufshcd_scsi_block_requests/ ufshcd_scsi_unblock_requests
functions not defined as static? They are not used outside ufshcd.c.

Regards
Stanislav

Hi
Thanks. Let me check this and get back.
I'll wait for comments on the other patches before posting a v2.

-asd

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