[PATCH 0/6] scsi: libsas: Use request tag in more drivers

[PATCH 0/6] scsi: libsas: Use request tag in more drivers

[John Garry]

Currently hisi_sas is the only libsas driver which uses the request tag
for per-HW IO tag.

All other libsas drivers manage the tags internally. Tag management in
pm8001 and mvsas is currently using a simple bitmap, so use the request
tag when available there. With this change we still need to manage tags
for libsas "internal" commands, like SMP commands, and any other
private commands so reserve some tags for this:
- For pm8001 I went with pre-existing and unused PM8001_RESERVE_SLOT size.
  The value is 8, which should be enough. It is greater than mvsas, below,
  but this driver sends a lot of other private commands to HW.
- For mvsas I went with 4, which still should be enough.

isci and aic9xx have elaborate tag alloc schemes, so I'm not going to
bother changing them, especially since I have no HW to test with.

Helper sas_task_find_rq() is added to get the request and associated tag
per sas_task when it is available.

This series looks not to conflict with
https://lore.kernel.org/linux-scsi/20220928070130.3657183-1-yanaijie@huawei.com/T/#mefdcb7b63b4e6ebc8b77a689b3923571ab3d33ab

Based on https://lore.kernel.org/linux-scsi/1664262298-239952-1-git-send-email-john.garry@huawei.com/T/#t

Igor Pylypiv (1):
  scsi: pm8001: Remove pm8001_tag_init()

John Garry (5):
  scsi: libsas: Add sas_task_find_rq()
  scsi: hisi_sas: Use sas_task_find_rq()
  scsi: pm8001: Use sas_task_find_rq() for tagging
  scsi: mvsas: Delete mvs_tag_init()
  scsi: mvsas: Use sas_task_find_rq() for tagging

 drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++----------------
 drivers/scsi/mvsas/mv_defs.h          |  1 +
 drivers/scsi/mvsas/mv_init.c          |  6 ++----
 drivers/scsi/mvsas/mv_sas.c           | 29 ++++++++++++++++-----------
 drivers/scsi/mvsas/mv_sas.h           |  2 --
 drivers/scsi/pm8001/pm8001_init.c     | 12 ++++-------
 drivers/scsi/pm8001/pm8001_sas.c      | 15 +++++++-------
 drivers/scsi/pm8001/pm8001_sas.h      |  7 +++++--
 include/scsi/libsas.h                 | 22 ++++++++++++++++++++
 9 files changed, 67 insertions(+), 53 deletions(-)

-- 
2.35.3

[PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[John Garry]

blk-mq already provides a unique tag per request. Some libsas LLDDs - like
hisi_sas - already use this tag as the unique per-IO HW tag.

Add a common function to provide the request associated with a sas_task
for all libsas LLDDs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/scsi/libsas.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index f86b56bf7833..bc51756a3317 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
 	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
 }
 
+static inline struct request *sas_task_find_rq(struct sas_task *task)
+{
+	struct scsi_cmnd *scmd;
+
+	if (!task || !task->uldd_task)
+		return NULL;
+
+	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
+		struct ata_queued_cmd *qc;
+
+		qc = task->uldd_task;
+		scmd = qc->scsicmd;
+	} else {
+		scmd = task->uldd_task;
+	}
+
+	if (!scmd)
+		return NULL;
+
+	return scsi_cmd_to_rq(scmd);
+}
+
 struct sas_domain_function_template {
 	/* The class calls these to notify the LLDD of an event. */
 	void (*lldd_port_formed)(struct asd_sas_phy *);
-- 
2.35.3

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Jason Yan]

On 2022/9/28 20:27, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   include/scsi/libsas.h | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index f86b56bf7833..bc51756a3317 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>   	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>   }
>   
> +static inline struct request *sas_task_find_rq(struct sas_task *task)
> +{
> +	struct scsi_cmnd *scmd;
> +
> +	if (!task || !task->uldd_task)
> +		return NULL;
> +
> +	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
> +		struct ata_queued_cmd *qc;
> +
> +		qc = task->uldd_task;
> +		scmd = qc->scsicmd;

Can we remove that local qc?

and
	scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd;

Thanks,
Jason

> +	} else {
> +		scmd = task->uldd_task;
> +	}
> +
> +	if (!scmd)
> +		return NULL;
> +
> +	return scsi_cmd_to_rq(scmd);
> +}
> +
>   struct sas_domain_function_template {
>   	/* The class calls these to notify the LLDD of an event. */
>   	void (*lldd_port_formed)(struct asd_sas_phy *);
> 

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[John Garry]

On 28/09/2022 14:13, Jason Yan wrote:
>> +++ b/include/scsi/libsas.h
>> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct 
>> sas_task *task)
>>       return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>   }
>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>> +{
>> +    struct scsi_cmnd *scmd;
>> +
>> +    if (!task || !task->uldd_task)
>> +        return NULL;
>> +
>> +    if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
>> +        struct ata_queued_cmd *qc;
>> +
>> +        qc = task->uldd_task;
>> +        scmd = qc->scsicmd;
> 
> Can we remove that local qc?
>

We could...

> and
>      scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd;

... but I am not really sure that this is much better, specifically 
because of the casting from void. If you feel really strongly about it I 
could.

thanks,
John

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Jason Yan]

