diff mbox series

[6/6] scsi: mvsas: Use sas_task_find_rq() for tagging

Message ID 1664368034-114991-7-git-send-email-john.garry@huawei.com
State New
Headers show
Series scsi: libsas: Use request tag in more drivers | expand

Commit Message

John Garry Sept. 28, 2022, 12:27 p.m. UTC
The request associated with a scsi command coming from the block layer
has a unique tag, so use that when possible for getting a slot.

Unfortunately we don't support reserved commands in the SCSI midlayer yet.
As such, SMP tasks - as an example - will not have a request associated, so
in the interim continue to manage those tags for that type of sas_task
internally.

We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
but what those 2 slots are used for is not obvious.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/mvsas/mv_defs.h |  1 +
 drivers/scsi/mvsas/mv_init.c |  4 ++--
 drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
 drivers/scsi/mvsas/mv_sas.h  |  1 -
 4 files changed, 20 insertions(+), 8 deletions(-)

Comments

Damien Le Moal Sept. 29, 2022, 2:22 a.m. UTC | #1
On 9/28/22 21:27, John Garry wrote:
> The request associated with a scsi command coming from the block layer
> has a unique tag, so use that when possible for getting a slot.
> 
> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
> As such, SMP tasks - as an example - will not have a request associated, so
> in the interim continue to manage those tags for that type of sas_task
> internally.
> 
> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
> but what those 2 slots are used for is not obvious.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/mvsas/mv_defs.h |  1 +
>  drivers/scsi/mvsas/mv_init.c |  4 ++--
>  drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
>  drivers/scsi/mvsas/mv_sas.h  |  1 -
>  4 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
> index 7123a2efbf58..8ef174cd4d37 100644
> --- a/drivers/scsi/mvsas/mv_defs.h
> +++ b/drivers/scsi/mvsas/mv_defs.h
> @@ -40,6 +40,7 @@ enum driver_configuration {
>  	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>  	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>  	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
> +	MVS_RSVD_SLOTS		= 4,
>  	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>  };
>  
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index c85fb812ad43..d834ed9e8e4a 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>  			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>  			goto err_out;
>  	}
> -	mvi->tags_num = slot_nr;
> +	mvi->tags_num = MVS_RSVD_SLOTS;

Same comment as for pm8001: do you really need this field if the value
is always MVS_RSVD_SLOTS ?

>  
>  	return 0;
>  err_out:
> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>  	mvi->sas = sha;
>  	mvi->shost = shost;
>  
> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
> +	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);

Field name ? reserved_tags ?
Also, the alloc seems wrong. This will allocate 4 bytes, but you only
need 4 bits. You could make this an unsigned long and not allocate
anything. Same remark for pm8001 by the way.

That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy
to check at compile time with a #if/#error.


