* [PATCH v4 0/3] Simplify and optimize the UFS driver
@ 2019-11-11 17:48 Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts Bart Van Assche
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Bart Van Assche @ 2019-11-11 17:48 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: Bean Huo, Avri Altman, Asutosh Das, Vignesh Raghavendra, Can Guo,
linux-scsi, Bart Van Assche
Hello Martin,
This patch series that simplifies and optimizes the UFS driver. Please consider
this patch series for kernel v5.5.
Thanks,
Bart.
Changes compared to v3:
- Left out "scsi" from the name of the functions that suspend and resume
command processing.
Changes compared to v2:
- Use a separate tag set for TMF tags.
Changes compared to v1:
- Use the block layer tag infrastructure for managing TMF tags.
Bart Van Assche (3):
ufs: Avoid busy-waiting by eliminating tag conflicts
ufs: Use blk_{get,put}_request() to allocate and free TMFs
ufs: Simplify the clock scaling mechanism implementation
drivers/scsi/ufs/ufshcd.c | 384 +++++++++++++++++---------------------
drivers/scsi/ufs/ufshcd.h | 21 +--
2 files changed, 173 insertions(+), 232 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts
2019-11-11 17:48 [PATCH v4 0/3] Simplify and optimize the UFS driver Bart Van Assche
@ 2019-11-11 17:48 ` Bart Van Assche
2019-11-12 12:25 ` [EXT] " Bean Huo (beanhuo)
2019-11-11 17:48 ` [PATCH v4 2/3] ufs: Use blk_{get,put}_request() to allocate and free TMFs Bart Van Assche
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2019-11-11 17:48 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: Bean Huo, Avri Altman, Asutosh Das, Vignesh Raghavendra, Can Guo,
linux-scsi, Bart Van Assche, Stanley Chu, Tomas Winkler
Instead of tracking which tags are in use in the ufs_hba.lrb_in_use
bitmask, rely on the block layer tag allocation mechanism. This patch
removes the following busy-waiting loop if ufshcd_issue_devman_upiu_cmd()
and the block layer accidentally allocate the same tag for a SCSI request:
* ufshcd_queuecommand() returns SCSI_MLQUEUE_HOST_BUSY.
* The SCSI core requeues the SCSI command.
Tested-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/ufs/ufshcd.c | 121 +++++++++++++++-----------------------
drivers/scsi/ufs/ufshcd.h | 6 +-
2 files changed, 50 insertions(+), 77 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9cbe3b45cf1c..93c920bf77d2 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -497,8 +497,8 @@ static void ufshcd_print_tmrs(struct ufs_hba *hba, unsigned long bitmap)
static void ufshcd_print_host_state(struct ufs_hba *hba)
{
dev_err(hba->dev, "UFS Host state=%d\n", hba->ufshcd_state);
- dev_err(hba->dev, "lrb in use=0x%lx, outstanding reqs=0x%lx tasks=0x%lx\n",
- hba->lrb_in_use, hba->outstanding_reqs, hba->outstanding_tasks);
+ dev_err(hba->dev, "outstanding reqs=0x%lx tasks=0x%lx\n",
+ hba->outstanding_reqs, hba->outstanding_tasks);
dev_err(hba->dev, "saved_err=0x%x, saved_uic_err=0x%x\n",
hba->saved_err, hba->saved_uic_err);
dev_err(hba->dev, "Device power mode=%d, UIC link state=%d\n",
@@ -1596,6 +1596,25 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
}
EXPORT_SYMBOL_GPL(ufshcd_hold);
+static bool ufshcd_is_busy(struct request *req, void *priv, bool reserved)
+{
+ int *busy = priv;
+
+ WARN_ON_ONCE(reserved);
+ (*busy)++;
+ return false;
+}
+
+/* Whether or not any tag is in use by a request that is in progress. */
+static bool ufshcd_any_tag_in_use(struct ufs_hba *hba)
+{
+ struct request_queue *q = hba->cmd_queue;
+ int busy = 0;
+
+ blk_mq_tagset_busy_iter(q->tag_set, ufshcd_is_busy, &busy);
+ return busy;
+}
+
static void ufshcd_gate_work(struct work_struct *work)
{
struct ufs_hba *hba = container_of(work, struct ufs_hba,
@@ -1619,7 +1638,7 @@ static void ufshcd_gate_work(struct work_struct *work)
if (hba->clk_gating.active_reqs
|| hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
- || hba->lrb_in_use || hba->outstanding_tasks
+ || ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks
|| hba->active_uic_cmd || hba->uic_async_done)
goto rel_lock;
@@ -1673,7 +1692,7 @@ static void __ufshcd_release(struct ufs_hba *hba)
if (hba->clk_gating.active_reqs || hba->clk_gating.is_suspended
|| hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL
- || hba->lrb_in_use || hba->outstanding_tasks
+ || ufshcd_any_tag_in_use(hba) || hba->outstanding_tasks
|| hba->active_uic_cmd || hba->uic_async_done
|| ufshcd_eh_in_progress(hba))
return;
@@ -2443,22 +2462,9 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
hba->req_abort_count = 0;
- /* acquire the tag to make sure device cmds don't use it */
- if (test_and_set_bit_lock(tag, &hba->lrb_in_use)) {
- /*
- * Dev manage command in progress, requeue the command.
- * Requeuing the command helps in cases where the request *may*
- * find different tag instead of waiting for dev manage command
- * completion.
- */
- err = SCSI_MLQUEUE_HOST_BUSY;
- goto out;
- }
-
err = ufshcd_hold(hba, true);
if (err) {
err = SCSI_MLQUEUE_HOST_BUSY;
- clear_bit_unlock(tag, &hba->lrb_in_use);
goto out;
}
WARN_ON(hba->clk_gating.state != CLKS_ON);
@@ -2479,7 +2485,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
err = ufshcd_map_sg(hba, lrbp);
if (err) {
lrbp->cmd = NULL;
- clear_bit_unlock(tag, &hba->lrb_in_use);
goto out;
}
/* Make sure descriptors are ready before ringing the doorbell */
@@ -2626,44 +2631,6 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
return err;
}
-/**
- * ufshcd_get_dev_cmd_tag - Get device management command tag
- * @hba: per-adapter instance
- * @tag_out: pointer to variable with available slot value
- *
- * Get a free slot and lock it until device management command
- * completes.
- *
- * Returns false if free slot is unavailable for locking, else
- * return true with tag value in @tag.
- */
-static bool ufshcd_get_dev_cmd_tag(struct ufs_hba *hba, int *tag_out)
-{
- int tag;
- bool ret = false;
- unsigned long tmp;
-
- if (!tag_out)
- goto out;
-
- do {
- tmp = ~hba->lrb_in_use;
- tag = find_last_bit(&tmp, hba->nutrs);
- if (tag >= hba->nutrs)
- goto out;
- } while (test_and_set_bit_lock(tag, &hba->lrb_in_use));
-
- *tag_out = tag;
- ret = true;
-out:
- return ret;
-}
-
-static inline void ufshcd_put_dev_cmd_tag(struct ufs_hba *hba, int tag)
-{
- clear_bit_unlock(tag, &hba->lrb_in_use);
-}
-
/**
* ufshcd_exec_dev_cmd - API for sending device management requests
* @hba: UFS hba
@@ -2676,6 +2643,8 @@ static inline void ufshcd_put_dev_cmd_tag(struct ufs_hba *hba, int tag)
static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
enum dev_cmd_type cmd_type, int timeout)
{
+ struct request_queue *q = hba->cmd_queue;
+ struct request *req;
struct ufshcd_lrb *lrbp;
int err;
int tag;
@@ -2689,7 +2658,11 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
* Even though we use wait_event() which sleeps indefinitely,
* the maximum wait time is bounded by SCSI request timeout.
*/
- wait_event(hba->dev_cmd.tag_wq, ufshcd_get_dev_cmd_tag(hba, &tag));
+ req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+ tag = req->tag;
+ WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
init_completion(&wait);
lrbp = &hba->lrb[tag];
@@ -2714,8 +2687,7 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
err ? "query_complete_err" : "query_complete");
out_put_tag:
- ufshcd_put_dev_cmd_tag(hba, tag);
- wake_up(&hba->dev_cmd.tag_wq);
+ blk_put_request(req);
up_read(&hba->clk_scaling_lock);
return err;
}
@@ -4834,7 +4806,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
cmd->result = result;
/* Mark completed command as NULL in LRB */
lrbp->cmd = NULL;
- clear_bit_unlock(index, &hba->lrb_in_use);
/* Do not touch lrbp after scsi done */
cmd->scsi_done(cmd);
__ufshcd_release(hba);
@@ -4856,9 +4827,6 @@ static void __ufshcd_transfer_req_compl(struct ufs_hba *hba,
hba->outstanding_reqs ^= completed_reqs;
ufshcd_clk_scaling_update_busy(hba);
-
- /* we might have free'd some tags above */
- wake_up(&hba->dev_cmd.tag_wq);
}
/**
@@ -5787,6 +5755,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
enum dev_cmd_type cmd_type,
enum query_opcode desc_op)
{
+ struct request_queue *q = hba->cmd_queue;
+ struct request *req;
struct ufshcd_lrb *lrbp;
int err = 0;
int tag;
@@ -5796,7 +5766,11 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
down_read(&hba->clk_scaling_lock);
- wait_event(hba->dev_cmd.tag_wq, ufshcd_get_dev_cmd_tag(hba, &tag));
+ req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
+ if (IS_ERR(req))
+ return PTR_ERR(req);
+ tag = req->tag;
+ WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
init_completion(&wait);
lrbp = &hba->lrb[tag];
@@ -5870,8 +5844,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
}
}
- ufshcd_put_dev_cmd_tag(hba, tag);
- wake_up(&hba->dev_cmd.tag_wq);
+ blk_put_request(req);
up_read(&hba->clk_scaling_lock);
return err;
}
@@ -6166,9 +6139,6 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
hba->lrb[tag].cmd = NULL;
spin_unlock_irqrestore(host->host_lock, flags);
- clear_bit_unlock(tag, &hba->lrb_in_use);
- wake_up(&hba->dev_cmd.tag_wq);
-
out:
if (!err) {
err = SUCCESS;
@@ -8172,6 +8142,7 @@ void ufshcd_remove(struct ufs_hba *hba)
{
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
+ blk_cleanup_queue(hba->cmd_queue);
scsi_remove_host(hba->host);
/* disable interrupts */
ufshcd_disable_intr(hba, hba->intr_mask);
@@ -8335,9 +8306,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
init_rwsem(&hba->clk_scaling_lock);
- /* Initialize device management tag acquire wait queue */
- init_waitqueue_head(&hba->dev_cmd.tag_wq);
-
ufshcd_init_clk_gating(hba);
ufshcd_init_clk_scaling(hba);
@@ -8371,6 +8339,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
goto exit_gating;
}
+ err = -ENOMEM;
+ hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
+ if (!hba->cmd_queue)
+ goto out_remove_scsi_host;
+
/* Reset the attached device */
ufshcd_vops_device_reset(hba);
@@ -8380,7 +8353,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
dev_err(hba->dev, "Host controller enable failed\n");
ufshcd_print_host_regs(hba);
ufshcd_print_host_state(hba);
- goto out_remove_scsi_host;
+ goto free_cmd_queue;
}
/*
@@ -8417,6 +8390,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
return 0;
+free_cmd_queue:
+ blk_cleanup_queue(hba->cmd_queue);
out_remove_scsi_host:
scsi_remove_host(hba->host);
exit_gating:
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index e0fe247c719e..6d9521892a84 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -212,13 +212,11 @@ struct ufs_query {
* @type: device management command type - Query, NOP OUT
* @lock: lock to allow one command at a time
* @complete: internal commands completion
- * @tag_wq: wait queue until free command slot is available
*/
struct ufs_dev_cmd {
enum dev_cmd_type type;
struct mutex lock;
struct completion *complete;
- wait_queue_head_t tag_wq;
struct ufs_query query;
};
@@ -483,7 +481,7 @@ struct ufs_stats {
* @host: Scsi_Host instance of the driver
* @dev: device handle
* @lrb: local reference block
- * @lrb_in_use: lrb in use
+ * @cmd_queue: Used to allocate command tags from hba->host->tag_set.
* @outstanding_tasks: Bits representing outstanding task requests
* @outstanding_reqs: Bits representing outstanding transfer requests
* @capabilities: UFS Controller Capabilities
@@ -541,6 +539,7 @@ struct ufs_hba {
struct Scsi_Host *host;
struct device *dev;
+ struct request_queue *cmd_queue;
/*
* This field is to keep a reference to "scsi_device" corresponding to
* "UFS device" W-LU.
@@ -561,7 +560,6 @@ struct ufs_hba {
u32 ahit;
struct ufshcd_lrb *lrb;
- unsigned long lrb_in_use;
unsigned long outstanding_tasks;
unsigned long outstanding_reqs;
--
2.24.0.rc1.363.gb1bccd3e3d-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 2/3] ufs: Use blk_{get,put}_request() to allocate and free TMFs
2019-11-11 17:48 [PATCH v4 0/3] Simplify and optimize the UFS driver Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts Bart Van Assche
@ 2019-11-11 17:48 ` Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation Bart Van Assche
2019-11-12 7:07 ` [PATCH v4 0/3] Simplify and optimize the UFS driver Avri Altman
3 siblings, 0 replies; 10+ messages in thread
From: Bart Van Assche @ 2019-11-11 17:48 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: Bean Huo, Avri Altman, Asutosh Das, Vignesh Raghavendra, Can Guo,
linux-scsi, Bart Van Assche, Stanley Chu, Tomas Winkler
Manage TMF tags with blk_{get,put}_request() instead of
ufshcd_get_tm_free_slot() / ufshcd_put_tm_slot(). Store a per-request
completion pointer in request.end_io_data instead of using a waitqueue
to report TMF completion.
Tested-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/ufs/ufshcd.c | 122 +++++++++++++++++++++++---------------
drivers/scsi/ufs/ufshcd.h | 12 ++--
2 files changed, 77 insertions(+), 57 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 93c920bf77d2..00c567bbbb64 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -645,40 +645,6 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
}
-/**
- * ufshcd_get_tm_free_slot - get a free slot for task management request
- * @hba: per adapter instance
- * @free_slot: pointer to variable with available slot value
- *
- * Get a free tag and lock it until ufshcd_put_tm_slot() is called.
- * Returns 0 if free slot is not available, else return 1 with tag value
- * in @free_slot.
- */
-static bool ufshcd_get_tm_free_slot(struct ufs_hba *hba, int *free_slot)
-{
- int tag;
- bool ret = false;
-
- if (!free_slot)
- goto out;
-
- do {
- tag = find_first_zero_bit(&hba->tm_slots_in_use, hba->nutmrs);
- if (tag >= hba->nutmrs)
- goto out;
- } while (test_and_set_bit_lock(tag, &hba->tm_slots_in_use));
-
- *free_slot = tag;
- ret = true;
-out:
- return ret;
-}
-
-static inline void ufshcd_put_tm_slot(struct ufs_hba *hba, int slot)
-{
- clear_bit_unlock(slot, &hba->tm_slots_in_use);
-}
-
/**
* ufshcd_utrl_clear - Clear a bit in UTRLCLR register
* @hba: per adapter instance
@@ -5517,17 +5483,38 @@ static void ufshcd_check_errors(struct ufs_hba *hba)
*/
}
+struct ctm_info {
+ struct ufs_hba *hba;
+ unsigned long pending;
+};
+
+static bool ufshcd_compl_tm(struct request *req, void *priv, bool reserved)
+{
+ const struct ctm_info *const ci = priv;
+ struct completion *c;
+
+ WARN_ON_ONCE(reserved);
+ if (test_bit(req->tag, &ci->pending))
+ return true;
+ c = req->end_io_data;
+ if (c)
+ complete(c);
+ return true;
+}
+
/**
* ufshcd_tmc_handler - handle task management function completion
* @hba: per adapter instance
*/
static void ufshcd_tmc_handler(struct ufs_hba *hba)
{
- u32 tm_doorbell;
+ struct request_queue *q = hba->tmf_queue;
+ struct ctm_info ci = {
+ .hba = hba,
+ .pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL),
+ };
- tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
- hba->tm_condition = tm_doorbell ^ hba->outstanding_tasks;
- wake_up(&hba->tm_wq);
+ blk_mq_tagset_busy_iter(q->tag_set, ufshcd_compl_tm, &ci);
}
/**
@@ -5620,7 +5607,10 @@ static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag)
static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
struct utp_task_req_desc *treq, u8 tm_function)
{
+ struct request_queue *q = hba->tmf_queue;
struct Scsi_Host *host = hba->host;
+ DECLARE_COMPLETION_ONSTACK(wait);
+ struct request *req;
unsigned long flags;
int free_slot, task_tag, err;
@@ -5629,7 +5619,10 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
* Even though we use wait_event() which sleeps indefinitely,
* the maximum wait time is bounded by %TM_CMD_TIMEOUT.
*/
- wait_event(hba->tm_tag_wq, ufshcd_get_tm_free_slot(hba, &free_slot));
+ req = blk_get_request(q, REQ_OP_DRV_OUT, BLK_MQ_REQ_RESERVED);
+ req->end_io_data = &wait;
+ free_slot = req->tag;
+ WARN_ON_ONCE(free_slot < 0 || free_slot >= hba->nutmrs);
ufshcd_hold(hba, false);
spin_lock_irqsave(host->host_lock, flags);
@@ -5655,10 +5648,14 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_send");
/* wait until the task management command is completed */
- err = wait_event_timeout(hba->tm_wq,
- test_bit(free_slot, &hba->tm_condition),
+ err = wait_for_completion_io_timeout(&wait,
msecs_to_jiffies(TM_CMD_TIMEOUT));
if (!err) {
+ /*
+ * Make sure that ufshcd_compl_tm() does not trigger a
+ * use-after-free.
+ */
+ req->end_io_data = NULL;
ufshcd_add_tm_upiu_trace(hba, task_tag, "tm_complete_err");
dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n",
__func__, tm_function);
@@ -5677,9 +5674,7 @@ static int __ufshcd_issue_tm_cmd(struct ufs_hba *hba,
__clear_bit(free_slot, &hba->outstanding_tasks);
spin_unlock_irqrestore(hba->host->host_lock, flags);
- clear_bit(free_slot, &hba->tm_condition);
- ufshcd_put_tm_slot(hba, free_slot);
- wake_up(&hba->tm_tag_wq);
+ blk_put_request(req);
ufshcd_release(hba);
return err;
@@ -8142,6 +8137,8 @@ void ufshcd_remove(struct ufs_hba *hba)
{
ufs_bsg_remove(hba);
ufs_sysfs_remove_nodes(hba->dev);
+ blk_cleanup_queue(hba->tmf_queue);
+ blk_mq_free_tag_set(&hba->tmf_tag_set);
blk_cleanup_queue(hba->cmd_queue);
scsi_remove_host(hba->host);
/* disable interrupts */
@@ -8221,6 +8218,18 @@ int ufshcd_alloc_host(struct device *dev, struct ufs_hba **hba_handle)
}
EXPORT_SYMBOL(ufshcd_alloc_host);
+/* This function exists because blk_mq_alloc_tag_set() requires this. */
+static blk_status_t ufshcd_queue_tmf(struct blk_mq_hw_ctx *hctx,
+ const struct blk_mq_queue_data *qd)
+{
+ WARN_ON_ONCE(true);
+ return BLK_STS_NOTSUPP;
+}
+
+static const struct blk_mq_ops ufshcd_tmf_ops = {
+ .queue_rq = ufshcd_queue_tmf,
+};
+
/**
* ufshcd_init - Driver initialization routine
* @hba: per-adapter instance
@@ -8290,10 +8299,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
hba->max_pwr_info.is_valid = false;
- /* Initailize wait queue for task management */
- init_waitqueue_head(&hba->tm_wq);
- init_waitqueue_head(&hba->tm_tag_wq);
-
/* Initialize work queues */
INIT_WORK(&hba->eh_work, ufshcd_err_handler);
INIT_WORK(&hba->eeh_work, ufshcd_exception_event_handler);
@@ -8344,6 +8349,21 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
if (!hba->cmd_queue)
goto out_remove_scsi_host;
+ hba->tmf_tag_set = (struct blk_mq_tag_set) {
+ .nr_hw_queues = 1,
+ .queue_depth = hba->nutmrs,
+ .ops = &ufshcd_tmf_ops,
+ .flags = BLK_MQ_F_NO_SCHED,
+ };
+ err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
+ if (err < 0)
+ goto free_cmd_queue;
+ hba->tmf_queue = blk_mq_init_queue(&hba->tmf_tag_set);
+ if (IS_ERR(hba->tmf_queue)) {
+ err = PTR_ERR(hba->tmf_queue);
+ goto free_tmf_tag_set;
+ }
+
/* Reset the attached device */
ufshcd_vops_device_reset(hba);
@@ -8353,7 +8373,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
dev_err(hba->dev, "Host controller enable failed\n");
ufshcd_print_host_regs(hba);
ufshcd_print_host_state(hba);
- goto free_cmd_queue;
+ goto free_tmf_queue;
}
/*
@@ -8390,6 +8410,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
return 0;
+free_tmf_queue:
+ blk_cleanup_queue(hba->tmf_queue);
+free_tmf_tag_set:
+ blk_mq_free_tag_set(&hba->tmf_tag_set);
free_cmd_queue:
blk_cleanup_queue(hba->cmd_queue);
out_remove_scsi_host:
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 6d9521892a84..5865e16f53a6 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -493,11 +493,9 @@ struct ufs_stats {
* @irq: Irq number of the controller
* @active_uic_cmd: handle of active UIC command
* @uic_cmd_mutex: mutex for uic command
- * @tm_wq: wait queue for task management
- * @tm_tag_wq: wait queue for free task management slots
- * @tm_slots_in_use: bit map of task management request slots in use
+ * @tmf_tag_set: TMF tag set.
+ * @tmf_queue: Used to allocate TMF tags.
* @pwr_done: completion for power mode change
- * @tm_condition: condition variable for task management
* @ufshcd_state: UFSHCD states
* @eh_flags: Error handling flags
* @intr_mask: Interrupt Mask Bits
@@ -641,10 +639,8 @@ struct ufs_hba {
/* Device deviations from standard UFS device spec. */
unsigned int dev_quirks;
- wait_queue_head_t tm_wq;
- wait_queue_head_t tm_tag_wq;
- unsigned long tm_condition;
- unsigned long tm_slots_in_use;
+ struct blk_mq_tag_set tmf_tag_set;
+ struct request_queue *tmf_queue;
struct uic_command *active_uic_cmd;
struct mutex uic_cmd_mutex;
--
2.24.0.rc1.363.gb1bccd3e3d-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation
2019-11-11 17:48 [PATCH v4 0/3] Simplify and optimize the UFS driver Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 2/3] ufs: Use blk_{get,put}_request() to allocate and free TMFs Bart Van Assche
@ 2019-11-11 17:48 ` Bart Van Assche
2019-11-12 9:06 ` cang
2019-11-12 7:07 ` [PATCH v4 0/3] Simplify and optimize the UFS driver Avri Altman
3 siblings, 1 reply; 10+ messages in thread
From: Bart Van Assche @ 2019-11-11 17:48 UTC (permalink / raw)
To: Martin K . Petersen, James E . J . Bottomley
Cc: Bean Huo, Avri Altman, Asutosh Das, Vignesh Raghavendra, Can Guo,
linux-scsi, Bart Van Assche, Stanley Chu, Tomas Winkler
Scaling the clock is only safe while no commands are in progress. Use
blk_mq_{un,}freeze_queue() to block submission of new commands and to
wait for ongoing commands to complete. Remove "_scsi" from the name of
the functions that block and unblock requests since these functions
now block both SCSI commands and non-SCSI commands.
Tested-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Cc: Stanley Chu <stanley.chu@mediatek.com>
Cc: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/scsi/ufs/ufshcd.c | 143 +++++++++++++-------------------------
drivers/scsi/ufs/ufshcd.h | 3 -
2 files changed, 47 insertions(+), 99 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00c567bbbb64..65f05ca8d76f 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -290,16 +290,50 @@ static inline void ufshcd_disable_irq(struct ufs_hba *hba)
}
}
-static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
+static void ufshcd_unblock_requests(struct ufs_hba *hba)
{
- if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
- scsi_unblock_requests(hba->host);
+ struct scsi_device *sdev;
+
+ blk_mq_unfreeze_queue(hba->tmf_queue);
+ blk_mq_unfreeze_queue(hba->cmd_queue);
+ shost_for_each_device(sdev, hba->host)
+ blk_mq_unfreeze_queue(sdev->request_queue);
}
-static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
+static int ufshcd_block_requests(struct ufs_hba *hba, unsigned long timeout)
{
- if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
- scsi_block_requests(hba->host);
+ struct scsi_device *sdev;
+ unsigned long deadline = jiffies + timeout;
+
+ if (timeout == ULONG_MAX) {
+ shost_for_each_device(sdev, hba->host)
+ blk_mq_freeze_queue(sdev->request_queue);
+ blk_mq_freeze_queue(hba->cmd_queue);
+ blk_mq_freeze_queue(hba->tmf_queue);
+ return 0;
+ }
+
+ shost_for_each_device(sdev, hba->host)
+ blk_freeze_queue_start(sdev->request_queue);
+ blk_freeze_queue_start(hba->cmd_queue);
+ blk_freeze_queue_start(hba->tmf_queue);
+ shost_for_each_device(sdev, hba->host) {
+ if (blk_mq_freeze_queue_wait_timeout(sdev->request_queue,
+ max_t(long, 0, deadline - jiffies)) <= 0) {
+ goto err;
+ }
+ }
+ if (blk_mq_freeze_queue_wait_timeout(hba->cmd_queue,
+ max_t(long, 0, deadline - jiffies)) <= 0)
+ goto err;
+ if (blk_mq_freeze_queue_wait_timeout(hba->tmf_queue,
+ max_t(long, 0, deadline - jiffies)) <= 0)
+ goto err;
+ return 0;
+
+err:
+ ufshcd_unblock_requests(hba);
+ return -ETIMEDOUT;
}
static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
@@ -971,65 +1005,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
return false;
}
-static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
- u64 wait_timeout_us)
-{
- unsigned long flags;
- int ret = 0;
- u32 tm_doorbell;
- u32 tr_doorbell;
- bool timeout = false, do_last_check = false;
- ktime_t start;
-
- ufshcd_hold(hba, false);
- spin_lock_irqsave(hba->host->host_lock, flags);
- /*
- * Wait for all the outstanding tasks/transfer requests.
- * Verify by checking the doorbell registers are clear.
- */
- start = ktime_get();
- do {
- if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
- ret = -EBUSY;
- goto out;
- }
-
- tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
- tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
- if (!tm_doorbell && !tr_doorbell) {
- timeout = false;
- break;
- } else if (do_last_check) {
- break;
- }
-
- spin_unlock_irqrestore(hba->host->host_lock, flags);
- schedule();
- if (ktime_to_us(ktime_sub(ktime_get(), start)) >
- wait_timeout_us) {
- timeout = true;
- /*
- * We might have scheduled out for long time so make
- * sure to check if doorbells are cleared by this time
- * or not.
- */
- do_last_check = true;
- }
- spin_lock_irqsave(hba->host->host_lock, flags);
- } while (tm_doorbell || tr_doorbell);
-
- if (timeout) {
- dev_err(hba->dev,
- "%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
- __func__, tm_doorbell, tr_doorbell);
- ret = -EBUSY;
- }
-out:
- spin_unlock_irqrestore(hba->host->host_lock, flags);
- ufshcd_release(hba);
- return ret;
-}
-
/**
* ufshcd_scale_gear - scale up/down UFS gear
* @hba: per adapter instance
@@ -1079,27 +1054,16 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
{
- #define DOORBELL_CLR_TOUT_US (1000 * 1000) /* 1 sec */
- int ret = 0;
/*
* make sure that there are no outstanding requests when
* clock scaling is in progress
*/
- 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);
- ufshcd_scsi_unblock_requests(hba);
- }
-
- return ret;
+ return ufshcd_block_requests(hba, HZ);
}
static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
{
- up_write(&hba->clk_scaling_lock);
- ufshcd_scsi_unblock_requests(hba);
+ ufshcd_unblock_requests(hba);
}
/**
@@ -1471,7 +1435,7 @@ static void ufshcd_ungate_work(struct work_struct *work)
hba->clk_gating.is_suspended = false;
}
unblock_reqs:
- ufshcd_scsi_unblock_requests(hba);
+ ufshcd_unblock_requests(hba);
}
/**
@@ -1528,7 +1492,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
*/
/* fallthrough */
case CLKS_OFF:
- ufshcd_scsi_block_requests(hba);
+ ufshcd_block_requests(hba, ULONG_MAX);
hba->clk_gating.state = REQ_CLKS_ON;
trace_ufshcd_clk_gating(dev_name(hba->dev),
hba->clk_gating.state);
@@ -2395,9 +2359,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
BUG();
}
- if (!down_read_trylock(&hba->clk_scaling_lock))
- return SCSI_MLQUEUE_HOST_BUSY;
-
spin_lock_irqsave(hba->host->host_lock, flags);
switch (hba->ufshcd_state) {
case UFSHCD_STATE_OPERATIONAL:
@@ -2463,7 +2424,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
out_unlock:
spin_unlock_irqrestore(hba->host->host_lock, flags);
out:
- up_read(&hba->clk_scaling_lock);
return err;
}
@@ -2617,8 +2577,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
struct completion wait;
unsigned long flags;
- down_read(&hba->clk_scaling_lock);
-
/*
* Get free slot, sleep if slots are unavailable.
* Even though we use wait_event() which sleeps indefinitely,
@@ -2654,7 +2612,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
out_put_tag:
blk_put_request(req);
- up_read(&hba->clk_scaling_lock);
return err;
}
@@ -5329,7 +5286,7 @@ static void ufshcd_err_handler(struct work_struct *work)
out:
spin_unlock_irqrestore(hba->host->host_lock, flags);
- ufshcd_scsi_unblock_requests(hba);
+ ufshcd_unblock_requests(hba);
ufshcd_release(hba);
pm_runtime_put_sync(hba->dev);
}
@@ -5452,8 +5409,8 @@ 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 */
- ufshcd_scsi_block_requests(hba);
+ /* block all commands, incl. SCSI commands */
+ ufshcd_block_requests(hba, ULONG_MAX);
hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED;
@@ -5759,8 +5716,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
unsigned long flags;
u32 upiu_flags;
- down_read(&hba->clk_scaling_lock);
-
req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
if (IS_ERR(req))
return PTR_ERR(req);
@@ -5840,7 +5795,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
}
blk_put_request(req);
- up_read(&hba->clk_scaling_lock);
return err;
}
@@ -8309,8 +8263,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
/* Initialize mutex for device management commands */
mutex_init(&hba->dev_cmd.lock);
- init_rwsem(&hba->clk_scaling_lock);
-
ufshcd_init_clk_gating(hba);
ufshcd_init_clk_scaling(hba);
@@ -8396,7 +8348,6 @@ 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 5865e16f53a6..c44bfcdb26a3 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -520,7 +520,6 @@ 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;
@@ -724,9 +723,7 @@ struct ufs_hba {
enum bkops_status urgent_bkops_lvl;
bool is_urgent_bkops_lvl_checked;
- struct rw_semaphore clk_scaling_lock;
struct ufs_desc_size desc_size;
- atomic_t scsi_block_reqs_cnt;
struct device bsg_dev;
struct request_queue *bsg_queue;
--
2.24.0.rc1.363.gb1bccd3e3d-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH v4 0/3] Simplify and optimize the UFS driver
2019-11-11 17:48 [PATCH v4 0/3] Simplify and optimize the UFS driver Bart Van Assche
` (2 preceding siblings ...)
2019-11-11 17:48 ` [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation Bart Van Assche
@ 2019-11-12 7:07 ` Avri Altman
3 siblings, 0 replies; 10+ messages in thread
From: Avri Altman @ 2019-11-12 7:07 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley
Cc: Bean Huo, Asutosh Das, Vignesh Raghavendra, Can Guo, linux-scsi
>
> Hello Martin,
>
> This patch series that simplifies and optimizes the UFS driver. Please consider
> this patch series for kernel v5.5.
>
> Thanks,
>
> Bart.
>
> Changes compared to v3:
> - Left out "scsi" from the name of the functions that suspend and resume
> command processing.
Looks good to me.
Thanks,
Avri
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation
2019-11-11 17:48 ` [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation Bart Van Assche
@ 2019-11-12 9:06 ` cang
2019-11-12 15:43 ` [EXT] " Bean Huo (beanhuo)
0 siblings, 1 reply; 10+ messages in thread
From: cang @ 2019-11-12 9:06 UTC (permalink / raw)
To: Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, Bean Huo,
Avri Altman, Asutosh Das, Vignesh Raghavendra, linux-scsi,
Stanley Chu, Tomas Winkler
On 2019-11-12 01:48, Bart Van Assche wrote:
> Scaling the clock is only safe while no commands are in progress. Use
> blk_mq_{un,}freeze_queue() to block submission of new commands and to
> wait for ongoing commands to complete. Remove "_scsi" from the name of
> the functions that block and unblock requests since these functions
> now block both SCSI commands and non-SCSI commands.
>
> Tested-by: Bean Huo <beanhuo@micron.com>
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/scsi/ufs/ufshcd.c | 143 +++++++++++++-------------------------
> drivers/scsi/ufs/ufshcd.h | 3 -
> 2 files changed, 47 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 00c567bbbb64..65f05ca8d76f 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -290,16 +290,50 @@ static inline void ufshcd_disable_irq(struct
> ufs_hba *hba)
> }
> }
>
> -static void ufshcd_scsi_unblock_requests(struct ufs_hba *hba)
> +static void ufshcd_unblock_requests(struct ufs_hba *hba)
> {
> - if (atomic_dec_and_test(&hba->scsi_block_reqs_cnt))
> - scsi_unblock_requests(hba->host);
> + struct scsi_device *sdev;
> +
> + blk_mq_unfreeze_queue(hba->tmf_queue);
> + blk_mq_unfreeze_queue(hba->cmd_queue);
> + shost_for_each_device(sdev, hba->host)
> + blk_mq_unfreeze_queue(sdev->request_queue);
> }
>
> -static void ufshcd_scsi_block_requests(struct ufs_hba *hba)
> +static int ufshcd_block_requests(struct ufs_hba *hba, unsigned long
> timeout)
> {
> - if (atomic_inc_return(&hba->scsi_block_reqs_cnt) == 1)
> - scsi_block_requests(hba->host);
> + struct scsi_device *sdev;
> + unsigned long deadline = jiffies + timeout;
> +
> + if (timeout == ULONG_MAX) {
> + shost_for_each_device(sdev, hba->host)
> + blk_mq_freeze_queue(sdev->request_queue);
> + blk_mq_freeze_queue(hba->cmd_queue);
> + blk_mq_freeze_queue(hba->tmf_queue);
> + return 0;
> + }
> +
> + shost_for_each_device(sdev, hba->host)
> + blk_freeze_queue_start(sdev->request_queue);
> + blk_freeze_queue_start(hba->cmd_queue);
> + blk_freeze_queue_start(hba->tmf_queue);
> + shost_for_each_device(sdev, hba->host) {
> + if (blk_mq_freeze_queue_wait_timeout(sdev->request_queue,
> + max_t(long, 0, deadline - jiffies)) <= 0) {
> + goto err;
> + }
> + }
> + if (blk_mq_freeze_queue_wait_timeout(hba->cmd_queue,
> + max_t(long, 0, deadline - jiffies)) <= 0)
> + goto err;
> + if (blk_mq_freeze_queue_wait_timeout(hba->tmf_queue,
> + max_t(long, 0, deadline - jiffies)) <= 0)
> + goto err;
> + return 0;
> +
> +err:
> + ufshcd_unblock_requests(hba);
> + return -ETIMEDOUT;
> }
>
> static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned
> int tag,
> @@ -971,65 +1005,6 @@ static bool
> ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
> return false;
> }
>
> -static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
> - u64 wait_timeout_us)
> -{
> - unsigned long flags;
> - int ret = 0;
> - u32 tm_doorbell;
> - u32 tr_doorbell;
> - bool timeout = false, do_last_check = false;
> - ktime_t start;
> -
> - ufshcd_hold(hba, false);
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - /*
> - * Wait for all the outstanding tasks/transfer requests.
> - * Verify by checking the doorbell registers are clear.
> - */
> - start = ktime_get();
> - do {
> - if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
> - ret = -EBUSY;
> - goto out;
> - }
> -
> - tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> - tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> - if (!tm_doorbell && !tr_doorbell) {
> - timeout = false;
> - break;
> - } else if (do_last_check) {
> - break;
> - }
> -
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - schedule();
> - if (ktime_to_us(ktime_sub(ktime_get(), start)) >
> - wait_timeout_us) {
> - timeout = true;
> - /*
> - * We might have scheduled out for long time so make
> - * sure to check if doorbells are cleared by this time
> - * or not.
> - */
> - do_last_check = true;
> - }
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - } while (tm_doorbell || tr_doorbell);
> -
> - if (timeout) {
> - dev_err(hba->dev,
> - "%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
> - __func__, tm_doorbell, tr_doorbell);
> - ret = -EBUSY;
> - }
> -out:
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - ufshcd_release(hba);
> - return ret;
> -}
> -
> /**
> * ufshcd_scale_gear - scale up/down UFS gear
> * @hba: per adapter instance
> @@ -1079,27 +1054,16 @@ static int ufshcd_scale_gear(struct ufs_hba
> *hba, bool scale_up)
>
> static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> {
> - #define DOORBELL_CLR_TOUT_US (1000 * 1000) /* 1 sec */
> - int ret = 0;
> /*
> * make sure that there are no outstanding requests when
> * clock scaling is in progress
> */
> - 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);
> - ufshcd_scsi_unblock_requests(hba);
> - }
> -
> - return ret;
> + return ufshcd_block_requests(hba, HZ);
> }
>
> static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba)
> {
> - up_write(&hba->clk_scaling_lock);
> - ufshcd_scsi_unblock_requests(hba);
> + ufshcd_unblock_requests(hba);
> }
>
> /**
> @@ -1471,7 +1435,7 @@ static void ufshcd_ungate_work(struct work_struct
> *work)
> hba->clk_gating.is_suspended = false;
> }
> unblock_reqs:
> - ufshcd_scsi_unblock_requests(hba);
> + ufshcd_unblock_requests(hba);
> }
>
> /**
> @@ -1528,7 +1492,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async)
> */
> /* fallthrough */
> case CLKS_OFF:
> - ufshcd_scsi_block_requests(hba);
> + ufshcd_block_requests(hba, ULONG_MAX);
> hba->clk_gating.state = REQ_CLKS_ON;
> trace_ufshcd_clk_gating(dev_name(hba->dev),
> hba->clk_gating.state);
> @@ -2395,9 +2359,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
> BUG();
> }
>
> - if (!down_read_trylock(&hba->clk_scaling_lock))
> - return SCSI_MLQUEUE_HOST_BUSY;
> -
> spin_lock_irqsave(hba->host->host_lock, flags);
> switch (hba->ufshcd_state) {
> case UFSHCD_STATE_OPERATIONAL:
> @@ -2463,7 +2424,6 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
> out_unlock:
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> out:
> - up_read(&hba->clk_scaling_lock);
> return err;
> }
>
> @@ -2617,8 +2577,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> struct completion wait;
> unsigned long flags;
>
> - down_read(&hba->clk_scaling_lock);
> -
> /*
> * Get free slot, sleep if slots are unavailable.
> * Even though we use wait_event() which sleeps indefinitely,
> @@ -2654,7 +2612,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
>
> out_put_tag:
> blk_put_request(req);
> - up_read(&hba->clk_scaling_lock);
> return err;
> }
>
> @@ -5329,7 +5286,7 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>
> out:
> spin_unlock_irqrestore(hba->host->host_lock, flags);
> - ufshcd_scsi_unblock_requests(hba);
> + ufshcd_unblock_requests(hba);
> ufshcd_release(hba);
> pm_runtime_put_sync(hba->dev);
> }
> @@ -5452,8 +5409,8 @@ 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 */
> - ufshcd_scsi_block_requests(hba);
> + /* block all commands, incl. SCSI commands */
> + ufshcd_block_requests(hba, ULONG_MAX);
>
ufshcd_block_requests() may sleep, but ufshcd_check_errors is called
from IRQ(atmic) context.
Regards,
Can Guo.
> hba->ufshcd_state = UFSHCD_STATE_EH_SCHEDULED;
>
> @@ -5759,8 +5716,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
> unsigned long flags;
> u32 upiu_flags;
>
> - down_read(&hba->clk_scaling_lock);
> -
> req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> if (IS_ERR(req))
> return PTR_ERR(req);
> @@ -5840,7 +5795,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
> }
>
> blk_put_request(req);
> - up_read(&hba->clk_scaling_lock);
> return err;
> }
>
> @@ -8309,8 +8263,6 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem *mmio_base, unsigned int irq)
> /* Initialize mutex for device management commands */
> mutex_init(&hba->dev_cmd.lock);
>
> - init_rwsem(&hba->clk_scaling_lock);
> -
> ufshcd_init_clk_gating(hba);
>
> ufshcd_init_clk_scaling(hba);
> @@ -8396,7 +8348,6 @@ 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 5865e16f53a6..c44bfcdb26a3 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -520,7 +520,6 @@ 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;
> @@ -724,9 +723,7 @@ struct ufs_hba {
> enum bkops_status urgent_bkops_lvl;
> bool is_urgent_bkops_lvl_checked;
>
> - struct rw_semaphore clk_scaling_lock;
> struct ufs_desc_size desc_size;
> - atomic_t scsi_block_reqs_cnt;
>
> struct device bsg_dev;
> struct request_queue *bsg_queue;
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts
2019-11-11 17:48 ` [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts Bart Van Assche
@ 2019-11-12 12:25 ` Bean Huo (beanhuo)
2019-11-12 16:51 ` Bart Van Assche
0 siblings, 1 reply; 10+ messages in thread
From: Bean Huo (beanhuo) @ 2019-11-12 12:25 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen, James E . J . Bottomley,
Avri Altman
Cc: Asutosh Das, Vignesh Raghavendra, Can Guo, linux-scsi,
Stanley Chu, Tomas Winkler
Hi, Bart
> + req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> + if (IS_ERR(req))
> + return PTR_ERR(req);
> + tag = req->tag;
> + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
....
> &tag));
> + req = blk_get_request(q, REQ_OP_DRV_OUT, 0);
> + if (IS_ERR(req))
> + return PTR_ERR(req);
> + tag = req->tag;
> + WARN_ON_ONCE(!ufshcd_valid_tag(hba, tag));
>
......
>
> + err = -ENOMEM;
> + hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
> + if (!hba->cmd_queue)
It is possible to return ERR_PTR(-ENOMEM) in blk_mq_init_queue(),,
Here just checkup if not NULL is not safe enough.
//Bean
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [EXT] Re: [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation
2019-11-12 9:06 ` cang
@ 2019-11-12 15:43 ` Bean Huo (beanhuo)
2019-11-12 16:04 ` Bart Van Assche
0 siblings, 1 reply; 10+ messages in thread
From: Bean Huo (beanhuo) @ 2019-11-12 15:43 UTC (permalink / raw)
To: cang, Bart Van Assche
Cc: Martin K . Petersen, James E . J . Bottomley, Avri Altman,
Asutosh Das, Vignesh Raghavendra, linux-scsi, Stanley Chu,
Tomas Winkler
Hi, Bart
> > /* handle fatal errors only when link is functional */
> > if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
> > - /* block commands from scsi mid-layer */
> > - ufshcd_scsi_block_requests(hba);
> > + /* block all commands, incl. SCSI commands */
> > + ufshcd_block_requests(hba, ULONG_MAX);
> >
>
> ufshcd_block_requests() may sleep, but ufshcd_check_errors is called from
> IRQ(atmic) context.
>
> Regards,
> Can Guo.
[Bean Huo]
Can Guo's comment is correct, I triggered an error manually, my UFS being killed.
And removing this patch, no problem. Please take this seriously.
Thanks,
//Bean
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] Re: [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation
2019-11-12 15:43 ` [EXT] " Bean Huo (beanhuo)
@ 2019-11-12 16:04 ` Bart Van Assche
0 siblings, 0 replies; 10+ messages in thread
From: Bart Van Assche @ 2019-11-12 16:04 UTC (permalink / raw)
To: Bean Huo (beanhuo), cang
Cc: Martin K . Petersen, James E . J . Bottomley, Avri Altman,
Asutosh Das, Vignesh Raghavendra, linux-scsi, Stanley Chu,
Tomas Winkler
On 11/12/19 7:43 AM, Bean Huo (beanhuo) wrote:
> Can Guo wrote:
>> Bart Van Assche wrote:
>>> /* handle fatal errors only when link is functional */
>>> if (hba->ufshcd_state == UFSHCD_STATE_OPERATIONAL) {
>>> - /* block commands from scsi mid-layer */
>>> - ufshcd_scsi_block_requests(hba);
>>> + /* block all commands, incl. SCSI commands */
>>> + ufshcd_block_requests(hba, ULONG_MAX);
>>>
>>
>> ufshcd_block_requests() may sleep, but ufshcd_check_errors is called from
>> IRQ(atmic) context.
>
> [Bean Huo]
> Can Guo's comment is correct, I triggered an error manually, my UFS being killed.
> And removing this patch, no problem. Please take this seriously.
Hi Can and Bean,
Thank you for having reported this. I'll look into keeping
scsi_block_requests() and scsi_unblock_requests() in this code path.
Thanks,
Bart.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [EXT] [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts
2019-11-12 12:25 ` [EXT] " Bean Huo (beanhuo)
@ 2019-11-12 16:51 ` Bart Van Assche
0 siblings, 0 replies; 10+ messages in thread
From: Bart Van Assche @ 2019-11-12 16:51 UTC (permalink / raw)
To: Bean Huo (beanhuo),
Martin K . Petersen, James E . J . Bottomley, Avri Altman
Cc: Asutosh Das, Vignesh Raghavendra, Can Guo, linux-scsi,
Stanley Chu, Tomas Winkler
On 11/12/19 4:25 AM, Bean Huo (beanhuo) wrote:
>> + err = -ENOMEM;
>> + hba->cmd_queue = blk_mq_init_queue(&hba->host->tag_set);
>> + if (!hba->cmd_queue)
>
> It is possible to return ERR_PTR(-ENOMEM) in blk_mq_init_queue(),,
> Here just checkup if not NULL is not safe enough.
Agreed, this should be changed into an IS_ERR() check.
Bart.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-12 16:51 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-11 17:48 [PATCH v4 0/3] Simplify and optimize the UFS driver Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 1/3] ufs: Avoid busy-waiting by eliminating tag conflicts Bart Van Assche
2019-11-12 12:25 ` [EXT] " Bean Huo (beanhuo)
2019-11-12 16:51 ` Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 2/3] ufs: Use blk_{get,put}_request() to allocate and free TMFs Bart Van Assche
2019-11-11 17:48 ` [PATCH v4 3/3] ufs: Simplify the clock scaling mechanism implementation Bart Van Assche
2019-11-12 9:06 ` cang
2019-11-12 15:43 ` [EXT] " Bean Huo (beanhuo)
2019-11-12 16:04 ` Bart Van Assche
2019-11-12 7:07 ` [PATCH v4 0/3] Simplify and optimize the UFS driver Avri Altman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).