[PATCH v2 0/2] ATA port deferred qc fixes

[PATCH v2 0/2] ATA port deferred qc fixes

[Damien Le Moal]

The first patch addresses a use-after-free issue when a deferred qc
times out. The second patch avoids a call to a potentially sleeping
function while a port spinlock is held.

Changes from v1:
 - Corrected typo in patch 1 message, improved comment in code and added
   a WARN_ON_ONCE() call to verify that a timed out qc is not active.
 - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
   port lock held. This call is in fact removed: it is not needed as
   ata_scsi_requeue_deferred_qc() is called in EH, which is always run
   when removing a port.

Damien Le Moal (2):
  ata: libata-eh: correctly handle deferred qc timeouts
  ata: libata-core: fix cancellation of a port deferred qc work

 drivers/ata/libata-core.c |  8 +++-----
 drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 8 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

A deferred qc may timeout while waiting for the device queue to drain
to be submitted. In such case, since the qc is not active,
ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
which frees the qc. But as the port deferred_qc field still references
this finished/freed qc, the deferred qc work may eventually attempt to
call ata_qc_issue() against this invalid qc, leading to errors such as
reported by UBSAN (syzbot run):

UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
...
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
 ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
 ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
 process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
 process_scheduled_works kernel/workqueue.c:3358 [inline]
 worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
 kthread+0x370/0x450 kernel/kthread.c:467
 ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
 </TASK>

Fix this by checking if the qc of a timed out SCSI command is a deferred
one, and in such case, clear the port deferred_qc field and finish the
SCSI command with DID_TIME_OUT.

Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 72a22b6c9682..b373cceb95d2 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
 		set_host_byte(scmd, DID_OK);
 
 		ata_qc_for_each_raw(ap, qc, i) {
-			if (qc->flags & ATA_QCFLAG_ACTIVE &&
-			    qc->scsicmd == scmd)
+			if (qc->scsicmd != scmd)
+				continue;
+			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
+			    qc == ap->deferred_qc)
 				break;
 		}
 
-		if (i < ATA_MAX_QUEUE) {
+		if (qc == ap->deferred_qc) {
+			/*
+			 * This is a deferred command that timed out while
+			 * waiting for the command queue to drain. Since the qc
+			 * is not active yet (deferred_qc is still set, so the
+			 * deferred qc work has not issued the command yet),
+			 * simply signal the timeout by finishing the SCSI
+			 * command and clear the deferred qc to prevent the
+			 * deferred qc work from issuing this qc.
+			 */
+			WARN_ON_ONCE(qc->flags & ATA_QCFLAG_ACTIVE);
+			ap->deferred_qc = NULL;
+			set_host_byte(scmd, DID_TIME_OUT);
+			scsi_eh_finish_cmd(scmd, &ap->eh_done_q);
+		} else if (i < ATA_MAX_QUEUE) {
 			/* the scmd has an associated qc */
 			if (!(qc->flags & ATA_QCFLAG_EH)) {
 				/* which hasn't failed yet, timeout */
-- 
2.53.0

[PATCH v2 2/2] ata: libata-core: fix cancellation of a port deferred qc work

[Damien Le Moal]

cancel_work_sync() is a sleeping function so it cannot be called with
the spin lock of a port being held. Move the call to this function in
ata_port_detach() after EH completes, with the port lock released,
together with other work cancellation calls.

Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index beb6984b379a..d470b7bc92c7 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6269,10 +6269,6 @@ static void ata_port_detach(struct ata_port *ap)
 		}
 	}
 
-	/* Make sure the deferred qc work finished. */
-	cancel_work_sync(&ap->deferred_qc_work);
-	WARN_ON(ap->deferred_qc);
-
 	/* Tell EH to disable all devices */
 	ap->pflags |= ATA_PFLAG_UNLOADING;
 	ata_port_schedule_eh(ap);
@@ -6283,9 +6279,11 @@ static void ata_port_detach(struct ata_port *ap)
 	/* wait till EH commits suicide */
 	ata_port_wait_eh(ap);
 
-	/* it better be dead now */
+	/* It better be dead now and not have any remaining deferred qc. */
 	WARN_ON(!(ap->pflags & ATA_PFLAG_UNLOADED));
+	WARN_ON(ap->deferred_qc);
 
+	cancel_work_sync(&ap->deferred_qc_work);
 	cancel_delayed_work_sync(&ap->hotplug_task);
 	cancel_delayed_work_sync(&ap->scsi_rescan_task);
 
-- 
2.53.0

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Hannes Reinecke]

On 2/20/26 23:14, Damien Le Moal wrote:
> A deferred qc may timeout while waiting for the device queue to drain
> to be submitted. In such case, since the qc is not active,
> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> which frees the qc. But as the port deferred_qc field still references
> this finished/freed qc, the deferred qc work may eventually attempt to
> call ata_qc_issue() against this invalid qc, leading to errors such as
> reported by UBSAN (syzbot run):
> 
> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> ...
> Call Trace:
>   <TASK>
>   __dump_stack lib/dump_stack.c:94 [inline]
>   dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>   ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>   __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>   ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>   ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>   process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>   process_scheduled_works kernel/workqueue.c:3358 [inline]
>   worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>   kthread+0x370/0x450 kernel/kthread.c:467
>   ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>   </TASK>
> 
> Fix this by checking if the qc of a timed out SCSI command is a deferred
> one, and in such case, clear the port deferred_qc field and finish the
> SCSI command with DID_TIME_OUT.
> 
> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 2/2] ata: libata-core: fix cancellation of a port deferred qc work

[Hannes Reinecke]

On 2/20/26 23:14, Damien Le Moal wrote:
> cancel_work_sync() is a sleeping function so it cannot be called with
> the spin lock of a port being held. Move the call to this function in
> ata_port_detach() after EH completes, with the port lock released,
> together with other work cancellation calls.
> 
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   drivers/ata/libata-core.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Igor Pylypiv]

On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
> A deferred qc may timeout while waiting for the device queue to drain
> to be submitted. In such case, since the qc is not active,
> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> which frees the qc. But as the port deferred_qc field still references
> this finished/freed qc, the deferred qc work may eventually attempt to
> call ata_qc_issue() against this invalid qc, leading to errors such as
> reported by UBSAN (syzbot run):
> 
> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> ...
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>  process_scheduled_works kernel/workqueue.c:3358 [inline]
>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>  kthread+0x370/0x450 kernel/kthread.c:467
>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>  </TASK>
> 
> Fix this by checking if the qc of a timed out SCSI command is a deferred
> one, and in such case, clear the port deferred_qc field and finish the
> SCSI command with DID_TIME_OUT.
> 
> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thanks,
Igor

Re: [PATCH v2 2/2] ata: libata-core: fix cancellation of a port deferred qc work

[Igor Pylypiv]

On Sat, Feb 21, 2026 at 07:14:39AM +0900, Damien Le Moal wrote:
> cancel_work_sync() is a sleeping function so it cannot be called with
> the spin lock of a port being held. Move the call to this function in
> ata_port_detach() after EH completes, with the port lock released,
> together with other work cancellation calls.
> 
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Igor Pylypiv <ipylypiv@google.com>

Thanks,
Igor

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Damien Le Moal]

On 2/21/26 7:14 AM, Damien Le Moal wrote:
> The first patch addresses a use-after-free issue when a deferred qc
> times out. The second patch avoids a call to a potentially sleeping
> function while a port spinlock is held.
> 
> Changes from v1:
>  - Corrected typo in patch 1 message, improved comment in code and added
>    a WARN_ON_ONCE() call to verify that a timed out qc is not active.
>  - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
>    port lock held. This call is in fact removed: it is not needed as
>    ata_scsi_requeue_deferred_qc() is called in EH, which is always run
>    when removing a port.
> 
> Damien Le Moal (2):
>   ata: libata-eh: correctly handle deferred qc timeouts
>   ata: libata-core: fix cancellation of a port deferred qc work
> 
>  drivers/ata/libata-core.c |  8 +++-----
>  drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
>  2 files changed, 22 insertions(+), 8 deletions(-)

Applied to for-7.0-fixes.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Guenter Roeck]

Hi,