On 2022/9/28 21:50, John Garry wrote:
> On 28/09/2022 14:13, Jason Yan wrote:
>>> +++ b/include/scsi/libsas.h
>>> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct 
>>> sas_task *task)
>>>       return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>>   }
>>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>>> +{
>>> +    struct scsi_cmnd *scmd;
>>> +
>>> +    if (!task || !task->uldd_task)
>>> +        return NULL;
>>> +
>>> +    if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
>>> +        struct ata_queued_cmd *qc;
>>> +
>>> +        qc = task->uldd_task;
>>> +        scmd = qc->scsicmd;
>>
>> Can we remove that local qc?
>>
> 
> We could...
> 
>> and
>>      scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd;
> 
> ... but I am not really sure that this is much better, specifically 
> because of the casting from void. If you feel really strongly about it I 
> could.
> 

There are plenty of examples in the kernel, and in scsi itself. Such as

#define fc_host_node_name(x) \
	(((struct fc_host_attrs *)(x)->shost_data)->node_name)


> thanks,
> John
> .

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Johannes Thumshirn]

On 28.09.22 14:34, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  include/scsi/libsas.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index f86b56bf7833..bc51756a3317 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>  	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>  }
>  
> +static inline struct request *sas_task_find_rq(struct sas_task *task)
> +{
> +	struct scsi_cmnd *scmd;
> +
> +	if (!task || !task->uldd_task)
> +		return NULL;

Is anyone actually calling sas_task_find_rq with a NULL task?
That doesn't make a lot of sense from an API POV for me, having
the only argument allowed to be NULL (and not being a *free()
kind of function).

> +
> +	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
> +		struct ata_queued_cmd *qc;
> +
> +		qc = task->uldd_task;
> +		scmd = qc->scsicmd;
> +	} else {
> +		scmd = task->uldd_task;
> +	}
> +
> +	if (!scmd)
> +		return NULL;
> +
> +	return scsi_cmd_to_rq(scmd);
> +}
> +
>  struct sas_domain_function_template {
>  	/* The class calls these to notify the LLDD of an event. */
>  	void (*lldd_port_formed)(struct asd_sas_phy *);

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[John Garry]

On 28/09/2022 14:17, Johannes Thumshirn wrote:
>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>> +{
>> +	struct scsi_cmnd *scmd;
>> +
>> +	if (!task || !task->uldd_task)
>> +		return NULL;
> Is anyone actually calling sas_task_find_rq with a NULL task?

Yeah, unfortunately. An example - the only one I think - is in the 
pm8001 driver: pm8001_ccb_alloc() function which takes a task pointer 
which may be NULL, and this function calls sas_task_find_rq()

> That doesn't make a lot of sense from an API POV for me, having
> the only argument allowed to be NULL (and not being a *free()
> kind of function).

I suppose that I could change to only call sas_task_find_rq() for 
non-NULL sas_task pointers (and remove the task check).

Thanks,
John

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Johannes Thumshirn]

On 28.09.22 15:56, John Garry wrote:
> On 28/09/2022 14:17, Johannes Thumshirn wrote:
>>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>>> +{
>>> +	struct scsi_cmnd *scmd;
>>> +
>>> +	if (!task || !task->uldd_task)
>>> +		return NULL;
>> Is anyone actually calling sas_task_find_rq with a NULL task?
> 
> Yeah, unfortunately. An example - the only one I think - is in the 
> pm8001 driver: pm8001_ccb_alloc() function which takes a task pointer 
> which may be NULL, and this function calls sas_task_find_rq()
> 
>> That doesn't make a lot of sense from an API POV for me, having
>> the only argument allowed to be NULL (and not being a *free()
>> kind of function).
> 
> I suppose that I could change to only call sas_task_find_rq() for 
> non-NULL sas_task pointers (and remove the task check).

If this is done in only a few drivers I'd go that route TBH.

AFAICS hishi sas doesn't call sas_task_find_rq with a NULL task,
so it's only pm8001

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Damien Le Moal]

On 9/28/22 21:27, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  include/scsi/libsas.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index f86b56bf7833..bc51756a3317 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>  	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>  }
>  
> +static inline struct request *sas_task_find_rq(struct sas_task *task)
> +{
> +	struct scsi_cmnd *scmd;
> +
> +	if (!task || !task->uldd_task)
> +		return NULL;
> +
> +	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
> +		struct ata_queued_cmd *qc;
> +
> +		qc = task->uldd_task;

I would change these 2 lines into a single line:

		struct ata_queued_cmd *qc = task->uldd_task;

And no cast as suggested.

> +		scmd = qc->scsicmd;
> +	} else {
> +		scmd = task->uldd_task;
> +	}
> +
> +	if (!scmd)
> +		return NULL;
> +
> +	return scsi_cmd_to_rq(scmd);
> +}
> +
>  struct sas_domain_function_template {
>  	/* The class calls these to notify the LLDD of an event. */
>  	void (*lldd_port_formed)(struct asd_sas_phy *);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[John Garry]

On 29/09/2022 03:09, Damien Le Moal wrote:
> On 9/28/22 21:27, John Garry wrote:
>> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
>> hisi_sas - already use this tag as the unique per-IO HW tag.
>>
>> Add a common function to provide the request associated with a sas_task
>> for all libsas LLDDs.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   include/scsi/libsas.h | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index f86b56bf7833..bc51756a3317 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>>   	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>   }
>>   
>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>> +{
>> +	struct scsi_cmnd *scmd;
>> +
>> +	if (!task || !task->uldd_task)
>> +		return NULL;
>> +
>> +	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
>> +		struct ata_queued_cmd *qc;
>> +
>> +		qc = task->uldd_task;
> 
> I would change these 2 lines into a single line:
> 
> 		struct ata_queued_cmd *qc = task->uldd_task;
> 
> And no cast as suggested.
> 
>> +		scmd = qc->scsicmd;

So do you prefer:

  scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd

As Jason suggested?

Thanks,
John

Re: [PATCH 1/6] scsi: libsas: Add sas_task_find_rq()

[Damien Le Moal]

On 9/29/22 16:33, John Garry wrote:
> On 29/09/2022 03:09, Damien Le Moal wrote:
>> On 9/28/22 21:27, John Garry wrote:
>>> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
>>> hisi_sas - already use this tag as the unique per-IO HW tag.
>>>
>>> Add a common function to provide the request associated with a sas_task
>>> for all libsas LLDDs.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   include/scsi/libsas.h | 22 ++++++++++++++++++++++
>>>   1 file changed, 22 insertions(+)
>>>
>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>> index f86b56bf7833..bc51756a3317 100644
>>> --- a/include/scsi/libsas.h
>>> +++ b/include/scsi/libsas.h
>>> @@ -644,6 +644,28 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>>>   	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>>   }
>>>   
>>> +static inline struct request *sas_task_find_rq(struct sas_task *task)
>>> +{
>>> +	struct scsi_cmnd *scmd;
>>> +
>>> +	if (!task || !task->uldd_task)
>>> +		return NULL;
>>> +
>>> +	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
>>> +		struct ata_queued_cmd *qc;
>>> +
>>> +		qc = task->uldd_task;
>>
>> I would change these 2 lines into a single line:
>>
>> 		struct ata_queued_cmd *qc = task->uldd_task;
>>
>> And no cast as suggested.
>>
>>> +		scmd = qc->scsicmd;
> 
> So do you prefer:
> 
>   scmd = ((struct ata_queued_cmd *)task->uldd_task)->scsicmd

Not a fan of the cast approach suggested by Jason. I prefer my above
suggestion, but no strong feeling about it. Either way will be OK.

> 
> As Jason suggested?
> 
> Thanks,
> John

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/6] scsi: hisi_sas: Use sas_task_find_rq()