>  	if (!mvi->tags)
>  		goto err_out;
>  
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 0810e6c930e1..549d2ec89f60 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -20,7 +20,7 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag)
>  	return 0;
>  }
>  
> -void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
> +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
>  {
>  	void *bitmap = mvi->tags;
>  	clear_bit(tag, bitmap);
> @@ -28,6 +28,11 @@ void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
>  
>  void mvs_tag_free(struct mvs_info *mvi, u32 tag)
>  {
> +	if (tag < mvi->shost->can_queue)
> +		return;
> +
> +	tag -= mvi->shost->can_queue;
> +
>  	mvs_tag_clear(mvi, tag);
>  }
>  
> @@ -47,6 +52,7 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
>  	if (tag >= mvi->tags_num)
>  		return -SAS_QUEUE_FULL;
>  	mvs_tag_set(mvi, tag);
> +	tag += mvi->shost->can_queue;
>  	*tag_out = tag;
>  	return 0;
>  }
> @@ -696,6 +702,7 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
>  	struct mvs_task_exec_info tei;
>  	struct mvs_slot_info *slot;
>  	u32 tag = 0xdeadbeef, n_elem = 0;
> +	struct request *rq;
>  	int rc = 0;
>  
>  	if (!dev->port) {
> @@ -760,9 +767,14 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
>  		n_elem = task->num_scatter;
>  	}
>  
> -	rc = mvs_tag_alloc(mvi, &tag);
> -	if (rc)
> -		goto err_out;
> +	rq = sas_task_find_rq(task);
> +	if (rq) {
> +		tag = rq->tag;
> +	} else {
> +		rc = mvs_tag_alloc(mvi, &tag);
> +		if (rc)
> +			goto err_out;
> +	}
>  
>  	slot = &mvi->slot_info[tag];
>  
> @@ -857,7 +869,7 @@ int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
>  static void mvs_slot_free(struct mvs_info *mvi, u32 rx_desc)
>  {
>  	u32 slot_idx = rx_desc & RXQ_SLOT_MASK;
> -	mvs_tag_clear(mvi, slot_idx);
> +	mvs_tag_free(mvi, slot_idx);
>  }
>  
>  static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task,
> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
> index fe57665bdb50..e6c70786ded9 100644
> --- a/drivers/scsi/mvsas/mv_sas.h
> +++ b/drivers/scsi/mvsas/mv_sas.h
> @@ -424,7 +424,6 @@ struct mvs_task_exec_info {
>  
>  /******************** function prototype *********************/
>  void mvs_get_sas_addr(void *buf, u32 buflen);
> -void mvs_tag_clear(struct mvs_info *mvi, u32 tag);
>  void mvs_tag_free(struct mvs_info *mvi, u32 tag);
>  void mvs_tag_set(struct mvs_info *mvi, unsigned int tag);
>  int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);
John Garry Sept. 29, 2022, 7:49 a.m. UTC | #2
On 29/09/2022 03:22, Damien Le Moal wrote:
> On 9/28/22 21:27, John Garry wrote:
>> The request associated with a scsi command coming from the block layer
>> has a unique tag, so use that when possible for getting a slot.
>>
>> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
>> As such, SMP tasks - as an example - will not have a request associated, so
>> in the interim continue to manage those tags for that type of sas_task
>> internally.
>>
>> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
>> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
>> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
>> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
>> but what those 2 slots are used for is not obvious.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/mvsas/mv_defs.h |  1 +
>>   drivers/scsi/mvsas/mv_init.c |  4 ++--
>>   drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
>>   drivers/scsi/mvsas/mv_sas.h  |  1 -
>>   4 files changed, 20 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
>> index 7123a2efbf58..8ef174cd4d37 100644
>> --- a/drivers/scsi/mvsas/mv_defs.h
>> +++ b/drivers/scsi/mvsas/mv_defs.h
>> @@ -40,6 +40,7 @@ enum driver_configuration {
>>   	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>>   	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>>   	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
>> +	MVS_RSVD_SLOTS		= 4,
>>   	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>>   };
>>   
>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>> index c85fb812ad43..d834ed9e8e4a 100644
>> --- a/drivers/scsi/mvsas/mv_init.c
>> +++ b/drivers/scsi/mvsas/mv_init.c
>> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>>   			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>>   			goto err_out;
>>   	}
>> -	mvi->tags_num = slot_nr;
>> +	mvi->tags_num = MVS_RSVD_SLOTS;
> 
> Same comment as for pm8001: do you really need this field if the value
> is always MVS_RSVD_SLOTS ?

Right, I don't need this struct member. Again I can just use this macro 
directly.

> 
>>   
>>   	return 0;
>>   err_out:
>> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>>   	mvi->sas = sha;
>>   	mvi->shost = shost;
>>   
>> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
>> +	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
> 
> Field name ? reserved_tags ?
> Also, the alloc seems wrong. This will allocate 4 bytes, but you only
> need 4 bits. You could make this an unsigned long and not allocate
> anything. 

Well spotted. I should have questioned more why they had >>3 previously.

But I would rather keep as a bitmap, i.e. *unsigned long for simplicity.

> Same remark for pm8001 by the way.

I think it's ok as it uses bitmap_zalloc()

> 
> That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy
> to check at compile time with a #if/#error.
> 

As above, I'd rather keep as a bitmap. It's a little inefficient, but is 
a one off in the driver.

Thanks,
John
Damien Le Moal Sept. 29, 2022, 8:02 a.m. UTC | #3
On 9/29/22 16:49, John Garry wrote:
> On 29/09/2022 03:22, Damien Le Moal wrote:
>> On 9/28/22 21:27, John Garry wrote:
>>> The request associated with a scsi command coming from the block layer
>>> has a unique tag, so use that when possible for getting a slot.
>>>
>>> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
>>> As such, SMP tasks - as an example - will not have a request associated, so
>>> in the interim continue to manage those tags for that type of sas_task
>>> internally.
>>>
>>> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
>>> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
>>> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
>>> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
>>> but what those 2 slots are used for is not obvious.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/scsi/mvsas/mv_defs.h |  1 +
>>>   drivers/scsi/mvsas/mv_init.c |  4 ++--
>>>   drivers/scsi/mvsas/mv_sas.c  | 22 +++++++++++++++++-----
>>>   drivers/scsi/mvsas/mv_sas.h  |  1 -
>>>   4 files changed, 20 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
>>> index 7123a2efbf58..8ef174cd4d37 100644
>>> --- a/drivers/scsi/mvsas/mv_defs.h
>>> +++ b/drivers/scsi/mvsas/mv_defs.h
>>> @@ -40,6 +40,7 @@ enum driver_configuration {
>>>   	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>>>   	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>>>   	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
>>> +	MVS_RSVD_SLOTS		= 4,
>>>   	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>>>   };
>>>   
>>> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
>>> index c85fb812ad43..d834ed9e8e4a 100644
>>> --- a/drivers/scsi/mvsas/mv_init.c
>>> +++ b/drivers/scsi/mvsas/mv_init.c
>>> @@ -284,7 +284,7 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>>>   			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>>>   			goto err_out;
>>>   	}
>>> -	mvi->tags_num = slot_nr;
>>> +	mvi->tags_num = MVS_RSVD_SLOTS;
>>
>> Same comment as for pm8001: do you really need this field if the value
>> is always MVS_RSVD_SLOTS ?
> 
> Right, I don't need this struct member. Again I can just use this macro 
> directly.
> 
>>
>>>   
>>>   	return 0;
>>>   err_out:
>>> @@ -367,7 +367,7 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>>>   	mvi->sas = sha;
>>>   	mvi->shost = shost;
>>>   
>>> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
>>> +	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
>>
>> Field name ? reserved_tags ?
>> Also, the alloc seems wrong. This will allocate 4 bytes, but you only
>> need 4 bits. You could make this an unsigned long and not allocate
>> anything. 
> 
> Well spotted. I should have questioned more why they had >>3 previously.
> 
> But I would rather keep as a bitmap, i.e. *unsigned long for simplicity.>
>> Same remark for pm8001 by the way.
> 
> I think it's ok as it uses bitmap_zalloc()