On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
> A deferred qc may timeout while waiting for the device queue to drain
> to be submitted. In such case, since the qc is not active,
> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> which frees the qc. But as the port deferred_qc field still references
> this finished/freed qc, the deferred qc work may eventually attempt to
> call ata_qc_issue() against this invalid qc, leading to errors such as
> reported by UBSAN (syzbot run):
> 
> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> ...
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>  process_scheduled_works kernel/workqueue.c:3358 [inline]
>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>  kthread+0x370/0x450 kernel/kthread.c:467
>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>  </TASK>
> 
> Fix this by checking if the qc of a timed out SCSI command is a deferred
> one, and in such case, clear the port deferred_qc field and finish the
> SCSI command with DID_TIME_OUT.
> 
> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 72a22b6c9682..b373cceb95d2 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>  		set_host_byte(scmd, DID_OK);
>  
>  		ata_qc_for_each_raw(ap, qc, i) {
> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
> -			    qc->scsicmd == scmd)
> +			if (qc->scsicmd != scmd)
> +				continue;
> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
> +			    qc == ap->deferred_qc)
>  				break;
>  		}
>  
> -		if (i < ATA_MAX_QUEUE) {
> +		if (qc == ap->deferred_qc) {

An experimental AI code review agent tagged this patch with the following
comment.

  If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
  `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
  If this last element happens to be `ap->deferred_qc`, the condition
  `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
  match.

  Could this mistakenly intercept a command that completed normally after a SCSI
  timeout, returning a timeout error instead of success? Would this also
  incorrectly clear `ap->deferred_qc`, dropping the deferred command?
  Should we verify that the loop actually found a match, for instance by checking
  `if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)`?

It does seem to be a real problem to me, but I don't know the code well
enough to be sure. Please take a look and let me know if the problem is
real. If so, I'll be happy to submit a patch to fix it. If not, please let
me know what the agent is missing.

Thanks,
Guenter

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Niklas Cassel]

On 5 March 2026 18:59:08 CET, Guenter Roeck <linux@roeck-us.net> wrote:
>Hi,
>
>On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>> A deferred qc may timeout while waiting for the device queue to drain
>> to be submitted. In such case, since the qc is not active,
>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
>> which frees the qc. But as the port deferred_qc field still references
>> this finished/freed qc, the deferred qc work may eventually attempt to
>> call ata_qc_issue() against this invalid qc, leading to errors such as
>> reported by UBSAN (syzbot run):
>> 
>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
>> ...
>> Call Trace:
>>  <TASK>
>>  __dump_stack lib/dump_stack.c:94 [inline]
>>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>>  process_scheduled_works kernel/workqueue.c:3358 [inline]
>>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>>  kthread+0x370/0x450 kernel/kthread.c:467
>>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>>  </TASK>
>> 
>> Fix this by checking if the qc of a timed out SCSI command is a deferred
>> one, and in such case, clear the port deferred_qc field and finish the
>> SCSI command with DID_TIME_OUT.
>> 
>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
>> ---
>>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 72a22b6c9682..b373cceb95d2 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>  		set_host_byte(scmd, DID_OK);
>>  
>>  		ata_qc_for_each_raw(ap, qc, i) {
>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>> -			    qc->scsicmd == scmd)
>> +			if (qc->scsicmd != scmd)
>> +				continue;
>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
>> +			    qc == ap->deferred_qc)
>>  				break;
>>  		}
>>  
>> -		if (i < ATA_MAX_QUEUE) {
>> +		if (qc == ap->deferred_qc) {
>
>An experimental AI code review agent tagged this patch with the following
>comment.
>
>  If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
>  `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).

I think the AI is wrong here.

That last element assigned to QC will be ATA_MAX_QUEUE - 1.


>  If this last element happens to be `ap->deferred_qc`, the condition
>  `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
>  match.
>
>  Could this mistakenly intercept a command that completed normally after a SCSI
>  timeout, returning a timeout error instead of success? Would this also
>  incorrectly clear `ap->deferred_qc`, dropping the deferred command?

I think the AI is partially wrong here.

If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.

But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...

On NCQ error, we complete the deferred QC as a failed command.

However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.

So... I actually think that the change suggested by the AI is something we want.
(Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)


Kind regards,
Niklas

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

On 3/6/26 02:59, Guenter Roeck wrote:
> Hi,
> 
> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>> A deferred qc may timeout while waiting for the device queue to drain
>> to be submitted. In such case, since the qc is not active,
>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
>> which frees the qc. But as the port deferred_qc field still references
>> this finished/freed qc, the deferred qc work may eventually attempt to
>> call ata_qc_issue() against this invalid qc, leading to errors such as
>> reported by UBSAN (syzbot run):
>>
>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
>> ...
>> Call Trace:
>>  <TASK>
>>  __dump_stack lib/dump_stack.c:94 [inline]
>>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>>  process_scheduled_works kernel/workqueue.c:3358 [inline]
>>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>>  kthread+0x370/0x450 kernel/kthread.c:467
>>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>>  </TASK>
>>
>> Fix this by checking if the qc of a timed out SCSI command is a deferred
>> one, and in such case, clear the port deferred_qc field and finish the
>> SCSI command with DID_TIME_OUT.
>>
>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
>> ---
>>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 72a22b6c9682..b373cceb95d2 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>  		set_host_byte(scmd, DID_OK);
>>  
>>  		ata_qc_for_each_raw(ap, qc, i) {
>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>> -			    qc->scsicmd == scmd)
>> +			if (qc->scsicmd != scmd)
>> +				continue;
>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
>> +			    qc == ap->deferred_qc)
>>  				break;
>>  		}
>>  
>> -		if (i < ATA_MAX_QUEUE) {
>> +		if (qc == ap->deferred_qc) {
> 
> An experimental AI code review agent tagged this patch with the following
> comment.

Thanks for that.

>   If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
>   `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
>   If this last element happens to be `ap->deferred_qc`, the condition
>   `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
>   match.
> 
>   Could this mistakenly intercept a command that completed normally after a SCSI
>   timeout, returning a timeout error instead of success? Would this also
>   incorrectly clear `ap->deferred_qc`, dropping the deferred command?
>   Should we verify that the loop actually found a match, for instance by checking
>   `if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)`?

Yeah. Something like this. But that condition cannot actually happen. i ==
ATA_MAX_QUEUE correspond to internal QCs, and these never go through the
deferred path.

> It does seem to be a real problem to me, but I don't know the code well
> enough to be sure. Please take a look and let me know if the problem is
> real. If so, I'll be happy to submit a patch to fix it. If not, please let
> me know what the agent is missing.

See above. That is not a real problem, but it would still be good to check. I
think your change is fine. Car to send a proper patch ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

On 3/6/26 08:27, Niklas Cassel wrote:
> On 5 March 2026 18:59:08 CET, Guenter Roeck <linux@roeck-us.net> wrote:
>> Hi,
>> 
>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>>> A deferred qc may timeout while waiting for the device queue to drain to
>>> be submitted. In such case, since the qc is not active, 
>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(), which
>>> frees the qc. But as the port deferred_qc field still references this
>>> finished/freed qc, the deferred qc work may eventually attempt to call
>>> ata_qc_issue() against this invalid qc, leading to errors such as 
>>> reported by UBSAN (syzbot run):
>>> 
>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24 shift
>>> exponent 4210818301 is too large for 64-bit type 'long long unsigned
>>> int' ... Call Trace: <TASK> __dump_stack lib/dump_stack.c:94 [inline] 
>>> dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120 ubsan_epilogue+0xa/0x30
>>> lib/ubsan.c:233 __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/
>>> ubsan.c:494 ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166 
>>> ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679 
>>> process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275 
>>> process_scheduled_works kernel/workqueue.c:3358 [inline] 
>>> worker_thread+0x5da/0xe40 kernel/workqueue.c:3439 kthread+0x370/0x450
>>> kernel/kthread.c:467 ret_from_fork+0x754/0xd80 arch/x86/kernel/
>>> process.c:158 ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245 
>>> </TASK>
>>> 
>>> Fix this by checking if the qc of a timed out SCSI command is a deferred 
>>> one, and in such case, clear the port deferred_qc field and finish the 
>>> SCSI command with DID_TIME_OUT.
>>> 
>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com 
>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command
>>> starvation") Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-
>>> by: Hannes Reinecke <hare@suse.de> Reviewed-by: Igor Pylypiv
>>> <ipylypiv@google.com> --- drivers/ata/libata-eh.c | 22 ++++++++++++++++++
>>> +--- 1 file changed, 19 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index
>>> 72a22b6c9682..b373cceb95d2 100644 --- a/drivers/ata/libata-eh.c +++ b/
>>> drivers/ata/libata-eh.c @@ -640,12 +640,28 @@ void
>>> ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap, 
>>> set_host_byte(scmd, DID_OK);
>>> 
>>> ata_qc_for_each_raw(ap, qc, i) { -			if (qc->flags & ATA_QCFLAG_ACTIVE
>>> && -			    qc->scsicmd == scmd) +			if (qc->scsicmd != scmd) +
>>> continue; +			if ((qc->flags & ATA_QCFLAG_ACTIVE) || +			    qc == ap-
>>> >deferred_qc) break; }
>>> 
>>> -		if (i < ATA_MAX_QUEUE) { +		if (qc == ap->deferred_qc) {
>> 
>> An experimental AI code review agent tagged this patch with the following 
>> comment.
>> 
>> If the `ata_qc_for_each_raw()` loop finishes without finding a matching
>> `scmd`, `qc` will hold a pointer to the last element examined (`i ==
>> ATA_MAX_QUEUE`).
> 
> I think the AI is wrong here.
> 
> That last element assigned to QC will be ATA_MAX_QUEUE - 1.
> 
> 
>> If this last element happens to be `ap->deferred_qc`, the condition `qc ==
>> ap->deferred_qc` evaluates to true despite the loop not breaking on a 
>> match.
>> 
>> Could this mistakenly intercept a command that completed normally after a
>> SCSI timeout, returning a timeout error instead of success? Would this
>> also incorrectly clear `ap->deferred_qc`, dropping the deferred command?
> 
> I think the AI is partially wrong here.
> 
> If you read the comment below it if (), we know that ap->deferred_qc is only
> set until that command has been issued. So if it is set, that qc has not
> been issued, so it can't have successfully completed.

The request for the qc/scsi command was started from the block layer perspective
and so can still timeout. So this is all valid.

BUT ATA_MAX_QUEUE qc (last in the loop if there is no match) is the one reserved
for internal commands issued from EH. Internal QCs do not go through the
deferred issue path, so even without checking for the index when there is no
match, we have:
 - qc is still a valid pointer (the array of QCs is ATA_MAX_QUEUE + 1 sized)
 - We can never have qc == ap->deferred_qc.

As-is, the code is fine, but the above is not super clear :)

> But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up
> completing the deferred QC as a failed command, even though it did not time
> out...

Nope. If there is no match, it means that we do not have a QC anymore and so
completing the command is OK (this is the race case with the ATA driver
signaling a completion with an IRQ).

> On NCQ error, we complete the deferred QC as a failed command.

No we do not, we requeue the deferred QC in EH. That qc has not been issued, so
the NCQ error did not affect it.

> However, if there was a timeout of a command, which was not the deferred QC,
> but the deferred QC did not timeout, I think it is wrong to complete the
> deferred QC as a failed command.

We do not complete it, we requeue it from EH.

> So... I actually think that the change suggested by the AI is something we
> want. (Especially after Damien commit queued in for-next where we will not
> invoke error_handler() if there were no timed out commands.)

Yes, I think the proposed fix is fine. When the deferred qc is not a match, that
restores the same behavior of this function as before we added the deferred qc
stuff.



-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Guenter Roeck]

On Fri, Mar 06, 2026 at 12:27:34AM +0100, Niklas Cassel wrote:
> On 5 March 2026 18:59:08 CET, Guenter Roeck <linux@roeck-us.net> wrote:
> >Hi,
> >
> >On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
> >> A deferred qc may timeout while waiting for the device queue to drain
> >> to be submitted. In such case, since the qc is not active,
> >> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> >> which frees the qc. But as the port deferred_qc field still references
> >> this finished/freed qc, the deferred qc work may eventually attempt to
> >> call ata_qc_issue() against this invalid qc, leading to errors such as
> >> reported by UBSAN (syzbot run):
> >> 
> >> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> >> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> >> ...
> >> Call Trace:
> >>  <TASK>
> >>  __dump_stack lib/dump_stack.c:94 [inline]
> >>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
> >>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
> >>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
> >>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
> >>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
> >>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
> >>  process_scheduled_works kernel/workqueue.c:3358 [inline]
> >>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
> >>  kthread+0x370/0x450 kernel/kthread.c:467
> >>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
> >>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> >>  </TASK>
> >> 
> >> Fix this by checking if the qc of a timed out SCSI command is a deferred
> >> one, and in such case, clear the port deferred_qc field and finish the
> >> SCSI command with DID_TIME_OUT.
> >> 
> >> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> >> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> >> ---
> >>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
> >>  1 file changed, 19 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> >> index 72a22b6c9682..b373cceb95d2 100644
> >> --- a/drivers/ata/libata-eh.c
> >> +++ b/drivers/ata/libata-eh.c
> >> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
> >>  		set_host_byte(scmd, DID_OK);
> >>  
> >>  		ata_qc_for_each_raw(ap, qc, i) {
> >> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
> >> -			    qc->scsicmd == scmd)
> >> +			if (qc->scsicmd != scmd)
> >> +				continue;
> >> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
> >> +			    qc == ap->deferred_qc)
> >>  				break;
> >>  		}
> >>  
> >> -		if (i < ATA_MAX_QUEUE) {
> >> +		if (qc == ap->deferred_qc) {
> >
> >An experimental AI code review agent tagged this patch with the following
> >comment.
> >
> >  If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
> >  `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
> 
> I think the AI is wrong here.
> 
> That last element assigned to QC will be ATA_MAX_QUEUE - 1.
> 

I think that is what it means with "`qc` will hold a pointer to the
last element examined". The "(`i == ATA_MAX_QUEUE`) part is a bit
confusing.

I think what it is trying to say is that if i == ATA_MAX_QUEUE,
qc would point to the last examined element, which would not
have ATA_QCFLAG_ACTIVE set because otherwise it would have
exited the loop. Yet, ap->deferred_qc could be set, and the
if statement would be true even though i == ATA_MAX_QUEUE
and there was no qc match.

> 
> >  If this last element happens to be `ap->deferred_qc`, the condition
> >  `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
> >  match.
> >

That is pretty much much repeating what I said above, without
the confusing "(`i == ATA_MAX_QUEUE`)" part.

> >  Could this mistakenly intercept a command that completed normally after a SCSI
> >  timeout, returning a timeout error instead of success? Would this also
> >  incorrectly clear `ap->deferred_qc`, dropping the deferred command?
> 

This part is beyond my understanding, primarily because I don't know
what "qc->deferred" actually refers to.

> I think the AI is partially wrong here.
> 
> If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.
> 
> But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...
> 
> On NCQ error, we complete the deferred QC as a failed command.
> 
> However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.
> 
> So... I actually think that the change suggested by the AI is something we want.
> (Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)
> 

So should I send a patch, or do you want to handle it ?
It might be better if you handle it since I don't know
how to exactly describe the problem differently than the AI.

Thanks,
Guenter

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

On 3/6/26 09:14, Guenter Roeck wrote:
> On Fri, Mar 06, 2026 at 12:27:34AM +0100, Niklas Cassel wrote:
>> On 5 March 2026 18:59:08 CET, Guenter Roeck <linux@roeck-us.net> wrote:
>>> Hi,
>>>
>>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>>>> A deferred qc may timeout while waiting for the device queue to drain
>>>> to be submitted. In such case, since the qc is not active,
>>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
>>>> which frees the qc. But as the port deferred_qc field still references
>>>> this finished/freed qc, the deferred qc work may eventually attempt to
>>>> call ata_qc_issue() against this invalid qc, leading to errors such as
>>>> reported by UBSAN (syzbot run):
>>>>
>>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
>>>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
>>>> ...
>>>> Call Trace:
>>>>  <TASK>
>>>>  __dump_stack lib/dump_stack.c:94 [inline]
>>>>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>>>>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>>>>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>>>>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>>>>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>>>>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>>>>  process_scheduled_works kernel/workqueue.c:3358 [inline]
>>>>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>>>>  kthread+0x370/0x450 kernel/kthread.c:467
>>>>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>>>>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>>>>  </TASK>
>>>>
>>>> Fix this by checking if the qc of a timed out SCSI command is a deferred
>>>> one, and in such case, clear the port deferred_qc field and finish the
>>>> SCSI command with DID_TIME_OUT.
>>>>
>>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
>>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
>>>> ---
>>>>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>>>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>>> index 72a22b6c9682..b373cceb95d2 100644
>>>> --- a/drivers/ata/libata-eh.c
>>>> +++ b/drivers/ata/libata-eh.c
>>>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>>>  		set_host_byte(scmd, DID_OK);
>>>>  
>>>>  		ata_qc_for_each_raw(ap, qc, i) {
>>>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>>>> -			    qc->scsicmd == scmd)
>>>> +			if (qc->scsicmd != scmd)
>>>> +				continue;
>>>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
>>>> +			    qc == ap->deferred_qc)
>>>>  				break;
>>>>  		}
>>>>  
>>>> -		if (i < ATA_MAX_QUEUE) {
>>>> +		if (qc == ap->deferred_qc) {
>>>
>>> An experimental AI code review agent tagged this patch with the following
>>> comment.
>>>
>>>  If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
>>>  `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
>>
>> I think the AI is wrong here.
>>
>> That last element assigned to QC will be ATA_MAX_QUEUE - 1.
>>
> 
> I think that is what it means with "`qc` will hold a pointer to the
> last element examined". The "(`i == ATA_MAX_QUEUE`) part is a bit
> confusing.
> 
> I think what it is trying to say is that if i == ATA_MAX_QUEUE,
> qc would point to the last examined element, which would not
> have ATA_QCFLAG_ACTIVE set because otherwise it would have
> exited the loop. Yet, ap->deferred_qc could be set, and the
> if statement would be true even though i == ATA_MAX_QUEUE
> and there was no qc match.
> 
>>
>>>  If this last element happens to be `ap->deferred_qc`, the condition
>>>  `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
>>>  match.
>>>
> 
> That is pretty much much repeating what I said above, without
> the confusing "(`i == ATA_MAX_QUEUE`)" part.
> 
>>>  Could this mistakenly intercept a command that completed normally after a SCSI
>>>  timeout, returning a timeout error instead of success? Would this also
>>>  incorrectly clear `ap->deferred_qc`, dropping the deferred command?
>>
> 
> This part is beyond my understanding, primarily because I don't know
> what "qc->deferred" actually refers to.

There are 2 types of ATA commands: queueable ones (NCQ == Native Command
Queueing) and non-queueable ones (legacy/old ATA commands). The 2 types cannot
be mixed. When NCQ commands are on-going, you cannot issue a non-NCQ command,
and vice-versa. This has always been handled with command requeueing in
libata-scsi (since forever), but with blk-mq introduction, there was a potential
command starvation issue for non-NCQ commands that has existed for a long time.

We fixed that recently by keeping on hand any non-NCQ command that must wait for
on-going NCQ commands to complete first. This is ap->deferred_qc.

> 
>> I think the AI is partially wrong here.
>>
>> If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.
>>
>> But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...
>>
>> On NCQ error, we complete the deferred QC as a failed command.
>>
>> However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.
>>
>> So... I actually think that the change suggested by the AI is something we want.
>> (Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)
>>
> 
> So should I send a patch, or do you want to handle it ?
> It might be better if you handle it since I don't know
> how to exactly describe the problem differently than the AI.

Send a patch. Write a commit message based on the information I sent in my
previous email. We can correct the commit message if needed.

> 
> Thanks,
> Guenter


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Guenter Roeck]

On 3/5/26 15:59, Damien Le Moal wrote:
> On 3/6/26 02:59, Guenter Roeck wrote:
>> Hi,
>>
>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>>> A deferred qc may timeout while waiting for the device queue to drain
>>> to be submitted. In such case, since the qc is not active,
>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
>>> which frees the qc. But as the port deferred_qc field still references
>>> this finished/freed qc, the deferred qc work may eventually attempt to
>>> call ata_qc_issue() against this invalid qc, leading to errors such as
>>> reported by UBSAN (syzbot run):
>>>
>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
>>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
>>> ...
>>> Call Trace:
>>>   <TASK>
>>>   __dump_stack lib/dump_stack.c:94 [inline]
>>>   dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>>>   ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>>>   __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>>>   ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>>>   ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>>>   process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>>>   process_scheduled_works kernel/workqueue.c:3358 [inline]
>>>   worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>>>   kthread+0x370/0x450 kernel/kthread.c:467
>>>   ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>>>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>>>   </TASK>
>>>
>>> Fix this by checking if the qc of a timed out SCSI command is a deferred
>>> one, and in such case, clear the port deferred_qc field and finish the
>>> SCSI command with DID_TIME_OUT.
>>>
>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
>>> ---
>>>   drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index 72a22b6c9682..b373cceb95d2 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>>   		set_host_byte(scmd, DID_OK);
>>>   
>>>   		ata_qc_for_each_raw(ap, qc, i) {
>>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>>> -			    qc->scsicmd == scmd)
>>> +			if (qc->scsicmd != scmd)
>>> +				continue;
>>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
>>> +			    qc == ap->deferred_qc)
>>>   				break;
>>>   		}
>>>   
>>> -		if (i < ATA_MAX_QUEUE) {
>>> +		if (qc == ap->deferred_qc) {
>>
>> An experimental AI code review agent tagged this patch with the following
>> comment.
> 
> Thanks for that.
> 
>>    If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
>>    `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
>>    If this last element happens to be `ap->deferred_qc`, the condition
>>    `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
>>    match.
>>
>>    Could this mistakenly intercept a command that completed normally after a SCSI
>>    timeout, returning a timeout error instead of success? Would this also
>>    incorrectly clear `ap->deferred_qc`, dropping the deferred command?
>>    Should we verify that the loop actually found a match, for instance by checking
>>    `if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)`?
> 
> Yeah. Something like this. But that condition cannot actually happen. i ==
> ATA_MAX_QUEUE correspond to internal QCs, and these never go through the
> deferred path.
> 

Pardon my ignorance, but doesn't i == ATA_MAX_QUEUE mean that qc points
to the _previously_ examined qc, i.e., the one associated with i ==
ATA_MAX_QUEUE - 1 ? Are you saying that this qc will always always be
internal and never match ap->deferred_qc ?

>> It does seem to be a real problem to me, but I don't know the code well
>> enough to be sure. Please take a look and let me know if the problem is
>> real. If so, I'll be happy to submit a patch to fix it. If not, please let
>> me know what the agent is missing.
> 
> See above. That is not a real problem, but it would still be good to check. I
> think your change is fine. Car to send a proper patch ?
> 

I can try, but if the AI analysis is missing the point I don't really know
how to describe/explain it. Can you give me some guidance ?

Thanks,
Guenter

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Guenter Roeck]

On Fri, Mar 06, 2026 at 09:21:26AM +0900, Damien Le Moal wrote:
> On 3/6/26 09:14, Guenter Roeck wrote:
> > On Fri, Mar 06, 2026 at 12:27:34AM +0100, Niklas Cassel wrote:
> >> On 5 March 2026 18:59:08 CET, Guenter Roeck <linux@roeck-us.net> wrote:
> >>> Hi,
> >>>
> >>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
> >>>> A deferred qc may timeout while waiting for the device queue to drain
> >>>> to be submitted. In such case, since the qc is not active,
> >>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> >>>> which frees the qc. But as the port deferred_qc field still references
> >>>> this finished/freed qc, the deferred qc work may eventually attempt to
> >>>> call ata_qc_issue() against this invalid qc, leading to errors such as
> >>>> reported by UBSAN (syzbot run):
> >>>>
> >>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> >>>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> >>>> ...
> >>>> Call Trace:
> >>>>  <TASK>
> >>>>  __dump_stack lib/dump_stack.c:94 [inline]
> >>>>  dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
> >>>>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
> >>>>  __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
> >>>>  ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
> >>>>  ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
> >>>>  process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
> >>>>  process_scheduled_works kernel/workqueue.c:3358 [inline]
> >>>>  worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
> >>>>  kthread+0x370/0x450 kernel/kthread.c:467
> >>>>  ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
> >>>>  ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> >>>>  </TASK>
> >>>>
> >>>> Fix this by checking if the qc of a timed out SCSI command is a deferred
> >>>> one, and in such case, clear the port deferred_qc field and finish the
> >>>> SCSI command with DID_TIME_OUT.
> >>>>
> >>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> >>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> >>>> ---
> >>>>  drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
> >>>>  1 file changed, 19 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> >>>> index 72a22b6c9682..b373cceb95d2 100644
> >>>> --- a/drivers/ata/libata-eh.c
> >>>> +++ b/drivers/ata/libata-eh.c
> >>>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
> >>>>  		set_host_byte(scmd, DID_OK);
> >>>>  
> >>>>  		ata_qc_for_each_raw(ap, qc, i) {
> >>>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
> >>>> -			    qc->scsicmd == scmd)
> >>>> +			if (qc->scsicmd != scmd)
> >>>> +				continue;
> >>>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
> >>>> +			    qc == ap->deferred_qc)
> >>>>  				break;
> >>>>  		}
> >>>>  
> >>>> -		if (i < ATA_MAX_QUEUE) {
> >>>> +		if (qc == ap->deferred_qc) {
> >>>
> >>> An experimental AI code review agent tagged this patch with the following
> >>> comment.
> >>>
> >>>  If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
> >>>  `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
> >>
> >> I think the AI is wrong here.
> >>
> >> That last element assigned to QC will be ATA_MAX_QUEUE - 1.
> >>
> > 
> > I think that is what it means with "`qc` will hold a pointer to the
> > last element examined". The "(`i == ATA_MAX_QUEUE`) part is a bit
> > confusing.
> > 
> > I think what it is trying to say is that if i == ATA_MAX_QUEUE,
> > qc would point to the last examined element, which would not
> > have ATA_QCFLAG_ACTIVE set because otherwise it would have
> > exited the loop. Yet, ap->deferred_qc could be set, and the
> > if statement would be true even though i == ATA_MAX_QUEUE
> > and there was no qc match.
> > 
> >>
> >>>  If this last element happens to be `ap->deferred_qc`, the condition
> >>>  `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
> >>>  match.
> >>>
> > 
> > That is pretty much much repeating what I said above, without
> > the confusing "(`i == ATA_MAX_QUEUE`)" part.
> > 
> >>>  Could this mistakenly intercept a command that completed normally after a SCSI
> >>>  timeout, returning a timeout error instead of success? Would this also
> >>>  incorrectly clear `ap->deferred_qc`, dropping the deferred command?
> >>
> > 
> > This part is beyond my understanding, primarily because I don't know
> > what "qc->deferred" actually refers to.
> 
> There are 2 types of ATA commands: queueable ones (NCQ == Native Command
> Queueing) and non-queueable ones (legacy/old ATA commands). The 2 types cannot
> be mixed. When NCQ commands are on-going, you cannot issue a non-NCQ command,
> and vice-versa. This has always been handled with command requeueing in
> libata-scsi (since forever), but with blk-mq introduction, there was a potential
> command starvation issue for non-NCQ commands that has existed for a long time.
> 
> We fixed that recently by keeping on hand any non-NCQ command that must wait for
> on-going NCQ commands to complete first. This is ap->deferred_qc.
> 
> > 
> >> I think the AI is partially wrong here.
> >>
> >> If you read the comment below it if (), we know that ap->deferred_qc is only set until that command has been issued. So if it is set, that qc has not been issued, so it can't have successfully completed.
> >>
> >> But... Since we don't verify that i < ATA_MAX_QUEUE, we might end up completing the deferred QC as a failed command, even though it did not time out...
> >>
> >> On NCQ error, we complete the deferred QC as a failed command.
> >>
> >> However, if there was a timeout of a command, which was not the deferred QC, but the deferred QC did not timeout, I think it is wrong to complete the deferred QC as a failed command.
> >>
> >> So... I actually think that the change suggested by the AI is something we want.
> >> (Especially after Damien commit queued in for-next where we will not invoke error_handler() if there were no timed out commands.)
> >>
> > 
> > So should I send a patch, or do you want to handle it ?
> > It might be better if you handle it since I don't know
> > how to exactly describe the problem differently than the AI.
> 
> Send a patch. Write a commit message based on the information I sent in my
> previous email. We can correct the commit message if needed.
> 
I'll give it a try.

Thanks,
Guenter

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

On 3/6/26 09:32, Guenter Roeck wrote:
> On 3/5/26 15:59, Damien Le Moal wrote:
>> On 3/6/26 02:59, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
>>>> A deferred qc may timeout while waiting for the device queue to drain
>>>> to be submitted. In such case, since the qc is not active,
>>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
>>>> which frees the qc. But as the port deferred_qc field still references
>>>> this finished/freed qc, the deferred qc work may eventually attempt to
>>>> call ata_qc_issue() against this invalid qc, leading to errors such as
>>>> reported by UBSAN (syzbot run):
>>>>
>>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
>>>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
>>>> ...
>>>> Call Trace:
>>>>   <TASK>
>>>>   __dump_stack lib/dump_stack.c:94 [inline]
>>>>   dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
>>>>   ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>>>>   __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
>>>>   ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
>>>>   ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
>>>>   process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
>>>>   process_scheduled_works kernel/workqueue.c:3358 [inline]
>>>>   worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
>>>>   kthread+0x370/0x450 kernel/kthread.c:467
>>>>   ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
>>>>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
>>>>   </TASK>
>>>>
>>>> Fix this by checking if the qc of a timed out SCSI command is a deferred
>>>> one, and in such case, clear the port deferred_qc field and finish the
>>>> SCSI command with DID_TIME_OUT.
>>>>
>>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
>>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
>>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
>>>> ---
>>>>   drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>>> index 72a22b6c9682..b373cceb95d2 100644
>>>> --- a/drivers/ata/libata-eh.c
>>>> +++ b/drivers/ata/libata-eh.c
>>>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
>>>>   		set_host_byte(scmd, DID_OK);
>>>>   
>>>>   		ata_qc_for_each_raw(ap, qc, i) {
>>>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
>>>> -			    qc->scsicmd == scmd)
>>>> +			if (qc->scsicmd != scmd)
>>>> +				continue;
>>>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
>>>> +			    qc == ap->deferred_qc)
>>>>   				break;
>>>>   		}
>>>>   
>>>> -		if (i < ATA_MAX_QUEUE) {
>>>> +		if (qc == ap->deferred_qc) {
>>>
>>> An experimental AI code review agent tagged this patch with the following
>>> comment.
>>
>> Thanks for that.
>>
>>>    If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
>>>    `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
>>>    If this last element happens to be `ap->deferred_qc`, the condition
>>>    `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
>>>    match.
>>>
>>>    Could this mistakenly intercept a command that completed normally after a SCSI
>>>    timeout, returning a timeout error instead of success? Would this also
>>>    incorrectly clear `ap->deferred_qc`, dropping the deferred command?
>>>    Should we verify that the loop actually found a match, for instance by checking
>>>    `if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)`?
>>
>> Yeah. Something like this. But that condition cannot actually happen. i ==
>> ATA_MAX_QUEUE correspond to internal QCs, and these never go through the
>> deferred path.
>>
> 
> Pardon my ignorance, but doesn't i == ATA_MAX_QUEUE mean that qc points
> to the _previously_ examined qc, i.e., the one associated with i ==
> ATA_MAX_QUEUE - 1 ? Are you saying that this qc will always always be
> internal and never match ap->deferred_qc ?

Arg !!!! Yes ! I screwed up. Checking ata_qc_for_each_raw(), the tag is checked
before setting the qc pointer. So yes, you are absolutely correct, and we have a
problem.

>>> It does seem to be a real problem to me, but I don't know the code well
>>> enough to be sure. Please take a look and let me know if the problem is
>>> real. If so, I'll be happy to submit a patch to fix it. If not, please let
>>> me know what the agent is missing.
>>
>> See above. That is not a real problem, but it would still be good to check. I
>> think your change is fine. Car to send a proper patch ?
>>
> 
> I can try, but if the AI analysis is missing the point I don't really know
> how to describe/explain it. Can you give me some guidance ?

I think the AI is correct. That "if" without the index check can lead to false
positives and we can endup timeout-failing a deferred qc that has not timed out yet.

Send a patch please. As mentioned, we can touch-up the commit message if needed.

> 
> Thanks,
> Guenter
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Damien Le Moal]

On 3/6/26 09:11, Damien Le Moal wrote:
>>> Could this mistakenly intercept a command that completed normally after a
>>> SCSI timeout, returning a timeout error instead of success? Would this
>>> also incorrectly clear `ap->deferred_qc`, dropping the deferred command?
>>
>> I think the AI is partially wrong here.
>>
>> If you read the comment below it if (), we know that ap->deferred_qc is only
>> set until that command has been issued. So if it is set, that qc has not
>> been issued, so it can't have successfully completed.
> 
> The request for the qc/scsi command was started from the block layer perspective
> and so can still timeout. So this is all valid.
> 
> BUT ATA_MAX_QUEUE qc (last in the loop if there is no match) is the one reserved
> for internal commands issued from EH. Internal QCs do not go through the
> deferred issue path, so even without checking for the index when there is no
> match, we have:
>  - qc is still a valid pointer (the array of QCs is ATA_MAX_QUEUE + 1 sized)
>  - We can never have qc == ap->deferred_qc.

This second part of my comment is obviously wrong. I need more coffee :)

The first part stands: a deferred qc that has not been issued can timeout.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Guenter Roeck]

On Fri, Mar 06, 2026 at 09:50:49AM +0900, Damien Le Moal wrote:
> On 3/6/26 09:32, Guenter Roeck wrote:
> > On 3/5/26 15:59, Damien Le Moal wrote:
> >> On 3/6/26 02:59, Guenter Roeck wrote:
> >>> Hi,
> >>>
> >>> On Sat, Feb 21, 2026 at 07:14:38AM +0900, Damien Le Moal wrote:
> >>>> A deferred qc may timeout while waiting for the device queue to drain
> >>>> to be submitted. In such case, since the qc is not active,
> >>>> ata_scsi_cmd_error_handler() ends up calling scsi_eh_finish_cmd(),
> >>>> which frees the qc. But as the port deferred_qc field still references
> >>>> this finished/freed qc, the deferred qc work may eventually attempt to
> >>>> call ata_qc_issue() against this invalid qc, leading to errors such as
> >>>> reported by UBSAN (syzbot run):
> >>>>
> >>>> UBSAN: shift-out-of-bounds in drivers/ata/libata-core.c:5166:24
> >>>> shift exponent 4210818301 is too large for 64-bit type 'long long unsigned int'
> >>>> ...
> >>>> Call Trace:
> >>>>   <TASK>
> >>>>   __dump_stack lib/dump_stack.c:94 [inline]
> >>>>   dump_stack_lvl+0x100/0x190 lib/dump_stack.c:120
> >>>>   ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
> >>>>   __ubsan_handle_shift_out_of_bounds+0x279/0x2a0 lib/ubsan.c:494
> >>>>   ata_qc_issue.cold+0x38/0x9f drivers/ata/libata-core.c:5166
> >>>>   ata_scsi_deferred_qc_work+0x154/0x1f0 drivers/ata/libata-scsi.c:1679
> >>>>   process_one_work+0x9d7/0x1920 kernel/workqueue.c:3275
> >>>>   process_scheduled_works kernel/workqueue.c:3358 [inline]
> >>>>   worker_thread+0x5da/0xe40 kernel/workqueue.c:3439
> >>>>   kthread+0x370/0x450 kernel/kthread.c:467
> >>>>   ret_from_fork+0x754/0xd80 arch/x86/kernel/process.c:158
> >>>>   ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> >>>>   </TASK>
> >>>>
> >>>> Fix this by checking if the qc of a timed out SCSI command is a deferred
> >>>> one, and in such case, clear the port deferred_qc field and finish the
> >>>> SCSI command with DID_TIME_OUT.
> >>>>
> >>>> Reported-by: syzbot+1f77b8ca15336fff21ff@syzkaller.appspotmail.com
> >>>> Fixes: 0ea84089dbf6 ("ata: libata-scsi: avoid Non-NCQ command starvation")
> >>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>>> Reviewed-by: Hannes Reinecke <hare@suse.de>
> >>>> Reviewed-by: Igor Pylypiv <ipylypiv@google.com>
> >>>> ---
> >>>>   drivers/ata/libata-eh.c | 22 +++++++++++++++++++---
> >>>>   1 file changed, 19 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> >>>> index 72a22b6c9682..b373cceb95d2 100644
> >>>> --- a/drivers/ata/libata-eh.c
> >>>> +++ b/drivers/ata/libata-eh.c
> >>>> @@ -640,12 +640,28 @@ void ata_scsi_cmd_error_handler(struct Scsi_Host *host, struct ata_port *ap,
> >>>>   		set_host_byte(scmd, DID_OK);
> >>>>   
> >>>>   		ata_qc_for_each_raw(ap, qc, i) {
> >>>> -			if (qc->flags & ATA_QCFLAG_ACTIVE &&
> >>>> -			    qc->scsicmd == scmd)
> >>>> +			if (qc->scsicmd != scmd)
> >>>> +				continue;
> >>>> +			if ((qc->flags & ATA_QCFLAG_ACTIVE) ||
> >>>> +			    qc == ap->deferred_qc)
> >>>>   				break;
> >>>>   		}
> >>>>   
> >>>> -		if (i < ATA_MAX_QUEUE) {
> >>>> +		if (qc == ap->deferred_qc) {
> >>>
> >>> An experimental AI code review agent tagged this patch with the following
> >>> comment.
> >>
> >> Thanks for that.
> >>
> >>>    If the `ata_qc_for_each_raw()` loop finishes without finding a matching `scmd`,
> >>>    `qc` will hold a pointer to the last element examined (`i == ATA_MAX_QUEUE`).
> >>>    If this last element happens to be `ap->deferred_qc`, the condition
> >>>    `qc == ap->deferred_qc` evaluates to true despite the loop not breaking on a
> >>>    match.
> >>>
> >>>    Could this mistakenly intercept a command that completed normally after a SCSI
> >>>    timeout, returning a timeout error instead of success? Would this also
> >>>    incorrectly clear `ap->deferred_qc`, dropping the deferred command?
> >>>    Should we verify that the loop actually found a match, for instance by checking
> >>>    `if (i < ATA_MAX_QUEUE && qc == ap->deferred_qc)`?
> >>
> >> Yeah. Something like this. But that condition cannot actually happen. i ==
> >> ATA_MAX_QUEUE correspond to internal QCs, and these never go through the
> >> deferred path.
> >>
> > 
> > Pardon my ignorance, but doesn't i == ATA_MAX_QUEUE mean that qc points
> > to the _previously_ examined qc, i.e., the one associated with i ==
> > ATA_MAX_QUEUE - 1 ? Are you saying that this qc will always always be
> > internal and never match ap->deferred_qc ?
> 
> Arg !!!! Yes ! I screwed up. Checking ata_qc_for_each_raw(), the tag is checked
> before setting the qc pointer. So yes, you are absolutely correct, and we have a
> problem.
> 
> >>> It does seem to be a real problem to me, but I don't know the code well
> >>> enough to be sure. Please take a look and let me know if the problem is
> >>> real. If so, I'll be happy to submit a patch to fix it. If not, please let
> >>> me know what the agent is missing.
> >>
> >> See above. That is not a real problem, but it would still be good to check. I
> >> think your change is fine. Car to send a proper patch ?
> >>
> > 
> > I can try, but if the AI analysis is missing the point I don't really know
> > how to describe/explain it. Can you give me some guidance ?
> 
> I think the AI is correct. That "if" without the index check can lead to false
> positives and we can endup timeout-failing a deferred qc that has not timed out yet.
> 
> Send a patch please. As mentioned, we can touch-up the commit message if needed.
> 
Working on it.

Thnaks,
Guenter

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Niklas Cassel]

On Fri, Mar 06, 2026 at 09:59:38AM +0900, Damien Le Moal wrote:
> On 3/6/26 09:11, Damien Le Moal wrote:
> >>> Could this mistakenly intercept a command that completed normally after a
> >>> SCSI timeout, returning a timeout error instead of success? Would this
> >>> also incorrectly clear `ap->deferred_qc`, dropping the deferred command?
> >>
> >> I think the AI is partially wrong here.
> >>
> >> If you read the comment below it if (), we know that ap->deferred_qc is only
> >> set until that command has been issued. So if it is set, that qc has not
> >> been issued, so it can't have successfully completed.
> > 
> > The request for the qc/scsi command was started from the block layer perspective
> > and so can still timeout. So this is all valid.
> > 
> > BUT ATA_MAX_QUEUE qc (last in the loop if there is no match) is the one reserved
> > for internal commands issued from EH. Internal QCs do not go through the
> > deferred issue path, so even without checking for the index when there is no
> > match, we have:
> >  - qc is still a valid pointer (the array of QCs is ATA_MAX_QUEUE + 1 sized)
> >  - We can never have qc == ap->deferred_qc.
> 
> This second part of my comment is obviously wrong. I need more coffee :)
> 
> The first part stands: a deferred qc that has not been issued can timeout.

Yes, did someone claim otherwise?


Kind regards,
Niklas

Re: [PATCH v2 1/2] ata: libata-eh: correctly handle deferred qc timeouts

[Niklas Cassel]

On Fri, Mar 06, 2026 at 09:50:49AM +0900, Damien Le Moal wrote:
> > 
> > I can try, but if the AI analysis is missing the point I don't really know
> > how to describe/explain it. Can you give me some guidance ?
> 
> I think the AI is correct. That "if" without the index check can lead to false
> positives and we can endup timeout-failing a deferred qc that has not timed out yet.

Which is exactly what I wrote yesterday:

>> However, if there was a timeout of a command, which was not the deferred QC,
>> but the deferred QC did not timeout, I think it is wrong to complete the
>> deferred QC as a failed command.
>>
>> So... I actually think that the change suggested by the AI is something we
>> want.

Glad that you came to the same conclusion after some coffee :)