[John Garry]

Use sas_task_find_rq() to lookup the request per task for its driver tag.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 4c37ae9eb6b6..1011dffed51f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -177,13 +177,13 @@ static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
 }
 
 static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
-				     struct scsi_cmnd *scsi_cmnd)
+				     struct request *rq)
 {
 	int index;
 	void *bitmap = hisi_hba->slot_index_tags;
 
-	if (scsi_cmnd)
-		return scsi_cmd_to_rq(scsi_cmnd)->tag;
+	if (rq)
+		return rq->tag;
 
 	spin_lock(&hisi_hba->lock);
 	index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
@@ -461,11 +461,11 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	struct asd_sas_port *sas_port = device->port;
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	bool internal_abort = sas_is_internal_abort(task);
-	struct scsi_cmnd *scmd = NULL;
 	struct hisi_sas_dq *dq = NULL;
 	struct hisi_sas_port *port;
 	struct hisi_hba *hisi_hba;
 	struct hisi_sas_slot *slot;
+	struct request *rq = NULL;
 	struct device *dev;
 	int rc;
 
@@ -520,22 +520,12 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 				return -ECOMM;
 		}
 
-		if (task->uldd_task) {
-			struct ata_queued_cmd *qc;
-
-			if (dev_is_sata(device)) {
-				qc = task->uldd_task;
-				scmd = qc->scsicmd;
-			} else {
-				scmd = task->uldd_task;
-			}
-		}
-
-		if (scmd) {
+		rq = sas_task_find_rq(task);
+		if (rq) {
 			unsigned int dq_index;
 			u32 blk_tag;
 
-			blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
+			blk_tag = blk_mq_unique_tag(rq);
 			dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
 			dq = &hisi_hba->dq[dq_index];
 		} else {
@@ -580,7 +570,7 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	if (!internal_abort && hisi_hba->hw->slot_index_alloc)
 		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, device);
 	else
-		rc = hisi_sas_slot_index_alloc(hisi_hba, scmd);
+		rc = hisi_sas_slot_index_alloc(hisi_hba, rq);
 
 	if (rc < 0)
 		goto err_out_dif_dma_unmap;
-- 
2.35.3

Re: [PATCH 2/6] scsi: hisi_sas: Use sas_task_find_rq()

[Damien Le Moal]

On 9/28/22 21:27, John Garry wrote:
> Use sas_task_find_rq() to lookup the request per task for its driver tag.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>

Looks good, modulo the question below.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 4c37ae9eb6b6..1011dffed51f 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -177,13 +177,13 @@ static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
>  }
>  
>  static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
> -				     struct scsi_cmnd *scsi_cmnd)
> +				     struct request *rq)
>  {
>  	int index;
>  	void *bitmap = hisi_hba->slot_index_tags;
>  
> -	if (scsi_cmnd)
> -		return scsi_cmd_to_rq(scsi_cmnd)->tag;
> +	if (rq)
> +		return rq->tag;
>  
>  	spin_lock(&hisi_hba->lock);
>  	index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
> @@ -461,11 +461,11 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>  	struct asd_sas_port *sas_port = device->port;
>  	struct hisi_sas_device *sas_dev = device->lldd_dev;
>  	bool internal_abort = sas_is_internal_abort(task);
> -	struct scsi_cmnd *scmd = NULL;
>  	struct hisi_sas_dq *dq = NULL;
>  	struct hisi_sas_port *port;
>  	struct hisi_hba *hisi_hba;
>  	struct hisi_sas_slot *slot;
> +	struct request *rq = NULL;

Do you really need the NULL initialization here ?

>  	struct device *dev;
>  	int rc;
>  
> @@ -520,22 +520,12 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>  				return -ECOMM;
>  		}
>  
> -		if (task->uldd_task) {
> -			struct ata_queued_cmd *qc;
> -
> -			if (dev_is_sata(device)) {
> -				qc = task->uldd_task;
> -				scmd = qc->scsicmd;
> -			} else {
> -				scmd = task->uldd_task;
> -			}
> -		}
> -
> -		if (scmd) {
> +		rq = sas_task_find_rq(task);
> +		if (rq) {
>  			unsigned int dq_index;
>  			u32 blk_tag;
>  
> -			blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
> +			blk_tag = blk_mq_unique_tag(rq);
>  			dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
>  			dq = &hisi_hba->dq[dq_index];
>  		} else {
> @@ -580,7 +570,7 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>  	if (!internal_abort && hisi_hba->hw->slot_index_alloc)
>  		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, device);
>  	else
> -		rc = hisi_sas_slot_index_alloc(hisi_hba, scmd);
> +		rc = hisi_sas_slot_index_alloc(hisi_hba, rq);
>  
>  	if (rc < 0)
>  		goto err_out_dif_dma_unmap;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 2/6] scsi: hisi_sas: Use sas_task_find_rq()

