Re: [PATCH v4 4/4] scsi: core: Call blk_mq_free_tag_set() earlier

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

On 7/14/22 05:25, John Garry wrote:
> On 13/07/2022 21:04, Bart Van Assche wrote:
>>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>>> index 2aca0a838ca5..295c48fdb650 100644
>>>> --- a/drivers/scsi/scsi_lib.c
>>>> +++ b/drivers/scsi/scsi_lib.c
>>>> @@ -1990,7 +1990,10 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>>>>   void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>>>>   {
>>>> +    if (!shost->tag_set.tags)
>>>> +        return;
>>>>       blk_mq_free_tag_set(&shost->tag_set);
>>>> +    WARN_ON_ONCE(shost->tag_set.tags);
>>>
>>> blk_mq_free_tag_set() clears the tagset tags pointer, so I don't know 
>>> why you don't trust the semantics of that API - this seems like 
>>> paranoia.
>>
>> Semantics of the API? Shouldn't this rather be called an undocumented 
>> aspect of blk_mq_free_tag_set()?
>>
>> My concern is that the "set->tags = NULL" statement might be removed 
>> in the future from blk_mq_free_tag_set() and also that it is possible 
>> that scsi_mq_destroy_tags() is not updated after that change.
> 
> Sure, so how it is possible that set->tags == NULL ever when calling 
> scsi_mq_setup_tags()? I'm just wondering why you even care.

How about applying the patch below on top of patch 4/4?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 295c48fdb650..2aca0a838ca5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1990,10 +1990,7 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)

  void scsi_mq_destroy_tags(struct Scsi_Host *shost)
  {
-	if (!shost->tag_set.tags)
-		return;
  	blk_mq_free_tag_set(&shost->tag_set);
-	WARN_ON_ONCE(shost->tag_set.tags);
  }


Thanks,

Bart.