(Yes, I did actually look at ata_qc_for_each_raw() before writing my reply
yesterday.)


Kind regards,
Niklas

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Tommy Kelly]

On 2/20/26 14:14, Damien Le Moal wrote:
> The first patch addresses a use-after-free issue when a deferred qc
> times out. The second patch avoids a call to a potentially sleeping
> function while a port spinlock is held.
> 
> Changes from v1:
>  - Corrected typo in patch 1 message, improved comment in code and added
>    a WARN_ON_ONCE() call to verify that a timed out qc is not active.
>  - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
>    port lock held. This call is in fact removed: it is not needed as
>    ata_scsi_requeue_deferred_qc() is called in EH, which is always run
>    when removing a port.
> 
> Damien Le Moal (2):
>   ata: libata-eh: correctly handle deferred qc timeouts
>   ata: libata-core: fix cancellation of a port deferred qc work
> 
>  drivers/ata/libata-core.c |  8 +++-----
>  drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 


Hello, this is my first message on the LKML so please forgive me for
likely not following etiquette.

I have what might be a regression from somewhere in the recent series of
NCQ / QC patches.

I have a peculiar setup that might reveal the source of the bug. I'm
using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1
SATA port to 5 drives.
The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda.
The platform does not (yet) support FIS-Based Switching (FBS), so it is
using Command-Based Switching (CBS). FBS support may land in Linux 7.1.

> kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
> kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS

When switching from kernel 6.18.13 to 6.19.7, drive access became
extremely slow, causing lock timeouts at the filesystem level. I solved
the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.

I also looked through the recent LPM patches, but the drives'
performance didn't improve after changing LPM settings for the scsi host
in sysfs.

Let me know if I need to provide more details or logs. I can see
COMRESET a lot in each device's SMART logs but they aren't timestamped
so I haven't proved that they are recent. Perhaps the slow drive
switching is causing command starvation.


Thank you,
Tommy

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Damien Le Moal]

On 2026/04/02 11:22, Tommy Kelly wrote:
> On 2/20/26 14:14, Damien Le Moal wrote:
>> The first patch addresses a use-after-free issue when a deferred qc
>> times out. The second patch avoids a call to a potentially sleeping
>> function while a port spinlock is held.
>>
>> Changes from v1:
>>  - Corrected typo in patch 1 message, improved comment in code and added
>>    a WARN_ON_ONCE() call to verify that a timed out qc is not active.
>>  - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
>>    port lock held. This call is in fact removed: it is not needed as
>>    ata_scsi_requeue_deferred_qc() is called in EH, which is always run
>>    when removing a port.
>>
>> Damien Le Moal (2):
>>   ata: libata-eh: correctly handle deferred qc timeouts
>>   ata: libata-core: fix cancellation of a port deferred qc work
>>
>>  drivers/ata/libata-core.c |  8 +++-----
>>  drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
>>  2 files changed, 22 insertions(+), 8 deletions(-)
>>
> 
> 
> Hello, this is my first message on the LKML so please forgive me for
> likely not following etiquette.
> 
> I have what might be a regression from somewhere in the recent series of
> NCQ / QC patches.
> 
> I have a peculiar setup that might reveal the source of the bug. I'm
> using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1
> SATA port to 5 drives.
> The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda.
> The platform does not (yet) support FIS-Based Switching (FBS), so it is
> using Command-Based Switching (CBS). FBS support may land in Linux 7.1.
> 
>> kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
>> kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS
> 
> When switching from kernel 6.18.13 to 6.19.7, drive access became
> extremely slow, causing lock timeouts at the filesystem level. I solved
> the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.

