Re: [PATCH AUTOSEL 6.18-5.10] ata: libata-sff: don't busy-wait for PIO data-in command completion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: libata-sff: race between ata_sff_hsm_move() and ata_sff_flush_pio_task()
--

commit ffe3d0726a492588a046ac4c8f5eafd55e0c371b
Author: Richard Weinberger <richard@nod.at>

ata: libata-sff: don't busy-wait for PIO data-in command completion

This commit bounds the inline busy-wait for PIO data-in completion in
ata_sff_hsm_move() to ~100us. If the device remains busy after draining the
final data block, it sets ATA_TFLAG_POLLING and queues ata_sff_pio_task() to
poll for completion in process context.

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 1e2a2c33cdc80..416e1239ed56d 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -1114,8 +1114,14 @@ int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
>  
>  			if (ap->hsm_task_state == HSM_ST_LAST &&
>  			    (!(qc->tf.flags & ATA_TFLAG_WRITE))) {
> -				/* all data read */
> -				status = ata_wait_idle(ap);
> +				status = ata_sff_busy_wait(ap,
> +						ATA_BUSY | ATA_DRQ, 10);
> +				if (status != 0xff &&
> +				    (status & (ATA_BUSY | ATA_DRQ))) {
> +					qc->tf.flags |= ATA_TFLAG_POLLING;
> +					ata_sff_queue_pio_task(link, 0);
> +					return 0;
> +				}
>  				goto fsm_start;
>  			}
>  		}

[Severity: High]
Can queuing ap->sff_pio_task from the interrupt handler here race with
ata_sff_flush_pio_task() during timeout recovery and trigger a BUG_ON()?

Previously, ap->sff_pio_task was only initially queued at command issue time
and re-queued by ata_sff_pio_task() itself, so cancel_delayed_work_sync() in
ata_sff_flush_pio_task() guaranteed the work item would not be re-armed
before ap->hsm_task_state was reset to HSM_ST_IDLE:

drivers/ata/libata-sff.c:ata_sff_flush_pio_task() {
	...
	cancel_delayed_work_sync(&ap->sff_pio_task);

	spin_lock_irq(ap->lock);
	ap->hsm_task_state = HSM_ST_IDLE;
	spin_unlock_irq(ap->lock);

	ap->sff_pio_task_link = NULL;
}

When an interrupt-driven PIO data-in command times out (via
ata_scsi_cmd_error_handler() -> ata_sff_flush_pio_task() or
ata_exec_internal_sg() -> ata_sff_flush_pio_task()), ap->sff_pio_task is not
yet pending, so cancel_delayed_work_sync() returns immediately.

If the final data block interrupt arrives on another CPU before
ata_sff_flush_pio_task() acquires ap->lock:

CPU0                                  CPU1 (IRQ)
ata_sff_flush_pio_task()
  cancel_delayed_work_sync()
                                      __ata_sff_port_intr()
                                        ata_sff_hsm_move()
                                          ata_pio_sectors(qc)
                                          qc->tf.flags |= ATA_TFLAG_POLLING;
                                          ata_sff_queue_pio_task(link, 0);
  spin_lock_irq(ap->lock);
  ap->hsm_task_state = HSM_ST_IDLE;
  spin_unlock_irq(ap->lock);
  ap->sff_pio_task_link = NULL;

When ata_sff_pio_task() then executes on ata_sff_wq, wouldn't it hit
BUG_ON(ap->sff_pio_task_link == NULL), or enter fsm_start with
ap->hsm_task_state == HSM_ST_IDLE while error handling is active?

[ ... ]

>
endasi

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-182-sashal@kernel.org?part=1