[John Garry]

On 29/09/2022 03:11, Damien Le Moal wrote:
> On 9/28/22 21:27, John Garry wrote:
>> Use sas_task_find_rq() to lookup the request per task for its driver tag.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
> 
> Looks good, modulo the question below.
> 
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 

Cheers

>> ---
>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++------------------
>>   1 file changed, 8 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index 4c37ae9eb6b6..1011dffed51f 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -177,13 +177,13 @@ static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
>>   }
>>   
>>   static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
>> -				     struct scsi_cmnd *scsi_cmnd)
>> +				     struct request *rq)
>>   {
>>   	int index;
>>   	void *bitmap = hisi_hba->slot_index_tags;
>>   
>> -	if (scsi_cmnd)
>> -		return scsi_cmd_to_rq(scsi_cmnd)->tag;
>> +	if (rq)
>> +		return rq->tag;
>>   
>>   	spin_lock(&hisi_hba->lock);
>>   	index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
>> @@ -461,11 +461,11 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>>   	struct asd_sas_port *sas_port = device->port;
>>   	struct hisi_sas_device *sas_dev = device->lldd_dev;
>>   	bool internal_abort = sas_is_internal_abort(task);
>> -	struct scsi_cmnd *scmd = NULL;
>>   	struct hisi_sas_dq *dq = NULL;
>>   	struct hisi_sas_port *port;
>>   	struct hisi_hba *hisi_hba;
>>   	struct hisi_sas_slot *slot;
>> +	struct request *rq = NULL;
> 
> Do you really need the NULL initialization here ?

Yes, as rq is only set in one case of the switch statement and checked 
outside the switch statement.

Thanks,
John

[PATCH 3/6] scsi: pm8001: Remove pm8001_tag_init()

[John Garry]

From: Igor Pylypiv <ipylypiv@google.com>

In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding
I/O supported to 1024") the pm8001_ha->tags allocation was moved into
pm8001_init_ccb_tag(). This changed the execution order of allocation.
pm8001_tag_init() used to be called after the pm8001_ha->tags allocation
and now it is called before the allocation.

Before:

pm8001_pci_probe()
`--> pm8001_pci_alloc()
     `--> pm8001_alloc()
          `--> pm8001_ha->tags = kzalloc(...)
          `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated

After:

pm8001_pci_probe()
`--> pm8001_pci_alloc()
|    `--> pm8001_alloc()
|         `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated
|
`--> pm8001_init_ccb_tag()
     `-->  pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc()

Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does
nothing. Tags memory is allocated with bitmap_zalloc() so there is no need
to manually clear each bit with pm8001_tag_free().

Reviewed-by: Changyuan Lyu <changyuanl@google.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 2 --
 drivers/scsi/pm8001/pm8001_sas.c  | 7 -------
 drivers/scsi/pm8001/pm8001_sas.h  | 1 -
 3 files changed, 10 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index a0028e130a7e..0edc9857a8bd 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -436,8 +436,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
 		atomic_set(&pm8001_ha->devices[i].running_req, 0);
 	}
 	pm8001_ha->flags = PM8001F_INIT_TIME;
-	/* Initialize tags */
-	pm8001_tag_init(pm8001_ha);
 	return 0;
 
 err_out_nodev:
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index d5ec29f69be3..066dfa9f4683 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -96,13 +96,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
 	return 0;
 }
 
-void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha)
-{
-	int i;
-	for (i = 0; i < pm8001_ha->tags_num; ++i)
-		pm8001_tag_free(pm8001_ha, i);
-}
-
 /**
  * pm8001_mem_alloc - allocate memory for pm8001.
  * @pdev: pci device.
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 8ab0654327f9..9acaadf02150 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -632,7 +632,6 @@ extern struct workqueue_struct *pm8001_wq;
 
 /******************** function prototype *********************/
 int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
-void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);
 u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag);
 void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
 			  struct pm8001_ccb_info *ccb);
-- 
2.35.3

Re: [PATCH 3/6] scsi: pm8001: Remove pm8001_tag_init()

[Damien Le Moal]

On 9/28/22 21:27, John Garry wrote:
> From: Igor Pylypiv <ipylypiv@google.com>
> 
> In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding
> I/O supported to 1024") the pm8001_ha->tags allocation was moved into
> pm8001_init_ccb_tag(). This changed the execution order of allocation.
> pm8001_tag_init() used to be called after the pm8001_ha->tags allocation
> and now it is called before the allocation.
> 
> Before:
> 
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
>      `--> pm8001_alloc()
>           `--> pm8001_ha->tags = kzalloc(...)
>           `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated
> 
> After:
> 
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
> |    `--> pm8001_alloc()
> |         `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated
> |
> `--> pm8001_init_ccb_tag()
>      `-->  pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc()
> 
> Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does
> nothing. Tags memory is allocated with bitmap_zalloc() so there is no need
> to manually clear each bit with pm8001_tag_free().
> 
> Reviewed-by: Changyuan Lyu <changyuanl@google.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: John Garry <john.garry@huawei.com>

Looks good.

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/pm8001/pm8001_init.c | 2 --
>  drivers/scsi/pm8001/pm8001_sas.c  | 7 -------
>  drivers/scsi/pm8001/pm8001_sas.h  | 1 -
>  3 files changed, 10 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index a0028e130a7e..0edc9857a8bd 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -436,8 +436,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
>  		atomic_set(&pm8001_ha->devices[i].running_req, 0);
>  	}
>  	pm8001_ha->flags = PM8001F_INIT_TIME;
> -	/* Initialize tags */
> -	pm8001_tag_init(pm8001_ha);
>  	return 0;
>  
>  err_out_nodev:
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index d5ec29f69be3..066dfa9f4683 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -96,13 +96,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>  	return 0;
>  }
>  
> -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha)
> -{
> -	int i;
> -	for (i = 0; i < pm8001_ha->tags_num; ++i)
> -		pm8001_tag_free(pm8001_ha, i);
> -}
> -
>  /**
>   * pm8001_mem_alloc - allocate memory for pm8001.
>   * @pdev: pci device.
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 8ab0654327f9..9acaadf02150 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -632,7 +632,6 @@ extern struct workqueue_struct *pm8001_wq;
>  
>  /******************** function prototype *********************/
>  int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
> -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);
>  u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag);
>  void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
>  			  struct pm8001_ccb_info *ccb);

