All of lore.kernel.org
 help / color / mirror / Atom feed
From: Niklas Cassel <cassel@kernel.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Damien Le Moal <dlemoal@kernel.org>,
	Marco Elver <elver@google.com>,
	linux-ide@vger.kernel.org
Subject: Re: [PATCH] ata: libata-core: Enable context analysis
Date: Fri, 15 May 2026 14:11:07 +0200	[thread overview]
Message-ID: <agcNW8pFnALXHW3Q@ryzen> (raw)
In-Reply-To: <8a831bdf-6d32-45c4-bddc-9b14d6367407@acm.org>

Hello Bart,

On Tue, May 12, 2026 at 01:00:58PM -0700, Bart Van Assche wrote:
> On 5/11/26 9:53 AM, Niklas Cassel wrote:
> > 1) It seems that you are only checking for the EH mutex.
> > 
> > $ git grep -A 1 "LOCKING" drivers/ata/
> > 
> > Does have many functions with:
> > 	LOCKING:
> > 	spin_lock_irqsave(host lock)
> > 
> > Would it be possible to add __must_hold() annotations for these functions
> > too, but for ap->lock instead of EH mutex ?
> 
> This is something I can't do myself. This is something that should be done
> by an ATA expert. I'm not an ATA expert.

In my mind you are as much of an ATA expert as me and Damien :)


> 
> > 2) There seems to be some files that did not get any annotations, e.g.
> > drivers/ata/libata-scsi.c, drivers/ata/libata-sata.c,
> > drivers/ata/libata-acpi.c, drivers/ata/libata-pmp.c.
> > 
> > Would it be possible to add annotations for these files too?
> 
> Just like for (1), since there are inconsistencies between the
> "LOCKING:" documentation and the implementation, this is best done by
> an ATA expert.

Are there really a lot of inconsistencies though?

If you simply add annotations to e.g. drivers/ata/libata-scsi.c and
drivers/ata/libata-sata.c, which matches the kdoc, how many functions are
there that will not compile?

Considering how old and mostly unchanged most of these functions are, I
would expect the vast majority to actually be consistent with the kdoc.


> 
> The goal of the annotations in this patch is to make sure that the build
> doesn't break with CONTEXT_ANALYSIS := y. Any additional annotations can
> be implemented as follow-up patches.

Sure, doing so in follow-up patches would also be fine, although,
personally I would prefer to include at least a few ap->lock annotations
in this patch, so that people can see that it is possible to have
annotations also for ap->lock and not only EH mutex.


I could see in another thread that function pointer annotations require
clang 23:
https://lore.kernel.org/all/acqELlVC6vEeEF-W@elver.google.com/

Do you want us to pick up the patch in $subject now, or wait for the above
to land first?


Oh, there were some Sashiko review comments on the patch in $subject:
https://sashiko.dev/#/patchset/20260505042227.909666-1-bvanassche%40acm.org

Could you please check if they are valid review comments?


Kind regards,
Niklas

      reply	other threads:[~2026-05-15 12:11 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-05  4:22 [PATCH] ata: libata-core: Enable context analysis Bart Van Assche
2026-05-11 16:53 ` Niklas Cassel
2026-05-12 20:00   ` Bart Van Assche
2026-05-15 12:11     ` Niklas Cassel [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=agcNW8pFnALXHW3Q@ryzen \
    --to=cassel@kernel.org \
    --cc=bvanassche@acm.org \
    --cc=dlemoal@kernel.org \
    --cc=elver@google.com \
    --cc=linux-ide@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.