Re: [PATCH v2 2/2] libata: Inline ata_qc_new_init() in ata_scsi_qc_new()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/4/22 23:53, John Garry wrote:
From: Christoph Hellwig <hch@xxxxxx>

It is a bit pointless to have ata_qc_new_init() in libata-core.c since it
pokes scsi internals, so inline it in ata_scsi_qc_new() (in libata-scsi.c).

<Christoph, please provide signed-off-by>
[jpg, Take Christoph's change from list and form into a patch]
Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
---
  Documentation/driver-api/libata.rst | 11 -------
  drivers/ata/libata-core.c           | 44 +---------------------------
  drivers/ata/libata-sata.c           |  8 -----
  drivers/ata/libata-scsi.c           | 45 ++++++++++++++++++++++-------
  drivers/ata/libata.h                | 12 --------
  5 files changed, 35 insertions(+), 85 deletions(-)

diff --git a/Documentation/driver-api/libata.rst b/Documentation/driver-api/libata.rst
index d477e296bda5..311af516a3fd 100644
--- a/Documentation/driver-api/libata.rst
+++ b/Documentation/driver-api/libata.rst
@@ -424,12 +424,6 @@ How commands are issued
  -----------------------
Internal commands
-    First, qc is allocated and initialized using :c:func:`ata_qc_new_init`.
-    Although :c:func:`ata_qc_new_init` doesn't implement any wait or retry
-    mechanism when qc is not available, internal commands are currently
-    issued only during initialization and error recovery, so no other
-    command is active and allocation is guaranteed to succeed.
-
      Once allocated qc's taskfile is initialized for the command to be
      executed. qc currently has two mechanisms to notify completion. One
      is via ``qc->complete_fn()`` callback and the other is completion
@@ -447,11 +441,6 @@ SCSI commands
      translated. No qc is involved in processing a simulated scmd. The
      result is computed right away and the scmd is completed.
- For a translated scmd, :c:func:`ata_qc_new_init` is invoked to allocate a
-    qc and the scmd is translated into the qc. SCSI midlayer's
-    completion notification function pointer is stored into
-    ``qc->scsidone``.
-
      ``qc->complete_fn()`` callback is used for completion notification. ATA
      commands use :c:func:`ata_scsi_qc_complete` while ATAPI commands use
      :c:func:`atapi_qc_complete`. Both functions end up calling ``qc->scsidone``
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1067b2e2be28..5e7d6ccad5da 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4563,43 +4563,6 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words)
  #endif /* __BIG_ENDIAN */
  }
-/**
- *	ata_qc_new_init - Request an available ATA command, and initialize it
- *	@dev: Device from whom we request an available command structure
- *	@scmd: scmd for which to get qc
- *
- *	LOCKING:
- *	None.
- */
-
-struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd)
-{
-	int tag = scsi_cmd_to_rq(scmd)->tag;
-	struct ata_port *ap = dev->link->ap;
-	struct ata_queued_cmd *qc;
-
-	/* no command while frozen */
-	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
-		return NULL;
-
-	/* libsas case */
-	if (ap->flags & ATA_FLAG_SAS_HOST) {
-		tag = ata_sas_get_tag(scmd);
-		if (tag < 0)
-			return NULL;
-	}
-
-	qc = __ata_qc_from_tag(ap, tag);
-	qc->tag = qc->hw_tag = tag;
-	qc->scsicmd = NULL;
-	qc->ap = ap;
-	qc->dev = dev;
-
-	ata_qc_reinit(qc);
-
-	return qc;
-}
-
  /**
   *	ata_qc_free - free unused ata_queued_cmd
   *	@qc: Command to complete
@@ -4612,13 +4575,8 @@ struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd
   */
  void ata_qc_free(struct ata_queued_cmd *qc)
  {
-	unsigned int tag;
-
-	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
-
  	qc->flags = 0;
-	tag = qc->tag;
-	if (ata_tag_valid(tag))
+	if (ata_tag_valid(qc->tag))
  		qc->tag = ATA_TAG_POISON;
  }
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index c3e9fd7d920c..7a5fe41aa5ae 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1268,14 +1268,6 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap)
  }
  EXPORT_SYMBOL_GPL(ata_sas_queuecmd);
