Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()

[Linus Torvalds]

On Tue, 4 Jun 2024 at 11:25, Rodrigo Vivi  wrote:
>
> (I believe that the new _match_string(str1, size, str2) deserves a better 
> name,
> but since I'm bad with naming stuff, I don't have any good suggestion)

I hated the enormous cc list, so I didn't reply to all. But clearly
everybody else is just doing so.

Anyway, here's my NAK for this patch with explanation:


https://lore.kernel.org/all/CAHk-=wg5f99-gzpetsasjd0jb0jgcdmmpehrxctt4_i83h8...@mail.gmail.com/

and part of it was the naming, but there were other oddities there too.

   Linus

Re: [Intel-gfx] [linus:master] [file] 0ede61d858: will-it-scale.per_thread_ops -2.9% regression

[Linus Torvalds]

On Mon, 27 Nov 2023 at 02:27, Christian Brauner  wrote:
>
> So I've picked up your patch (vfs.misc). It's clever alright so thanks
> for the comments in there otherwise I would've stared at this for far
> too long.

Note that I should probably have commented on one other thing: that
whole "just load from fd[0] is always safe, because the fd[] array
always exists".

IOW, that whole "load and mask" thing only works when you know the
array exists at all.

Doing that "just mask the index" wouldn't be valid if "size = 0" is an
option and might mean that we don't have an array at all (ie if "->fd"
itself could be NULL.

But we never have a completely empty file descriptor array, and
fdp->fd is never NULL.  At a minimum 'max_fds' is NR_OPEN_DEFAULT.

(The whole 'tsk->files' could be NULL, but only for kernel threads or
when exiting, so fget_task() will check for *that*, but it's a
separate thing)

So that's why it's safe to *entirely* remove the whole

if (unlikely(fd >= fdt->max_fds))

test, and do it *all* with just "mask the index, and mask the resulting load".

Because we can *always* do that load at "fdt->fd[0]", and we want to
check the result for NULL anyway, so the "mask at the end and check
for NULL" is both natural and generates very good code.

Anyway, not a big deal, bit it might be worth noting before somebody
tries the same trick on some other array that *could* be zero-sized
and with a NULL base pointer, and where that 'array[0]' access isn't
necessarily guaranteed to be ok.

> It's a little unpleasant because of the cast-orama going on before we
> check the file pointer but I don't see that it's in any way wrong.

In my cleanup phase - which was a bit messy - I did wonder if I should
have some helper for it, since it shows up in both __fget_files_rcu()
and in files_lookup_fd_raw().

So I *could* have tried to add something like a
"masked_rcu_dereference()" that took the base pointer, the index, and
the mask, and did that whole dance.

Or I could have had just a "mask_pointer()" function, which we do
occasionally do in other places too (ie we hide data in low bits, and
then we mask them away when the pointer is used as a pointer).

But with only two users, it seemed to add more conceptual complexity
than it's worth, and I was not convinced that we'd want to expose that
pattern and have others use it.

So having a helper might clarify things, but it might also encourage
wrong users. I dunno.

I suspect the only real use for this ends up being this very special
"access the fdt->fd[] array using a file descriptor".

Anyway, that's why I largely just did it with comments, and commented
both places - and just kept the cast there in the open.

 Linus

Re: [Intel-gfx] [linus:master] [file] 0ede61d858: will-it-scale.per_thread_ops -2.9% regression

[Linus Torvalds]

On Sun, 26 Nov 2023 at 12:23, Linus Torvalds
 wrote:
>
> IOW, I might have messed up some "trivial cleanup" when prepping for
> sending it out...

Bah. Famous last words. One of the "trivial cleanups" made the code
more "obvious" by renaming the nospec mask as just "mask".

And that trivial rename broke that patch *entirely*, because now that
name shadowed the "fmode_t" mask argument.

Don't even ask how long it took me to go from "I *tested* this,
dammit, now it doesn't work at all" to "Oh God, I'm so stupid".

So that nobody else would waste any time on this, attached is a new
attempt. This time actually tested *after* the changes.

  Linus
From 45f70b5413a654d20ead410c533ec1d604bdb1e2 Mon Sep 17 00:00:00 2001
From: Linus Torvalds 
Date: Sun, 26 Nov 2023 12:24:38 -0800
Subject: [PATCH] Improve __fget_files_rcu() code generation (and thus __fget_light())

Commit 0ede61d8589c ("file: convert to SLAB_TYPESAFE_BY_RCU") caused a
performance regression as reported by the kernel test robot.

The __fget_light() function is one of those critical ones for some
loads, and the code generation was unnecessarily impacted.  Let's just
write that function to better.

Reported-by: kernel test robot 
Cc: Christian Brauner 
Cc: Jann Horn 
Cc: Mateusz Guzik 
Closes: https://lore.kernel.org/oe-lkp/202311201406.2022ca3f-oliver.s...@intel.com
Signed-off-by: Linus Torvalds 
---
 fs/file.c   | 48 +
 include/linux/fdtable.h | 15 -
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 5fb0b146e79e..608b4802c214 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -959,31 +959,42 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 		struct file *file;
 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
 		struct file __rcu **fdentry;
+		unsigned long nospec;
 
-		if (unlikely(fd >= fdt->max_fds))
+		/* Mask is a 0 for invalid fd's, ~0 for valid ones */
+		nospec = array_index_mask_nospec(fd, fdt->max_fds);
+
+		/* fdentry points to the 'fd' offset, or fdt->fd[0] */
+		fdentry = fdt->fd + (fd);
+
+		/* Do the load, then mask any invalid result */
+		file = rcu_dereference_raw(*fdentry);
+		file = (void *)(nospec & (unsigned long)file);
+
+		if (unlikely(!file))
 			return NULL;
 
-		fdentry = fdt->fd + array_index_nospec(fd, fdt->max_fds);
+		/*
+		 * Ok, we have a file pointer that was valid at
+		 * some point, but it might have become stale since.
+		 *
+		 * We need to confirm it by incrementing the refcount
+		 * and then check the lookup again.
+		 *
+		 * atomic_long_inc_not_zero() gives us a full memory
+		 * barrier. We only really need an 'acquire' one to
+		 * protect the loads below, but we don't have that.
+		 */
+		if (unlikely(!atomic_long_inc_not_zero(>f_count)))
+			continue;
 
 		/*
-		 * Ok, we have a file pointer. However, because we do
-		 * this all locklessly under RCU, we may be racing with
-		 * that file being closed.
-		 *
 		 * Such a race can take two forms:
 		 *
 		 *  (a) the file ref already went down to zero and the
 		 *  file hasn't been reused yet or the file count
 		 *  isn't zero but the file has already been reused.
-		 */
-		file = __get_file_rcu(fdentry);
-		if (unlikely(!file))
-			return NULL;
-
-		if (unlikely(IS_ERR(file)))
-			continue;
-
-		/*
+		 *
 		 *  (b) the file table entry has changed under us.
 		 *   Note that we don't need to re-check the 'fdt->fd'
 		 *   pointer having changed, because it always goes
@@ -991,7 +1002,8 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 		 *
 		 * If so, we need to put our ref and try again.
 		 */
