[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-kernel
Subject:    Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops
From:       Marco Elver <elver () google ! com>
Date:       2024-05-02 19:29:04
Message-ID: CANpmjNN0n9UkYheZyBCQykrLYM9EDsmkp41a=x4hbgYyKDPZxw () mail ! gmail ! com
[Download RAW message or body]

On Thu, 2 May 2024 at 20:14, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, May 02, 2024 at 10:29:52AM -0700, Linus Torvalds wrote:
>
> > Yes, this is unusual. The *common* thing is to mark pointers as being
> > volatile. But this really is something entirely different from that.
>
> The common thing is to mark pointers are pointers to volatile;
> calling them "volatile pointers" is common and incorrect, and the only
> reason why that sloppy turn of phrase persists is that real "volatile
> pointers" are rare...
>
> Marco,

I think we agree on what we want. I misread the intention of Tetsuo in
[1], and provided incorrect feedback.

[1] https://lore.kernel.org/all/CANpmjNPtoKf1ysbKd=E8o753JT0DzBanzFBP234VBsazfufVAQ@mail.gmail.com/T/#u

>         struct foo volatile *p;
> declares p as a (non-volatile) pointer to volatile struct foo.
>         struct foo * volatile p;
> declares p as volatile pointer to (non-volatile) struct foo.
>
> The former is a statement about the objects whose addresses might
> be stored in p; the latter is a statement about the object p itself.
>
> Replace volatile with const and it becomes easier to experiment with:
>         char const *p;
>         char s[] = "barf";
>         char * const q = s;
>         ...
>         p = "yuck";     - fine, p itself can be modified
>         *p = 'a';       - error *p can not be modified, it's an l-value of type const char
>         q = s + 1;      - error, can't modify q
>         *q = 'a';       - fine, *q is l-value of type char
>         p = q;          - fine, right-hand side of assignment loses the top
>                           qualifier, so q (const pointer to char as l-value)
>                           becomes a plain pointer to char, which can be
>                           converted to pointer to const char, and stored in
>                           p (l-value of type pointer to const char)
>         strlen(q);      - almost the same story, except that it's passing
>                           an argument rather than assignment; they act the
>                           same way.
>         strcpy(q, "s"); - almost the same, except that here the type of
>                           argument is pointer to char rather than pointer to
>                           const char (strlen() promises not to modify the
>                           string passed to it, strcpy() obviously doesn't)
>         strcpy(p, "s"); - error; pointer to char converts to a pointer
>                           to const char, but not the other way round.
>
> The situations where you want a const (or volatile) pointer (as opposed to
> pointer to const or volatile object) are rare, but this is exactly what
> you are asking for - you want to say that the value of 'f_op' member
> in any struct file can change at any time.  That value is an address of
> some instance of struct file_operations and what you want to express is
> the property of f_op member itself, not that of the objects whose addresses
> might end up stored there.
>
> So having a driver do
>         const struct file_operations *ops = file->f_op;
> is fine - it's basically "take the value of 'file'; it will be an address
> of some struct file instance.  Fetch 'f_op' from that instance, without
> any assumptions of the stability of that member.  Use whatever value
> you find there as initial value of 'ops'".
>
> That's fine, and since nobody is going to change 'ops' itself behind your
> back, you don't need any qualifiers on it.  The type of 'ops' here is
> "(unqualified) pointer to const struct file_operations".

Is this feedback for the __data_racy attribute patch [2], or a comment
for the patch "tty: tty_io: remove hung_up_tty_fops"? [ With the
former I can help, with the latter Tetsuo can help. ]

[2] https://lore.kernel.org/all/20240502141242.2765090-1-elver@google.com/

The __data_racy attribute should behave like any other type qualifier
(be it const, volatile), and what you point out above applies equally,
no doubt about it. But I think it's important to treat it as a
completely distinct type qualifier - volatile is an implementation
detail (in non-KCSAN kernels it's a no-op).

Thanks,
-- Marco

[prev in list] [next in list] [prev in thread] [next in thread]