We may have a bad interaction between PMP CBS and the deferred QC fix, though I
do not see how as long as NCQ commands are the main workload. Not sure what is
going on and we'll need to dig on this. Though this will be difficult as all the
PMP I have are FBS. I do not have any command based switching port multiplier.

Note that because of I do not have manual access to our lab for a while, this
may take some time. Please be patient.

> I also looked through the recent LPM patches, but the drives'
> performance didn't improve after changing LPM settings for the scsi host
> in sysfs.
> 
> Let me know if I need to provide more details or logs. I can see
> COMRESET a lot in each device's SMART logs but they aren't timestamped
> so I haven't proved that they are recent. Perhaps the slow drive
> switching is causing command starvation.

Please send the relevant parts of dmesg showing these resets and what leads to
them (e.g. any error).

> 
> 
> Thank you,
> Tommy


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Niklas Cassel]

On Thu, Apr 02, 2026 at 02:22:49AM -0700, Tommy Kelly wrote:
> On 2/20/26 14:14, Damien Le Moal wrote:
> > The first patch addresses a use-after-free issue when a deferred qc
> > times out. The second patch avoids a call to a potentially sleeping
> > function while a port spinlock is held.
> > 
> > Changes from v1:
> >  - Corrected typo in patch 1 message, improved comment in code and added
> >    a WARN_ON_ONCE() call to verify that a timed out qc is not active.
> >  - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
> >    port lock held. This call is in fact removed: it is not needed as
> >    ata_scsi_requeue_deferred_qc() is called in EH, which is always run
> >    when removing a port.
> > 
> > Damien Le Moal (2):
> >   ata: libata-eh: correctly handle deferred qc timeouts
> >   ata: libata-core: fix cancellation of a port deferred qc work
> > 
> >  drivers/ata/libata-core.c |  8 +++-----
> >  drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
> >  2 files changed, 22 insertions(+), 8 deletions(-)
> > 
> 
> 
> Hello, this is my first message on the LKML so please forgive me for
> likely not following etiquette.
> 
> I have what might be a regression from somewhere in the recent series of
> NCQ / QC patches.
> 
> I have a peculiar setup that might reveal the source of the bug. I'm
> using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1
> SATA port to 5 drives.
> The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda.
> The platform does not (yet) support FIS-Based Switching (FBS), so it is
> using Command-Based Switching (CBS). FBS support may land in Linux 7.1.
> 
> > kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
> > kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS
> 
> When switching from kernel 6.18.13 to 6.19.7, drive access became
> extremely slow, causing lock timeouts at the filesystem level. I solved
> the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.
> 
> I also looked through the recent LPM patches, but the drives'
> performance didn't improve after changing LPM settings for the scsi host
> in sysfs.
> 
> Let me know if I need to provide more details or logs. I can see
> COMRESET a lot in each device's SMART logs but they aren't timestamped
> so I haven't proved that they are recent. Perhaps the slow drive
> switching is causing command starvation.