-int ata_sas_get_tag(struct scsi_cmnd *scmd)
-{
-	if (WARN_ON_ONCE(scmd->budget_token >= ATA_MAX_QUEUE))
-		return -1;
-
-	return scmd->budget_token;
-}
-
  /**
   *	sata_async_notification - SATA async notification handler
   *	@ap: ATA port where async notification is received
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 61dd7f7c7743..50ef132ec48c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -638,24 +638,47 @@ EXPORT_SYMBOL_GPL(ata_scsi_ioctl);
  static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev,
  					      struct scsi_cmnd *cmd)
  {
+	struct ata_port *ap = dev->link->ap;
  	struct ata_queued_cmd *qc;
+	int tag;
- qc = ata_qc_new_init(dev, cmd);
-	if (qc) {
-		qc->scsicmd = cmd;
-		qc->scsidone = scsi_done;
-
-		qc->sg = scsi_sglist(cmd);
-		qc->n_elem = scsi_sg_count(cmd);
+	if (unlikely(ap->pflags & ATA_PFLAG_FROZEN))
+		goto fail;
- if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
-			qc->flags |= ATA_QCFLAG_QUIET;
+	if (ap->flags & ATA_FLAG_SAS_HOST) {
+		/*
+		 * SAS hosts may queue > ATA_MAX_QUEUE commands so use
+		 * unique per-device budget token as a tag.
+		 */
+		if (WARN_ON_ONCE(cmd->budget_token >= ATA_MAX_QUEUE))
+			goto fail;
+		tag = cmd->budget_token;
  	} else {
-		cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;
-		scsi_done(cmd);
+		tag = scsi_cmd_to_rq(cmd)->tag;
  	}
+ qc = __ata_qc_from_tag(ap, tag);
+	qc->tag = qc->hw_tag = tag;
+	qc->scsicmd = NULL;
+	qc->ap = ap;
+	qc->dev = dev;
+
+	ata_qc_reinit(qc);
+
+	qc->scsicmd = cmd;
+	qc->scsidone = scsi_done;
+
+	qc->sg = scsi_sglist(cmd);
+	qc->n_elem = scsi_sg_count(cmd);
+
+	if (scsi_cmd_to_rq(cmd)->rq_flags & RQF_QUIET)
+		qc->flags |= ATA_QCFLAG_QUIET;

Please add a blank line here.
I like to have return statements stand out :)

  	return qc;
+
+fail:
+	cmd->result = (DID_OK << 16) | SAM_STAT_TASK_SET_FULL;

While at it, it may be better to use:

	set_host_byte(cmd, DID_OK);
	set_status_byte(cmd, SAM_STAT_TASK_SET_FULL);

+	scsi_done(cmd);
+	return NULL;
  }
static void ata_qc_set_pc_nbytes(struct ata_queued_cmd *qc)
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 92e52090165b..926a7f41303d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -44,7 +44,6 @@ static inline void ata_force_cbl(struct ata_port *ap) { }
  #endif
  extern u64 ata_tf_to_lba(const struct ata_taskfile *tf);
  extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf);
-extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, struct scsi_cmnd *scmd);
  extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
  			   u64 block, u32 n_block, unsigned int tf_flags,
  			   unsigned int tag, int class);
@@ -91,17 +90,6 @@ extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
#define to_ata_port(d) container_of(d, struct ata_port, tdev) -/* libata-sata.c */
-#ifdef CONFIG_SATA_HOST
-int ata_sas_get_tag(struct scsi_cmnd *scmd);
-#else
-static inline int ata_sas_get_tag(struct scsi_cmnd *scmd)
-{
-	return -EOPNOTSUPP;
-}
-static inline void ata_sas_free_tag(unsigned int tag, struct ata_port *ap) { }
-#endif
-
  /* libata-acpi.c */
  #ifdef CONFIG_ATA_ACPI
  extern unsigned int ata_acpi_gtf_filter;


--
Damien Le Moal
Western Digital Research



[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux