On 4/11/24 20:00, Niklas Cassel wrote:
> On Thu, Apr 11, 2024 at 05:31:58PM +0900, 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>
>> ---
>> 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,
>> - 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;
>
> Here you stack allocate a sgl, and save that stack allocated address
> for sgl in in qc->sg (using ata_sg_init()), however, a stack allocated
> variable is only valid until the scope ends.
> (See my comment at the end of this email.)
>
> So by having the stack allocated variable here (instead of at e.g. the
> top of the function), when __ata_qc_complete() calls ata_sg_clean(),
> or when ata_qc_issue() uses qc->sg, the value that they access might
> be something else, since this variable has already went out of scope,
> and some other variable might have been allocated at that stack address.
Oh, crap. How did I not see crashes with that... Embarassing :)
I will move the declaration of sg at the top of the function.
--
Damien Le Moal
Western Digital Research