-- 
Damien Le Moal
Western Digital Research

[PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

The request associated with a scsi command coming from the block layer
has a unique tag, so use that when possible for getting a CCB.

Unfortunately we don't support reserved commands in the SCSI midlayer yet,
so in the interim continue to manage those tags internally (along with
tags for private commands).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 10 ++++------
 drivers/scsi/pm8001/pm8001_sas.c  |  8 ++++++++
 drivers/scsi/pm8001/pm8001_sas.h  |  6 +++++-
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 0edc9857a8bd..0868836e7391 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -1208,17 +1208,14 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 	struct Scsi_Host *shost = pm8001_ha->shost;
 	struct device *dev = pm8001_ha->dev;
 	u32 max_out_io, ccb_count;
-	u32 can_queue;
 	int i;
 
 	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
 	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
 
-	/* Update to the scsi host*/
-	can_queue = ccb_count - PM8001_RESERVE_SLOT;
-	shost->can_queue = can_queue;
+	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
 
-	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
+	pm8001_ha->tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
 	if (!pm8001_ha->tags)
 		goto err_out;
 
@@ -1244,9 +1241,10 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 		pm8001_ha->ccb_info[i].task = NULL;
 		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
 		pm8001_ha->ccb_info[i].device = NULL;
-		++pm8001_ha->tags_num;
 	}
 
