Re: [PATCH v3] tty: tty_io: remove hung_up_tty_fops

From: Marco Elver
Date: Thu May 02 2024 - 15:29:58 EST


On Thu, 2 May 2024 at 20:14, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/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@xxxxxxxxxx/

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