Re: [PATCH v2 9/9] ata: Annotate the code that uses the host lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Evasion of static analysis through `__assume_ctx_lock` and dangerous TO DO comments.
- [Medium] The `qc_issue` annotation incorrectly takes the address of a lock pointer, creating a double pointer.
- [Medium] Missing address-of operator in `__must_hold(host->lock)` annotations.
- [Medium] `ata_do_link_abort` asserts the wrong lock (`eh_mutex` instead of the port lock).
- [Low] Misleading comment refers to `scsi_host` instead of `lock`.
--

commit 1c3594816cc461100251a14c8ea9e6b3febb1a63
Author: Bart Van Assche <bvanassche@acm.org>

ata: Annotate the code that uses the host lock

This commit adds __must_hold() annotations for functions requiring the host
lock, along with context assertions to help the compiler understand lock
equivalencies. It identifies missing locks in ata_port_freeze() and
nv_do_interrupt().

> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
[ ... ]
> @@ -450,7 +450,8 @@ void ahci_error_handler(struct ata_port *ap)
>  	__must_hold(&ap->host->eh_mutex);
> -u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked);
> +u32 ahci_handle_port_intr(struct ata_host *host, u32 irq_masked)
> +	__must_hold(host->lock);

[Severity: Medium]
Since host->lock is an embedded struct (spinlock_t lock) rather than a
pointer, should this be __must_hold(&host->lock)? As written, passing the
struct by value might cause the static analyzer to fail when matching it
against spin_lock(&host->lock). This same pattern appears in
xgene_ahci_handle_broken_edge_irq, adma_intr_pkt, and other host->lock
annotations in this patch.

> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
[ ... ]
> @@ -1057,6 +1059,9 @@ static int ata_do_link_abort(struct ata_port *ap, struct ata_link *link)
>  	/* include internal tag in iteration */
>  	ata_qc_for_each_with_internal(ap, qc, tag) {
>  		if (qc && (!link || qc->dev->link == link)) {
> +			/* Tell the compiler that link->ap == ap. */
> +			__assume_ctx_lock(&link->ap->host->eh_mutex);
> +
>  			qc->flags |= ATA_QCFLAG_EH;
>  			ata_qc_complete(ap, qc);

[Severity: Medium]
The comment indicates the intent is to assert that link->ap == ap for the
port lock, but the code asserts the sleepable eh_mutex is held instead.
Could this be a copy-paste error that will prevent the static analyzer from
verifying ap->lock for the inner ata_qc_complete() call?

> @@ -1149,6 +1154,9 @@ static void __ata_port_freeze(struct ata_port *ap)
>   */
>  int ata_port_freeze(struct ata_port *ap)
>  {
> +	/* TO DO: fix all callers of this function that do not hold ap->lock. */
> +	__assume_ctx_lock(ap->lock);
> +
>  	__ata_port_freeze(ap);

[Severity: High]
Does using __assume_ctx_lock() here and in nv_do_interrupt() to silence
static analysis warnings defeat the purpose of the locking annotations?
If there are missing locks, it might be safer to fix the actual locking
bugs or use __must_hold() to properly flag the missing lock context.

> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
[ ... ]
> @@ -1465,7 +1473,10 @@ static irqreturn_t nv_do_interrupt(struct ata_host *host, u8 irq_stat)
>  {
>  	int i, handled = 0;
>  
> +	/* TO DO: protect the code below with &host->lock. */
> +
>  	for (i = 0; i < host->n_ports; i++) {
> +		__assume_ctx_lock(host->ports[i]->lock);
>  		handled += nv_host_intr(host->ports[i], irq_stat);
>  		irq_stat >>= NV_INT_PORT_SHIFT;
>  	}

[Severity: High]
If future developers follow this TO DO comment and add a lock acquisition
here, will it introduce a guaranteed hard deadlock? Existing callers like
nv_nf2_interrupt and nv_ck804_interrupt already hold host->lock before
calling this function, as it is a non-recursive spinlock.

> diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
> --- a/drivers/ata/sata_qstor.c
> +++ b/drivers/ata/sata_qstor.c
[ ... ]
> @@ -376,6 +381,11 @@ static inline unsigned int qs_intr_pkt(struct ata_host *host)
>  				continue;
>  			qc = ata_qc_from_tag(ap, ap->link.active_tag);
>  			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING))) {
> +				/*
> +				 * Tell the compiler that qc->dev->link->ap->
> +				 * scsi_host == host.
> +				 */
> +				__assume_ctx_lock(qc->dev->link->ap->lock);

[Severity: Low]
This isn't a bug, but the comment mentions scsi_host == host while the
code asserts the lock context. Is this a leftover copy-paste from another
annotation?

> diff --git a/include/linux/libata.h b/include/linux/libata.h
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
[ ... ]
> @@ -962,7 +962,8 @@ struct ata_port_operations {
>  	int (*qc_defer)(struct ata_queued_cmd *qc);
>  	int (*check_atapi_dma)(struct ata_queued_cmd *qc);
>  	enum ata_completion_errors (*qc_prep)(struct ata_queued_cmd *qc);
> -	unsigned int (*qc_issue)(struct ata_queued_cmd *qc);
> +	unsigned int (*qc_issue)(struct ata_queued_cmd *qc)
> +		__must_hold(&qc->dev->link->ap->lock);

[Severity: Medium]
Since ap->lock is already a pointer (spinlock_t *lock), does taking its
address with & here yield a spinlock_t **? This double pointer won't
match the lock context expression expected by the static analyzer when
spin_lock(ap->lock) is used. Should this be updated to drop the & operator?

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