On 11/04/2024 09:31, Damien Le Moal wrote:
ata_exec_internal() is the only caller of ata_exec_internal_sg() and
always calls this function with a single element scattergather list.
Remove ata_exec_internal_sg() and code it directly in
ata_exec_internal(), simplifying a little the sgl handling for the
command.
While at it, cleanup comments (capitalization) and change the variable
auto_timeout type to a boolean.
No functional change.
Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
Regardless of some minor comments, below:
Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
---
drivers/ata/libata-core.c | 105 ++++++++++++--------------------------
1 file changed, 34 insertions(+), 71 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index be3412cdb22e..ec7e57a0f684 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1480,19 +1480,19 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
}
/**
- * ata_exec_internal_sg - execute libata internal command
+ * ata_exec_internal - execute libata internal command
* @dev: Device to which the command is sent
* @tf: Taskfile registers for the command and the result
* @cdb: CDB for packet command
* @dma_dir: Data transfer direction of the command
- * @sgl: sg list for the data buffer of the command
- * @n_elem: Number of sg entries
+ * @buf: Data buffer of the command
+ * @buflen: Length of data buffer
* @timeout: Timeout in msecs (0 for default)
*
- * Executes libata internal command with timeout. @tf contains
- * command on entry and result on return. Timeout and error
- * conditions are reported via return value. No recovery action
- * is taken after a command times out. It's caller's duty to
+ * Executes libata internal command with timeout. @tf contains
+ * the command on entry and the result on return. Timeout and error
+ * conditions are reported via the return value. No recovery action
+ * is taken after a command times out. It is the caller's duty to
* clean up after timeout.
*
* LOCKING:
@@ -1501,34 +1501,37 @@ static void ata_qc_complete_internal(struct ata_queued_cmd *qc)
* RETURNS:
* Zero on success, AC_ERR_* mask on failure
*/
-static unsigned ata_exec_internal_sg(struct ata_device *dev,
- struct ata_taskfile *tf, const u8 *cdb,
- int dma_dir, struct scatterlist *sgl,
strictly speaking, dma_dir type can be enum dma_data_direction
- unsigned int n_elem, unsigned int timeout)
+unsigned int ata_exec_internal(struct ata_device *dev,
+ struct ata_taskfile *tf, const u8 *cdb,
+ int dma_dir, void *buf, unsigned int buflen,
+ unsigned int timeout)
{
struct ata_link *link = dev->link;
struct ata_port *ap = link->ap;
u8 command = tf->command;
- int auto_timeout = 0;
struct ata_queued_cmd *qc;
unsigned int preempted_tag;
u32 preempted_sactive;
u64 preempted_qc_active;
int preempted_nr_active_links;
+ bool auto_timeout = false;
DECLARE_COMPLETION_ONSTACK(wait);
unsigned long flags;
unsigned int err_mask;
int rc;
+ if (WARN_ON(dma_dir != DMA_NONE && !buf))
+ return AC_ERR_INVALID;
+
spin_lock_irqsave(ap->lock, flags);
- /* no internal command while frozen */
+ /* No internal command while frozen */
if (ata_port_is_frozen(ap)) {
spin_unlock_irqrestore(ap->lock, flags);
return AC_ERR_SYSTEM;
}
- /* initialize internal qc */
+ /* Initialize internal qc */
qc = __ata_qc_from_tag(ap, ATA_TAG_INTERNAL);
qc->tag = ATA_TAG_INTERNAL;
@@ -1547,12 +1550,12 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
ap->qc_active = 0;
ap->nr_active_links = 0;
- /* prepare & issue qc */
+ /* Prepare and issue qc */
qc->tf = *tf;
if (cdb)
memcpy(qc->cdb, cdb, ATAPI_CDB_LEN);
- /* some SATA bridges need us to indicate data xfer direction */
+ /* Some SATA bridges need us to indicate data xfer direction */
if (tf->protocol == ATAPI_PROT_DMA && (dev->flags & ATA_DFLAG_DMADIR) &&
dma_dir == DMA_FROM_DEVICE)
qc->tf.feature |= ATAPI_DMADIR;
@@ -1560,13 +1563,10 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
qc->flags |= ATA_QCFLAG_RESULT_TF;
qc->dma_dir = dma_dir;
if (dma_dir != DMA_NONE) {
- unsigned int i, buflen = 0;
- struct scatterlist *sg;
+ struct scatterlist sgl;
- for_each_sg(sgl, sg, n_elem, i)
- buflen += sg->length;
-
- ata_sg_init(qc, sgl, n_elem);
+ sg_init_one(&sgl, buf, buflen);
+ ata_sg_init(qc, &sgl, 1);
qc->nbytes = buflen;
}
@@ -1578,11 +1578,11 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
spin_unlock_irqrestore(ap->lock, flags);
if (!timeout) {
- if (ata_probe_timeout)
+ if (ata_probe_timeout) {
timeout = ata_probe_timeout * 1000;
- else {
+ } else {
timeout = ata_internal_cmd_timeout(dev, command);
- auto_timeout = 1;
+ auto_timeout = true;
}
}
@@ -1597,28 +1597,29 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
if (!rc) {
spin_lock_irqsave(ap->lock, flags);
- /* We're racing with irq here. If we lose, the
- * following test prevents us from completing the qc
- * twice. If we win, the port is frozen and will be
- * cleaned up by ->post_internal_cmd().
+ /*
+ * We are racing with irq here. If we lose, the following test
+ * prevents us from completing the qc twice. If we win, the port
+ * is frozen and will be cleaned up by ->post_internal_cmd().
*/
Personally I would put the spin_lock_irqsave() call here, below the
comment, but really not important.
if (qc->flags & ATA_QCFLAG_ACTIVE) {
qc->err_mask |= AC_ERR_TIMEOUT;
ata_port_freeze(ap);
- ata_dev_warn(dev, "qc timeout after %u msecs (cmd 0x%x)\n",
+ ata_dev_warn(dev,
+ "qc timeout after %u msecs (cmd 0x%x)\n",
is spanning an extra line really any more readable?
timeout, command);
}
spin_unlock_irqrestore(ap->lock, flags);
}
- /* do post_internal_cmd */
+ /* Do post_internal_cmd */
is that comment useful? I suppose we have many other verbose comments
already..
if (ap->ops->post_internal_cmd)
ap->ops->post_internal_cmd(qc);
- /* perform minimal error analysis */
+ /* Perform minimal error analysis */
if (qc->flags & ATA_QCFLAG_EH) {
if (qc->result_tf.status & (ATA_ERR | ATA_DF))
qc->err_mask |= AC_ERR_DEV;
@@ -1632,7 +1633,7 @@ static unsigned ata_exec_internal_sg(struct ata_device *dev,
qc->result_tf.status |= ATA_SENSE;
}
- /* finish up */
+ /* Finish up */