+	pm8001_ha->tags_num = PM8001_RESERVE_SLOT;
+
 	return 0;
 
 err_out_noccb:
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 066dfa9f4683..9d25855af657 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -68,6 +68,11 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
 	void *bitmap = pm8001_ha->tags;
 	unsigned long flags;
 
+	if (tag < pm8001_ha->shost->can_queue)
+		return;
+
+	tag -= pm8001_ha->shost->can_queue;
+
 	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
 	__clear_bit(tag, bitmap);
 	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
@@ -92,6 +97,9 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
 	}
 	__set_bit(tag, bitmap);
 	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
+
+	/* reserved tags are in the upper region of the tagset */
+	tag += pm8001_ha->shost->can_queue;
 	*tag_out = tag;
 	return 0;
 }
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 9acaadf02150..9ff8d1fa84b0 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -737,9 +737,13 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
 		 struct pm8001_device *dev, struct sas_task *task)
 {
 	struct pm8001_ccb_info *ccb;
+	struct request *rq = NULL;
 	u32 tag;
 
-	if (pm8001_tag_alloc(pm8001_ha, &tag)) {
+	rq = sas_task_find_rq(task);
+	if (rq) {
+		tag = rq->tag;
+	} else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
 		pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
 		return NULL;
 	}
-- 
2.35.3

Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[Damien Le Moal]

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 CCB.
> 
> Unfortunately we don't support reserved commands in the SCSI midlayer yet,
> so in the interim continue to manage those tags internally (along with
> tags for private commands).
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 10 ++++------
>  drivers/scsi/pm8001/pm8001_sas.c  |  8 ++++++++
>  drivers/scsi/pm8001/pm8001_sas.h  |  6 +++++-
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 0edc9857a8bd..0868836e7391 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -1208,17 +1208,14 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>  	struct Scsi_Host *shost = pm8001_ha->shost;
>  	struct device *dev = pm8001_ha->dev;
>  	u32 max_out_io, ccb_count;
> -	u32 can_queue;
>  	int i;
>  
>  	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
>  	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
>  
> -	/* Update to the scsi host*/
> -	can_queue = ccb_count - PM8001_RESERVE_SLOT;
> -	shost->can_queue = can_queue;
> +	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
>  
> -	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
> +	pm8001_ha->tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);

