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

[Niklas Cassel <cassel@kernel.org>]

On Thu, May 28, 2026 at 10:33:30AM -0700, Bart Van Assche wrote:
> On 5/28/26 10:28 AM, Niklas Cassel wrote:
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 6733f2b14521..8bfd3ffd8b89 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1852,8 +1852,6 @@ static int ata_scsi_translate(struct ata_device *dev, struct scsi_cmnd *cmd,
> >   	struct ata_port *ap = dev->link->ap;
> >   	struct ata_queued_cmd *qc;
> > -	lockdep_assert_held(ap->lock);
> > -
> >   	/*
> >   	 * ata_scsi_qc_new() calls scsi_done(cmd) in case of failure. So we
> >   	 * have nothing further to do when allocating a qc fails.
> 
> This patch removes a useful runtime check. I think it is better to
> add the following annotation to ata_scsi_translate() if the goal is
> to make the compiler check that all callers hold ap->lock:
> 
> __must_hold(dev->link->ap->lock)

Well, this runtime check was added in 2025-12-17. We did not have it before
then, so it is a very recent check.

As you can see in patch 2/3, I opted to add a "struct ata_port *ap" parameter
to ata_scsi_translate() in order for the context to be provided all the way
from the function taking the lock (e.g. ata_scsi_queuecmd()) to ata_qc_issue().

So I guess you mean __must_hold(ap->lock) in ata_scsi_translate() ?
This series already does have that.


As you know lockdep_assert_held() does a __assume_ctx_lock().
What I don't like about this is that, if the lockdep_assert_held() is there,
you can even remove the __must_hold(ap->lock) from from ata_scsi_translate(),
and we will still not get a build error...

If I remove the lockdep_assert_held() and I don't have a __must_hold(ap->lock)
annotation on ata_scsi_translate(), then I get a build error.

So I wanted to remove it such that we don't have any "fake assumptions".
There is no need for any assumptions, since Clang will be able to verify the
call chain all the way from ata_scsi_queuecmd() to ata_qc_issue().

(I'm kind of starting to see why you wanted to remove the __assume_ctx_lock()
from lockdep_assert_held().)

I guess my take is that, lockdep_assert_held() is nice since it will do
runtime checking... but if we can do context analysis without any assumption
(uninterrupted by any __assume_ctx_lock()), then is seems like context analysis
is better in every way.


If we really want to do runtime verification, and runtime verification only,
perhaps we can add a new macro that explicitly does not call __assume_ctx_lock().
Something like lockdep_assert_held_no_assume_ctx_lock() :)


Kind regards,
Niklas