Re: [PATCH v3] ata: libata: Document when host->eh_mutex should be held

[Niklas Cassel <cassel@kernel.org>]

Hello Bart,

On Wed, May 27, 2026 at 01:25:20PM -0700, Bart Van Assche wrote:
> Annotate the following functions with __must_hold(&host->eh_mutex):
>  * All ata_port_operations.error_handler() implementations.
>  * ata_eh_reset() and ata_eh_recover() because these functions call
>    ata_eh_release() and ata_eh_acquire().
>  * All callers of ata_eh_reset() and ata_eh_recover().
> 
> Enable Clang's context analysis. This will cause the build to fail if
> e.g. a locking bug would be introduced in an error path. This patch
> should not affect the generated assembler code.
> 
> Note: although the Linux kernel documentation specifies 22 as minimal
> version for Clang for context analysis support, annotating function
> pointers is a Clang 23 feature. As one can see here, a patch has been
> queued that fixes the kernel documentation:
> https://lore.kernel.org/all/177926568868.711.3058599932884307249.tip-bot2@tip-bot2/
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> 
> Changes compared to v2:
>  - Instead of annotating only function declarations, annotate both function
>    declarations and function definitions.
>  - Changed __assume_ctx_lock() into lockdep_assert_held().
>  - Left out the host lock changes because there is no agreement about how to
>    annotate functions that expect that the host lock is held.

I am happy with this patch, and think that it is basically ready to be
merged as it is.

It only adds two lockdep_assert_held() calls, so that seems okay,
considering that it is not a crazy amount.


Now for ap->lock: It would be a shame if the work you did on ap->lock
annotations were to go to waste.

In:
https://lore.kernel.org/linux-ide/fcc80572-88da-4e16-ad09-69819f43cd73@acm.org/

You said that you would leave out the patches that adds an "ap" argument to
several functions. That sounds good.

If we go with the option to not add "ap" arguments everywhere, how many
lockdep_assert_held() calls would a patch that adds ap->lock annotations
need to add?

If it is not a crazy number, I think we should just do it.


Kind regards,
Niklas