On 1/18/19 10:51 AM, Bart Van Assche wrote:
> On Fri, 2019-01-18 at 10:48 -0700, Jens Axboe wrote:
>> It's UFS that totally buggy, if you look at its queuecommand, it does:
>>
>> if (!down_read_trylock(&hba->clk_scaling_lock))
>> return SCSI_MLQUEUE_HOST_BUSY;
>>
>> UFS either needs to get fixed up, or we'll want a way to do something like
>> the below.
>>
>> Marc, can you test this?
>>
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index eaf329db3973..e28c3420a9d9 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -412,6 +412,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>> shost->hostt = sht;
>> shost->this_id = sht->this_id;
>> shost->can_queue = sht->can_queue;
>> + shost->queue_may_block = sht->queue_may_block;
>> shost->sg_tablesize = sht->sg_tablesize;
>> shost->sg_prot_tablesize = sht->sg_prot_tablesize;
>> shost->cmd_per_lun = sht->cmd_per_lun;
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index b13cc9288ba0..4e266af2871f 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1902,6 +1902,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>> shost->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
>> shost->tag_set.flags |=
>> BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy);
>> + if (shost->queue_may_blocK)
>> + shost->tag_set.flags |= BLK_MQ_F_BLOCKING;
>> shost->tag_set.driver_data = shost;
>>
>> return blk_mq_alloc_tag_set(&shost->tag_set);
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 9ba7671b84f8..9ab354e43630 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -6981,6 +6981,7 @@ static struct scsi_host_template ufshcd_driver_template = {
>> .sg_tablesize = SG_ALL,
>> .cmd_per_lun = UFSHCD_CMD_PER_LUN,
>> .can_queue = UFSHCD_CAN_QUEUE,
>> + .queue_may_block = 1,
>> .max_host_blocked = 1,
>> .track_queue_depth = 1,
>> .sdev_groups = ufshcd_driver_groups,
>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>> index 6ca954e9f752..30aa7b6c4342 100644
>> --- a/include/scsi/scsi_host.h
>> +++ b/include/scsi/scsi_host.h
>> @@ -339,6 +339,11 @@ struct scsi_host_template {
>> */
>> int can_queue;
>>
>> + /*
>> + * If the ->queuecommand() ever blocks, this should be set
>> + */
>> + int queue_may_block;
>> +
>> /*
>> * In many instances, especially where disconnect / reconnect are
>> * supported, our host also has an ID on the SCSI bus. If this is
>> @@ -584,6 +589,7 @@ struct Scsi_Host {
>>
>> int this_id;
>> int can_queue;
>> + int queue_may_block;
>> short cmd_per_lun;
>> short unsigned int sg_tablesize;
>> short unsigned int sg_prot_tablesize;
>
> Hi Jens,
>
> Did you perhaps want to include a change from down_read_trylock() into
> down_read() in the UFS queuecommand function in this patch?
Yeah, that should be done as well. But honestly, looking at UFS, it's in
dire need of love from someone that has some experience in writing a
driver. The locking is absolutely miserable.
For one, the clk_scaling_lock needs to die. The clock scaling change
should coordinate with the requests, not try to lock out request
handling. It's totally an upside down approach.
--
Jens Axboe