Yes !

> 
>>
>> That would cap MVS_RSVD_SLOTS to BITS_PER_LONG maximum, but that is easy
>> to check at compile time with a #if/#error.
>>
> 
> As above, I'd rather keep as a bitmap. It's a little inefficient, but is 
> a one off in the driver.
> 
> Thanks,
> John
> 
>
diff mbox series

Patch

diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
index 7123a2efbf58..8ef174cd4d37 100644
--- a/drivers/scsi/mvsas/mv_defs.h
+++ b/drivers/scsi/mvsas/mv_defs.h
@@ -40,6 +40,7 @@  enum driver_configuration {
 	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
 	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
 	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
+	MVS_RSVD_SLOTS		= 4,
 	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
 };
 
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index c85fb812ad43..d834ed9e8e4a 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -284,7 +284,7 @@  static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
 			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
 			goto err_out;
 	}
-	mvi->tags_num = slot_nr;
+	mvi->tags_num = MVS_RSVD_SLOTS;
 
 	return 0;
 err_out:
@@ -367,7 +367,7 @@  static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
 	mvi->sas = sha;
 	mvi->shost = shost;
 
-	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
+	mvi->tags = kzalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
 	if (!mvi->tags)
 		goto err_out;
 
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 0810e6c930e1..549d2ec89f60 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -20,7 +20,7 @@  static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag)
 	return 0;
 }
 
-void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
+static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
 {
 	void *bitmap = mvi->tags;
 	clear_bit(tag, bitmap);
@@ -28,6 +28,11 @@  void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
 
 void mvs_tag_free(struct mvs_info *mvi, u32 tag)
 {
+	if (tag < mvi->shost->can_queue)
+		return;
+
+	tag -= mvi->shost->can_queue;
+
 	mvs_tag_clear(mvi, tag);
 }
 
@@ -47,6 +52,7 @@  inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
 	if (tag >= mvi->tags_num)
 		return -SAS_QUEUE_FULL;
 	mvs_tag_set(mvi, tag);
+	tag += mvi->shost->can_queue;
 	*tag_out = tag;
 	return 0;
 }
@@ -696,6 +702,7 @@  static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
 	struct mvs_task_exec_info tei;
 	struct mvs_slot_info *slot;
 	u32 tag = 0xdeadbeef, n_elem = 0;
+	struct request *rq;
 	int rc = 0;
 
 	if (!dev->port) {
@@ -760,9 +767,14 @@  static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
 		n_elem = task->num_scatter;
 	}
 
-	rc = mvs_tag_alloc(mvi, &tag);
-	if (rc)
-		goto err_out;
+	rq = sas_task_find_rq(task);
+	if (rq) {
+		tag = rq->tag;
+	} else {
+		rc = mvs_tag_alloc(mvi, &tag);
+		if (rc)
+			goto err_out;
+	}
 
 	slot = &mvi->slot_info[tag];
 
@@ -857,7 +869,7 @@  int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
 static void mvs_slot_free(struct mvs_info *mvi, u32 rx_desc)
 {
 	u32 slot_idx = rx_desc & RXQ_SLOT_MASK;
-	mvs_tag_clear(mvi, slot_idx);
+	mvs_tag_free(mvi, slot_idx);
 }
 
 static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task,
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index fe57665bdb50..e6c70786ded9 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -424,7 +424,6 @@  struct mvs_task_exec_info {
 
 /******************** function prototype *********************/
 void mvs_get_sas_addr(void *buf, u32 buflen);
-void mvs_tag_clear(struct mvs_info *mvi, u32 tag);
 void mvs_tag_free(struct mvs_info *mvi, u32 tag);
 void mvs_tag_set(struct mvs_info *mvi, unsigned int tag);
 int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);