-		if (unlikely(rcu_dereference_raw(files->fdt) != fdt)) {
+		if (unlikely(file != rcu_dereference_raw(*fdentry)) ||
+		unlikely(rcu_dereference_raw(files->fdt) != fdt)) {
 			fput(file);
 			continue;
 		}
@@ -1128,13 +1140,13 @@ static unsigned long __fget_light(unsigned int fd, fmode_t mask)
 	 * atomic_read_acquire() pairs with atomic_dec_and_test() in
 	 * put_files_struct().
 	 */
-	if (atomic_read_acquire(>count) == 1) {
+	if (likely(atomic_read_acquire(>count) == 1)) {
 		file = files_lookup_fd_raw(files, fd);
 		if (!file || unlikely(file->f_mode & mask))
 			return 0;
 		return (unsigned long)file;
 	} else {
-		file = __fget(fd, mask);
+		file = __fget_files(files, fd, mask);
 		if (!file)
 			return 0;
 		return FDPUT_FPUT | (unsigned long)file;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index bc4c3287a65e..80bd7789bab1 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -83,12 +83,17 @@ struct dentry;
 static inline struct file *files_lookup_fd_raw(struct files_struct *files, unsigned int fd)
 {
 	struct fdtable *fdt = rcu_dereference_raw(files->fdt);
+	unsigned lon

Re: [Intel-gfx] [linus:master] [file] 0ede61d858: will-it-scale.per_thread_ops -2.9% regression

[Linus Torvalds]

On Sun, 19 Nov 2023 at 23:11, kernel test robot  wrote:
>
> kernel test robot noticed a -2.9% regression of will-it-scale.per_thread_ops 
> on:
>
> commit: 0ede61d8589cc2d93aa78230d74ac58b5b8d0244 ("file: convert to 
> SLAB_TYPESAFE_BY_RCU")

Ok, so __fget_light() is one of our more performance-critical things,
and that commit ends up making it call a rather more expensive version
in __get_file_rcu(), so we have:

>  30.90 ą  4% -20.6   10.35 ą  2%  
> perf-profile.self.cycles-pp.__fget_light
>   0.00   +26.5   26.48
> perf-profile.self.cycles-pp.__get_file_rcu

and that "20% decrease balanced by 26% increase elsewhere" then
directly causes the ~3% regression.

I took a look at the code generation, and honestly, I think we're
better off just making __fget_files_rcu() have special logic for this
all, and not use __get_file_rcu().

The 'fd' case really is special because we need to do that
non-speculative pointer access.

Because it turns out that we already have to use array_index_nospec()
to safely generate that pointer to the fd entry, and once you have to
do that "use non-speculative accesses to generate a safe pointer", you
might as well just go whole hog.

So this takes a different approach, and uses the
array_index_mask_nospec() thing that we have exactly to generate that
non-speculative mask for these things:

/* Mask is a 0 for invalid fd's, ~0 for valid ones */
mask = array_index_mask_nospec(fd, fdt->max_fds);

and then it does something you can consider either horribly clever, or
horribly ugly (this first part is basically just
array_index_nospec()):

/* fdentry points to the 'fd' offset, or fdt->fd[0] */
fdentry = fdt->fd + (fd);

and then we can just *unconditionally* do the load - but since we
might be loading fd[0] for an invalid case, we need to mask the result
too:

/* Do the load, then mask any invalid result */
file = rcu_dereference_raw(*fdentry);
file = (void *)(mask & (unsigned long)file);

but now we have done everything without any conditionals, and the only
conditional is now "did we load NULL" - which includes that "we masked
the bad value".

Then we just do that atomic_long_inc_not_zero() on the file, and
re-check the pointer chain we used.

I made files_lookup_fd_raw() do the same thing.

The end result is much nicer code generation at least from my quick
check. And I assume the regression will be gone, and hopefully even
turned into an improvement since this is so hot.

Comments? I also looked at that odd OPTIMIZER_HIDE_VAR() that
__get_file_rcu() does, and I don't get it. Both things come from
volatile accesses, I don't see the point of those games, but I also
didn't care, since it's no longer in a critical code path.

Christian?

NOTE! This patch is not well tested. I verified an earlier version of
this, but have been playing with it since, so caveat emptor.

IOW, I might have messed up some "trivial cleanup" when prepping for
sending it out...

  Linus
 fs/file.c   | 48 ++--
 include/linux/fdtable.h | 15 ++-
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index 5fb0b146e79e..c74a6e8687d9 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -959,31 +959,42 @@ static inline struct file *__fget_files_rcu(struct files_struct *files,
 		struct file *file;
 		struct fdtable *fdt = rcu_dereference_raw(files->fdt);
 		struct file __rcu **fdentry;
+		unsigned long mask;
 
-		if (unlikely(fd >= fdt->max_fds))
+		/* Mask is a 0 for invalid fd's, ~0 for valid ones */
+		mask = array_index_mask_nospec(fd, fdt->max_fds);
+
+		/* fdentry points to the 'fd' offset, or fdt->fd[0] */
+		fdentry = fdt->fd + (fd);
+
+		/* Do the load, then mask any invalid result */
+		file = rcu_dereference_raw(*fdentry);
+		file = (void *)(mask & (unsigned long)file);
+
+		if (unlikely(!file))
 			return NULL;
 
-		fdentry = fdt->fd + array_index_nospec(fd, fdt->max_fds);
+		/*
+		 * Ok, we have a file pointer that was valid at
+		 * some point, but it might have become stale since.
+		 *
+		 * We need to confirm it by incrementing the refcount
+		 * and then check the lookup again.
+		 *
+		 * atomic_long_inc_not_zero() gives us a full memory
+		 * barrier. We only really need an 'acquire' one to
+		 * protect the loads below, but we don't have that.
+		 */
+		if (unlikely(!atomic_long_inc_not_zero(>f_count)))
+			continue;
 
 		/*
-		 * Ok, we have a file pointer. However, because we do
-		 * this all locklessly under RCU, we may be racing with
-		 * that file being closed.
-		 *
 		 * Such a race can take two forms:
 		 *
 		 *  (a) the file ref already went down to zero and the
 		 *  file hasn't been reused yet or the file count
 		 *  isn't zero but the file has already been reused.
-		 */
-		file = __get_file_rcu(fdentry);
-		if (unlikely(!file))
-			return NULL;
-
-		if (unlikely(IS_ERR(file)))
-		

Re: [Intel-gfx] [PATCH] file, i915: fix file reference for mmap_singleton()

[Linus Torvalds]

On Wed, 25 Oct 2023 at 10:36, Christian Brauner  wrote:
>
> I did think that the loop didn't really matter for this case but since
> it seemingly does paper over the weird semantics here let's drop it.
> Anyway, pushed to:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs.misc

LGTM.

We could then make the i915 mmap_singleton() do the proper loop over
the whole thing. Not quite the way Jann suggested - which would not
increment the file refcount properly, but instead of the current

smp_store_mb(i915->gem.mmap_singleton, file);
drm_dev_get(>drm);
return file;

it could do something much more complicated like

drm_dev_get(>drm);
if (cmpxchg(>gem.mmap_singleton, NULL, file) == NULL)
return file;

// mmap_singleton wasn't NULL: it might be an old one in the
// process of being torn down (with a zero refcount), or a new
// one that was just installed that we should use instead
fput(file);
file = READ_ONCE(i915->gem.mmap_singleton);
if (!file)
goto repeat;
// Is it valid? Just try again
if (atomic_read(>f_count))
goto repeat;

// We have a file pointer, but it's in the process of being torn
// down but gem.mmap_singleton hasn't been cleared yet. Yield to
// make progress.
sched_yield();
goto repeat;

which is disgusting, but would probably work.

Note the "probably work". I'm handwaving: "something like the above".

Presumably not even worth doing - I assume a correct client always
just does a single mmap() before starting work.

   Linus

Re: [Intel-gfx] [PATCH] file, i915: fix file reference for mmap_singleton()

[Linus Torvalds]

On Wed, 25 Oct 2023 at 02:01, Christian Brauner  wrote:
>
> rcu_read_lock();
> -   file = get_file_rcu(>gem.mmap_singleton);
> +   file = get_file_rcu_once(>gem.mmap_singleton);
> rcu_read_unlock();
> if (file)
> return file;

Honestly, the get_file_rcu_once() function seems a bit pointless.

The above doesn't want a loop at all. Yet your "once" still does loop,
because "even get_file_rcu_once() is trying to deal with the whole
"the pointer itself changed".

And the i915 code is actually designed to just depend on the atomicity
of the mmap_singleton access itself, in how it uses

cmpxchg(>gem.mmap_singleton, file, NULL);
...
file = READ_ONCE(i915->gem.mmap_singleton);

and literally says "I'll remove my singleton as it is released". The
important part there is that the 'map_singleton' pointer itself isn't
actually reference-counted - it's the reverse, where
reference-counting *other* instances will then auto-clear it.

And that's what then makes that get_file_rcu() model not work with it,
because get_file_rcu() kind of assumes that the argument it gets is
*part* of the reference counting, not a cached *result* of the
reference counting that gets cleared as a result of the ref going down
to zero.

I may explain my objections badly, but hopefully you get what I mean.

And I think that also means that using that new get_file_rcu_once()
may be hiding the actual problem, but it's still conceptually wrong,
because it still has that conceptual model of "the pointer I'm getting
is part of the reference counting", when it really isn't.

So I think we'd actually be better off with something that is more
clearly different from get_file_rcu() in naming, so make that
conceptual difference clearer. Make it be something like
"get_active_file(struct file **)", and make the implementation be just
exactly what your current __get_file_rcu() is with no loops at all.

Then thew i915 code ends up being

rcu_read_lock();
file = get_active_file(>gem.mmap_singleton);
rcu_read_unlock();

if (!IS_ERR_OR_NULL(file))
return file;

   .. create new mmap_singleton ..

and that's it.

If you don't want to expose __get_file_rcu() as-is, you could maybe just do

  struct file *get_active_file(struct file **fp)
  {
struct file *file;
rcu_read_lock();
file = __get_file_rcu(fp);
rcu_read_unlock();
return file;
  }

and then the i916 code would just drop the RCU locking that it has no
business even knowing about.

I realize that my complaints are a bit conceptual, and that the
practical end result is pretty much the same, but I do think that it
is worth noting this conceptual difference between "file pointer is
ref-counted" and "file counter is *controlled* by ref-counting".

Add a comment to the effect at get_active_file() users.

The old i915 code is already racy, in that it's called a "singleton",
but if you have multiple concurrent callers to mmap_singleton(), they
all might see a NULL file at first, and then they all create
*separate* new "singleton" files, and they *all* do that

smp_store_mb(i915->gem.mmap_singleton, file);

and one random case of them happens to win the race and set *its* file
as "THE singleton" file.

So your "let's loop if it changes" is not fixing anything as-is, and
it's just actually hiding what is going on.

If the i915 code wants to be consistent and really have just one
singleton, it needs to do the looping with some cmpxchg or whatever
itself. Doing the loop in some get_file_rcu_once() function for when
the file pointer changed isn't going to fix the race.

Am I missing something?

  Linus

Re: [Intel-gfx] [BUG 6.3-rc1] Bad lock in ttm_bo_delayed_delete()

[Linus Torvalds]

On Wed, Mar 15, 2023 at 5:22 PM Steven Rostedt  wrote:
>
> I hope that this gets in by -rc3, as I want to start basing my next branch
> on that tag.

My tree should have it now as commit c00133a9e87e ("drm/ttm: drop
extra ttm_bo_put in ttm_bo_cleanup_refs").

Linus

Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

[Linus Torvalds]

On Sat, Nov 5, 2022 at 2:03 PM Jason A. Donenfeld  wrote:
>
> Something that might help here is changing the `...` into
> `... when exists` or into `... when != ptr` or similar.

I actually tried that.

You don't want "when exists", you'd want "when forall", but that seems
to be the default.

And trying "when != ptr->timer" actually does the right thing in that
it gets rid of the case where the timer is modified outside of the
del_timer() case, *but* it also causes odd other changes to the
output.

Look at what it generates for that

   drivers/media/usb/pvrusb2/pvrusb2-hdw.c

file, which finds a lot of triggers with the "when !=  ptr->timer",
but only does one without it.

So I gave up, just because I clearly don't understand the rules.

(Comparing output is also fun because the ordering of the patches is
random, so consecutive runs with the same rule will give different
patches. I assume that it's just because it's done in parallel, but it
doesn't help the "try to see what changes when you change the script"
;)

 Linus

Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

[Linus Torvalds]

On Sat, Nov 5, 2022 at 11:04 AM Steven Rostedt  wrote:
>
> Here's the changes I made after running the script

Please. No.

What part of "I don't want extra crud" was I unclear on?

I'm not interested in converting everything. That's clearly a 6.,2
issue, possibly even longer considering how complicated the networking
side has been.

I'm not AT ALL interested in "oh, I then added my own small cleanups
on top to random files because I happened to notice them".

Repeat after me: "If the script didn't catch them, they weren't
trivially obvious".

And it does seem that right now the script itself is a bit too
generous, which is why it didn't notice that sometimes there wasn't a
kfree after all because of a goto around it. So clearly that "..."
doesn't really work, I think it accepts "_any_ path leads to the
second situation" rather than "_all_ paths lead to the second
situation".

But yeah, my coccinelle-foo is very weak too, and maybe there's no
pattern for "no flow control".

I would also like the coccinelle script to notice the "timer is used
afterwards", so that it does *not* modify that case that does

del_timer(>timer);
dch->timer.function = NULL;

since now the timer is modified in between the del_timer() and the kfree.

Again, that timer modification is then made pointless by changing the
del_timer() to a "timer_shutdown()", but at that point it is no longer
a "so obvious non-semantic change that it should be scripted". At that
point it's a manual thing.

So I think the "..." in your script should be "no flow control, and no
access to the timer", but do not know how to do that in coccinelle.

Julia?

And this thread has way too many participants, I suspect some email
systems will just mark it as spam as a result. Which is partly *why* I
would like to get rid of noisy changes that really don't matter - but
I would like it to be truly mindlessly obvious that there are *zero*
questions about it, and absolutely no manual intervention because the
patch is so strict that it's just unquestionably correct.

  Linus

Re: [Intel-gfx] [PATCH v4a 00/38] timers: Use timer_shutdown*() before freeing timers

[Linus Torvalds]

On Fri, Nov 4, 2022 at 11:01 PM Steven Rostedt  wrote:
>
> Patch 1 fixes an issue with sunrpc/xprt where it incorrectly uses
> del_singleshot_timer_sync() for something that is not a oneshot timer. As this
> will be converted to shutdown, this needs to be fixed first.

So this is the kind of thing that I would *not* want to get eartly.

I really would want to get just the infrastructure in to let people
start doing conversions.

And then the "mindlessly obvious patches that are done by scripting
and can not possibly matter".

The kinds that do not *need* review, because they are mechanical, and
that just cause pointless noise for the rest of the patches that *do*
want review.

Not this kind of thing that is so subtle that you have to explain it.
That's not a "scripted patch for no semantic change".

So leave the del_singleshot_timer_sync() cases alone, they are
irrelevant for the new infrastructure and for the "mindless scripted
conversion" patches.

> Patches 2-4 changes existing timer_shutdown() functions used locally in ARM 
> and
> some drivers to better namespace names.

Ok, these are relevant.

> Patch 5 implements the new timer_shutdown() and timer_shutdown_sync() 
> functions
> that disable re-arming the timer after they are called.

This is obviously what I'd want early so that people can start doign
this in their trees.

> Patches 6-28 change all the locations where there's a kfree(), kfree_rcu(),
> kmem_cache_free() and one call_rcu() call where the RCU function frees the
> timer (the workqueue patch) in the same function as the del_timer{,_sync}() is
> called on that timer, and there's no extra exit path between the del_timer and
> freeing of the timer.

So honestly, I was literally hoping for a "this is the coccinelle
script" kind of patch.

Now there seems to be a number of patches here that are actualyl
really hard to see that they are "obviously correct" and I can't tell
if they are actually scripted or not.

They don't *look* scripted, but I can't really tell.  I looked at the
patches with ten lines of context, and I didn't see the immediately
following kfree() even in that expanded patch context, so it's fairly
far away.

Others in the series were *definitely* not scripted, doing clearly
manual cleanups:

-if (dch->timer.function) {
-del_timer(>timer);
-dch->timer.function = NULL;
-}
+timer_shutdown(>timer);

so no, this does *not* make me feel "ok, this is all trivial".

IOW, I'd really want *just* the infrastructure and *just* the provably
trivial stuff. If it wasn't some scripted really obvious thing that
cannot possibly change anything and that wasn't then edited manually
for some reason, I really don't want it early.

IOW, any early conversions I'd take are literally about removing pure
mindless noise. Not about doing conversions.

And I wouldn't mind it as a single conversion patch that has the
coccinelle script as the explanation.

Really just THAT kind of "100% mindless conversion".

   Linus

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Linus Torvalds]

On Fri, Nov 4, 2022 at 12:42 PM Steven Rostedt  wrote:
>
> Linus, should I also add any patches that has already been acked by the
> respective maintainer?

No, I'd prefer to keep only the ones that are 100% unambiguously not
changing any semantics.

  Linus

Re: [Intel-gfx] [RFC][PATCH v3 00/33] timers: Use timer_shutdown*() before freeing timers

[Linus Torvalds]

On Thu, Nov 3, 2022 at 10:48 PM Steven Rostedt  wrote:
>
> Ideally, I would have the first patch go into this rc cycle, which is mostly
> non functional as it will allow the other patches to come in via the 
> respective
> subsystems in the next merge window.

Ack.

I also wonder if we could do the completely trivially correct
conversions immediately.

I'm talking about the scripted ones where it's currently a
"del_timer_sync()", and the very next action is freeing whatever data
structure the timer is in (possibly with something like free_irq() in
between - my point is that there's an unconditional free that is very
clear and unambiguous), so that there is absolutely no question about
whether they should use "timer_shutdown_sync()" or not.

IOW, things like patches 03, 17 and 31, and at least parts others in
this series.

This series clearly has several much more complex cases that need
actual real code review, and I think it would help to have the
completely unambiguous cases out of the way, just to get rid of noise.

So I'd take that first patch, and a scripted set of "this cannot
change any semantics" patches early.

Linus

Re: [Intel-gfx] [PATCH v13 5/9] drm/i915: Check for integer truncation on scatterlist creation

[Linus Torvalds]

On Wed, Sep 28, 2022 at 1:15 AM Gwan-gyeong Mun
 wrote:
>
> +   if (check_assign(obj->base.size >> PAGE_SHIFT, ))
> +   return -E2BIG;

I have to say, I find that new "check_assign()" macro use to be disgusting.

It's one thing to check for overflows.

It's another thing entirely to just assign something to a local variable.

This disgusting "let's check and assign" needs to die. It makes the
code a completely unreadable mess. The "user" wersion is even worse.

If you worry about overflow, then use a mix of

 (a) use a sufficiently large type to begin with

 (b) check for value range separately

and in this particular case, I also suspect that the whole range check
should have been somewhere else entirely - at the original creation of
that "obj" structure, not at one random end-point where it is used.

In other words, THIS WHOLE PATCH is just end-points checking the size
requirements of that "base.size" thing much too late, when it should
have been checked originally for some "maximum acceptable base size"
instead.

And that "maximum acceptable base size" should *not* be about "this is
the size of the variables we use". It should be a sanity check of
"this value is sane and fits in sane use cases".

Because "let's plug security checks" is most definitely not about
picking random assignments and saying "let's check this one". It's
about trying to catch things earlier than that.

Kees, you need to reign in the craziness in overflow.h.

 Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Wed, Mar 2, 2022 at 12:07 PM Kees Cook  wrote:
>
> I've long wanted to change kfree() to explicitly set pointers to NULL on
> free. https://github.com/KSPP/linux/issues/87

We've had this discussion with the gcc people in the past, and gcc
actually has some support for it, but it's sadly tied to the actual
function name (ie gcc has some special-casing for "free()")

See

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94527

for some of that discussion.

Oh, and I see some patch actually got merged since I looked there last
so that you can mark "deallocator" functions, but I think it's only
for the context matching, not for actually killing accesses to the
pointer afterwards.

   Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Tue, Mar 1, 2022 at 3:19 PM David Laight  wrote:
>
> Having said that there are so few users of list_entry_is_head()
> it is reasonable to generate two new names.

Well, the problem is that the users of list_entry_is_head() may be few
- but there are a number of _other_ ways to check "was that the HEAD
pointer", and not all of them are necessarily correct.

IOW, different places do different random tests for "did we walk the
whole loop without breaking out". And many of them happen to work. In
fact, in practice, pretty much *all* of them happen to work, and you
have to have the right struct layout and really really bad luck to hit
a case of "type confusion ended up causing the test to not work".

And *THAT* is the problem here. It's not the "there are 25ish places
that current use list_entry_is_head()".

It's the "there are ~480 places that use the type-confused HEAD entry
that has been cast to the wrong type".

And THAT is why I think we'd be better off with that bigger change
that simply means that you can't use the iterator variable at all
outside the loop, and try to make it something where the compiler can
help catch mis-uses.

Now, making the list_for_each_entry() thing force the iterator to NULL
at the end of the loop does fix the problem. The issue I have with it
is really just that you end up getting no warning at all from the
compiler if you mix old-style and new-style semantics. Now, you *will*
get an oops (if using a new-style iterator with an old-style check),
but many of these things will be in odd driver code and may happen
only for error cases.

And if you use a new-style check with an old-style iterator (ie some
backport problem), you will probably end up getting random memory
corruption, because you'll decide "it's not a HEAD entry", and then
you'll actually *use* the HEAD that has the wrong type cast associated
with it.

See what my worry is?

With the "don't use iterator outside the loop" approach, the exact
same code works in both the old world order and the new world order,
and you don't have the semantic confusion. And *if* you try to use the
iterator outside the loop, you'll _mostly_ (*) get a compiler warning
about it not being initialized.

 Linus

(*) Unless somebody initializes the iterator pointer pointlessly.
Which clearly does happen. Thus the "mostly". It's not perfect, and
that's most definitely not nice - but it should at least hopefully
make it that much harder to mess up.

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Tue, Mar 1, 2022 at 2:58 PM David Laight  wrote:
>
> Can it be resolved by making:
> #define list_entry_is_head(pos, head, member) ((pos) == NULL)
> and double-checking that it isn't used anywhere else (except in
> the list macros themselves).

Well, yes, except for the fact that then the name is entirely misleading...

And somebody possibly uses it together with list_first_entry() etc, so
it really is completely broken to mix that change with the list
traversal change.

 Linus

   Linus

Re: [Intel-gfx] [PATCH 6/6] treewide: remove check of list iterator against head past the loop body

[Linus Torvalds]

So looking at this patch, I really reacted to the fact that quite
often the "use outside the loop" case is all kinds of just plain
unnecessary, but _used_ to be a convenience feature.

I'll just quote the first chunk in it's entirely as an example - not
because I think this chunk is particularly important, but because it's
a good example:

On Mon, Feb 28, 2022 at 3:09 AM Jakob Koschel  wrote:
>
> diff --git a/arch/arm/mach-mmp/sram.c b/arch/arm/mach-mmp/sram.c
> index 6794e2db1ad5..fc47c107059b 100644
> --- a/arch/arm/mach-mmp/sram.c
> +++ b/arch/arm/mach-mmp/sram.c
> @@ -39,19 +39,22 @@ static LIST_HEAD(sram_bank_list);
>  struct gen_pool *sram_get_gpool(char *pool_name)
>  {
> struct sram_bank_info *info = NULL;
> +   struct sram_bank_info *tmp;
>
> if (!pool_name)
> return NULL;
>
> mutex_lock(_lock);
>
> -   list_for_each_entry(info, _bank_list, node)
> -   if (!strcmp(pool_name, info->pool_name))
> +   list_for_each_entry(tmp, _bank_list, node)
> +   if (!strcmp(pool_name, tmp->pool_name)) {
> +   info = tmp;
> break;
> +   }
>
> mutex_unlock(_lock);
>
> -   if (>node == _bank_list)
> +   if (!info)
> return NULL;
>
> return info->gpool;

I realize this was probably at least auto-generated with coccinelle,
but maybe that script could be taught to do avoid the "use after loop"
by simply moving the code _into_ the loop.

IOW, this all would be cleaner and clear written as

if (!pool_name)
return NULL;

mutex_lock(_lock);
list_for_each_entry(info, _bank_list, node) {
if (!strcmp(pool_name, info->pool_name)) {
mutex_unlock(_lock);
return info;
}
}
mutex_unlock(_lock);
return NULL;

Ta-daa - no use outside the loop, no need for new variables, just a
simple "just do it inside the loop". Yes, we end up having that lock
thing twice, but it looks worth it from a "make the code obvious"
standpoint.

Would it be even cleaner if the locking was done in the caller, and
the loop was some simple helper function? It probably would. But that
would require a bit more smarts than probably a simple coccinelle
script would do.

Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Tue, Mar 1, 2022 at 11:06 AM Linus Torvalds
 wrote:
>
> So instead of that simple "if (!entry)", we'd effectively have to
> continue to use something that still works with the old world order
> (ie that "if (list_entry_is_head())" model).

Just to prove my point about how this is painful, that doesn't work at all.

If the loop iterator at the end is NULL (good, in theory), we can't
use "list_entry_is_head()" to check whether we ended. We'd have to use
a new thing entirely, to handle the "list_for_each_entry() has the
old/new semantics" cases.

That's largely why I was pushing for the "let's make it impossible to
use the loop iterator at all outside the loop". It avoids the
confusing case, and the patches to move to that stricter semantic can
be merged independently (and before) doing the actual semantic change.

I'm not saying my suggested approach is wonderful either. Honestly,
it's painful that we have so nasty semantics for the end-of-loop case
for list_for_each_entry().

The minimal patch would clearly be to keep those broken semantics, and
just force everybody to use the list_entry_is_head() case. That's the
"we know we messed up, we are too lazy to fix it, we'll just work
around it and people need to be careful" approach.

And laziness is a virtue. But bad semantics are bad semantics. So it's
a question of balancing those two issues.

   Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 2:29 PM James Bottomley
 wrote:
>
> However, if the desire is really to poison the loop variable then we
> can do
>
> #define list_for_each_entry(pos, head, member)  \
> for (pos = list_first_entry(head, typeof(*pos), member);\
>  !list_entry_is_head(pos, head, member) && ((pos = NULL) == NULL; 
>   \
>  pos = list_next_entry(pos, member))
>
> Which would at least set pos to NULL when the loop completes.

That would actually have been excellent if we had done that
originally. It would not only avoid the stale and incorrectly typed
head entry left-over turd, it would also have made it very easy to
test for "did I find an entry in the loop".

But I don't much like it in the situation we are now.

Why? Mainly because it basically changes the semantics of the loop
_without_ any warnings about it.  And we don't actually get the
advantage of the nicer semantics, because we can't actually make code
do

list_for_each_entry(entry, ) {
..
}
if (!entry)
return -ESRCH;
.. use the entry we found ..

because that would be a disaster for back-porting, plus it would be a
flag-day issue (ie we'd have to change the semantics of the loop at
the same time we change every single user).

So instead of that simple "if (!entry)", we'd effectively have to
continue to use something that still works with the old world order
(ie that "if (list_entry_is_head())" model).

So we couldn't really take _advantage_ of the nicer semantics, and
we'd not even get a warning if somebody does it wrong - the code would
just silently do the wrong thing.

IOW: I don't think you are wrong about that patch: it would solve the
problem that Jakob wants to solve, and it would have absolutely been
much better if we had done this from the beginning. But I think that
in our current situation, it's actually a really fragile solution to
the "don't do that then" problem we have.

  Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Tue, Mar 1, 2022 at 10:14 AM Kees Cook  wrote:
>
> The first big glitch with -Wshadow was with shadowed global variables.
> GCC 4.8 fixed that, but it still yells about shadowed functions. What
> _almost_ works is -Wshadow=local.

Heh. Yeah, I just have long memories of "-Wshadow was a disaster". You
looked into the details.

> Another way to try to catch misused shadow variables is
> -Wunused-but-set-varible, but it, too, has tons of false positives.

That on the face of it should be an easy warning to get technically
right for a compiler.

So I assume the "false positives" are simply because we end up having
various variables that really don't end up being used - and
"intentionally" so).

Or rather, they might only be used under some config option - perhaps
the use is even syntactically there and parsed, but the compiler
notices that it's turned off under some

if (IS_ENABLED(..))

option? Because yeah, we have a lot of those.

I think that's a common theme with a lot of compiler warnings: on the
face of it they sound "obviously sane" and nobody should ever write
code like that.

A conditional that is always true? Sounds idiotic, and sounds like a
reasonable thing for a compiler to warn about, since why would you
have a conditional in the first place for that?

But then you realize that maybe the conditional is a build config
option, and "always true" suddenly makes sense. Or it's a test for
something that is always true on _that_architecture_ but not in some
general sense (ie testing "sizeof()"). Or it's a purely syntactic
conditional, like "do { } while (0)".

It's why I'm often so down on a lot of the odd warnings that are
hiding under W=1 and friends. They all may make sense in the trivial
case ("That is insane") but then in the end they happen for sane code.

And yeah, -Wshadow has had tons of history with macro nesting, and
just being badly done in the first place (eg "strlen" can be a
perfectly fine local variable).

That said, maybe people could ask the gcc and clan people for a way to
_mark_ the places where we expect to validly see shadowing. For
example, that "local variable in a macro expression statement" thing
is absolutely horrendous to fix with preprocessor tricks to try to
make for unique identifiers.

But I think it would be much more syntactically reasonable to add (for
example) a "shadow" attribute to such a variable exactly to tell the
compiler "yeah, yeah, I know this identifier could shadow an outer
one" and turn it off that way.

   Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 4:45 PM Linus Torvalds
 wrote:
>
> Yeah, except that's ugly beyond belief, plus it's literally not what
> we do in the kernel.

(Of course, I probably shouldn't have used 'min()' as an example,
because that is actually one of the few places where we do exactly
that, using our __UNIQUE_ID() macros. Exactly because people _have_
tried to do -Wshadow when doing W=2).

 Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 4:38 PM Segher Boessenkool
 wrote:
>
> In C its scope is the rest of the declaration and the entire loop, not
> anything after it.  This was the same in C++98 already, btw (but in
> pre-standard versions of C++ things were like you remember, yes, and it
> was painful).

Yeah, the original C++ model was just unadulterated garbage, with no
excuse for it, and the scope was not the loop, but the block the loop
existed in.

That would never have been acceptable for the kernel - it's basically
just an even uglier version of "put variable declarations in the
middle of code" (and we use "-Wdeclaration-after-statement" to
disallow that for kernel code, although apparently some of our user
space tooling code doesn't enforce or follow that rule).

The actual C99 version is the sane one which actually makes it easier
and clearer to have loop iterators that are clearly just in loop
scope.

That's a good idea in general, and I have wanted to start using that
in the kernel even aside from some of the loop construct macros.
Because putting variables in natural minimal scope is a GoodThing(tm).

Of course, we shouldn't go crazy with it. Even after we do that
-std=gnu11 thing, we'll have backports to worry about. And it's not
clear that we necessarily want to backport that gnu11 thing - since
people who run old stable kernels also may be still using those really
old compilers...

Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 3:26 PM Matthew Wilcox  wrote:
>
> #define ___PASTE(a, b)  a##b
> #define __PASTE(a, b) ___PASTE(a, b)
> #define _min(a, b, u) ({ \

Yeah, except that's ugly beyond belief, plus it's literally not what
we do in the kernel.

Really. The "-Wshadow doesn't work on the kernel" is not some new
issue, because you have to do completely insane things to the source
code to enable it.

Just compare your uglier-than-sin version to my straightforward one.
One does the usual and obvious "use a private variable to avoid the
classic multi-use of a macro argument". And the other one is an
abomination.

  Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 1:47 PM Jakob Koschel  wrote:
>
> The goal of this is to get compiler warnings right? This would indeed be 
> great.

Yes, so I don't mind having a one-time patch that has been gathered
using some automated checker tool, but I don't think that works from a
long-term maintenance perspective.

So if we have the basic rule being "don't use the loop iterator after
the loop has finished, because it can cause all kinds of subtle
issues", then in _addition_ to fixing the existing code paths that
have this issue, I really would want to (a) get a compiler warning for
future cases and (b) make it not actually _work_ for future cases.

Because otherwise it will just happen again.

> Changing the list_for_each_entry() macro first will break all of those cases
> (e.g. the ones using 'list_entry_is_head()).

So I have no problems with breaking cases that we basically already
have a patch for due to  your automated tool. There were certainly
more than a handful, but it didn't look _too_ bad to just make the
rule be "don't use the iterator after the loop".

Of course, that's just based on that patch of yours. Maybe there are a
ton of other cases that your patch didn't change, because they didn't
match your trigger case, so I may just be overly optimistic here.

But basically to _me_, the important part is that the end result is
maintainable longer-term. I'm more than happy to have a one-time patch
to fix a lot of dubious cases if we can then have clean rules going
forward.

> I assumed it is better to fix those cases first and then have a simple
> coccinelle script changing the macro + moving the iterator into the scope
> of the macro.

So that had been another plan of mine, until I actually looked at
changing the macro. In the one case I looked at, it was ugly beyond
belief.

It turns out that just syntactically, it's really nice to give the
type of the iterator from outside the way we do now. Yeah, it may be a
bit odd, and maybe it's partly because I'm so used to the
"list_for_each_list_entry()" syntax, but moving the type into the loop
construct really made it nasty - either one very complex line, or
having to split it over two lines which was even worse.

Maybe the place I looked at just happened to have a long typename, but
it's basically always going to be a struct, so it's never a _simple_
type. And it just looked very odd adn unnatural to have the type as
one of the "arguments" to that list_for_each_entry() macro.

So yes, initially my idea had been to just move the iterator entirely
inside the macro. But specifying the type got so ugly that I think
that

typeof (pos) pos

trick inside the macro really ends up giving us the best of all worlds:

 (a) let's us keep the existing syntax and code for all the nice cases
that did everything inside the loop anyway

 (b) gives us a nice warning for any normal use-after-loop case
(unless you explicitly initialized it like that
sgx_mmu_notifier_release() function did for no good reason

 (c) also guarantees that even if you don't get a warning,
non-converted (or newly written) bad code won't actually _work_

so you end up getting the new rules without any ambiguity or mistaken

> With this you are no longer able to set the 'outer' pos within the list
> iterator loop body or am I missing something?

Correct. Any assignment inside the loop will be entirely just to the
local loop case. So any "break;" out of the loop will have to set
another variable - like your updated patch did.

> I fail to see how this will make most of the changes in this
> patch obsolete (if that was the intention).

I hope my explanation above clarifies my thinking: I do not dislike
your patch, and in fact your patch is indeed required to make the new
semantics work.

What I disliked was always the maintainability of your patch - making
the rules be something that isn't actually visible in the source code,
and letting the old semantics still work as well as they ever did, and
having to basically run some verification pass to find bad users.

(I also disliked your original patch that mixed up the "CPU
speculation type safety" with the actual non-speculative problems, but
that was another issue).

Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 12:29 PM Johannes Berg
 wrote:
>
> If we're willing to change the API for the macro, we could do
>
>   list_for_each_entry(type, pos, head, member)
>
> and then actually take advantage of -Wshadow?

See my reply to Willy. There is no way -Wshadow will ever happen.

I considered that (type, pos, head, member) kind of thing, to the
point of trying it for one file, but it ends up as horrendous syntax.
It turns out that declaring the type separately really helps, and
avoids crazy long lines among other things.

It would be unacceptable for another reason too - the amount of churn
would just be immense. Every single use of that macro (and related
macros) would change, even the ones that really don't need it or want
it (ie the good kinds that already only use the variable inside the
loop).

So "typeof(pos) pos" may be ugly - but it's a very localized ugly.

Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 12:16 PM Matthew Wilcox  wrote:
>
> Then we can never use -Wshadow ;-(  I'd love to be able to turn it on;
> it catches real bugs.

Oh, we already can never use -Wshadow regardless of things like this.
That bridge hasn't just been burned, it never existed in the first
place.

The whole '-Wshadow' thing simply cannot work with local variables in
macros - something that we've used since day 1.

Try this (as a "p.c" file):

#define min(a,b) ({ \
typeof(a) __a = (a);\
typeof(b) __b = (b);\
__a < __b ? __a : __b; })

int min3(int a, int b, int c)
{
return min(a,min(b,c));
}

and now do "gcc -O2 -S t.c".

Then try it with -Wshadow.

In other words, -Wshadow is simply not acceptable. Never has been,
never will be, and that has nothing to do with the

typeof(pos) pos

kind of thing.

Your argument just isn't an argument.

  Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 12:10 PM Linus Torvalds
 wrote:
>
> We can do
>
> typeof(pos) pos
>
> in the 'for ()' loop, and never use __iter at all.
>
> That means that inside the for-loop, we use a _different_ 'pos' than outside.

The thing that makes me throw up in my mouth a bit is that in that

typeof(pos) pos

the first 'pos' (that we use for just the typeof) is that outer-level
'pos', IOW it's a *different* 'pos' than the second 'pos' in that same
declaration that declares the inner level shadowing new 'pos'
variable.

If I was a compiler person, I would say "Linus, that thing is too ugly
to live", and I would hate it. I'm just hoping that even compiler
people say "that's *so* ugly it's almost beautiful".

Because it does seem to work. It's not pretty, but hey, it's not like
our headers are really ever be winning any beauty contests...

Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 12:03 PM Linus Torvalds
 wrote:
>
> Side note: we do need *some* way to do it.

Ooh.

This patch is a work of art.

And I mean that in the worst possible way.

We can do

typeof(pos) pos

in the 'for ()' loop, and never use __iter at all.

That means that inside the for-loop, we use a _different_ 'pos' than outside.

And then the compiler will not see some "might be uninitialized", but
the outer 'pos' *will* be uninitialized.

Unless, of course, the outer 'pos' had that pointless explicit initializer.

Here - can somebody poke holes in this "work of art" patch?

 Linus
 Makefile   | 2 +-
 arch/x86/kernel/cpu/sgx/encl.c | 2 +-
 include/linux/list.h   | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index daeb5c88b50b..cc4b0a266af0 100644
--- a/Makefile
+++ b/Makefile
@@ -515,7 +515,7 @@ KBUILD_CFLAGS   := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \
 		   -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \
 		   -Werror=implicit-function-declaration -Werror=implicit-int \
 		   -Werror=return-type -Wno-format-security \
-		   -std=gnu89
+		   -std=gnu11
 KBUILD_CPPFLAGS := -D__KERNEL__
 KBUILD_AFLAGS_KERNEL :=
 KBUILD_CFLAGS_KERNEL :=
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..87db2f3936b0 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,7 @@ static void sgx_mmu_notifier_release(struct mmu_notifier *mn,
  struct mm_struct *mm)
 {
 	struct sgx_encl_mm *encl_mm = container_of(mn, struct sgx_encl_mm, mmu_notifier);
-	struct sgx_encl_mm *tmp = NULL;
+	struct sgx_encl_mm *tmp;
 
 	/*
 	 * The enclave itself can remove encl_mm.  Note, objects can't be moved
diff --git a/include/linux/list.h b/include/linux/list.h
index dd6c2041d09c..708078b2f24d 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -634,9 +634,9 @@ static inline void list_splice_tail_init(struct list_head *list,
  * @head:	the head for your list.
  * @member:	the name of the list_head within the struct.
  */
-#define list_for_each_entry(pos, head, member)\
-	for (pos = list_first_entry(head, typeof(*pos), member);	\
-	 !list_entry_is_head(pos, head, member);			\
+#define list_for_each_entry(pos, head, member)	\
+	for (typeof(pos) pos = list_first_entry(head, typeof(*pos), member);	\
+	 !list_entry_is_head(pos, head, member);	\
 	 pos = list_next_entry(pos, member))
 
 /**

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 11:56 AM Linus Torvalds
 wrote:
>
> I do wish we could actually poison the 'pos' value after the loop
> somehow - but clearly the "might be uninitialized" I was hoping for
> isn't the way to do it.

Side note: we do need *some* way to do it.

Because if we make that change, and only set it to another pointer
when not the head, then we very much change the semantics of
"list_for_each_head()". The "list was empty" case would now exit with
'pos' not set at all (or set to NULL if we add that). Or it would be
set to the last entry.

And regardless, that means that all the

if (pos == head)

kinds of checks after the loop would be fundamentally broken.

Darn. I really hoped for (and naively expected) that we could actually
have the compiler warn about the use-after-loop case. That whole
"compiler will now complain about bad use" was integral to my clever
plan to use the C99 feature of declaring the iterator inside the loop.

But my "clever plan" was apparently some ACME-level Wile E. Coyote sh*t.

Darn.

   Linus

Re: [Intel-gfx] [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr

[Linus Torvalds]

On Mon, Feb 28, 2022 at 4:19 AM Christian König
 wrote:
>
> I don't think that using the extra variable makes the code in any way
> more reliable or easier to read.

So I think the next step is to do the attached patch (which requires
that "-std=gnu11" that was discussed in the original thread).

That will guarantee that the 'pos' parameter of list_for_each_entry()
is only updated INSIDE the for_each_list_entry() loop, and can never
point to the (wrongly typed) head entry.

And I would actually hope that it should actually cause compiler
warnings about possibly uninitialized variables if people then use the
'pos' pointer outside the loop. Except

 (a) that code in sgx/encl.c currently initializes 'tmp' to NULL for
inexplicable reasons - possibly because it already expected this
behavior

 (b) when I remove that NULL initializer, I still don't get a warning,
because we've disabled -Wno-maybe-uninitialized since it results in so
many false positives.

Oh well.

Anyway, give this patch a look, and at least if it's expanded to do
"(pos) = NULL" in the entry statement for the for-loop, it will avoid
the HEAD type confusion that Jakob is working on. And I think in a
cleaner way than the horrid games he plays.

(But it won't avoid possible CPU speculation of such type confusion.
That, in my opinion, is a completely different issue)

I do wish we could actually poison the 'pos' value after the loop
somehow - but clearly the "might be uninitialized" I was hoping for
isn't the way to do it.

Anybody have any ideas?

Linus


p
Description: Binary data

Re: [Intel-gfx] [PATCH] [v2] Kbuild: move to -std=gnu11

[Linus Torvalds]

On Mon, Feb 28, 2022 at 3:37 AM Arnd Bergmann  wrote:
>
> I think the KBUILD_USERCFLAGS portion and the modpost.c fix for it
> make sense regardless of the -std=gnu11 change

I do think they make sense, but I want to note again that people doing
cross builds obviously use different tools for user builds than for
the kernel. In fact, even not cross-building, we've had situations
where the "kbuild" compiler is different from the host compiler,
because people have upgraded one but not the other (upgrading the
kernel build environment is actually much easier than upgrading the
host build environment, because you don't need all the random
libraries etc, and you can literally _just_ build your own gcc and
binutils)

And we have *not* necessarily required that the host tools match the
kernel tools.

So I could well imagine that there are people who build their kernels,
but their host build environment might be old enough that -std=gnu11
is problematic for that part.

And note how any change to  KBUILD_USERCFLAGS is reflected in KBUILD_HOSTCFLAGS.

So I would suggest that the KBUILD_USERCFLAGS part of the patch (and
the modpost.c change that goes with it) be done as a separate commit.
Because we might end up reverting that part.

Hmm?

   Linus

Re: [Intel-gfx] [greybus-dev] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments

[Linus Torvalds]

On Sun, Feb 27, 2022 at 3:04 PM Alex Elder  wrote:
>
> Glancing at the Greybus code, I don't believe there's any
> reason it needs to shift a negative value.  Such warnings
> could be fixed by making certain variables unsigned, for
> example.

As mentioned in the original thread, making things unsigned actually
is likely to introduce bugs and make things worse.

The warning is simply bogus, and the fact that it was enabled by
-Wextra in gcc for std=gnu99 and up was a mistake that looks likely to
be fixed in gcc.

So don't try to "fix" the code to make any possible warnings go away.
You may just make things worse.

(That is often true in general for the more esoteric warnings, but in
this case it's just painfully more obvious).

  Linus

Re: [Intel-gfx] [PATCH] Kbuild: remove -std=gnu89 from compiler arguments

[Linus Torvalds]

On Sun, Feb 27, 2022 at 1:54 PM Arnd Bergmann  wrote:
>
> Since the differences between gnu99, gnu11 and gnu17 are fairly minimal
> and mainly impact warnings at the -Wpedantic level that the kernel
> never enables, the easiest way is to just leave out the -std=gnu89
> argument entirely, and rely on the compiler default language setting,
> which is gnu11 for gcc-5, and gnu1x/gnu17 for all other supported
> versions of gcc or clang.

Honestly, I'd rather keep the C version we support as some explicit
thing, instead of "whatever the installed compiler is".

Not only do I suspect that you can set it in gcc spec files (so the
standard version might actually be site-specific, not compiler version
specific), but particularly with clang, I'd like that "GNU extensions
enabled" to be explicit. Yes, maybe it's the default, but let's make
sure.

The C version level has traditionally had a lot of odd semantic
meaning details - you mention "inline", others have existed. So it's
not just the actual new features that some C version implements, it's
those kind of "same syntax, different meaning" issues. I really don't
think that's something we want in the kernel any more.

Been there, done that, and we did the explicit standards level for a reason.

It may be true that c99/c11/c17 are all very similar, and don't have
those issues. Or maybe they do.

And I don't want somebody with a newer compiler version to not notice
that he or she ended up using a c17 feature, just because _that_
compiler supported it, and then other people get build errors because
their compilers use gnu11 instead by default.

Put another way: I see absolutely no upside to allowing different
users using higher/lower versions of the standard. There are only
downsides.

If gnu11 is supported by gcc-5.1 and up, and all the relevant clang
versions, then let's just pick that.

And if there are any possible future advantages to gnu17 (or eventual
gnu2x versions), let's document those, so that we can say "once our
compiler version requirements go up sufficiently, we'll move to gnuXX
because we want to take advantage of YY".

Please?

   Linus

   Linus

Re: [Intel-gfx] [BUG 5.15-rc3] kernel BUG at drivers/gpu/drm/i915/i915_sw_fence.c:245!

[Linus Torvalds]

On Sat, Oct 2, 2021 at 5:17 AM Steven Rostedt  wrote:
>
> On Sat, 2 Oct 2021 03:17:29 -0700 (PDT)
> Hugh Dickins  wrote:
>
> > Yes (though bisection doesn't work right on this one): the fix
>
> Interesting, as it appeared to be very reliable. But I didn't do the
> "try before / after" on the patch.

Well, even the before/after might well have worked, since the problem
depended on how that sw_fence_dummy_notify() function ended up
aligned. So random unrelated changes could re-align it just by
mistake.

Patch applied directly.

I'd also like to point out how that BUG_ON() actually made things
worse, and made this harder to debug. If it had been a WARN_ON_ONCE(),
this would presumably not even have needed bisecting, it would have
been obvious.

BUG_ON() really is pretty much *always* the wrong thing to do. It
onl;y results in problems being harder to see because you end up with
a dead machine and the message is often hidden.

  Linus

Re: [Intel-gfx] Public i915 CI shardruns are disabled

[Linus Torvalds]

Ok, slightly delayed by dinner, but commit caf6912f3f4a ("swap: fix
swapfile read/write offset") is out in my tree now.

Dave - can you check that the current -git works for your CI people?

Thanks,
  Linus

On Tue, Mar 2, 2021 at 5:18 PM Jens Axboe  wrote:
>
> On 3/2/21 6:01 PM, Linus Torvalds wrote:
> > On Tue, Mar 2, 2021 at 4:36 PM Jens Axboe  wrote:
> >>
> >> Or if you want a pull, just let me know. Have another misc patch to
> >> flush out anyway that doesn't belong in any of my usual branches.
> >
> > Ok, if you have something else pending anyway, let's do that. Send me
> > the pull request, and I'll take it asap.
>
> Done
>
> --
> Jens Axboe
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Public i915 CI shardruns are disabled

[Linus Torvalds]

On Tue, Mar 2, 2021 at 4:36 PM Jens Axboe  wrote:
>
> Or if you want a pull, just let me know. Have another misc patch to
> flush out anyway that doesn't belong in any of my usual branches.

Ok, if you have something else pending anyway, let's do that. Send me
the pull request, and I'll take it asap.

Thanks,
   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Public i915 CI shardruns are disabled

[Linus Torvalds]

On Tue, Mar 2, 2021 at 3:38 PM Dave Airlie  wrote:
>
> Looks like Jens saw it at least, he posted this on twitter a few mins
> ago so I assume it'll be incoming soon.
>
> https://git.kernel.dk/cgit/linux-block/commit/?h=swap-fix

Ahh. You use a swap file. This might be the same thing that I think
the phoronix people hit as ext4 corruption this merge window.

Jens, if that can get confirmed, please send it my way asap.. Thanks,

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Public i915 CI shardruns are disabled

[Linus Torvalds]

Adding the right people.

It seems that the three commits that needed reverting are

  f885056a48cc ("mm: simplify swapdev_block")
  3e3126cf2a6d ("mm: only make map_swap_entry available for CONFIG_HIBERNATION")
  48d15436fde6 ("mm: remove get_swap_bio")

and while they look very harmless to me, let's bring in Christoph and
Jens who were actually involved with them.

I'm assuming that it's that third one that is the real issue (and the
two other ones were to get to it), but it would also be good to know
what the actual details of the regression actually were.

Maybe that's obvious to somebody who has more context about the 9815
CI runs and its web interface, but it sure isn't clear to me.

Jens, Christoph?

  Linus

On Tue, Mar 2, 2021 at 11:31 AM Dave Airlie  wrote:
>
> On Wed, 3 Mar 2021 at 03:27, Sarvela, Tomi P  wrote:
> >
> > The regression has been identified; Chris Wilson found commits touching
> >
> > swapfile.c, and reverting them the issue couldn’t be reproduced any more.
> >
> >
> >
> > https://patchwork.freedesktop.org/series/87549/
> >
> >
> >
> > This revert will be applied to core-for-CI branch. When new CI_DRM has
> >
> > been built, shard-testing will be enabled again.
>
> Just making sure this is on the radar upstream.
>
> Dave.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [BUG] on reboot: bisected to: drm/i915: Shut down displays gracefully on reboot

[Linus Torvalds]

On Thu, Jan 14, 2021 at 2:01 PM Steven Rostedt  wrote:
>
> Thanks, I take it, it will be going into mainline soon.

Just got merged - it might be a good idea to verify that your problem is solved.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch V3 22/37] highmem: High implementation details and document API

[Linus Torvalds]

On Tue, Nov 3, 2020 at 2:33 AM Thomas Gleixner  wrote:
>
> +static inline void *kmap(struct page *page)
> +{
> +   void *addr;
> +
> +   might_sleep();
> +   if (!PageHighMem(page))
> +   addr = page_address(page);
> +   else
> +   addr = kmap_high(page);
> +   kmap_flush_tlb((unsigned long)addr);
> +   return addr;
> +}
> +
> +static inline void kunmap(struct page *page)
> +{
> +   might_sleep();
> +   if (!PageHighMem(page))
> +   return;
> +   kunmap_high(page);
> +}

I have no complaints about the patch, but it strikes me that if people
want to actually have much better debug coverage, this is where it
should be (I like the "every other address" thing too, don't get me
wrong).

In particular, instead of these PageHighMem(page) tests, I think
something like this would be better:

   #ifdef CONFIG_DEBUG_HIGHMEM
 #define page_use_kmap(page) ((page),1)
   #else
 #define page_use_kmap(page) PageHighMem(page)
   #endif

adn then replace those "if (!PageHighMem(page))" tests with "if
(!page_use_kmap())" instead.

IOW, in debug mode, it would _always_ remap the page, whether it's
highmem or not. That would really stress the highmem code and find any
fragilities.

No?

Anyway, this is all sepatrate from the series, which still looks fine
to me. Just a reaction to seeing the patch, and Thomas' earlier
mention that the highmem debugging doesn't actually do much.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Mon, Sep 21, 2020 at 12:39 AM Thomas Gleixner  wrote:
>
> If a task is migrated to a different CPU then the mapping address will
> change which will explode in colourful ways.

Heh.

Right you are.

Maybe we really *could* call this new kmap functionality something
like "kmap_percpu()" (or maybe "local" is good enough), and make it
act like your RT code does for spinlocks - not disable preemption, but
only disabling CPU migration.

That would probably be good enough for a lot of users that don't want
to expose excessive latencies, but where it's really not a huge deal
to say "stick to this CPU for a short while".

The crypto code certainly sounds like one such case.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Sun, Sep 20, 2020 at 10:42 AM Linus Torvalds
 wrote:
>
> Yeah, that looks much easier to explain. Ack.

Btw, one thing that might be a good idea at least initially is to add
a check for p->kmap_ctrl.idx being zero at fork, exit and maybe
syscall return time (but that last one may be too cumbersome to really
worry about).

The kmap_atomic() interface basically has a lot of coverage for leaked
as part of all the "might_sleep()" checks sprinkled around,  The new
kmap_temporary/local/whatever wouldn't have that kind of incidental
debug checking, and any leaked kmap indexes would be rather hard to
debug (much) later when they cause index overflows or whatever.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Sun, Sep 20, 2020 at 10:40 AM Thomas Gleixner  wrote:
>
> I think the more obvious solution is to split the whole exercise:
>
>   schedule()
>  prepare_switch()
> unmap()
>
> switch_to()
>
> finish_switch()
> map()

Yeah, that looks much easier to explain. Ack.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Sun, Sep 20, 2020 at 1:49 AM Thomas Gleixner  wrote:
>
> Actually most usage sites of kmap atomic do not need page faults to be
> disabled at all.

Right. I think the pagefault disabling has (almost) nothing at all to
do with the kmap() itself - it comes from the "atomic" part, not the
"kmap" part.

I say *almost*, because there is one issue that needs some thought:
the amount of kmap nesting.

The kmap_atomic() interface - and your local/temporary/whatever
versions of it - depends very much inherently on being strictly
nesting. In fact, it depends so much on it that maybe that should be
part of the new name?

It's very wrong to do

addr1 = kmap_atomic();
addr2 = kmap_atomic();
..do something with addr 1..
kunmap_atomic(addr1);
.. do something with addr 2..
kunmap_atomic(addr2);

because the way we allocate the slots is by using a percpu-atomic
inc-return (and we deallocate using dec).

So it's fundamentally a stack.

And that's perfectly fine for page faults - if they do any kmaps,
those will obviously nest.

So the only issue with page faults might be that the stack grows
_larger_. And that might need some thought. We already make the kmap
stack bigger for CONFIG_DEBUG_HIGHMEM, and it's possibly that if we
allow page faults we need to make the kmap stack bigger still.

Btw, looking at the stack code, Ithink your new implementation of it
is a bit scary:

   static inline int kmap_atomic_idx_push(void)
   {
  -   int idx = __this_cpu_inc_return(__kmap_atomic_idx) - 1;
  +   int idx = current->kmap_ctrl.idx++;

and now that 'current->kmap_ctrl.idx' is not atomic wrt

 (a) NMI's (this may be ok, maybe we never do kmaps in NMIs, and with
nesting I think it's fine anyway - the NMI will undo whatever it did)

 (b) the prev/next switch

And that (b) part worries me. You do the kmap_switch_temporary() to
switch the entries, but you do that *separately* from actually
switching 'current' to the new value.

So kmap_switch_temporary() looks safe, but I don't think it actually
is. Because while it first unmaps the old entries and then remaps the
new ones, an interrupt can come in, and at that point it matters what
is *CURRENT*.

And regardless of whether 'current' is 'prev' or 'next', that
kmap_switch_temporary() loop may be doing the wrong thing, depending
on which one had the deeper stack. The interrupt will be using
whatever "current->kmap_ctrl.idx" is, but that might overwrite entries
that are in the process of being restored (if current is still 'prev',
but kmap_switch_temporary() is in the "restore @next's kmaps" pgase),
or it might stomp on entries that have been pte_clear()'ed by the
'prev' thing.

I dunno. The latter may be one of those "it works anyway, it
overwrites things we don't care about", but the former will most
definitely not work.

And it will be completely impossible to debug, because it will depend
on an interrupt that uses kmap_local/atomic/whatever() coming in
_just_ at the right point in the scheduler, and only when the
scheduler has been entered with the right number of kmap entries on
the prev/next stack.

And no developer will ever see this with any amount of debug code
enabled, because it will only hit on legacy platforms that do this
kmap anyway.

So honestly, that code scares me. I think it's buggy. And even if it
"happens to work", it does so for all the wrong reasons, and is very
fragile.

So I would suggest:

 - continue to use an actual per-cpu kmap_atomic_idx

 - make the switching code save the old idx, then unmap the old
entries one by one (while doing the proper "pop" action), and then map
the new entries one by one (while doing the proper "push" action).

which would mean that the only index that is actually ever *USED* is
the percpu one, and it's always up-to-date and pushed/popped for
individual entries, rather than this - imho completely bogus -
optimization where you use "p->kmap_ctrl.idx" directly and very very
unsafely.

Alternatively, that process counter would need about a hundred lines
of commentary about exactly why it's safe. Because I don't think it
is.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Sat, Sep 19, 2020 at 10:39 AM Matthew Wilcox  wrote:
>
> My concern with that is people might use kmap() and then pass the address
> to a different task.  So we need to audit the current users of kmap()
> and convert any that do that into using vmap() instead.

Ahh. Yes, I guess they might do that. It sounds strange, but not
entirely crazy - I could imagine some "PIO thread" that does IO to a
page that has been set up by somebody else using kmap(). Or similar.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch RFC 00/15] mm/highmem: Provide a preemptible variant of kmap_atomic & friends

[Linus Torvalds]

On Sat, Sep 19, 2020 at 2:50 AM Thomas Gleixner  wrote:
>
> this provides a preemptible variant of kmap_atomic & related
> interfaces. This is achieved by:

Ack. This looks really nice, even apart from the new capability.

The only thing I really reacted to is that the name doesn't make sense
to me: "kmap_temporary()" seems a bit odd.

Particularly for an interface that really is basically meant as a
better replacement of "kmap_atomic()" (but is perhaps also a better
replacement for "kmap()").

I think I understand how the name came about: I think the "temporary"
is there as a distinction from the "longterm" regular kmap(). So I
think it makes some sense from an internal implementation angle, but I
don't think it makes a lot of sense from an interface name.

I don't know what might be a better name, but if we want to emphasize
that it's thread-private and a one-off, maybe "local" would be a
better naming, and make it distinct from the "global" nature of the
old kmap() interface?

However, another solution might be to just use this new preemptible
"local" kmap(), and remove the old global one entirely. Yes, the old
global one caches the page table mapping and that sounds really
efficient and nice. But it's actually horribly horribly bad, because
it means that we need to use locking for them. Your new "temporary"
implementation seems to be fundamentally better locking-wise, and only
need preemption disabling as locking (and is equally fast for the
non-highmem case).

So I wonder if the single-page TLB flush isn't a better model, and
whether it wouldn't be a lot simpler to just get rid of the old
complex kmap() entirely, and replace it with this?

I agree we can't replace the kmap_atomic() version, because maybe
people depend on the preemption disabling it also implied. But what
about replacing the non-atomic kmap()?

Maybe I've missed something.  Is it because the new interface still
does "pagefault_disable()" perhaps?

But does it even need the pagefault_disable() at all? Yes, the
*atomic* one obviously needed it. But why does this new one need to
disable page faults?

[ I'm just reading the patches, I didn't try to apply them and look at
the end result, so I might have missed something ]

The only other worry I would have is just test coverage of this
change. I suspect very few developers use HIGHMEM. And I know the
various test robots don't tend to test 32-bit either.

But apart from that question about naming (and perhaps replacing
kmap() entirely), I very much like it.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Tue, Sep 15, 2020 at 12:57 PM Thomas Gleixner  wrote:
>
> You wish. I just found a 7 year old bug in a 10G network driver which
> surely would have been found if people would enable debug configs and
> not just run the crap on their PREEMPT_NONE, all debug off kernel. And
> that driver is not subject to bitrot, it gets regular bug fixes from
> people who seem to care (distro folks).

That driver clearly cannot be very well maintained. All the distro
kernels have the basic debug checks in place, afaik.

Is it some wonderful "enterprise hardware" garbage again that only
gets used in special data centers?

Becasue the "enterprise" people really are special. Very much in the
"short bus" special kind of way. The fact that they have fooled so
much of the industry into thinking that they are the competent and
serious people is a disgrace.

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Wed, Sep 16, 2020 at 8:29 AM Paul E. McKenney  wrote:
>
> All fair, but some of us need to write code that must handle being
> invoked from a wide variety of contexts.

Note that I think that core functionality is different from random drivers.

Of course core code can (and will) look at things like

if (in_interrupt())
.. schedule work asynchronously ..

because core code ends up being called from odd places, and code like
that is expected to have understanding of the rules it plays with.

But something like RCU is a very different beast from some "walk the
scatter-gather list" code.

RCU does its work in the background, and works with lots of different
things. And it's so core and used everywhere that it knows about these
things. I mean, we literally have special code explicitly to let RCU
know "we entered kernel context now".

But something like a driver list walking thing should not be doing
different things behind peoples back depending on whether they hold
spinlocks or not. It should either just work regardless, or there
should be a flag (or special interface) for the "you're being called
in a crtitical region".

Because dynamically changing behavior really is very confusing.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Tue, Sep 15, 2020 at 1:39 AM Thomas Gleixner  wrote:
>
> OTOH, having a working 'preemptible()' or maybe better named
> 'can_schedule()' check makes tons of sense to make decisions about
> allocation modes or other things.

No. I think that those kinds of decisions about actual behavior are
always simply fundamentally wrong.

Note that this is very different from having warnings about invalid
use. THAT is correct. It may not warn in all configurations, but that
doesn't matter: what matters is that it warns in common enough
configurations that developers will catch it.

So having a warning in "might_sleep()" that doesn't always trigger,
because you have a limited configuration that can't even detect the
situation, that's fine and dandy and intentional.

But having code like

   if (can_schedule())
   .. do something different ..

is fundamentally complete and utter garbage.

It's one thing if you test for "am I in hardware interrupt context".
Those tests aren't great either, but at least they make sense.

But a driver - or some library routine - making a difference based on
some nebulous "can I schedule" is fundamentally and basically WRONG.

If some code changes behavior, it needs to be explicit to the *caller*
of that code.

So this is why GFP_ATOMIC is fine, but "if (!can_schedule())
do_something_atomic()" is pure shite.

And I am not IN THE LEAST interested in trying to help people doing
pure shite. We need to fix them. Like the crypto code is getting
fixed.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Tue, Sep 15, 2020 at 12:24 AM Thomas Gleixner  wrote:
>
> Alternatively we just make highmem a bit more expensive by making these
> maps preemptible. RT is doing this for a long time and it's not that
> horrible.

Ack.

In fact, I've wanted to start just removing kmap support entirely. At
some point it's not so much about "I have an old machine that wants
HIGHMEM" but about "I have an old CPU, and I'll just run an old
kernel".

It's not that 32-bit is irrelevant, it's that 32-bit with large
amounts of memory is irrelevant.

Last time this was discussed, iirc the main issue was some
questionable old ARM chips that were still very common in embedded
environments, even with large memory.

But we could definitely start de-emphasizing HIGHMEM.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Mon, Sep 14, 2020 at 11:24 PM Herbert Xu  wrote:
>
> On Tue, Sep 15, 2020 at 09:20:59AM +0300, Ard Biesheuvel wrote:
> >
> > The documentation of kmap_atomic() states the following:
> >
> >  * The use of kmap_atomic/kunmap_atomic is discouraged - kmap/kunmap
> >  * gives a more generic (and caching) interface. But kmap_atomic can
> >  * be used in IRQ contexts, so in some (very limited) cases we need
> >  * it.
> >
> > so if this is no longer accurate, perhaps we should fix it?
>
> This hasn't been accurate for at least ten years :)

Yeah, that used to be true a long long time ago, but the comment is very stale.

> > But another reason I tried to avoid kmap_atomic() is that it disables
> > preemption unconditionally, even on 64-bit architectures where HIGHMEM
> > is irrelevant. So using kmap_atomic() here means that the bulk of
> > WireGuard packet encryption runs with preemption disabled, essentially
> > for legacy reasons.
>
> Agreed.  We should definitely fix that.

Well, honestly, one big reason for that is debugging.

The *semantics* of the kmap_atomic() is in the name - you can't sleep
in between it and the kunmap_atomic().

On any sane architecture, kmap_atomic() ends up being a no-op from an
implementation standpoint, and sleeping would work just fine.

But we very much want to make sure that people don't then write code
that doesn't work on the bad old 32-bit machines where it really needs
that sequence to be safe from preemption.

So it's mostly a debug thing.

I say "mostly", because there might be small other details too, like
shared code, and perhaps even a couple of users out in the wild that
depend on the pagefault_disable() inherent in the current
kmap_atomic(), who knows..

So no, the preemption disabling isn't inherent in the operation
itself. But it does have some argument for it.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Mon, Sep 14, 2020 at 3:24 PM Linus Torvalds
 wrote:
>
> Ard and Herbert added to participants: see
> chacha20poly1305_crypt_sg_inplace(), which does
>
> flags = SG_MITER_TO_SG;
> if (!preemptible())
> flags |= SG_MITER_ATOMIC;
>
> introduced in commit d95312a3ccc0 ("crypto: lib/chacha20poly1305 -
> reimplement crypt_from_sg() routine").

As far as I can tell, the only reason for this all is to try to use
"kmap()" rather than "kmap_atomic()".

And kmap() actually has the much more complex "might_sleep()" tests,
and apparently the "preemptible()" check wasn't even the proper full
debug check, it was just a complete hack to catch the one that
triggered.

>From a quick look, that code should probably just get rid of
SG_MITER_ATOMIC entirely, and alwayse use kmap_atomic().

kmap_atomic() is actually the faster and proper interface to use
anyway (never mind that any of this matters on any sane hardware). The
old kmap() and kunmap() interfaces should generally be avoided like
the plague - yes, they allow sleeping in the middle and that is
sometimes required, but if you don't need that, you should never ever
use them.

We used to have a very nasty kmap_atomic() that required people to be
very careful and know exactly which atomic entry to use, and that was
admitedly quite nasty.

So it _looks_ like this code started using kmap() - probably back when
kmap_atomic() was so cumbersome to use - and was then converted
(conditionally) to kmap_atomic() rather than just changed whole-sale.
Is there actually something that wants to use those sg_miter functions
and sleep?

Because if there is, that choice should come from the outside, not
from inside lib/scatterlist.c trying to make some bad guess based on
the wrong thing entirely.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Mon, Sep 14, 2020 at 2:55 PM Thomas Gleixner  wrote:
>
> Yes it does generate better code, but I tried hard to spot a difference
> in various metrics exposed by perf. It's all in the noise and I only
> can spot a difference when the actual preemption check after the
> decrement

I'm somewhat more worried about the small-device case.

That said, the diffstat certainly has its very clear charm, and I do
agree that it makes things simpler.

I'm just not convinced people should ever EVER do things like that "if
(preemptible())" garbage. It sounds like somebody is doing seriously
bad things.

The chacha20poly1305 code does look fundamentally broken. But no, the
fix is not to make "preemptible" work with spinlocks, the fix is to
not *do* that kind of broken things.

Those things would be broken even if you changed the semantics of
preemptible. There's no way that it's valid to say "use this debug
flag to decide if we should do atomic allocations or not".

It smells like "I got a debug failure, so I'm papering it over by
checking the thing the debug code checks for".

The debug check is to catch the obvious bad cases. It's not the _only_
bad cases, so copying the debug check test is just completely wrong.

Ard and Herbert added to participants: see
chacha20poly1305_crypt_sg_inplace(), which does

flags = SG_MITER_TO_SG;
if (!preemptible())
flags |= SG_MITER_ATOMIC;

introduced in commit d95312a3ccc0 ("crypto: lib/chacha20poly1305 -
reimplement crypt_from_sg() routine").

You *fundamentally* cannot do that. Seriously. It's completely wrong.
Pick one or the other, or have the caller *tell* you what the context
is.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [patch 00/13] preempt: Make preempt count unconditional

[Linus Torvalds]

On Mon, Sep 14, 2020 at 1:45 PM Thomas Gleixner  wrote:
>
> Recently merged code does:
>
>  gfp = preemptible() ? GFP_KERNEL : GFP_ATOMIC;
>
> Looks obviously correct, except for the fact that preemptible() is
> unconditionally false for CONFIF_PREEMPT_COUNT=n, i.e. all allocations in
> that code use GFP_ATOMIC on such kernels.

I don't think this is a good reason to entirely get rid of the no-preempt thing.

The above is just garbage. It's bogus. You can't do it.

Blaming the no-preempt code for this bug is extremely unfair, imho.

And the no-preempt code does help make for much better code generation
for simple spinlocks.

Where is that horribly buggy recent code? It's not in that exact
format, certainly, since 'grep' doesn't find it.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

On Wed, Aug 26, 2020 at 1:53 PM Harald Arnesen  wrote:
>
> It's a Thinkpad T520.

Oh, so this is a 64-bit machine? Yeah, that patch to flush vmalloc
ranges won't make any difference on x86-64.

Or are you for some reason running a 32-bit kernel on that thing? Have
you tried building a 64-bit one (user-space can be 32-bit, it should
all just work. Knock wood).

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

On Wed, Aug 26, 2020 at 2:30 AM Harald Arnesen  wrote:
>
> Somehow related to lightdm or xfce4? However, it is a regression, since
> kernel 5.8 works.

Yeah, apparently there's something else wrong with the relocation changes too.

That said, does that patch at

  https://lore.kernel.org/intel-gfx/20200821123746.16904-1-j...@8bytes.org/

change things at all? If there are two independent bugs, maybe
applying that patch might at least give you an oops that gets saved in
the logs?

(it might be worth waiting a bit after the machine locks up in case
the machine is alive enough so sync logs after a bit.. If ssh works,
that's obviously better yet)

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

On Tue, Aug 25, 2020 at 9:32 AM Harald Arnesen  wrote:
>
> > For posterity, I'm told the fix is [1].
> >
> > [1] 
> > https://lore.kernel.org/intel-gfx/20200821123746.16904-1-j...@8bytes.org/
>
> Doesn't fix it for me. As soon as I start XFCE, the mouse and keyboard
> freeezes. I can still ssh into the machine
>
> The three reverts (763fedd6a216, 7ac2d2536dfa and 9e0f9464e2ab) fixes
> the bug for me.

Do you get any oops or other indication of what ends up going wrong?
Since ssh works that should be fairly easy to see.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] mm: Track page table modifications in __apply_to_page_range()

[Linus Torvalds]

On Fri, Aug 21, 2020 at 5:38 AM Joerg Roedel  wrote:
>
> From: Joerg Roedel 
>
> The __apply_to_page_range() function is also used to change and/or
> allocate page-table pages in the vmalloc area of the address space.
> Make sure these changes get synchronized to other page-tables in the
> system by calling arch_sync_kernel_mappings() when necessary.

I get the strong feeling that these functions should be using a
"struct apply_details *" or something like that (the way the
zap_page_range() code has that "zap_details" thing).

Because adding more and more arguments gets pretty painful after a
while. But maybe the compiler inlining it all makes it a non-issue.

It also strikes me that I think the only architecture that uses the
whole arch_sync_kernel_mappings() thing is now just x86-32.

[ Well, x86-64 still has it, but that's because we undid the 64-bit
removal, but it's on the verge of going away and x86-64 shouldn't
actually _need_ it any more ]

So all of this seems to be purely for 32-bit x86. Which kind of makes
this all fail the smell test.

But the patch does seem to be the minimal fix for a real issue - I'm
just pointing out ugly details, not actual problems with the patch.

IOW, a somewhat reluctant Ack, hoping that this will be cleaned up
some day. Possibly/hopefully because arch_sync_kernel_mappings() just
goes away entirely.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/4] drm/i915/gem: Sync the vmap PTEs upon construction

[Linus Torvalds]

On Fri, Aug 21, 2020 at 1:50 AM Chris Wilson  wrote:
>
> Since synchronising the PTE after assignment is a manual step, use the
> newly exported interface to flush the PTE after assigning via
> alloc_vm_area().

This commit message doesn't make much sense to me.

Are you talking about synchronizing the page directory structure
across processes after possibly creating new kernel page tables?

Because that has nothing to do with the PTE. It's all about making
sure the _upper_ layers of the page directories are populated
everywhere..

The name seems off to me too - what are you "flushing"? (And yes, I
know about the flush_cache_vmap(), but that looks just bogus, since
any non-mapped area shouldn't have any virtual caches to begin with,
so I suspect that is just the crazy architectures being confused -
flush_cache_vmap() is a no-op on any sane architecture - and powerpc
that mis-uses it for other things).

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

On Thu, Aug 20, 2020 at 2:23 AM Pavel Machek  wrote:
>
> Yes, it seems they make things work. (Chris asked for new patch to be
> tested, so I am switching to his kernel, but it survived longer than
> it usually does.)

Ok, so at worst we know how to solve it, at best the reverts won't be
needed because Chris' patch will fix the issue properly.

So I'll archive this thread, but remind me if this hasn't gotten
sorted out in the later rc's.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

On Tue, Aug 18, 2020 at 6:13 PM Dave Airlie  wrote:
>
> I think there's been some discussion about reverting that change for
> other reasons, but it's quite likely the culprit.

Hmm. It reverts cleanly, but the end result doesn't work, because of
other changes.

Reverting all of

   763fedd6a216 ("drm/i915: Remove i915_gem_object_get_dirty_page()")
   7ac2d2536dfa ("drm/i915/gem: Delete unused code")
   9e0f9464e2ab ("drm/i915/gem: Async GPU relocations only")

seems to at least build.

Pavel, does doing those three reverts make things work for you?

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.9-rc1: graphics regression moved from -next to mainline

[Linus Torvalds]

Ping on this?

The code disassembles to

  24: 8b 85 d0 fd ff ffmov-0x230(%ebp),%eax
  2a:* c7 03 01 00 40 10movl   $0x1041,(%ebx) <-- trapping instruction
  30: 89 43 04  mov%eax,0x4(%ebx)
  33: 8b 85 b4 fd ff ffmov-0x24c(%ebp),%eax
  39: 89 43 08  mov%eax,0x8(%ebx)
  3c: e9jmp ...

which looks like is one of the cases in __reloc_entry_gpu(). I *think*
it's this one:

} else if (gen >= 3 &&
   !(IS_I915G(eb->i915) || IS_I915GM(eb->i915))) {
*batch++ = MI_STORE_DWORD_IMM | MI_MEM_VIRTUAL;
*batch++ = addr;
*batch++ = target_addr;

where that "batch" pointer is 0xf8601000, so it looks like it just
overflowed into the next page that isn't there.

The cleaned-up call trace is

  drm_ioctl+0x1f4/0x38b ->
drm_ioctl_kernel+0x87/0xd0 ->
  i915_gem_execbuffer2_ioctl+0xdd/0x360 ->
i915_gem_do_execbuffer+0xaab/0x2780 ->
  eb_relocate_vma

but there's a lot of inling going on, so..

The obvious suspect is commit 9e0f9464e2ab ("drm/i915/gem: Async GPU
relocations only") but that's going purely by "that seems to be the
main relocation change this mmrge window".

 Linus

On Mon, Aug 17, 2020 at 9:11 AM Pavel Machek  wrote:
>
> Hi!
>
> After about half an hour of uptime, screen starts blinking on thinkpad
> x60 and machine becomes unusable.
>
> I already reported this in -next, and now it is in mainline. It is
> 32-bit x86 system.
>
>
> Pavel
>
>
> Aug 17 17:36:04 amd ovpn-castor[2828]: UDPv4 link local (bound):
> [undef]
> Aug 17 17:36:04 amd ovpn-castor[2828]: UDPv4 link remote:
> [AF_INET]87.138.219.28:1194
> Aug 17 17:36:23 amd kernel: BUG: unable to handle page fault for
> address: f8601000
> Aug 17 17:36:23 amd kernel: #PF: supervisor write access in kernel
> mode
> Aug 17 17:36:23 amd kernel: #PF: error_code(0x0002) - not-present page
> Aug 17 17:36:23 amd kernel: *pdpt = 318f2001 *pde =
> 
> Aug 17 17:36:23 amd kernel: Oops: 0002 [#1] PREEMPT SMP PTI
> Aug 17 17:36:23 amd kernel: CPU: 1 PID: 3004 Comm: Xorg Not tainted
> 5.9.0-rc1+ #86
> Aug 17 17:36:23 amd kernel: Hardware name: LENOVO 17097HU/17097HU,
> BIOS 7BETD8WW (2.19 ) 03/31
> /2011
> Aug 17 17:36:23 amd kernel: EIP: eb_relocate_vma+0xcf6/0xf20
> Aug 17 17:36:23 amd kernel: Code: e9 ff f7 ff ff c7 85 c0 fd ff ff ed
> ff ff ff c7 85 c4 fd ff
> ff ff ff ff ff 8b 85 c0 fd ff ff e9 a5 f8 ff ff 8b 85 d0 fd ff ff 
> 03 01 00 40 10 89 43 04
>  8b 85 b4 fd ff ff 89 43 08 e9 9f f7 ff
>  Aug 17 17:36:23 amd kernel: EAX: 003c306c EBX: f8601000 ECX: 00847000
>  EDX: 
>  Aug 17 17:36:23 amd kernel: ESI: 00847000 EDI:  EBP: f1947c68
>  ESP: f19479fc
>  Aug 17 17:36:23 amd kernel: DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS:
>  0068 EFLAGS: 00210246
>  Aug 17 17:36:23 amd kernel: CR0: 80050033 CR2: f8601000 CR3: 31a1e000
>  CR4: 06b0
>  Aug 17 17:36:23 amd kernel: Call Trace:
>  Aug 17 17:36:23 amd kernel: ? i915_vma_pin+0xc5/0x8c0
>  Aug 17 17:36:23 amd kernel: ? __mutex_unlock_slowpath+0x2b/0x280
>  Aug 17 17:36:23 amd kernel: ? __active_retire+0x7e/0xd0
>  Aug 17 17:36:23 amd kernel: ? mutex_unlock+0xb/0x10
>  Aug 17 17:36:23 amd kernel: ? i915_vma_pin+0xc5/0x8c0
>  Aug 17 17:36:23 amd kernel: ? __lock_acquire.isra.31+0x261/0x530
>  Aug 17 17:36:23 amd kernel: ? eb_lookup_vmas+0x1f5/0x9e0
>  Aug 17 17:36:23 amd kernel: i915_gem_do_execbuffer+0xaab/0x2780
>  Aug 17 17:36:23 amd kernel: ? _raw_spin_unlock_irqrestore+0x27/0x40
>  Aug 17 17:36:23 amd kernel: ? __lock_acquire.isra.31+0x261/0x530
>  Aug 17 17:36:23 amd kernel: ? __lock_acquire.isra.31+0x261/0x530
>  Aug 17 17:36:23 amd kernel: ? kvmalloc_node+0x69/0x70
>  Aug 17 17:36:23 amd kernel: i915_gem_execbuffer2_ioctl+0xdd/0x360
>  Aug 17 17:36:23 amd kernel: ? i915_gem_execbuffer_ioctl+0x2b0/0x2b0
>  Aug 17 17:36:23 amd kernel: drm_ioctl_kernel+0x87/0xd0
>  Aug 17 17:36:23 amd kernel: drm_ioctl+0x1f4/0x38b
>  Aug 17 17:36:23 amd kernel: ? i915_gem_execbuffer_ioctl+0x2b0/0x2b0
>  Aug 17 17:36:23 amd kernel: ? posix_get_monotonic_timespec+0x1c/0x90
>  Aug 17 17:36:23 amd kernel: ? ktime_get_ts64+0x7a/0x1e0
>  Aug 17 17:36:23 amd kernel: ? drm_ioctl_kernel+0xd0/0xd0
>  Aug 17 17:36:23 amd kernel: __ia32_sys_ioctl+0x1ad/0x799
>  Aug 17 17:36:23 amd kernel: ? debug_smp_processor_id+0x12/0x20
>  Aug 17 17:36:23 amd kernel: ? exit_to_user_mode_prepare+0x4f/0x100
>  Aug 17 17:36:23 amd kernel: do_int80_syscall_32+0x2c/0x40
>  Aug 17 17:36:23 amd kernel: entry_INT80_32+0x111/0x111
>  Aug 17 17:36:23 amd kernel: EIP: 0xb7fbc092
>  Aug 17 17:36:23 amd kernel: Code: 00 00 00 e9 90 ff ff ff ff a3 24 00
>  00 00 68 30 00 00 00 e9 80 ff ff ff ff a3 e8 ff ff ff 66 90 00 00 00
>  00 00 00 00 00 cd 80  8d b4 26 00 00 00 00 8d b6 00 00 00 00 8b
>  1c 24 c3 8d b4 26 00
>  Aug 17 17:36:23 amd kernel: EAX: ffda EBX: 000a ECX: c0406469
>  

Re: [Intel-gfx] [PATCH v12 2/3] drm/i915: add syncobj timeline support

[Linus Torvalds]

On Wed, Jul 29, 2020 at 5:24 AM Daniel Vetter  wrote:
>
> Do we have a access_ok_array or so? Instead of duplicating overflow checks
> everywhere and getting it all wrong ...

I really really think you should get away from access_ok() entirely.

Please just get rid of it, and use "copy_from_user()" instead.

Seriously.

access_ok() is completely wrong, because

 (a) it doesn't actually protect from any fault returns, it only doe
sthe high-level check of "is the pointer even ok".

So you can't say "ok, I did access_ok(), so I don't have to check the
return value", and you're actually making the source code more
complicated, and only introducing the possibility of bugs.

Overflow is just _one_ such bug. Missing the access_ok() entirely
because it was in some caller but not another is another common bug.

 (b) it no longer even makes the code faster.

It never really did for the the copy_to/from_user() case _anyway_, it
was always for the "I will now do several get/put_user() accesses and
I only want to do the range check once".

And that has simply not been true for the last few CPU generations -
because the cost isn't in the range check any more. Now allk the real
costs are about CLAC/STAC. The range check takes two cycles and
schedules well (so it's generally not even visible). The CLAC/STAC
takes 30+ cycles, and stalls the pipeline.

>Similar I guess for copy_from/to_user_array.

No. I refuse to add complexity onto the garbage that is access_ok().

Just remove it. It's not helping. People who think it's helping
haven't actually looked at profiles, or are working with hardware that
is no longer relevant.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/5] uaccess: Rename user_access_begin/end() to user_full_access_begin/end()

[Linus Torvalds]

On Mon, Apr 20, 2020 at 7:49 PM Al Viro  wrote:
>
> The only source I'd been able to find speaks of >= 60 cycles
> (and possibly much more) for non-pipelined coprocessor instructions;
> the list of such does contain loads and stores to a bunch of registers.
> However, the register in question (p15/c3) has only store mentioned there,
> so loads might be cheap; no obvious reasons for those to be slow.
> That's a question to arm folks, I'm afraid...  rmk?

_If_ it turns out to be expensive, is there any reason we couldn't
just cache the value in general?

That's what x86 tends to do with expensive system registers. One
example would be "msr_misc_features_shadow".

But maybe that's something to worry about when/if it turns out to
actually be a problem?

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Linus, please revert 7dc8f11437: regression in 5.7-rc0, hangs while attempting to run X

[Linus Torvalds]

On Tue, Apr 7, 2020 at 12:48 AM Pavel Machek  wrote:
>
> >
> > Beyond the fix already submitted?
>
> I did not get that one, can I have a pointer?

What's the status of this one?

I'm assuming the fix is commit 721017cf4bd8 ("drm/i915/gem: Ignore
readonly failures when updating relics"), but didn't see a reply to
the query or a confirmation of things working..

Btw, Chris, that __put_user() not testing the error should at least
have a comment. We don't have a working "__must_check" for those
things (because they are subtle macros, not functions), but if we did,
we'd get a compiler warning for not checking the error value.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 5/5] uaccess: Rename user_access_begin/end() to user_full_access_begin/end()

[Linus Torvalds]

On Fri, Apr 3, 2020 at 12:21 AM Christophe Leroy
 wrote:
>
> Now we have user_read_access_begin() and user_write_access_begin()
> in addition to user_access_begin().

I realize Al asked for this, but I don't think it really adds anything
to the series.

The "full" makes the names longer, but not really any more legible.

So I like 1-4, but am unconvinced about 5 and would prefer that to be
dropped. Sorry for the bikeshedding.

And I like this series much better without the cookie that was
discussed, and just making the hard rule be that they can't nest.

Some architecture may obviously use a cookie internally if they have
some nesting behavior of their own, but it doesn't look like we have
any major reason to expose that as the actual interface.

The only other question is how to synchronize this? I'm ok with it
going through the ppc tree, for example, and just let others build on
that.  Maybe using a shared immutable branch with 5.6 as a base?

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 00/16] x86, crypto: remove always-defined CONFIG_AS_* and cosolidate Kconfig/Makefiles

[Linus Torvalds]

On Tue, Mar 24, 2020 at 1:49 AM Masahiro Yamada  wrote:
>
> If it is OK to queue this up to Kbuild tree,
> I will send a pull request to Linus.

Looks fine to me, assuming we didn't now get some confusion due to
duplicate patches (I think Jason got his tree added to -next already).

And yeah, that end result looks much better.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] i915 GPU-hang regression in v5.6-rcx

[Linus Torvalds]

Let's add in some of the i915 people and list.

Everything quoted below for the new participants.

  Linus

On Mon, Feb 24, 2020 at 2:52 AM Jörg Otte  wrote:
>
> In v5.6-rcx I sporadically see a hanging GPU.
>
> [  640.919302] i915 :00:02.0: Resetting chip for stopped heartbeat on rcs0
> [  641.021808] i915 :00:02.0: Xorg[722] context reset due to GPU hang
>
> [ 2229.764709] i915 :00:02.0: Resetting chip for stopped heartbeat on rcs0
> [ 2229.867534] i915 :00:02.0: kwin_x11[1005] context reset due to GPU hang
>
> To recover Xorg must be killed and restarted or reboot is required.
> I've never seen this before v5.6-rcx.
>
> Best way to reproduce seem to be "heavily scrolling with the mouse wheel"
> in graphic applications. I also saw this once while video streaming in
> a browser.
>
>
> System:  Host: fichte Kernel: 5.6.0-rc1 x86_64 bits: 64 Console: tty 3
> Distro: Ubuntu 18.04.3 LTS
> Machine: Device: Notebook System: FUJITSU product: LIFEBOOK A544
> serial: 
>  Mobo: FUJITSU model: FJNBB35 serial: 
>  BIOS: FUJITSU // Phoenix v: Version 1.17 rv 1.17 date: 05/09/2014
> CPU: Dual core Intel Core i5-4200M (-MT-MCP-) cache: 3072 KB
>  clock speeds: max: 3100 MHz 1: 1127 MHz 2: 964 MHz 3: 1034
> MHz 4: 984 MHz
> Graphics:Card: Intel 4th Gen Core Processor Integrated Graphics Controller
>  Display Server: X.Org 1.19.6 drivers: modesetting (unloaded:
> fbdev,vesa) Resolution: 1366x768@60.00hz
>  OpenGL: renderer: Mesa DRI Intel Haswell Mobile version: 4.5
> Mesa 19.2.8
>
>
> Thanks, Jörg
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] 5.3-rc3: Frozen graphics with kcompactd migrating i915 pages

[Linus Torvalds]

On Thu, Sep 12, 2019 at 12:51 PM Martin Wilck  wrote:
>
> Is there an alternative to reverting aa56a292ce62 ("drm/i915/userptr:
> Acquire the page lock around set_page_dirty()")? And if we do, what
> would be the consequences? Would other patches need to be reverted,
> too?

Looking at that commit, and the backtrace of the lockup, I think that
reverting it is the correct thing to do.

You can't take the page lock in invalidate_range(), since it's called
from try_to_unmap(), which is called with the page lock already held.

So commit aa56a292ce62 is just fundamentally completely wrong and
should be reverted.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [GIT PULL] VLA removal for v4.20-rc1

[Linus Torvalds]

On Sun, Oct 28, 2018 at 10:24 AM Kees Cook  wrote:
>
> Please pull these VLA removal changes for v4.20-rc1.

Pulled,

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

On Wed, Aug 22, 2018 at 11:16 PM Zhenyu Wang  wrote:
>
> yeah, that's the clear way to fix this imo. We only depend on guest
> life cycle to access guest memory properly. Here's proposed fix, will
> verify and integrate it later.

Thanks, this looks sane to me,

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

On Wed, Aug 22, 2018 at 12:44 PM Felix Kuehling  wrote:
>
> You're right, but that's a bit fragile and convoluted. I'll fix KFD to
> handle this more robustly. See the attached (untested) patch.

Yes, this patch that makes the whole "has to use current mm" or uses
"get_task_mm()" looks good from a VM< worry standpoint.

Thanks.

> And
> obviously that opaque pointer didn't work as intended. It just gets
> promoted to an mm_struct * without a warning from the compiler. Maybe I
> should change that to a long to make abuse easier to spot.

Using a "void *" is actually just about the worst possible type for
something that should be a cookie, because it silently translates to
any pointer.

"long" is actually not much better, becuase it will silently convert
to any integer type.

A good fairly type-safe cookie type is actually this:

typedef volatile const struct no_such_struct *cookie_ptr_t;

and now something of type "cookie_ptr_t" is actually very  hard to
convert to other types by mistake.

Note that the "volatile const" is not just random noise - it's so that
it won't even convert without warnings to things that take a "const
void *" as an argument (like, say, the source of 'memcpy()').

So you almost _have_ to explicitly cast it to use it.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

On Wed, Aug 22, 2018 at 12:37 PM Oded Gabbay  wrote:
>
> Having said that, I think we *are* protected by the mmu_notifier
> release because if the process suddenly dies, we will gracefully clean
> the process's data in our driver and on the H/W before returning to
> the mm core code. And before we return to the mm core code, we set the
> mm pointer to NULL. And the graceful cleaning should be serialized
> with the load_hqd uses.

So I'm a bit nervous about the mmu_notifier model (and the largely
equivalent exit_aio() model for the USB gardget AIO uses).

The reason I'm nervous about it is that the mmu_notifier() gets called
only after the mm_users count has already been decremented to zero
(and the exact same thing goes for exit_aio()).

Now that's fine if you actually get rid of all accesses in
mmu_notifier_release() or in exit_aio(), because the page tables still
exist at that point - they are in the process of being torn down, but
they haven't been torn down yet.

But for something like a kernel thread doing use_mm(), the thing that
worries me is a pattern something like this:

  kwork thread  exit thread
    

mmput() ->
  mm_users goes to zero

  use_mm(mmptr);
  ..

  mmu_notifier_release();
  exit_mm() ->
exit_aio()

and the pattern is basically the same regatdless of whether you use
mmu_notifier_release() or depend on some exit_aio() flushing your aio
work: the use_mm() can be called with a mm that has already had its
mm_users count decremented to zero, and that is now scheduled to be
free'd.

Does it "work"? Yes. Kind of. At least if the mmu notifier and/or
exit_aio() actually makes sure to wait for any kwork thread thing. But
it's a bit of a worrisome pattern.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

On Wed, Aug 22, 2018 at 11:33 AM Linus Torvalds
 wrote:
>
> On Wed, Aug 22, 2018 at 11:21 AM Paolo Bonzini  wrote:
> >
> > Yes, KVM is correct but the i915 bits are at least fishy.  It's probably
> > as simple as adding a mmget/mmput pair respectively in kvmgt_guest_init
> > and kvmgt_guest_exit, or maybe mmget_not_zero.
>
> Definitely mmget_not_zero(). If it was just mmgrab()'ed earlier, the
> actual page tables might already be gone.

Side note: we _could_ do the mmget_not_zero() inside use_mm() itself,
if we just knew that the mm was at least mmgrab()'ed correctly.

But for some of the uses, even that isn't clear. It's not entirely
obvious that the "struct mm_struct" exists _at_all_ at that point, and
that a mmget_not_zero() wouldn't just have some use-after-free access.

Again, independent lifetime rules could show that this isn't the case
(ie "exit_aio() is always called before exit_mmap(), and kill_ioctx()
takes care of it all"), but it would be good to have the users of
"use_mm()" actually verify their lifetime rules are correct and
enforced.

Because quite often, the lifetime rule might nbot be a mmu notifier or
aio_exit at all, but just be "oh, the user won't exit until this is
all done". But do you *control* the user? What if the user is buggy?

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

On Wed, Aug 22, 2018 at 11:21 AM Paolo Bonzini  wrote:
>
> Yes, KVM is correct but the i915 bits are at least fishy.  It's probably
> as simple as adding a mmget/mmput pair respectively in kvmgt_guest_init
> and kvmgt_guest_exit, or maybe mmget_not_zero.

Definitely mmget_not_zero(). If it was just mmgrab()'ed earlier, the
actual page tables might already be gone.

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] Possible use_mm() mis-uses

[Linus Torvalds]

Guys and gals,
 this is a *very* random list of people on the recipients list, but we
had a subtle TLB shootdown issue in the VM, and that brought up some
issues when people then went through the code more carefully.

I think we have a handle on the TLB shootdown bug itself. But when
people were discussing all the possible situations, one thing that
came up was "use_mm()" that takes a mm, and makes it temporarily the
mm for a kernel thread (until "unuse_mm()", duh).

And it turns out that some of those uses are definitely wrong, some of
them are right, and some of them are suspect or at least so overly
complicated that it's hard for the VM people to know if they are ok.

Basically, the rule for "use_mm()" is that the mm in question *has* to
have a valid page table associated with it over the whole use_mm() ->
unuse_mm() sequence. That may sound obvious, and I guess it actually
is so obvious that there isn't even a comment about it, but the actual
users are showing that it's sadly apparently not so obvious after all.

There is one user that uses the "obviously correct" model: the vhost
driver does a "mmget()" and "mmput()" pair around its use of it,
thanks to vhost_dev_set_owner() doing a

dev->mm = get_task_mm(current);

to look up the mm, and then the teardown case does a

if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;

This is the *right* sequence. A gold star to the vhost people.

Sadly, the vhost people are the only ones who seem to get things
unquestionably right. And some of those gold star people are also
apparently involved in the cases that didn't get things right.

An example of something that *isn't* right, is the i915 kvm interface,
which does

use_mm(kvm->mm);

on an mm that was initialized in virt/kvm/kvm_main.c using

mmgrab(current->mm);
kvm->mm = current->mm;

which is *not* right. Using "mmgrab()" does indeed guarantee the
lifetime of the 'struct mm_struct' itself, but it does *not* guarantee
the lifetime of the page tables. You need to use "mmget()" and
"mmput()", which get the reference to the actual process address
space!

Now, it is *possible* that the kvm use is correct too, because kvm
does register a mmu_notifier chain, and in theory you can avoid the
proper refcounting by just making sure the mmu "release" notifier
kills any existing uses, but I don't really see kvm doing that. Kvm
does register a release notifier, but that just flushes the shadow
page tables, it doesn't kill any use_mm() use by some i915 use case.

So while the vhost use looks right, the kvm/i915 use looks definitely wrong.

The other users of "use_mm()" and "unuse_mm()" are less
black-and-white right vs wrong..

One of the complex ones is the amdgpu driver. It does a
"use_mm(mmptr)" deep deep in the guts of a macro that ends up being
used in fa few places, and it's very hard to tell if it's right.

It looks almost certainly buggy (there is no mmget/mmput to get the
refcount), but there _is_ a "release" mmu_notifier function and that
one - unlike the kvm case - looks like it might actually be trying to
flush the existing pending users of that mm.

But on the whole, I'm suspicious of the amdgpu use. It smells. Jann
Horn pointed out that even if it migth be ok due to the mmu_notifier,
the comments are garbage:

>  Where "process" in the uniquely-named "struct queue" is a "struct
>  kfd_process"; that struct's definition has this comment in it:
>
>/*
> * Opaque pointer to mm_struct. We don't hold a reference to
> * it so it should never be dereferenced from here. This is
> * only used for looking up processes by their mm.
> */
>void *mm;
>
>  So I think either that comment is wrong, or their code is wrong?

so I'm chalking the amdgpu use up in the "broken" column.

It's also actually quite hard to synchronze with some other kernel
worker thread correctly, so just on general principles, if you use
"use_mm()" it really really should be on something that you've
properly gotten a mm refcount on with mmget(). Really. Even if you try
to synchronize things.

The two final cases are two uses in the USB gadget driver. Again, they
don't have the proper mmget/mmput, but they may br ok simply because
the uses are done for AIO, and the VM teardown is preceded by an AIO
teardown, so the proper serialization may come in from that.

Anyway, sorry for the long email, and the big list of participants and
odd mailing lists, but I'd really like people to look at their
"use_mm()" cases, and ask themselves if they have done enough to
guarantee that the full mm exists. Again, "mmgrab()" is *not* enough
on its own. You need either "mmget()" or some lifetime guarantee.

And if you do have those lifetime guarantees, it would be really nice
to get a good explanatory comment about said lifetime guarantees above
the "use_mm()" call. Ok?

Note that the lifetime rules are very important, because obviously
use_mm() itself is never called 

Re: [Intel-gfx] SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation)

[Linus Torvalds]

On Tue, Jul 31, 2018 at 10:49 AM Linus Torvalds
 wrote:
>
> So the re-use might initialize the fields lazily, not necessarily using a 
> ctor.

In particular, the pattern that nf_conntrack uses looks like it is safe.

If you have a well-defined refcount, and use "atomic_inc_not_zero()"
to guard the speculative RCU access section, and use
"atomic_dec_and_test()" in the freeing section, then you should be
safe wrt new allocations.

If you have a completely new allocation that has "random stale
content", you know that it cannot be on the RCU list, so there is no
speculative access that can ever see that random content.

So the only case you need to worry about is a re-use allocation, and
you know that the refcount will start out as zero even if you don't
have a constructor.

So you can think of the refcount itself as always having a zero
constructor, *BUT* you need to be careful with ordering.

In particular, whoever does the allocation needs to then set the
refcount to a non-zero value *after* it has initialized all the other
fields. And in particular, it needs to make sure that it uses the
proper memory ordering to do so.

And in this case, we have

  static struct nf_conn *
  __nf_conntrack_alloc(struct net *net,
  {
...
atomic_set(>ct_general.use, 0);

which is a no-op for the re-use case (whether racing or not, since any
"inc_not_zero" users won't touch it), but initializes it to zero for
the "completely new object" case.

And then, the thing that actually exposes it to the speculative walkers does:

  int
  nf_conntrack_hash_check_insert(struct nf_conn *ct)
  {
...
smp_wmb();
/* The caller holds a reference to this object */
atomic_set(>ct_general.use, 2);

which means that it stays as zero until everything is actually set up,
and then the optimistic walker can use the other fields (including
spinlocks etc) to verify that it's actually the right thing. The
smp_wmb() means that the previous initialization really will be
visible before the object is visible.

Side note: on some architectures it might help to make that "smp_wmb
-> atomic_set()" sequence be am "smp_store_release()" instead. Doesn't
matter on x86, but might matter on arm64.

NOTE! One thing to be very worried about is that re-initializing
whatever RCU lists means that now the RCU walker may be walking on the
wrong list so the walker may do the right thing for this particular
entry, but it may miss walking *other* entries. So then you can get
spurious lookup failures, because the RCU walker never walked all the
way to the end of the right list. That ends up being a much more
subtle bug.

But the nf_conntrack case seems to get that right too, see the restart
in nf_conntrack_find().

So I don't see anything wrong in nf_conntrack.

But yes, using SLAB_TYPESAFE_BY_RCU is very very subtle. But most of
the subtleties have nothing to do with having a constructor, they are
about those "make sure memory ordering wrt refcount is right" and
"restart speculative RCU walk" issues that actually happen regardless
of having a constructor or not.

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] SLAB_TYPESAFE_BY_RCU without constructors (was Re: [PATCH v4 13/17] khwasan: add hooks implementation)

[Linus Torvalds]

On Tue, Jul 31, 2018 at 10:36 AM Christopher Lameter  wrote:
>
> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?

.. because the object can be accessed (by RCU) after the refcount has
gone down to zero, and the thing has been released.

That's the whole and only point of SLAB_TYPESAFE_BY_RCU.

That flag basically says:

  "I may end up accessing this object *after* it has been free'd,
because there may be RCU lookups in flight"

This has nothing to do with constructors. It's ok if the object gets
reused as an object of the same type and does *not* get
re-initialized, because we're perfectly fine seeing old stale data.

What it guarantees is that the slab isn't shared with any other kind
of object, _and_ that the underlying pages are free'd after an RCU
quiescent period (so the pages aren't shared with another kind of
object either during an RCU walk).

And it doesn't necessarily have to have a constructor, because the
thing that a RCU walk will care about is

 (a) guaranteed to be an object that *has* been on some RCU list (so
it's not a "new" object)

 (b) the RCU walk needs to have logic to verify that it's still the
*same* object and hasn't been re-used as something else.

So the re-use might initialize the fields lazily, not necessarily using a ctor.

And the point of using SLAB_TYPESAFE_BY_RCU is that using the more
traditional RCU freeing - where you free each object one by one with
an RCU delay - can be prohibitively slow and have a huge memory
overhead (because of big chunks of memory that are queued for
freeing).

In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
immediately, but because it gets reused as the same kind of object,
the RCU walker can "know" what parts have meaning for re-use, in a way
it couidn't if the re-use was random.

That said, it *is* subtle, and people should be careful.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] kthread: finer-grained lockdep/cross-release completion

[Linus Torvalds]

On Sun, Dec 17, 2017 at 11:11 PM, Daniel Vetter  wrote:
>
> This didn't seem to have made it into -rc4. Anything needed to get it
> going?

Do you actually see the problem in -rc4?

Because we ended up removing the cross-release checking due to other
developers complaining. It seemed to need a lot more work before it
was ready.

So I suspect the patch is simply not relevant any more (even if it's
not necessarily wrong either).

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-misc-fixes

[Linus Torvalds]

On Thu, Dec 14, 2017 at 6:50 AM, Daniel Vetter  wrote:
>
> Imo that's enough that I figured better not delay until Dave is back.
> Linus, can you pls apply?

Pulled.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [haswell_crtc_enable] WARNING: CPU: 3 PID: 109 at drivers/gpu/drm/drm_vblank.c:1066 drm_wait_one_vblank+0x18f/0x1a0 [drm]

[Linus Torvalds]

On Mon, Oct 30, 2017 at 12:00 AM, Fengguang Wu  wrote:
> CC intel-gfx.

Thanks, these are all interesting (even if some of them seem to be
from random kernels).

Fengguang, is this a new script that you started running? Because I'm
*hoping* it's not that rc6 suddenly seems so flaky, and it's really
that you now have a nice new script that started reporting these
things better, even though many of them may be old?

This particular one I will have to leave to the intel gfx people to comment on.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] i915 memory allocation failure..

[Linus Torvalds]

So there's something wrong with the memory allocation changes since
4.11, which seem to be mostly credited to Chris Wilson.

In particular, I got this earlier today:

  Xorg: page allocation failure: order:0,
mode:0x14210d2(GFP_HIGHUSER|__GFP_NORETRY|__GFP_RECLAIMABLE),
nodemask=(null)

and then soon afterwards in the log I see

  chrome[13102]: segfault at 968 ip 7f472a7fda83 sp
7fffab9a6ef0 error 4 in libX11.so.6.3.0[7f472a7d1000+138000]
  gnome-session-f[13115]: segfault at 0 ip 7f7e765ab4b9 sp
7ffca5990470 error 4 in libgtk-3.so.0.2200.15[7f7e762cc000+6f9000]

which I assume is related to broken error handling.

So there's at least two bugs here:

 (a) order-0 memory allocation failure is generally a sign of
something bad. We clearly give up *much* too easily.

 (b) using __GFP_NORETRY and wanting the memory failure, but then not
using __GFP_NOWARN is just stupid.

Now, (b) initially made me go "I'll just add that __GFP_NOWARN
myself". Because it's true - if you intentionally tell the VM
subsystem that you'd rather get a failed allocation than try a bit
harder, then you obviously shouldn't get the warning either. I think
the VM people have talked about just considering NORETRY to imply
NOWARN.

However, the fact that this actually caused problems in downstream
user space, and the fact that this happened with an order-0 allocation
made me re-consider. That allocation clearly *is* important, and
returning NULL may "work" from a kernel standpoint, but it sure as
hell didn't work in the bigger picture, now did it?

So the warning was actually good in this case. This may in fact be an
example of why GFP_NORETRY should *not* imply NOWARN.

So instead of shutting up the warning, I pass it over to the i915
people. Making that allocation fail easily wasn't such a great idea
after all, was it? Maybe that NORETRY should be reconsidered, at least
for important (perhaps small?) allocations?

Also adding some VM people, because I think it's ridiculous that the
0-order allocation failed in the first place. Full report attached,
there's tons of memory that should have been trivial to free.

So I suspect GFP_NORETRY ends up being *much* too aggressive, and
basically doesn't even try any trivial freeing.

Maybe we want some middle ground between "retry forever" and "don't
try at all". In people trying to fight the "retry forever", we seem to
have gone too far in the "don't even bother, just return NULL"
direction.

Added a random mixture of i915 and MM people. Feel free to send this
message further if you feel I missed somebody,

Linus


gfp-noretry
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/2] drm/i915: Backport vma fixes for 4.10-rc6

[Linus Torvalds]

On Tue, Jan 31, 2017 at 1:21 AM, Maarten Lankhorst
 wrote:
>
> This is marked for rc6 because it seems the issue is triggerable on
> mainline and resulting in an oops.

So I did apply my obvious "avoid the oops and just warn about it"
patch: commit 39cb2c9a316e ("drm/i915: Check for NULL i915_vma in
intel_unpin_fb_obj()").

I haven't actually triggered the problem since, so I don't know if
there might be some other downstream issue, but the workaround *may*
just be acceptable from a 4.10 standpoint.

Some maintainer who knows the code better should make the judgment call.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Oops at shutdown in intel_unpin_fb_obj()

[Linus Torvalds]

Guys, I've gotten absolutely no response to this, and the problem
seems to still occur.

I just got a slightly different hang at shutdown, due to a kernel oops
that seems related. It's not identical - the call trace is very
different - but it's close.

In particular, it's once again the same NULL pointer dereference in
"intel_unpin_fb_obj()", except this time it looked like this:

  BUG: unable to handle kernel NULL pointer dereference at 0078
  IP: intel_unpin_fb_obj+0x69/0xe0 [i915]
  Oops:  [#1] SMP
  Modules linked in: fuse xt_CHECKSUM ipt_MASQUERADE
nf_nat_masquerade_ipv4 tun ip6t_rpfilter ip6t_REJECT nf_reject_ipv6
xt_conntrack ebtable_nat ebtable_broute bridge stp llc ip6ta$
   tpm_tis industrialio tpm_tis_core acpi_pad tpm nfsd auth_rpcgss
nfs_acl lockd grace sunrpc dm_crypt hid_logitech_hidpp hid_logitech_dj
i915 crct10dif_pclmul i2c_algo_bit crc32_pc$
  CPU: 4 PID: 26173 Comm: kworker/u16:9 Tainted: GW
4.10.0-rc5-00111-g49e555a932de #1
  Hardware name: System manufacturer System Product Name/Z170-K, BIOS
1803 05/06/2016
  Workqueue: i915 intel_unpin_work_fn [i915]
  RIP: 0010:intel_unpin_fb_obj+0x69/0xe0 [i915]
  RSP: :b95c4937bdc0 EFLAGS: 00010286
  RAX:  RBX: 96f284441340 RCX: 
  RDX: b95c4937bdc0 RSI: 96f29f273908 RDI: 96f284441340
  RBP: b95c4937be08 R08:  R09: 
  R10: fa83b2da R11: 00808111 R12: 96f20d878500
  R13: 0001 R14: 96f29f58c400 R15: 96f29f270068
  FS:  () GS:96f2b6d0() knlGS:
  CS:  0010 DS:  ES:  CR0: 80050033
  CR2: 0078 CR3: 00041ff4b000 CR4: 003406e0
  DR0:  DR1:  DR2: 
  DR3:  DR6: fffe0ff0 DR7: 0400
  Call Trace:
   intel_unpin_work_fn+0x58/0x140 [i915]
   process_one_work+0x1f1/0x480
   worker_thread+0x48/0x4d0
   kthread+0x101/0x140
   ret_from_fork+0x29/0x40
  Code: ff ff ff 74 67 48 8d 7d b8 44 89 ea 4c 89 e6 e8 ce 2c ff ff 48
8b 43 08 48 8d 55 b8 48 89 df 48 8d b0 08 39 00 00 e8 47 1b fc ff <48>
8b 50 78 48 85 d2 74 04 83 6a 20 01 48 $
  RIP: intel_unpin_fb_obj+0x69/0xe0 [i915] RSP: b95c4937bdc0
  CR2: 0078
  ---[ end trace afab57e9d299b42b ]---

so this time it was the worker thread that died and took the system
down with it.

Anyway, there is something *seriously* wrong with the i915 shutdown sequence.

Now, maybe this was fixed with the recent drm pull that did have some
i915 fixes in it, and I wasn't running on my desktop yet, but nothing
there looks very obvious.

And once again, I'd like to note that other users of
i915_gem_object_to_ggtt() do seem to check for a NULL vma, while
intel_unpin_fb_obj() simply passes any potential NULL vma to
i915_vma_unpin_fence().

Guys?

   Linus


On Sun, Jan 8, 2017 at 3:35 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> This has so far only happened once, so I don't know how repeatable it
> is, but here goes..
>
> My nice stable XPS13 just oopsed on shutdown. It is possibly related
> to the X server SIGSEGV'ing too, although honestly, I am not sure
> which caused which. Maybe the kernel oops caused the X problem. They
> definitely happened together, and happened as I was shutting down the
> machine.
>
> I'm including the syslog for the Xorg issue too, in case it ends up
> giving people ideas, but the kernel oops is what I actually looked at.
> The code decodes to
>
> 74 67   je 0x69
> 48 8d 7d b8 lea-0x48(%rbp),%rdi
> 44 89 eamov%r13d,%edx
> 4c 89 e6mov%r12,%rsi
> e8 3e 2d ff ff  callq  ..
> 48 8b 43 08 mov0x8(%rbx),%rax
> 48 8d 55 b8 lea-0x48(%rbp),%rdx
> 48 89 dfmov%rbx,%rdi
> 48 8d b0 08 39 00 00lea0x3908(%rax),%rsi
> e8 47 1a fc ff  callq  ..
> *   48 8b 50 78 mov0x78(%rax),%rdx  <--
> trapping instruction
> 48 85 d2test   %rdx,%rdx
> 74 04   je 0x35
> 83 6a 20 01 subl   $0x1,0x20(%rdx)
> 48 89 c7mov%rax,%rdi
> e8 c2 60 fc ff  callq  ..
>
>
> and just comparing it to the generted code it seems to be this:
>
> calli915_gem_obj_to_vma #
> movq120(%rax), %rdx # MEM[(struct drm_i915_fence_reg *
> *)_24 + 120B], _15
>
> where %rax (the return value from i915_gem_obj_to_vma()) is NULL.
>
> So it seems to be this code:
>
> ...
> vma = i915_gem_object_to_ggtt(obj, );
>
> i915_vma_u

Re: [Intel-gfx] drm/i915 connector list locking issues..

[Linus Torvalds]

On Wed, Jan 25, 2017 at 12:13 PM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
>
> Is nobody else seeing this? This is on my bog-standard intel laptop
> (Dell XPS13 as you can see in the warning).

Actually, it's on my desktop as well. So I'm assuming pretty much any
i915 config shows it. It's been introduced after rc5.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] drm/i915 connector list locking issues..

[Linus Torvalds]

I didn't notice this until now, because my laptop still *works*, but
there seems to be a locking issue wth the drm connector list in the
i915 driver init path.

See appended call trace.

I don't know what part of the trace is supposed to get the mode_config
locks - maybe it's the generic drm kms helpers that are broken rather
than the i915 driver.

Is nobody else seeing this? This is on my bog-standard intel laptop
(Dell XPS13 as you can see in the warning). I haven't tried to see
when it started, but it didn't happen in 4.10-rc4. So it's recent.

If I had to guess, I'd blame commit 3846fd9b8600 ("drm/probe-helpers:
Drop locking from poll_enable"). Is it just that the check is now
wrong?

Linus

---

  [drm] Found 64MB of eDRAM
  [drm] Memory usable by graphics device = 4096M
  checking generic (9000 15f9000) vs hw (9000 1000)
  fb: switching to inteldrmfb from EFI VGA
  Console: switching to colour dummy device 80x25
  [drm] Replacing VGA console driver
  [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
  [drm] Driver supports precise vblank timestamp query.
  [drm] Finished loading i915/skl_dmc_ver1_26.bin (v1.26)
  i915 :00:02.0: vgaarb: changed VGA decodes:
olddecodes=io+mem,decodes=io+mem:owns=io+mem
  [drm] GuC firmware load skipped
  [ cut here ]
  WARNING: CPU: 3 PID: 390 at ./include/drm/drm_crtc.h:857
drm_kms_helper_poll_enable.part.3+0xa8/0xc0 [drm_kms_helper]
  Modules linked in: rtsx_pci_sdmmc mmc_core crct10dif_pclmul
crc32_pclmul crc32c_intel i915(+) ghash_clmulni_intel serio_raw
i2c_algo_bit drm_kms_helper nvme rtsx_pci syscopyarea nvme_core
sysfillrect sysimgblt fb_sys_fops drm i2c_hid video fjes
  CPU: 3 PID: 390 Comm: systemd-udevd Not tainted
4.10.0-rc5-00071-ga4685d2f58e2 #10
  Hardware name: Dell Inc. XPS 13 9350/09JHRY, BIOS 1.4.12 11/30/2016
  Call Trace:
   drm_kms_helper_poll_enable.part.3+0xa8/0xc0 [drm_kms_helper]
   drm_kms_helper_poll_init+0x7e/0x90 [drm_kms_helper]
   i915_driver_load+0x13f0/0x1440 [i915]
   i915_pci_probe+0x4f/0x70 [i915]
   local_pci_probe+0x45/0xa0
   pci_device_probe+0x103/0x150
   driver_probe_device+0x2bb/0x460
   __driver_attach+0xdf/0xf0
   bus_for_each_dev+0x6c/0xc0
   driver_attach+0x1e/0x20
   bus_add_driver+0x170/0x270
   driver_register+0x60/0xe0
   __pci_register_driver+0x4c/0x50
   i915_init+0x57/0x5a [i915]
   do_one_initcall+0x52/0x1a0
   do_init_module+0x5f/0x1f8
   load_module+0x235f/0x2950
   SYSC_finit_module+0xdf/0x110
   SyS_finit_module+0xe/0x10
   do_syscall_64+0x61/0x170
   entry_SYSCALL64_slow_path+0x25/0x25
  RIP: 0033:0x7f772c536239
  RSP: 002b:7ffe3b26a3f8 EFLAGS: 0246 ORIG_RAX: 0139
  RAX: ffda RBX: 561083f72f40 RCX: 7f772c536239
  RDX:  RSI: 7f772cc3ea75 RDI: 0014
  RBP: 7f772cc3ea75 R08:  R09: 7ffe3b26a510
  R10: 0014 R11: 0246 R12: 
  R13: 561083f751d0 R14: 0002 R15: 561082b23f4a
  ---[ end trace 311d7fe73771357e ]---
  usb 1-4: new full-speed USB device number 3 using xhci_hcd
  ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
  input: Video Bus as
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input7
  [drm] Initialized i915 1.6.0 20161121 for :00:02.0 on minor 0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] Oops at shutdown in intel_unpin_fb_obj()

[Linus Torvalds]

This has so far only happened once, so I don't know how repeatable it
is, but here goes..

My nice stable XPS13 just oopsed on shutdown. It is possibly related
to the X server SIGSEGV'ing too, although honestly, I am not sure
which caused which. Maybe the kernel oops caused the X problem. They
definitely happened together, and happened as I was shutting down the
machine.

I'm including the syslog for the Xorg issue too, in case it ends up
giving people ideas, but the kernel oops is what I actually looked at.
The code decodes to

74 67   je 0x69
48 8d 7d b8 lea-0x48(%rbp),%rdi
44 89 eamov%r13d,%edx
4c 89 e6mov%r12,%rsi
e8 3e 2d ff ff  callq  ..
48 8b 43 08 mov0x8(%rbx),%rax
48 8d 55 b8 lea-0x48(%rbp),%rdx
48 89 dfmov%rbx,%rdi
48 8d b0 08 39 00 00lea0x3908(%rax),%rsi
e8 47 1a fc ff  callq  ..
*   48 8b 50 78 mov0x78(%rax),%rdx  <--
trapping instruction
48 85 d2test   %rdx,%rdx
74 04   je 0x35
83 6a 20 01 subl   $0x1,0x20(%rdx)
48 89 c7mov%rax,%rdi
e8 c2 60 fc ff  callq  ..


and just comparing it to the generted code it seems to be this:

calli915_gem_obj_to_vma #
movq120(%rax), %rdx # MEM[(struct drm_i915_fence_reg *
*)_24 + 120B], _15

where %rax (the return value from i915_gem_obj_to_vma()) is NULL.

So it seems to be this code:

...
vma = i915_gem_object_to_ggtt(obj, );

i915_vma_unpin_fence(vma);
i915_gem_object_unpin_from_display_plane(vma);
...

where vma is NULL.

The other user of i915_gem_object_to_ggtt() does have a test of !vma,
although with a warning. Which implies it does happen, but shouldn't.
Maybe consistent with the Xorg confusion?

Linus

---

  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:72
  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:78
  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:66
  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:65
  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:69
  gdm-x-session: (II) UnloadModule: "libinput"
  gdm-x-session: (II) systemd-logind: releasing fd for 13:67
  gdm-x-session: (EE)
  gdm-x-session: (EE) Backtrace:
  gdm-x-session: (EE) 0: /usr/libexec/Xorg (OsLookupColor+0x139) [0x59f859]
  gdm-x-session: (EE) 1: /lib64/libc.so.6 (__restore_rt+0x0) [0x7fe554e5a7df]
  gdm-x-session: (EE) 2: /usr/lib64/xorg/modules/libfb.so
(_fbGetWindowPixmap+0xd) [0x7fe54d16b6fd]
  gdm-x-session: (EE) 3: /usr/libexec/Xorg
(present_extension_init+0x5b7) [0x51b9b7]
  gdm-x-session: (EE) 4: /usr/libexec/Xorg
(present_extension_init+0x685) [0x51bb95]
  gdm-x-session: (EE) 5: /usr/libexec/Xorg
(present_extension_init+0xdf2) [0x51ca62]
  gdm-x-session: (EE) 6: /usr/libexec/Xorg (AddTraps+0x9133) [0x523973]
  gdm-x-session: (EE) 7: /usr/libexec/Xorg
(CompositeRegisterImplicitRedirectionException+0x4098) [0x4ccf58]
  gdm-x-session: (EE) 8: /usr/libexec/Xorg (AddTraps+0x73f4) [0x51fe84]
  gdm-x-session: (EE) 9: /usr/libexec/Xorg (remove_fs_handlers+0x581) [0x43af61]
  gdm-x-session: (EE) 10: /lib64/libc.so.6 (__libc_start_main+0xf1)
[0x7fe554e46731]
  gdm-x-session: (EE) 11: /usr/libexec/Xorg (_start+0x29) [0x424d59]
  gdm-x-session: (EE) 12: ? (?+0x29) [0x29]
  gdm-x-session: (EE)
  gdm-x-session: (EE) Segmentation fault at address 0x10
  gdm-x-session: (EE)
  gdm-x-session: Fatal server error:
  gdm-x-session: (EE) Caught signal 11 (Segmentation fault). Server aborting
  gdm-x-session: (EE)
  gdm-x-session: (EE)
  gdm-x-session: Please consult the Fedora Project support
  gdm-x-session:  at http://wiki.x.org
  gdm-x-session:  for help.
  gdm-x-session: (EE) Please also check the log file at
"/home/torvalds/.local/share/xorg/Xorg.0.log" for additional
information.
  gdm-x-session: (EE)
  gdm-x-session: (WW) xf86CloseConsole: KDSETMODE failed: Input/output error
  gdm-x-session: (WW) xf86CloseConsole: VT_GETMODE failed: Input/output error
  gdm-x-session: (WW) xf86CloseConsole: VT_ACTIVATE failed: Input/output error

  kernel: BUG: unable to handle kernel NULL pointer dereference at
0078
   IP: intel_unpin_fb_obj+0x69/0xe0 [i915]
   PGD 0
   Oops:  [#1] SMP
   Modules linked in: rfcomm fuse ccm ip6t_rpfilter ip6t_REJECT
nf_reject_ipv6 xt_conntrack ip_set nfnetlink ebtable_nat
ebtable_broute bridge stp llc ip6table_nat nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 ip6table_security ip6table_mangle
ip6table_raw 

Re: [Intel-gfx] [PULL] drm-intel-fixes

[Linus Torvalds]

On Thu, Jan 5, 2017 at 3:19 AM, Jani Nikula  wrote:
>
> Hi Dave, or Linus if Dave's on vacation, I'm a bit unsure,

I'm going to ignore this on the assumption that Dave is around. But if
nothing happens for a few days, ping me again and I'll pull it
directly. Ok?

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: tune down the fast link training vs boot fail

[Linus Torvalds]

On Tue, Dec 13, 2016 at 11:54 AM, Daniel Vetter  wrote:
>
> Feel free to apply directly in case it annoys too much and you don't
> want to wait for your presents ;-)

I'll wait for the proper unboxing of presents. Once I've rattled them
and know what to expect, I can be very patient.

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] i915 warning: WARN_ON_ONCE(!intel_dp->lane_count)

[Linus Torvalds]

This isn't new, but I thought I'd report it since it doesn't seem to
get fixed on its own..

  [drm] Initialized i915 1.6.0 20161121 for :00:02.0 on minor 0
  [ cut here ]
  WARNING: CPU: 1 PID: 130 at drivers/gpu/drm/i915/intel_dp.c:4018
intel_dp_check_link_status+0x1db$
  WARN_ON_ONCE(!intel_dp->lane_count)
  Modules linked in:
   rtsx_pci_sdmmc mmc_core crct10dif_pclmul crc32_pclmul crc32c_intel
i915 ghash_clmulni_intel i2c_$
  CPU: 1 PID: 130 Comm: kworker/u8:7 Not tainted 4.9.0-04822-g9439b3710df6 #6
  Hardware name: Dell Inc. XPS 13 9350/09JHRY, BIOS 1.4.4 06/14/2016
  Workqueue: events_unbound async_run_entry_fn
  Call Trace:
   intel_dp_check_link_status+0x1db/0x200 [i915]
   intel_dp_detect+0x697/0xa40 [i915]
   drm_helper_probe_single_connector_modes+0x2a3/0x500 [drm_kms_helper]
   drm_setup_crtcs+0x7b/0x9c0 [drm_kms_helper]
   drm_fb_helper_initial_config+0x79/0x3e0 [drm_kms_helper]
   intel_fbdev_initial_config+0x18/0x30 [i915]
   async_run_entry_fn+0x37/0x150
   process_one_work+0x1f1/0x480
   worker_thread+0x48/0x4d0
   kthread+0x101/0x140
   ret_from_fork+0x22/0x30
  ---[ end trace 10162024459bbe32 ]---

That's obviously my XPS13, and it has nothing attached to it, so just
the internal laptop display.

This is all intel, with the Skylake Iris Graphics 540 (i7-6560U).

Everything seems to work fine, it's just the ugly warning.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Critical regression in 4.7-rcX

[Linus Torvalds]

[ Adding the proper people to the cc. ]

On Thu, 14 Jul 2016, Larry Finger wrote:
>
> To anyone keeping track of regressions in kernel 4.7, I call your attention to
> https://bugs.freedesktop.org/show_bug.cgi?id=96675.
> 
> This bug causes driver i915 to fail to connect to the display, and results in
> a blank screen as soon as the kernel is loaded. The only way to operate with
> kernel 4.7 is to add "nomodeset" to the command line. The problem was bisected
> to commit f21a21983ef13a ("drm/i915: Splitting intel_dp_detect").

Daniel? Jani? Time to revert?

  Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1 00/12] PCI: Rework shadow ROM handling

[Linus Torvalds]

On Fri, Mar 11, 2016 at 4:49 PM, Andy Lutomirski  wrote:
>
> FWIW, if I disable all the checks in pci_get_rom_size, I learn that my
> video ROM consists entirely of 0xff bytes.  Maybe there just isn't a
> ROM shadow on my laptop.

I think most laptops end up having the graphics ROM be part of the
regular system flash, and there is no actual rom associated with the
PCI device that is the GPU itself.

The actual GPU ROM tends to be associated with plug-in cards, not
soldered-down chips in a laptop where they don't want extra flash
chips.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v1 00/12] PCI: Rework shadow ROM handling

[Linus Torvalds]

On Thu, Mar 3, 2016 at 8:53 AM, Bjorn Helgaas  wrote:
> The purpose of this series is to:
> [ .. ]

The patches look ok to me and seem to make sense.

Of course, let's see what they break. Hopefully nothing, but any time
the PCI resource code changes I get a bit worried. PTSD, I guess.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-intel-fixes

[Linus Torvalds]

On Jan 3, 2016 18:06, "Dave Airlie"  wrote:
>
> can you pull this directly, baby has arrived, but I'm not back at work
yet.

Assumed so, and already did. It's in rc8,

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-intel-fixes

[Linus Torvalds]

On Wed, Dec 23, 2015 at 2:40 AM, Jani Nikula  wrote:
>
>
>   git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2015-12-23

Btw, since you're already using tags, mind using *signed* tags
instead? It's just good housekeeping..

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-intel-fixes

[Linus Torvalds]

On Wed, Dec 23, 2015 at 2:40 AM, Jani Nikula  wrote:
>
> Hi Dave (and/or Linus, depending on the new arrival and eggnog
> schedules) -

I'll pull it directly. Dave just sent me his pending drm fixes, he may
or may not be around any more, it's already christmas eve down under.

Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [git pull] drm fixes

[Linus Torvalds]

On Sun, Nov 29, 2015 at 11:33 PM, Daniel Vetter  wrote:
>
> Yeah I just hunted down a test infrastructure failure on my Haswell the
> past few days with various loads and output configs. Seemed very happy.
> And not aware of anything else blowing up (bdw/skl would be less
> surprising).

So I'm currently suspecting that we may have had a power-brownout
situation. We ended up actually having a complete (but short) power
loss a bit after I saw two consecutive lockup events, and it hasn't
happened since.

I used to have a UPS on the machine, but our power has been stable
enough the last few years that when the battery gave out I just
stopped using it. I guess that next time I rebuild that machine (I'm
planning on upgrading to skylake some time), I'll just make sure to
replace the power supply too. It's probably ten years old by now (the
disks, motherboard and CPU's have all been replaced multiple times,
but the nice silent case and power supply I've just continually
re-used), so I could imagine that a brown-out together with a
weakening power supply might start to be an issue.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [git pull] drm fixes

[Linus Torvalds]

On Thu, Nov 19, 2015 at 8:07 PM, Dave Airlie  wrote:
>
> core: Atomic fixes and Atomic helper fixes
> i915: Revert for the backlight regression along with a bunch of fixes.

So I have no idea if the GPU updates are my problem, but my main
desktop machine has been hanging silently a few times lately.

It has never done it while unattended, even if it's doing things like
compiling the kernel. So I'm a bit inclined to blame graphics.

Sadly, when it hangs, it's a total hang and doesn't leave anything in
the logs - I just have to reboot.

I'll try to see if I can get anything at all out of the machine, but I
thought I'd ask if there is some known issue with Haswell graphics in
the 4.4-rc code base?

Sorry for the complete lack of details, and really any hard reason to
even blame the GPU people.. You may be entirely blameless.

   Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [REGRESSION] Re: i915 driver crashes on T540p if docking station attached

[Linus Torvalds]

On Mon, Aug 3, 2015 at 9:25 AM, Theodore Ts'o ty...@mit.edu wrote:

 I've just tried pulling in your updated fixes-stuff, and it avoids the
 oops and allows external the monitor to work correctly.

Good. Have either of you tested the suspend/resume behavior? Is that fixed too?

  However, I'm
 still seeing a large number of drm/i915 related warning messages and
 other kernel kvetching.

I suspect I can live with that for now. The lockdep one looks like
it's mainly an initialization issue, so you'd never get the actual
deadlock in practice, but it's obviously annoying.  The intel_pm.c one
I'll have to defer to the i915 people for..

I'll be travelling much of this week (flying to Finland tomorrow, back
on Sunday - yay, 30h in airplanes for three days on the ground, but
it's my dad's bday), and my internet will be sporadic. But I'll have a
laptop and be able to pull stuff every once in a while.

It would be good to have this one resolved, and I just need to worry
about the remaining VM problem..

   Linus

 [4.170749] WARNING: CPU: 0 PID: 463 at 
 drivers/gpu/drm/i915/intel_pm.c:2339 ilk_update_wm+0x71a/0xb27 [i915]()
 [4.170751] WARN_ON(!r-enable)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-intel-fixes

[Linus Torvalds]

On Fri, Jul 31, 2015 at 9:54 AM, Daniel Vetter daniel.vet...@ffwll.ch wrote:

 I delayed my -fixes pull a bit hoping that I could include a fix for the
 dp mst stuff but looks a bit more nasty than that. So just 3 other
 regression fixes, one 4.2 other two cc: stable.

Quick question: you can now reproduce the mst problem that Ted
reported? You said you found a mst bridge..

This matters mainly because I wonder if I should apply my temporary
workaround patch, or expect that a real fix is coming soon..

 Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx