Re: [PATCH v2 1/9] ata: libata: Fix ata_exec_internal()

[Niklas Cassel <cassel@kernel.org>]

On Thu, May 21, 2026 at 10:33:29AM -0700, Bart Van Assche wrote:
> Some but not all ata_exec_internal() calls happen from the context of
> the ATA error handler. Commit c0c362b60e25 ("libata: implement cross-port
> EH exclusion") added ata_eh_release() and ata_eh_acquire() calls in
> ata_exec_internal(). Calling these functions is necessary if the caller
> holds the eh_mutex but is not allowed if the caller doesn't hold that
> mutex. Fix this by only calling ata_eh_release() and ata_eh_acquire() if
> the caller holds the eh_mutex. An example of an indirect caller of
> ata_exec_internal() that does not hold the eh_mutex is
> ata_host_register().
> 
> Fixes: c0c362b60e25 ("libata: implement cross-port EH exclusion")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ata/libata-core.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e76d15411e2a..3d25dec6ac6c 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1540,6 +1540,7 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
>  {
>  	struct ata_link *link = dev->link;
>  	struct ata_port *ap = link->ap;
> +	const bool owns_eh_mutex = ap->host->eh_owner == current;

We have similar code in ata_msleep(), which does:

bool owns_eh = ap && ap->host->eh_owner == current;

do we perhaps want the "ap && " here as well?


>  	u8 command = tf->command;
>  	struct ata_queued_cmd *qc;
>  	struct scatterlist sgl;
> @@ -1617,11 +1618,25 @@ unsigned int ata_exec_internal(struct ata_device *dev, struct ata_taskfile *tf,
>  		}
>  	}
>  
> -	ata_eh_release(ap);
> +	if (owns_eh_mutex) {
> +		/*
> +		 * To prevent that the compiler complains about the
> +		 * ata_eh_release() call below.
> +		 */
> +		__acquire(&ap->host->eh_mutex);
> +		ata_eh_release(ap);
> +	}
>  
>  	rc = wait_for_completion_timeout(&wait, msecs_to_jiffies(timeout));
>  
> -	ata_eh_acquire(ap);
> +	if (owns_eh_mutex) {
> +		ata_eh_acquire(ap);
> +		/*
> +		 * To prevent that the compiler complains about the above
> +		 * ata_eh_acquire() call.
> +		 */
> +		__release(&ap->host->eh_mutex);
> +	}
>  
>  	ata_sff_flush_pio_task(ap);
>  

Otherwise, this looks good to me:
Reviewed-by: Niklas Cassel <cassel@kernel.org>