Re: [PATCH 1/3] ata: libata-scsi: Remove lockdep_assert_held() in ata_scsi_translate()

[Niklas Cassel <cassel@kernel.org>]

On Fri, May 29, 2026 at 03:42:31PM +0900, Damien Le Moal wrote:
> On 2026/05/29 2:28, Niklas Cassel wrote:
> > lockdep_assert_held() indirectly calls __assume_ctx_lock(), which will
> > suppress any real context analysis from being performed.
> > 
> > Remove the lockdep_assert_held(). This will be replaced with context
> > analysis in a follow-up commit.
> 
> I am not really liking the idea of removing runtime checks and replacing them
> with compile time checks that work only with clang. What about gcc ?

Intel 0-day CI testing robot builds with Clang as well.


> And are we sure that the compile time checks are as solid as the runtime
> ones ?

Runtime checking can be performed on more complex cases (like Marco said checks
in runtime does allow to compare two pointers which might point to the same
object, which is something that context analysis might cannot catch), however I
expect in the cases where compile time analysis is possible, as it is in this
case (since we now supply an ap pointer), it will be as solid as runtime checks.


> 
> As the sashiko comment mentions, can't we keep this and simply add a
> __must_hold() annotation in the declaration ?

What I don't like about it is that, since lockdep_assert_held() has a
__assume_ctx_lock(), it will tell the compiler that that the function
always has the lock, which renders the __must_hold() in ata_scsi_translate()
useless. I can even remove the annotation without getting a build error.
(So it basically yields the __most_hold() in ata_scsi_translate() a no-op.)

Without the lockdep_assert_held(), we do get a build error if
ata_scsi_translate() is missing the __must_hold() annotation.
I do like the fact that we get an error if we don't have annotations all
the way from ata_scsi_queuecmd() to ata_qc_issue(), if there is a function
in between that is missing annotations.

And since the lockdep_assert_held() was added only in December last year,
I don't see the big deal with replacing it with a compile time check.

But since both you and Bart seem to want to keep it, let's can keep it.


Kind regards,
Niklas