Hello Tommy,

Igor recently found a bug in the deferred QC feature:
https://lore.kernel.org/linux-ide/20260412153637.475606-1-ipylypiv@google.com/

His fix patch has been queued up on libata/for-next, so perhaps you could try
the libata/for-next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next

And see if you can still reproduce the problem there.


Kind regards,
Niklas

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Tommy Kelly]

On 4/12/26 11:22, Niklas Cassel wrote:
> On Thu, Apr 02, 2026 at 02:22:49AM -0700, Tommy Kelly wrote:
>> On 2/20/26 14:14, Damien Le Moal wrote:
>>> The first patch addresses a use-after-free issue when a deferred qc
>>> times out. The second patch avoids a call to a potentially sleeping
>>> function while a port spinlock is held.
>>>
>>> Changes from v1:
>>>  - Corrected typo in patch 1 message, improved comment in code and added
>>>    a WARN_ON_ONCE() call to verify that a timed out qc is not active.
>>>  - Fixed patch 2 to not call ata_scsi_requeue_deferred_qc() without the
>>>    port lock held. This call is in fact removed: it is not needed as
>>>    ata_scsi_requeue_deferred_qc() is called in EH, which is always run
>>>    when removing a port.
>>>
>>> Damien Le Moal (2):
>>>   ata: libata-eh: correctly handle deferred qc timeouts
>>>   ata: libata-core: fix cancellation of a port deferred qc work
>>>
>>>  drivers/ata/libata-core.c |  8 +++-----
>>>  drivers/ata/libata-eh.c   | 22 +++++++++++++++++++---
>>>  2 files changed, 22 insertions(+), 8 deletions(-)
>>>
>>
>>
>> Hello, this is my first message on the LKML so please forgive me for
>> likely not following etiquette.
>>
>> I have what might be a regression from somewhere in the recent series of
>> NCQ / QC patches.
>>
>> I have a peculiar setup that might reveal the source of the bug. I'm
>> using a SATA Port Multiplier (PMP), powered by JMB575, which expands 1
>> SATA port to 5 drives.
>> The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda.
>> The platform does not (yet) support FIS-Based Switching (FBS), so it is
>> using Command-Based Switching (CBS). FBS support may land in Linux 7.1.
>>
>>> kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst
>>> kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS
>>
>> When switching from kernel 6.18.13 to 6.19.7, drive access became
>> extremely slow, causing lock timeouts at the filesystem level. I solved
>> the problem by changing /sys/block/sd*/device/queue_depth from 32 to 1.
>>
>> I also looked through the recent LPM patches, but the drives'
>> performance didn't improve after changing LPM settings for the scsi host
>> in sysfs.
>>
>> Let me know if I need to provide more details or logs. I can see
>> COMRESET a lot in each device's SMART logs but they aren't timestamped
>> so I haven't proved that they are recent. Perhaps the slow drive
>> switching is causing command starvation.
> 
> Hello Tommy,
> 
> Igor recently found a bug in the deferred QC feature:
> https://lore.kernel.org/linux-ide/20260412153637.475606-1-ipylypiv@google.com/
> 
> His fix patch has been queued up on libata/for-next, so perhaps you could try
> the libata/for-next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next
> 
> And see if you can still reproduce the problem there.
> 
> 
> Kind regards,
> Niklas

Hi Niklas and Damien,

Thanks for the responses.
I managed to build libata/for-next into rpms for my Fedora CoreOS system.
Which includes Igor's "fix requeue of deferred ATA PASS-THROUGH commands".
But unfortunately, the NCQ problem remains.
Drive access is extremely slow, and on this test boot 3 out of 5 drives failed to even be initialized by the system.
`lsblk -f` only printed fstype for sda+b, none for sdc+d+e.
It is fixed by this udev rule which sets queue_depth on each drive to 1.

> SUBSYSTEM=="scsi", DRIVERS=="sd", ATTR{queue_depth}="1"

Here are some error logs, though I don't think they are particularly illuminating.

> (udev-worker)[549]: sdb: Spawned process 'ata_id --export /dev/sdb' [635] is taking longer than 59s to complete.
> systemd-udevd[473]: sda: Worker [551] processing SEQNUM=3406 is taking a long time.
> systemd-udevd[473]: sdd: Worker [542] processing SEQNUM=3414 is taking a long time.
> systemd-udevd[473]: sdc: Worker [562] processing SEQNUM=3408 is taking a long time.
> systemd-udevd[473]: sde: Worker [550] processing SEQNUM=3412 is taking a long time.
> systemd-udevd[473]: sdb: Worker [549] processing SEQNUM=3410 is taking a long time.
> systemd[1]: dev-sdc.device: Job dev-sdc.device/start timed out.
> systemd[1]: Timed out waiting for device dev-sdc.device - /dev/sdc.
> systemd[1]: dev-sdc.device: Job dev-sdc.device/start failed with result 'timeout'.
> systemd[1]: Unnecessary job was removed for dev-sda.device - /dev/sda.
> systemd[1]: Unnecessary job was removed for dev-sdd.device - /dev/sdd.
> systemd[1]: Unnecessary job was removed for dev-sdb.device - /dev/sdb.
> systemd[1]: Unnecessary job was removed for dev-sde.device - /dev/sde.
> systemd[1]: dev-sde.device: Job dev-sde.device/start timed out.
> systemd[1]: Timed out waiting for device dev-sde.device - /dev/sde.
> systemd[1]: dev-sde.device: Job dev-sde.device/start failed with result 'timeout'.
> systemd-udevd[856]: sde: Worker [869] processing SEQNUM=3696 killed.
> systemd-udevd[856]: sdd: Worker [871] processing SEQNUM=3693 killed.
> systemd-udevd[856]: sdc: Worker [875] processing SEQNUM=3690 killed.
> systemd-udevd[856]: sdc: Worker [875] terminated by signal 9 (KILL).
> systemd-udevd[856]: sde: Worker [869] terminated by signal 9 (KILL).

