Re: [PATCH v5 14/52] advansys: Use scsi_cmd_to_rq() instead of scsi_cmnd.request

[John Garry <john.garry@huawei.com>]

On 12/08/2021 07:03, Hannes Reinecke wrote:
> Go for it.
> I'm not particularly keen on the 'scsi_cmd_to_rq(cmd)->tag' construct, 
> as this implies that 'scsi_cmd_to_rq()' has to be a define, not a function.

What is the problem exactly?

> Having a wrapper for scsi_cmd_to_tag() resolves that.

So which tag does scsi_cmd_to_tag() return? The request tag? 
scsi_cmnd.tag? Or, if there is an ata_queued_cmd associated with the 
scsi_cmnd, ata_queued_cmd.tag? Maybe it's blindingly obvious, but I 
don't see value in introducing possibility of misuse.

Having said that, scsi_cmnd.tag could be removed:

---->8----

[PATCH 1/3] scsi: wd719: Stop using scsi_cmnd.tag

Use scsi_cmd_to_rq(cmd)->tag instead.

Signed-off-by: John Garry <john.garry@huawei.com>
---
  drivers/scsi/wd719x.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index edc8a139a60d..4f4ebb9c5b09 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -467,13 +467,13 @@ static int wd719x_abort(struct scsi_cmnd *cmd)
  	struct wd719x_scb *scb = scsi_cmd_priv(cmd);
  	struct wd719x *wd = shost_priv(cmd->device->host);

-	dev_info(&wd->pdev->dev, "abort command, tag: %x\n", cmd->tag);
+	dev_info(&wd->pdev->dev, "abort command, tag: %x\n", 
scsi_cmd_to_rq(cmd)->tag);

-	action = /*cmd->tag ? WD719X_CMD_ABORT_TAG : */WD719X_CMD_ABORT;
+	action = /*scsi_cmd_to_rq(cmd)->tag ? WD719X_CMD_ABORT_TAG : 
*/WD719X_CMD_ABORT;

  	spin_lock_irqsave(wd->sh->host_lock, flags);
  	result = wd719x_direct_cmd(wd, action, cmd->device->id,
-				   cmd->device->lun, cmd->tag, scb->phys, 0);
+				   cmd->device->lun, scsi_cmd_to_rq(cmd)->tag, scb->phys, 0);
  	wd719x_finish_cmd(scb, DID_ABORT);
  	spin_unlock_irqrestore(wd->sh->host_lock, flags);
  	if (result)
-- 
[PATCH 2/3] scsi: fnic: Stop setting scsi_cmnd.tag

It is never read. Setting it and the request tag seems dodgy
anyway.

Signed-off-by: John Garry <john.garry@huawei.com>
---
  drivers/scsi/fnic/fnic_scsi.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index 0f9cedf78872..f8afbfb468dc 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -2213,7 +2213,7 @@ fnic_scsi_host_start_tag(struct fnic *fnic, struct 
scsi_cmnd *sc)
  	if (IS_ERR(dummy))
  		return SCSI_NO_TAG;

-	sc->tag = rq->tag = dummy->tag;
+	rq->tag = dummy->tag;
  	sc->host_scribble = (unsigned char *)dummy;

  	return dummy->tag;
-- 
[PATCH 3/3] scsi: Remove scsi_cmnd.tag

There are no users.

Signed-off-by: John Garry <john.garry@huawei.com>
---
  drivers/scsi/scsi_lib.c  | 1 -
  include/scsi/scsi_cmnd.h | 1 -
  2 files changed, 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9ba1aa7530a9..572673873ddf 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1540,7 +1540,6 @@ static blk_status_t scsi_prepare_cmd(struct 
request *req)

  	scsi_init_command(sdev, cmd);

-	cmd->tag = req->tag;
  	cmd->prot_op = SCSI_PROT_NORMAL;
  	if (blk_rq_bytes(req))
  		cmd->sc_data_direction = rq_dma_dir(req);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 6c5a1c1c6b1e..eaf04c9a1dfc 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -139,7 +139,6 @@ struct scsi_cmnd {
  	int flags;		/* Command flags */
  	unsigned long state;	/* Command completion state */

-	unsigned char tag;	/* SCSI-II queued command tag */
  	unsigned int extra_len;	/* length of alignment and padding */
  };

-- 

----8<-----

Bart, feel free to make a similar change - I don't want you to think I'm 
hijacking your work.

Thanks!