Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
IronPort-SDR: nG+UQuNEFoK/+L6tjcN4lIU/VvTHIdxBwxjjVBctCrFFwQE4Ds1eUHjrobdBt8isTcNf3iXm9W
 /bI1k1/7gw91I/DZtpIMjDI1buhilVmsT4Pp3U7k+mMeDhiRKqF/CweExqHMiLvO4qvkiCZwr+
 5yLp92QRZ3/1PnPDkZ9/kpEm9zz0Ei5z/KqzoXChfjtB6P1GHl1fH6wbxv97JB9NDBY/WZgwYg
 UsFw2eVkbDgq1F8ovPwqIYX/v1zMO1anl/cRbsJFierLp+QbOUj0uUue/iJdIxgoMs2t9j0OxE
 Gl9C+dvEcKdWqSpMBgZ7xIb/
IronPort-SDR: Muc65ZZ49eDx4Z6fzfzGYKWxgL1CCxUiKKW7gJDgb+vscMoLUhlwuPbnLkeTGhK05+4aXLQKk4
 c7KCFnJi80TxayWnD/utv0dHBNy3OmddnetUGBt5vpiuBOx9pEhm4hFJ4NTO16s69+4U5R9CrQ
 wXnonWi89J3hRJbos7nPVd9rhos194hHKA2l64u+n/inBdtFJUYHyQSO51qx7HHSYhuGaUtNM8
 js7sKbsCxNjvRQk+TAf8AoYA58T+qsD6bPCGQo/TNPeMpBPwCau42RqtJV4YVNu0Ktlu/PvnvN
 AGk=
WDCIronportException: Internal
Message-ID: <4737cbca-6250-00b8-a2be-1d98e2b8d04a@opensource.wdc.com>
Date:   Thu, 29 Sep 2022 11:11:34 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH 2/6] scsi: hisi_sas: Use sas_task_find_rq()
Content-Language: en-US
To:     John Garry <john.garry@huawei.com>, jejb@linux.ibm.com,
        martin.petersen@oracle.com, jinpu.wang@cloud.ionos.com,
        damien.lemoal@wdc.com
Cc:     hare@suse.de, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linuxarm@huawei.com,
        ipylypiv@google.com, changyuanl@google.com, hch@lst.de
References: <1664368034-114991-1-git-send-email-john.garry@huawei.com>
 <1664368034-114991-3-git-send-email-john.garry@huawei.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1664368034-114991-3-git-send-email-john.garry@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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