On 12/1/19 9:39 PM, cang@xxxxxxxxxxxxxx wrote:
On 2019-11-26 09:05, Bart Van Assche wrote:
static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
{
- #define DOORBELL_CLR_TOUT_US (1000 * 1000) /* 1 sec */
+ unsigned long deadline = jiffies + HZ /* 1 sec */;
int ret = 0;
/*
* make sure that there are no outstanding requests when
* clock scaling is in progress
*/
ufshcd_scsi_block_requests(hba);
+ blk_freeze_queue_start(hba->cmd_queue);
+ blk_freeze_queue_start(hba->tmf_queue);
+ if (blk_mq_freeze_queue_wait_timeout(hba->cmd_queue,
+ max_t(long, 1, deadline - jiffies)) <= 0)
+ goto unblock;
+ if (blk_mq_freeze_queue_wait_timeout(hba->tmf_queue,
+ max_t(long, 1, deadline - jiffies)) <= 0)
+ goto unblock;
down_write(&hba->clk_scaling_lock);
Hi Bart,
It looks better, but there is a race condition here. Consider below
sequence,
thread A takes the clk_scaling_lock and waiting on blk_get_request(), while
thread B has frozen the queue and waiting for the lock.
Thread A
Call ufshcd_exec_dev_cmd() or ufshcd_issue_devman_upiu_cmd()
[a] down_write(hba->clk_scaling_lock)
[d] blk_get_request()
Thread B
Call ufshcd_clock_scaling_prepare()
[b] blk_freeze_queue_start(hba->cmd_queue)
[c] blk_mq_freeze_queue_wait_timeout(hba->cmd_queue) returns > 0
[e] down_write(hba->clk_scaling_lock)
BTW, I see no needs to freeze the hba->cmd_queue in scaling_prepare.
I went through our previous discussions and you mentioned freezing
hba->cmd_queue
can serialize scaling and err handler.
However, it is not necessary and not 100% true. We've already have
ufshcd_eh_in_progress()
check in ufshcd_devfreq_target() before call ufshcd_devfreq_scale().
If you think this is not enough(err handler may kick off after this
check), having
hba->cmd_queue frozen in scaling_prepare() does not mean the err handler
is finished either,
because device management commands are only used in certain steps during
err handler.
Actually, with the original design, we don't see any problems caused by
concurrency of scaling and err handler, and if the concurrency really
happens,
scaling would just fail.
Hi Can,
That's a good catch. When I repost this patch series I will leave out
the freeze and unfreeze operations for hba->cmd_queue.
Bart.