The "tags" name for this field is really confusing as it seems to be
implying "all tags". Could we rename that to reserved_tags or similar ?

>  	if (!pm8001_ha->tags)
>  		goto err_out;
>  
> @@ -1244,9 +1241,10 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>  		pm8001_ha->ccb_info[i].task = NULL;
>  		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
>  		pm8001_ha->ccb_info[i].device = NULL;
> -		++pm8001_ha->tags_num;
>  	}
>  
> +	pm8001_ha->tags_num = PM8001_RESERVE_SLOT;

Same here. reserved_tags_num ?
But given that this seems to always be equal to PM8001_RESERVE_SLOT, do
we even need this field at all ?

> +
>  	return 0;
>  
>  err_out_noccb:
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 066dfa9f4683..9d25855af657 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -68,6 +68,11 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>  	void *bitmap = pm8001_ha->tags;
>  	unsigned long flags;
>  
> +	if (tag < pm8001_ha->shost->can_queue)
> +		return;
> +
> +	tag -= pm8001_ha->shost->can_queue;
> +
>  	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>  	__clear_bit(tag, bitmap);
>  	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> @@ -92,6 +97,9 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>  	}
>  	__set_bit(tag, bitmap);
>  	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> +
> +	/* reserved tags are in the upper region of the tagset */
> +	tag += pm8001_ha->shost->can_queue;
>  	*tag_out = tag;
>  	return 0;
>  }
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 9acaadf02150..9ff8d1fa84b0 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -737,9 +737,13 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
>  		 struct pm8001_device *dev, struct sas_task *task)
>  {
>  	struct pm8001_ccb_info *ccb;
> +	struct request *rq = NULL;

I do not think you need the NULL initialization...

>  	u32 tag;
>  
> -	if (pm8001_tag_alloc(pm8001_ha, &tag)) {
> +	rq = sas_task_find_rq(task);
> +	if (rq) {
> +		tag = rq->tag;
> +	} else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
>  		pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
>  		return NULL;
>  	}

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

>> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
>> index 0edc9857a8bd..0868836e7391 100644
>> --- a/drivers/scsi/pm8001/pm8001_init.c
>> +++ b/drivers/scsi/pm8001/pm8001_init.c
>> @@ -1208,17 +1208,14 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>>   	struct Scsi_Host *shost = pm8001_ha->shost;
>>   	struct device *dev = pm8001_ha->dev;
>>   	u32 max_out_io, ccb_count;
>> -	u32 can_queue;
>>   	int i;
>>   
>>   	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
>>   	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
>>   
>> -	/* Update to the scsi host*/
>> -	can_queue = ccb_count - PM8001_RESERVE_SLOT;
>> -	shost->can_queue = can_queue;
>> +	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
>>   
>> -	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
>> +	pm8001_ha->tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
> 
> The "tags" name for this field is really confusing as it seems to be
> implying "all tags". Could we rename that to reserved_tags or similar ?

Sure

> 
>>   	if (!pm8001_ha->tags)
>>   		goto err_out;
>>   
>> @@ -1244,9 +1241,10 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>>   		pm8001_ha->ccb_info[i].task = NULL;
>>   		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
>>   		pm8001_ha->ccb_info[i].device = NULL;
>> -		++pm8001_ha->tags_num;
>>   	}
>>   
>> +	pm8001_ha->tags_num = PM8001_RESERVE_SLOT;
> 
> Same here. reserved_tags_num ?
> But given that this seems to always be equal to PM8001_RESERVE_SLOT, do
> we even need this field at all ?

I don't think so. I can zap it.

