Re: [PATCH 2/7] ata: libata: Improve __ata_qc_complete()

[Niklas Cassel <cassel@kernel.org>]

On Mon, Aug 26, 2024 at 04:31:01PM +0900, Damien Le Moal wrote:
> The function __ata_qc_complete() is always called with a qc that already
> has been dereferenced and so is guaranteed to be non-NULL (as otherwise
> the kernel would have crashed). So remove the warning for a NULL qc as
> it is useless.
> 
> Furthermore, the qc passed to __ata_qc_complete() must always be marked
> as active with the ATA_QCFLAG_ACTIVE flag. If that is not the case, in
> addition to the existing warning, return early so that we do not attempt
> to complete an invalid qc.
> 
> Finally, fix the comment related to clearing the qc active flag as that
> operation applies to all devices, not just ATAPI ones.
> 
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e4023fc288ac..5acc37397f4b 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4829,8 +4829,9 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
>  	struct ata_port *ap;
>  	struct ata_link *link;
>  
> -	WARN_ON_ONCE(qc == NULL); /* ata_qc_from_tag _might_ return NULL */
> -	WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE));
> +	if (WARN_ON_ONCE(!(qc->flags & ATA_QCFLAG_ACTIVE)))
> +		return;
> +
>  	ap = qc->ap;
>  	link = qc->dev->link;
>  
> @@ -4852,9 +4853,10 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
>  		     ap->excl_link == link))
>  		ap->excl_link = NULL;
>  
> -	/* atapi: mark qc as inactive to prevent the interrupt handler
> -	 * from completing the command twice later, before the error handler
> -	 * is called. (when rc != 0 and atapi request sense is needed)
> +	/*
> +	 * Mark qc as inactive to prevent the port interrupt handler from
> +	 * completing the command twice later, before the error handler is
> +	 * called.
>  	 */
>  	qc->flags &= ~ATA_QCFLAG_ACTIVE;
>  	ap->qc_active &= ~(1ULL << qc->tag);
> -- 
> 2.46.0
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>