There is one line from pcp-pmie which is interesting:

> pcp-pmie[1784]: High per disk average queue length 6280%await[sdd]@hostname

Some info about sdd and the port multiplier, if it is relevant. sdd is the only drive without HIPM

> kernel: ahci-dwc fc800000.sata: supply ahci not found, using dummy regulator
> kernel: ahci-dwc fc800000.sata: supply phy not found, using dummy regulator
> kernel: ahci-dwc fc800000.sata: supply target not found, using dummy regulator
> kernel: ahci-dwc fc800000.sata: PMPn is limited up to 5 ports
> kernel: ahci-dwc fc800000.sata: forcing port_map 0x0 -> 0x1
> kernel: ahci-dwc fc800000.sata: AHCI vers 0001.0300, 32 command slots, 6 Gbps, platform mode
> kernel: ahci-dwc fc800000.sata: 1/1 ports implemented (port mask 0x1)
> kernel: ahci-dwc fc800000.sata: flags: ncq sntf pm led clo only pmp fbs pio slum part ccc apst 
> kernel: ahci-dwc fc800000.sata: port 0 is not capable of FBS
> kernel: scsi host0: ahci-dwc
> kernel: ata1: SATA max UDMA/133 mmio [mem 0xfc800000-0xfc800fff] port 0x100 irq 77 lpm-pol 0
> kernel: ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> kernel: ata1.15: Port Multiplier 1.2, 0x197b:0x5755 r0, 5 ports, feat 0x5/0xf
> kernel: ata1.03: hard resetting link
> kernel: ata1.03: SATA link up 6.0 Gbps (SStatus 133 SControl 330)
> kernel: ata1.03: ATA-10: ADATA SU655, V8X01c62, max UDMA/133
> kernel: ata1.03: 468862128 sectors, multi 1: LBA48 NCQ (depth 32)
> kernel: ata1.03: Features: DIPM
> kernel: ata1.03: configured for UDMA/133
> kernel: scsi 0:3:0:0: Direct-Access     ATA      ADATA SU655      1c62 PQ: 0 ANSI: 5
> kernel: sd 0:3:0:0: Attached scsi generic sg3 type 0
> kernel: sd 0:3:0:0: [sdd] 468862128 512-byte logical blocks: (240 GB/224 GiB)
> kernel: sd 0:3:0:0: [sdd] Write Protect is off
> kernel: sd 0:3:0:0: [sdd] Mode Sense: 00 3a 00 00
> kernel: sd 0:3:0:0: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
> kernel: sd 0:3:0:0: [sdd] Preferred minimum I/O size 512 bytes
> kernel: sd 0:3:0:0: [sdd] Attached SCSI disk

ADATA SU680 was quirked with ATA_QUIRK_NOLPM recently. My sdd is ADATA SU655.
But changing link_power_management_policy to max_performance had no effect so I don't think LPM is the culprit here.

I am new to this, but perhaps linux is misunderstanding the queue depth. 
There are "32 command slots" for the PMP, but each drive advertises "NCQ (depth 32)" as well, but perhaps there is only actually 32/5=6 slots per drive.
Not sure if that matters wrt the bug.

Thanks for your time and energy,
Tommy

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Niklas Cassel]

On Thu, Apr 16, 2026 at 09:38:21PM -0700, Tommy Kelly wrote:
> Hi Niklas and Damien,
> 
> Thanks for the responses.
> I managed to build libata/for-next into rpms for my Fedora CoreOS system.

Thank you for testing!
I was worth a shot.
We will need to dig deeper then.


> I am new to this, but perhaps linux is misunderstanding the queue depth. 
> There are "32 command slots" for the PMP, but each drive advertises "NCQ (depth 32)" as well, but perhaps there is only actually 32/5=6 slots per drive.
> Not sure if that matters wrt the bug.

For a PMP with FBS, the PMP can communicate with multiple drives simultaneously.
Thus, the commands in the 32 command slots can be for different drives.
There is no fixed limit on how much each one drive can occupy. E.g. one drive
use all 32 if there is only I/O going on to a single drive.

For a PMP without FBS (only CBS), the PMP can only communicate with one drive at
a time. Thus, the commands in the 32 command slots will belong to the same
drive.


I think the problem is that the patch that implements the deferred QC logic
forgot to think about port multipliers.

A PMP with FBS can mix NCQ and non-NCQ commands (to different drives of course).
The current deferred QC design stores a single deferred QC in struct ata_port...

This must instead be stored in struct ata_device, or struct ata_link, or have
an array of deferred QCs in struct ata_port.

This means that, for PMPs with FBS, if we defer a QC to one drive (because you
cannot mix NCQ and non-NCQ commands - within a single drive), we will currently
defer commands to all other drives on the PMP.

This is obviously very bad for PMPs with FBS, as commands to other drives will
be deferred for no reason (it will kill performance).



Your problem however, is with PMPs without FBS (only CBS).

Spawned process 'ata_id --export /dev/sdb' [635] is taking longer than 59s to complete.

https://github.com/systemd/systemd/blob/main/src/udev/ata_id/ata_id.c#L93-L115

ata_id seems to send a ATA PASSTHROUGH (12) non-NCQ command.
Presumably some command is already running on sda.

https://github.com/torvalds/linux/blob/v7.0/drivers/ata/libahci.c#L1682-L1685
https://github.com/torvalds/linux/blob/v7.0/drivers/ata/libata-pmp.c#L115-L120

For CBS, it appears that sata_pmp_qc_defer_cmd_switch() will set:
ap->excl_link and will set qc->flags |= ATA_QCFLAG_CLEAR_EXCL.

and then:
__ata_qc_complete() will clear ap->excl_link, but only if qc->flags
ATA_QCFLAG_CLEAR_EXCL was set.

Once a command is completed, ata_scsi_schedule_deferred_qc() will either:
call ata_scsi_requeue_deferred_qc() which will requeue it in SCSI ML, or:
will call ap->ops->qc_defer(qc) and schedule the deferred QC worker.

Considering that we don't see any actual timeouts from SCSI or ATA in your
dmesg, my best guess is that the command is infinitely getting requeued in
SCSI ML. (Possibly by ata_scsi_requeue_deferred_qc().)

I don't really see it. I guess someone with a PMP would need to debug.
(Should be trivial to force disable FBS for a PMP that supports FBS.)

One behavior change that I see from before with a PMP CBS:
if (ap->deferred_ap) is set, we will now return SCSI_MLQUEUE_DEVICE_BUSY,
while before, sata_pmp_qc_defer_cmd_switch() would cause us to return
either SCSI_MLQUEUE_HOST_BUSY - if there was a command excuting on another
drive, or SCSI_MLQUEUE_DEVICE_BUSY - if the same drive was busy executing
NCQ commands.


Kind regards,
Niklas

Re: [PATCH v2 0/2] ATA port deferred qc fixes

[Niklas Cassel]

On Thu, Apr 02, 2026 at 02:22:49AM -0700, Tommy Kelly wrote:
> The device/driver/subsystem tree looks like rk3568-dwc-ahci -> scsi -> sda.
> The platform does not (yet) support FIS-Based Switching (FBS), so it is
> using Command-Based Switching (CBS). FBS support may land in Linux 7.1.

I still do not have access to a PMP.
Damien said he might give me access to one.

FWIW, since you are using rk368, I saw that:

commit 5918bf2a17f4689abfb94d341c5240088f8bd16e
Author: Heiko Stuebner <heiko@sntech.de>
Date:   Sun Feb 1 20:18:01 2026 +0100

    arm64: dts: rockchip: Add port subnodes to RK356x SATA controllers


Has now landed in Linus's tree, which adds PMP FBS support for this platform.


What we know:
1) There is a design flaw in the current deferral code which will make
   PMP FBS defer commands to all drives when sending a non-NCQ command.
   (Basically giving the FBS the same performance as CBS).

2) There is a bug that you found that seems to cause indefinite requeues on
   PMP CBS. This manifests as:
systemd[1]: dev-sdc.device: Job dev-sdc.device/start timed out.
systemd[1]: Timed out waiting for device dev-sdc.device - /dev/sdc.

   We don't know if this bug also affects PMP FBS or not.
   Out of curiousity, do you still see this bug if running with the above
   patch cherry-picked or not?


Kind regards,
Niklas