> 
>> +
>>   	return 0;
>>   
>>   err_out_noccb:
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>> index 066dfa9f4683..9d25855af657 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -68,6 +68,11 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>>   	void *bitmap = pm8001_ha->tags;
>>   	unsigned long flags;
>>   
>> +	if (tag < pm8001_ha->shost->can_queue)
>> +		return;
>> +
>> +	tag -= pm8001_ha->shost->can_queue;
>> +
>>   	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>>   	__clear_bit(tag, bitmap);
>>   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> @@ -92,6 +97,9 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>>   	}
>>   	__set_bit(tag, bitmap);
>>   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> +
>> +	/* reserved tags are in the upper region of the tagset */
>> +	tag += pm8001_ha->shost->can_queue;
>>   	*tag_out = tag;
>>   	return 0;
>>   }
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
>> index 9acaadf02150..9ff8d1fa84b0 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.h
>> +++ b/drivers/scsi/pm8001/pm8001_sas.h
>> @@ -737,9 +737,13 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
>>   		 struct pm8001_device *dev, struct sas_task *task)
>>   {
>>   	struct pm8001_ccb_info *ccb;
>> +	struct request *rq = NULL;
> 
> I do not think you need the NULL initialization...

Right, but I will actually need to do it if I change sas_task_find_rq() 
to no deal with NULL task

> 
>>   	u32 tag;
>>   
>> -	if (pm8001_tag_alloc(pm8001_ha, &tag)) {
>> +	rq = sas_task_find_rq(task);
>> +	if (rq) {
>> +		tag = rq->tag;
>> +	} else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
>>   		pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
>>   		return NULL;
>>   	}
> 

Thanks,
John

[PATCH 5/6] scsi: mvsas: Delete mvs_tag_init()

[John Garry]

All mvs_tag_init() does is zero the tag bitmap, but this is already done
with the kzalloc() call to alloc the tags, so delete this unneeded
function.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/mvsas/mv_init.c | 2 --
 drivers/scsi/mvsas/mv_sas.c  | 7 -------
 drivers/scsi/mvsas/mv_sas.h  | 1 -
 3 files changed, 10 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 2fde496fff5f..c85fb812ad43 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -286,8 +286,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
 	}
 	mvi->tags_num = slot_nr;
 
-	/* Initialize tags */
-	mvs_tag_init(mvi);
 	return 0;
 err_out:
 	return 1;
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a6867dae0e7c..0810e6c930e1 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -51,13 +51,6 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
 	return 0;
 }
 
-void mvs_tag_init(struct mvs_info *mvi)
-{
-	int i;
-	for (i = 0; i < mvi->tags_num; ++i)
-		mvs_tag_clear(mvi, i);
-}
-
 static struct mvs_info *mvs_find_dev_mvi(struct domain_device *dev)
 {
 	unsigned long i = 0, j = 0, hi = 0;
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index 509d8f32a04f..fe57665bdb50 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -428,7 +428,6 @@ 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);
-void mvs_tag_init(struct mvs_info *mvi);
 void mvs_iounmap(void __iomem *regs);
 int mvs_ioremap(struct mvs_info *mvi, int bar, int bar_ex);
 void mvs_phys_reset(struct mvs_info *mvi, u32 phy_mask, int hard);
-- 
2.35.3

Re: [PATCH 5/6] scsi: mvsas: Delete mvs_tag_init()

[Damien Le Moal]

On 9/28/22 21:27, John Garry wrote:
> All mvs_tag_init() does is zero the tag bitmap, but this is already done
> with the kzalloc() call to alloc the tags, so delete this unneeded
> function.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/mvsas/mv_init.c | 2 --
>  drivers/scsi/mvsas/mv_sas.c  | 7 -------
>  drivers/scsi/mvsas/mv_sas.h  | 1 -
>  3 files changed, 10 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index 2fde496fff5f..c85fb812ad43 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -286,8 +286,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>  	}
>  	mvi->tags_num = slot_nr;
>  
> -	/* Initialize tags */
> -	mvs_tag_init(mvi);
>  	return 0;
>  err_out:
>  	return 1;
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index a6867dae0e7c..0810e6c930e1 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -51,13 +51,6 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
>  	return 0;
>  }
>  
> -void mvs_tag_init(struct mvs_info *mvi)
> -{
> -	int i;
> -	for (i = 0; i < mvi->tags_num; ++i)
> -		mvs_tag_clear(mvi, i);
> -}
> -
>  static struct mvs_info *mvs_find_dev_mvi(struct domain_device *dev)
>  {
>  	unsigned long i = 0, j = 0, hi = 0;
> diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
> index 509d8f32a04f..fe57665bdb50 100644
> --- a/drivers/scsi/mvsas/mv_sas.h
> +++ b/drivers/scsi/mvsas/mv_sas.h
> @@ -428,7 +428,6 @@ 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);
> -void mvs_tag_init(struct mvs_info *mvi);
>  void mvs_iounmap(void __iomem *regs);
>  int mvs_ioremap(struct mvs_info *mvi, int bar, int bar_ex);
>  void mvs_phys_reset(struct mvs_info *mvi, u32 phy_mask, int hard);

-- 
Damien Le Moal
Western Digital Research

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

[John Garry]

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;
 
 	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);
-- 
2.35.3

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

[Damien Le Moal]

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);

-- 
Damien Le Moal
Western Digital Research

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

[John Garry]

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

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

[Damien Le Moal]

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
> 
> 

-- 
Damien Le Moal
Western Digital Research