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

Damien Le Moal
2022-09-29T02:11:44Z

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