Re: [PATCH v6 4/4] ufs: Simplify the clock scaling mechanism implementation

[Bart Van Assche <bvanassche@acm.org>]

On 12/1/19 9:39 PM, cang@codeaurora.org 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.