Re: [PATCH v2 4/9] ata: libata: Pass the ATA port argument directly to ata_qc_complete()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The kernel-doc comment for ata_qc_complete() is missing the new '@ap' parameter.

Pre-existing issues:
- [High] In `sata_fsl_host_intr`, a spurious interrupt will prematurely complete an active internal command.
--

commit 0bfdcf1e846fdd4f2753f05417c311388a3aa360
Author: Bart Van Assche <bvanassche@acm.org>
ata: libata: Pass the ATA port argument directly to ata_qc_complete()

This commit prepares for adding lock context annotations by passing the ATA
port argument directly to ata_qc_complete(). No functional changes were made.

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 5caae9a2aca39..307ab48fe04c2 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5024,9 +5024,8 @@ static void ata_verify_xfer(struct ata_queued_cmd *qc)
>   *	LOCKING:
>   *	spin_lock_irqsave(host lock)
>   */
> -void ata_qc_complete(struct ata_queued_cmd *qc)
> +void ata_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
>  {

[Severity: Low]
Is the kernel-doc block missing the new @ap parameter? Since this is an
exported symbol, omitting the new parameter might trigger kernel-doc
warnings during documentation builds.

> -	struct ata_port *ap = qc->ap;
>  	struct ata_device *dev = qc->dev;
>  	struct ata_eh_info *ehi = &dev->link->eh_info;

[ ... ]

> diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> index be829fcc584de..ce6f50b8e38c1 100644
> --- a/drivers/ata/sata_fsl.c
> +++ b/drivers/ata/sata_fsl.c
> @@ -1264,7 +1264,7 @@ static void sata_fsl_host_intr(struct ata_port *ap)
>  			 ioread32(hcr_base + CC));
>  
>  		if (qc) {
> -			ata_qc_complete(qc);
> +			ata_qc_complete(ap, qc);
>  		}

[Severity: High]
This is a pre-existing issue, but does this code prematurely complete an
active internal command on a spurious interrupt?

When an internal command is active, ap->qc_active has bit 32 set
(ATA_TAG_INTERNAL). The done_mask is a 32-bit value read from the CC
register, so it cannot have bit 32 set. Consequently,
done_mask & ap->qc_active always evaluates to 0.

The driver then falls back to the else if block this hunk modifies, which
blindly assumes the internal command completed without verifying that the
completion bit in CC is actually set. If a non-error or spurious interrupt
arrives while an internal command is in flight, it appears the driver
will incorrectly complete the command before the hardware finishes processing
it.

>  	} else {
>  		/* Spurious Interrupt!! */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521173347.2079560-1-bvanassche@acm.org?part=4