Re: [pull] amdgpu drm-fixes-6.4

[Linus Torvalds]

On Fri, 23 Jun 2023 at 14:18, Alex Deucher  wrote:
>
> are available in the Git repository at:
>
>   https://gitlab.freedesktop.org/agd5f/linux.git 
> tags/amd-drm-fixes-6.4-2023-06-23

That's not a valid signed tag.

Yes, it's a tag.  But it doesn't actually have any cryptographic
signing, and I'm not willing to pull random content from git sites
that I can't verify. In fact, these days I ask even kernel.org pull
requests to be proper signed tags, although I haven't really gotten to
the point where I *require* it.

So please sign your tags - use "git tag -s" (or "-u keyname" if you
have some specific key you want to use, rather than one described by
your regular git config file).

  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

Fwd: amdgpu: update from 5.10.145 to 5.10.146...149 breaks boot on Ryzen based computers

[Linus Torvalds]

This was sent to me, but should have gone to other people.

It may already be fixed, but note how the report is about -stable
kernels, including apparently the current 5.10 stable version (149(.

  Linus

-- Forwarded message -
From: Kevin Torkelson 
Date: Thu, Oct 20, 2022 at 8:09 AM
Subject: amdgpu: update from 5.10.145 to 5.10.146...149 breaks boot on
Ryzen based computers
To: 


Linus,

--- Possibly Important ---
I know several people who are crashing with Debian Bullseye (stable)
with the most current kernel put out by the distribution. AMD put out
a fix that seems like it might be related here:
https://gitlab.freedesktop.org/drm/amd/-/issues/2216

Re: [PATCH] drm/amd/display: Fix build breakage with CONFIG_DEBUG_FS=n

[Linus Torvalds]

On Fri, Oct 14, 2022 at 8:22 AM Nathan Chancellor  wrote:
>
> After commit 8799c0be89eb ("drm/amd/display: Fix vblank refcount in vrr
> transition"), a build with CONFIG_DEBUG_FS=n is broken due to a
> misplaced brace, along the lines of:

Thanks, applied.

  Linus

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Linus Torvalds]

On Thu, Oct 6, 2022 at 1:50 PM Sudip Mukherjee
 wrote:
>
> > And it looks like Sudip's proposed fix for this particular code is
> > additionally fixing unsigned vs signed as well. I think -Warray-bounds
> > did its job (though, with quite a confusing index range in the report).
>
> Not my. Linus's. I just tested. :)

I suspect Kees meant Stephen's other patch that Hamza pointed at, and
that is perhaps the cleaner version.

That said, I hate how this forces us to write random code changes just
to make a compiler just randomly _happen_ to not complain about it.

   Linus

Re: mainline build failure due to 5d8c3e836fc2 ("drm/amd/display: fix array-bounds error in dc_stream_remove_writeback()")

[Linus Torvalds]

On Thu, Oct 6, 2022 at 1:51 AM Sudip Mukherjee (Codethink)
 wrote:
>
> This is only seen with gcc-11, gcc-12 builds are ok.

Hmm. This seems to be some odd gcc issue.

I *think* that what is going on is that the test

j = 0 ; j < MAX_DWB_PIPES

makes gcc decide that "hey, j is in the range [0,MAX_DWB_PIPES[", and
then since MAX_DWB_PIPES is 1, that simplifies to "j must be zero".
Good range analysis so far.

But for 'i', we start off with that lower bound of 0, but the upper
bound is not fixed (the test for "i" is: "i < stream->num_wb_info").

And then that "if (i != j)", so now gcc decides that it can simplify
that to "if (i != 0)", and then simplifies *that* to "oh, the lower
bound of 'i' in that code is actually 1.

So then it decides that "stream->writeback_info[i]" must be out of bounds.

Of course, the *reality* is that stream->num_wb_info should be <=
MAX_DWB_PIPES, and as such (with the current MAX_DWB_PIPES value of 1)
it's not that 'i' can be 1, it's that the code in question cannot be
reached at all.

What confuses me is that error message ("array subscript [0, 0] is
outside array bounds of 'struct dc_writeback_info[1]') which seems to
be aware that the value is actually 0.

So this seems to be some gcc-11 range analysis bug, but I don't know
what the fix is. I suspect some random code change would magically
just make gcc realize it's ok after all, but since it all depends on
random gcc confusion, I don't know what the random code change would
be.

The fix *MAY* be to just add a '&& i < MAX_DWB_PIPES' to that loop
too, and then gcc will see that both i and j are always 0, and that
the code is unreachable and not warn about it. Hmm? Can you test that?

And the reason gcc-12 builds are ok probably isn't that gcc-12 gets
this right, it's simply that gcc-12 gets so many *opther* things wrong
that we already disabled -Warray-bounds with gcc-12 entirely.

If somebody cannot come up with a fix, I suspect the solution is "gcc
array bounds analysis is terminally buggy" and we just need to disable
it for gcc-11 too.

Kees, any idea? Who else might be interested in fixing a -Warray-bounds issue?

 Linus

Re: Linux 6.0-rc5

[Linus Torvalds]

On Mon, Sep 12, 2022 at 7:59 AM Sudip Mukherjee
 wrote:
>
> clang build failure as reported in [1] is still there. Nathan has
> posted a patch series at [2] to fix it, but it has not landed yet.Alex 
> Deucher 
>
> [1]. https://lore.kernel.org/lkml/YuwRyQYPCb1FD+mr@debian/#t
> [2]. https://lore.kernel.org/all/20220830203409.3491379-1-nat...@kernel.org/

Yes, I was hoping for the AMD GPU people to take it and have the
hardware people who actually wrote that code (?) verify it all.

It would be good to have clang finally do a full allmodconfig build
with no errors and warnings.

Adding in Alex and Rodrigo that have been involved in the previous
stack reduction stuff, and the amd-gfx list for anybody else
involved...

Anybody?

   Linus

Re: mainline build failure for x86_64 allmodconfig with clang

[Linus Torvalds]

On Sun, Aug 7, 2022 at 10:36 AM David Laight  wrote:
>
> Or just shoot the software engineer who thinks 100 arguments
> is sane. :-)

I suspect the issue is that it's not primarily a software engineer who
wrote that code.

Hardware people writing code are about as scary as software engineers
with a soldering iron.

   Linus

Re: mainline build failure for x86_64 allmodconfig with clang

[Linus Torvalds]

On Thu, Aug 4, 2022 at 11:37 AM Sudip Mukherjee (Codethink)
 wrote:
>
> git bisect points to 3876a8b5e241 ("drm/amd/display: Enable building new 
> display engine with KCOV enabled").

Ahh. So that was presumably why it was disabled before - because it
presumably does disgusting things that make KCOV generate even bigger
stack frames than it already has.

Those functions do seem to have fairly big stack footprints already (I
didn't try to look into why, I assume it's partly due to aggressive
inlining, and probably some automatic structures on stack). But gcc
doesn't seem to make it all that much worse with KCOV (and my clang
build doesn't enable KCOV).

So it's presumably some KCOV-vs-clang thing. Nathan?

  Linus

Re: mainline build failure for x86_64 allmodconfig with clang

[Linus Torvalds]

On Thu, Aug 4, 2022 at 1:43 PM Nathan Chancellor  wrote:
>
> I do note that commit 1b54a0121dba ("drm/amd/display: Reduce stack size
> in the mode support function") did have a workaround for GCC. It appears
> clang will still inline mode_support_configuration(). If I mark it as
> 'noinline', the warning disappears in that file.

That sounds like probably the best option for now. Gcc does not inline
that function (at least for allmodconfig builds in my testing), so if
that makes clang match what gcc does, it seems a reasonable thing to
do.

Linus

Re: mainline build failure due to 6fdd2077ec03 ("drm/amd/amdgpu: add memory training support for PSP_V13")

[Linus Torvalds]

On Thu, Aug 4, 2022 at 12:35 AM Sudip Mukherjee (Codethink)
 wrote:
>
> I will be happy to test any patch or provide any extra log if needed.

It sounds like that file just needs to get a

#include 

there, and for some reason architectures other than alpha and mips end
up getting it accidentally through other headers.

Mind testing just adding that header file, and perhaps even sending a
patch if (when) that works for you?

Linus

Re: [PATCH] drm/amdgpu: Re-enable DCN for 64-bit powerpc

[Linus Torvalds]

On Mon, Jul 25, 2022 at 5:39 AM Michael Ellerman  wrote:
>
> Further digging shows that the build failures only occur with compilers
> that default to 64-bit long double.

Where the heck do we have 'long double' things anywhere in the kernel?

I tried to grep for it, and failed miserably. I found some constants
that would qualify, but they were in the v4l colorspaces-details.rst
doc file.

Strange.

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Thu, Jul 14, 2022 at 10:24 AM Guenter Roeck  wrote:
>
> We can't use virt_to_phys() and phys_to_virt() because they are defined for
> the underlying architecture. Would uml_to_phys() and uml_to_virt() be
> acceptable ? If so, I'll submit a patch.

Sure, that would be good, and make th uml helpers clearly be in the
uml namespace.

Another more traditional approach is to use underscored versions, but
exactly because that's the normal thing, things like that may then
clash with the "native architecture" version, so for uml using an
explicit uml namespace might be the better option.

   Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Thu, Jul 14, 2022 at 12:23 AM Geert Uytterhoeven
 wrote:
>
> Oh, it's not just this one. The lists of build regressions between v5.18
> and v5.19-rc1 [1] resp. v5.19-rc6 [2] look surprisingly similar :-(
>
> [1] https://lore.kernel.org/all/20220606082201.2792145-1-ge...@linux-m68k.org
> [2] https://lore.kernel.org/all/20220711064425.3084093-1-ge...@linux-m68k.org

Hmm.

Some of them are because UM ends up defining and exposing helper
functions like "to_phys()", which it just shouldn't do. Very generic
name - so when some driver ends up using the same name, you get those
errors.

And some look positively strange. Like that

  drivers/mfd/asic3.c: error: unused variable 'asic'
[-Werror=unused-variable]:  => 941:23

which is clearly used three lines later by

iounmap(asic->tmio_cnf);

and I can't find any case of 'iounmap()' having been defined to an
empty macro or anything like that to explain it. The error in
drivers/tty/serial/sh-sci.c looks to be exactly the same issue, just
with ioremap() instead of iounmap().

It would be good to have some way to find which build/architecture it
is, because right now it just looks bogus.

Do you perhaps use some broken compiler that complains when the empty
inline functions don't use their arguments? Because that's what those
ioremap/iounmap() ones look like to me, but there might be some
magical architecture / config that has issues that aren't obvious.

IOW, I'd love to get those fixed, but I would also want a little bit more info.

Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 2:36 PM Sudip Mukherjee
 wrote:
>
> > >
> > > https://lore.kernel.org/all/20220524025139.40212-1-wangkefeng.w...@huawei.com/
> >
> > That patch looks sane to me, but I guess Guenter would need to check
>
> I still see the failure in my builds with this patch. But surprisingly
> I dont see the build failure (with or without this patch) with gcc-12,
> only with gcc-11.

Arrghs. "build failure"?

So is there another problem than the runtime issue that Guenter reports:

  OF: amba_device_add() failed (-19) for /amba/smc@1010

in this area? That patch looks harmless from a build standpoint, but
that's not saying much, so can you please quote the actual build
failure here?

  Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 2:01 PM Alex Deucher  wrote:
>
> If you want to apply Guenter's patch original patch:
> https://patchwork.freedesktop.org/patch/490184/
> That's fine with me.

Honestly, by this time I feel that it's too little, too late.

The ppc people apparently didn't care at all about the fact that this
driver didn't compile.

At least Michael Ellerman and Daniel Axtens were cc'd on that thread
with the proposed fix originally.

I don't see any replies from ppc people as to why it happened, even
though apparently a bog-standard "make allmodconfig" just doesn't
build.

I'd try it myself, since I do have a cross-build environment for some
earlier cross-build verification I did.

But since my upgrade to F36 it now uses gcc-12, and possibly due to
that I get hundreds of errors long before I get to any drm drivers:

  Cannot find symbol for section 19: .text.create_section_mapping.
  arch/powerpc/mm/mem.o: failed
  ...
  Cannot find symbol for section 19: .text.cpu_show_meltdown.
  drivers/base/cpu.o: failed
  Error: External symbol 'memset' referenced from prom_init.c

this cross environment used to work for me, but something changed (I
mention gcc-12, but honestly, that's based on nothing at all, except
for the few problems it caused on x86-64. It could be something
entirely unrelated, but it does look like some bad interaction with
-ffunction-sections).

So considering that the ppc people ignored this whole issue since the
merge window, I think it's entirely unreasonable to then apply a
ppc-specific patch for this at this time, when people literally asked
"why is this needed", and there was no reply from the powerpc side.

Does any of that sound like "we should support this driver on powerpc" to you?

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 1:46 PM Guenter Roeck  wrote:
>
> It does, but I can't imagine that the drm or ppc people would be happy
> about it.

When something has been reported as not building for five weeks?

I have zero f's to give at that point about their "happiness".

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 1:40 PM Guenter Roeck  wrote:
>
> That patch is (and has been) in linux-next for a long time,
> as commit d2ca1fd2bc70, and with the following tags.
>
>  Fixes: 7719a68b2fa4 ("ARM: 9192/1: amba: fix memory leak in 
> amba_device_try_add()")
>  Reported-by: Guenter Roeck 
>  Tested-by: Guenter Roeck 
>  Signed-off-by: Kefeng Wang 
>  Signed-off-by: Russell King (Oracle) 
>
> So, yes, it fixes the problem. I don't know where it is pulled from, though.
> I thought that it is from Russell's tree, given his Signed-off-by:,
> but I never really checked.

Heh. Yeah, with that sign-off, I bet it's in Russell's queue, bit it
just ended up in the "for next release" branch. Russell?

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 12:53 PM Alex Deucher  wrote:
>
> Does this patch fix it?
> https://patchwork.freedesktop.org/patch/493799/

Guenter? Willing to check this one too for your setup, and we can
hopefully close down both issues?

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Wed, Jul 13, 2022 at 12:49 PM Russell King (Oracle)
 wrote:
>
> There may be a patch that solves that, but it's never been submitted to
> my patch system:
>
> https://lore.kernel.org/all/20220524025139.40212-1-wangkefeng.w...@huawei.com/

That patch looks sane to me, but I guess Guenter would need to check
... Guenter?

 Linus

Re: Linux 5.19-rc6

[Linus Torvalds]

On Tue, Jul 12, 2022 at 10:07 PM Guenter Roeck  wrote:
>
> Same problems as every week.
>
> Building powerpc:allmodconfig ... failed

Ok, this has been going on since -rc1, which is much too long.

>From your patch submission that that was rejected:

> The problem was introduced with commit 41b7a347bf14 ("powerpc: Book3S
> 64-bit outline-only KASAN support") which adds support for KASAN. This
> commit in turn enables DRM_AMD_DC_DCN because KCOV_INSTRUMENT_ALL and
> KCOV_ENABLE_COMPARISONS are no longer enabled. As result, new files are
> compiled which lack the selection of hard-float.

And considering that neither the ppc people nor the drm people seem
interested in fixing this, and it doesn't revert cleanly I think the
sane solution seems to be to just remove PPC64 support for DRM_AMD_DC
entirely.

IOW, does something like this (obviously nor a proper patch, but you
get the idea) fix the ppc build for you?

  @@ -6,7 +6,7 @@ config DRM_AMD_DC
  bool "AMD DC - Enable new display engine"
  default y
  select SND_HDA_COMPONENT if SND_HDA_CORE
  -   select DRM_AMD_DC_DCN if (X86 || PPC64) &&
!(KCOV_INSTRUMENT_ALL && KCOV_ENABLE_COMPARISONS)
  +   select DRM_AMD_DC_DCN if X86 && !(KCOV_INSTRUMENT_ALL &&
KCOV_ENABLE_COMPARISONS)
  help
Choose this option if you want to use the new display engine
support for AMDGPU. This adds required support for Vega and

> OF: amba_device_add() failed (-19) for /amba/smc@1010
> [ cut here ]
> WARNING: CPU: 0 PID: 1 at lib/refcount.c:28 of_platform_bus_create+0x33c/0x3dc
> refcount_t: underflow; use-after-free.

This too has been going on since -rc1, but it's not obvious what caused it.

At a guess, looking around the amba changes, I'm assuming it's

  7719a68b2fa4 ("ARM: 9192/1: amba: fix memory leak in amba_device_try_add()")

Does reverting that commit make it go away?

Linus

Re: Annoying AMDGPU boot-time warning due to simplefb / amdgpu resource clash

[Linus Torvalds]

On Mon, Jun 27, 2022 at 1:02 AM Javier Martinez Canillas
 wrote:
>
> The flag was dropped because it was causing drivers that requested their
> memory resource with pci_request_region() to fail with -EBUSY (e.g: the
> vmwgfx driver):
>
> https://www.spinics.net/lists/dri-devel/msg329672.html

See, *that* link would have been useful in the commit.

Rather than the useless link it has.

Anyway, removing the busy bit just made things worse.

> > If simplefb is actually still using that frame buffer, it's a problem.
> > If it isn't, then maybe that resource should have been released?
>
> It's supposed to be released once amdgpu asks for conflicting framebuffers
> to be removed calling drm_aperture_remove_conflicting_pci_framebuffers().

That most definitely doesn't happen. This is on a running system:

  [torvalds@ryzen linux]$ cat /proc/iomem | grep BOOTFB
- : BOOTFB

so I suspect that the BUSY bit was never the problem - even for
vmwgfx). The problem was that simplefb doesn't remove its resource.

Guys, the *reason* for resource management is to catch people that
trample over each other's resources.

You literally basically disabled the code that checked for it by
removing the BUSY flag, and just continued to have conflicting
resources.

That isn't a "fix", that is literally "we are ignoring and breaking
the whole reason that the resource tree exists, but we'll still use it
for no good reason".

Yeah, yeah, most modern drivers ignore the IO resource tree, because
they end up working on another resource level entirely: they work on
not the IO resources, but on the "driver level" instead, and just
attach to PCI devices.

So these days, few enough drivers even care about the IO resource
tree, and it's mostly used for (a) legacy devices (think ISA) and (b)
the actual bus resource handling (so the PCI code itself uses it to
sort out resource use and avoid conflicts, but PCI drivers themselves
generally then don't care, because the bus has "taken care of it".

So that's why the amdgpu driver itself doesn't care about resource
allocations, and we only get a warning for that memory type case, not
for any deeper resource case.

And apparently the vmwgfx driver still uses that legacy "let's claim
all PCI resources in the resource tree" instead of just claiming the
device itself. Which is why it hit this whole BOOTFB resource thing
even harder.

But the real bug is that BOOTFB seems to claim this resource even
after it is done with it and other drivers want to take over.

Not the BUSY bit.

 Linus

Annoying AMDGPU boot-time warning due to simplefb / amdgpu resource clash

[Linus Torvalds]

So this has been going on for a while, and it's quite annoying.

At bootup, my main desktop (Threadripper 3970X with radeon graphics)
now complains about

  resource sanity check: requesting [mem 0xd000-0xdfff], which
spans more than BOOTFB [mem 0xd000-0xd02f]

and then gives me a nasty callchain that is basically the amdgpu probe
sequence ending in amdgpu_bo_init() doing the
arch_io_reserve_memtype_wc() which is then what complains.

That "BOOTFB" resource is from sysfb_simplefb.c, and I think what
started triggering this is commit c96898342c38 ("drivers/firmware:
Don't mark as busy the simple-framebuffer IO resource").

Because it turns out that that removed the IORESOURCE_BUSY, which in
turn is what makes the resource conflict code complain about it now,
because

/*
 * if a resource is "BUSY", it's not a hardware resource
 * but a driver mapping of such a resource; we don't want
 * to warn for those; some drivers legitimately map only
 * partial hardware resources. (example: vesafb)
 */

so the issue is that now the resource code - correctly - says "hey,
you have *two* conflicting driver mappings".

And that commit claims it did it because "which can lead to drivers
requesting the same memory resource to fail", but - once again - the
link in the commit that might actually tell more is just one of those
useless patch submission links again.

So who knows why that commit was actually done, but it's causing annoyance.

If simplefb is actually still using that frame buffer, it's a problem.
If it isn't, then maybe that resource should have been released?

I really think that commit c96898342c38 is buggy. It talks about "let
drivers to request it as busy instead", but then it registers a
resource that isn't actually a proper real resource. It's just a
random incomplete chunk of the actual real thing, so it will still
interfere with resource allocation, and in fact now interferes even
with that "set memtype" because of this valid warning.

 Linus

Re: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [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: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Tue, Jan 11, 2022 at 7:38 AM Harry Wentland  wrote:
>
> Attached is a v2 of the buggy patch that should get this right.
> If you have a chance to try it out let us know

I can confirm that I do not see the horribly flickering behavior with
this patch.

I didn't look at what the actual differences were from the one I
reverted, but at least on my machine this version works.

Linus

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 6:22 PM Linus Torvalds
 wrote:
>
> and I guess I'll do the few more bisections to pick out the exact one.

a896f870f8a5f23ec961d16baffd3fda1f8be57c is the first bad commit.

Attaching ther BISECT_LOG in case anybody cares.

I'll double-check to see if a revert fixes it at the top of my tree.

Linus


BISECT_LOG
Description: Binary data

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 5:21 PM Linus Torvalds
 wrote:
>
> I'll see if I can bisect it at least partially.

It seems to be very reliable. I can see the flickering even at early
boot before gdb has started - the graphical screen where you type the
encrypted disk password at boot already shows it as you type.

Right now it is

  bad: 9602044d1cc12280e20c5f2cd640ae80f69e
  good: 3867e3704f136beadf5e004b61696ef7f990bee4

so it's going to be one of these:

  9602044d1cc1 drm/amd/display: Fix for the no Audio bug with Tiled Displays
  a896f870f8a5 drm/amd/display: Fix for otg synchronization logic
  aba3c3fede54 drm/amd/display: Clear DPCD lane settings after repeater training
  9311ed1e1241 drm/amd/display: add hdmi disable debug check
  6421f7c750e9 drm/amd/display: Allow DSC on supported MST branch devices
  ebe5ffd8e271 drm/amd/display: Enable P010 for DCN3x ASICs
  c022375ae095 drm/amd/display: Add DP-HDMI FRL PCON Support in DC
  50b1f44ec547 drm/amd/display: Add DP-HDMI FRL PCON SST Support in DM
  81d104f4afbf drm/amdgpu: Don't halt RLC on GFX suspend
  fe9c5c9affc9 drm/amdgpu: Use MAX_HWIP instead of HW_ID_MAX
  370016988665 drm/amdgpu: fix the missed handling for SDMA2 and SDMA3
  6c18ecefaba7 drm/amdgpu: declare static function to fix compiler warning
  94a80b5bc7a2 amdgpu/pm: Modify implmentations of
get_power_profile_mode to use amdgpu_pp_profile_name

and I guess I'll do the few more bisections to pick out the exact one.

 Linus

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 6:44 PM Linus Torvalds
 wrote:
>
> I'll double-check to see if a revert fixes it at the top of my tree.

Yup. It reverts cleanly, and the end result builds and works fine, and
doesn't show the horrendous flickering.

I have done that revert, and will continue the merge window work.
Somebody else gets to figure out what the actual bug is, but that
commit was horribly broken on my machine (Sapphire Pulse RX 580 8GB,
fwiw).

   Linus

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 5:21 PM Linus Torvalds
 wrote:
>
> It also seems to depend a bit on the screen contents - or possibly on
> what else is going on. Hiding the browser window makes it happen less,
> I think. But I suspect that's about "less gpu activity" than anything
> else.

Actually, sometimes "more activity" makes it go away too. Moving a
window around wildly with the mouse makes it *stop* happen.

But moving the mouse over different elements of the screen - or
writing text in the web browser email window - seems to make it worse.

Funky.

It does "feel" to me like some bandwidth limitation, it has kind of
the same behavior that I remember from the bad old times when you were
pushing the video card past a resolution that it could really handle.

But that can't be the case, this card has had no problems with this before.

   Linus

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 5:11 PM Alex Deucher  wrote:
>
> We are putting together a system to try and repro the issue.  Does it
> happen with a single monitor or only with two?

Nope. With a single monitor everything seems to look fine. And when I
plug in the second monitor, it immediately starts happening again.

It also seems to depend a bit on the screen contents - or possibly on
what else is going on. Hiding the browser window makes it happen less,
I think. But I suspect that's about "less gpu activity" than anything
else.

I'll see if I can bisect it at least partially.

  Linus

Re: [git pull] drm for 5.17-rc1 (pre-merge window pull)

[Linus Torvalds]

On Mon, Jan 10, 2022 at 2:13 PM Alex Deucher  wrote:
>
> Sounds like something related to watermarks.  That said, we haven't
> really touched the display code for DCE11 cards in quite a while.  Can
> you provide your dmesg output?

I'm not seeing anything that would look interesting, but here's the
parts that look relevant for drm..

  Linus


dmesg-gpu
Description: Binary data

Re: Expecting to revert commit 55285e21f045 "fbdev/efifb: Release PCI device ..."

[Linus Torvalds]

[ Adding back in more amd people and the amd list, the people Daniel
added seem to have gotten lost again, but I think people at least saw
my original report thanks to Daniel ]

With "amdgpu.runpm=0", things are better, but not perfect. With that I
can lock the screen, and it has to go through *two* cycles of "No
signal, turning off", but on the second cycle it does finally work.

This was exposed by commit 55285e21f045 ("fbdev/efifb: Release PCI
device's runtime PM ref during FB destroy"), probably because that
made runtime PM actually potentially work, but it is then broken on
amdgpu.

Absolutely nothing odd in my setup. Two monitors, one GPU. PCI ID
1002:67df rev e7, subsystem ID 1da2:e353.

I'd expect pretty much any amdgpu person to see this.

On Mon, Dec 20, 2021 at 2:04 PM Linus Torvalds
 wrote:
>
> Note: on my machine, I get that
>
>amdgpu :49:00.0: amdgpu: Using BACO for runtime pm
>
> so maybe the other possible runtime pm models (ARPX and BOCO) are ok,
> and it's only that BACO case that is broken.

Hmm. The *documentation* says:

PX runtime pm
2 = force enable with BAMACO,
1 = force enable with BACO,
0 = disable,
-1 = PX only default

but the code actually makes anything != 0 enable it, except on VEGA20
and ARCTURUS, where it needs to be positive.

My card is apparently "POLARIS10", whatever that means, which means
that any non-zero value of amdgpu_runtime_pm will enable runtime PM as
long as "amdgpu_device_supports_baco()" is true. Which it is.

Whatever. Now I'm just kwetching about the documentation not matching
what I see the code doing, which is never a great sign when things
don't work.

  Linus

Re: [PATCH] Enable '-Werror' by default for all kernel builds

[Linus Torvalds]

On Wed, Sep 8, 2021 at 10:59 PM Christoph Hellwig  wrote:
>
> While we're at it, with -Werror something like this is really futile:

Yeah, I'm thinking we could do

 -Wno-error=cpp

to at least allow the cpp warnings to come through without being fatal.

Because while they can be annoying too, they are most definitely under
our direct control, so..

I didn't actually test that, but I think it should work.

That said, maybe they should just be removed. They might be better off
just as Kconfig rules, rather than as a "hey, you screwed up your
Kconfig" warning after the fact.

 Linus

Re: [PATCH] Enable '-Werror' by default for all kernel builds

[Linus Torvalds]

On Thu, Sep 9, 2021 at 4:43 AM Marco Elver  wrote:
>
> Sure, but the reality is that the real stack size is already doubled
> for KASAN. And that should be reflected in Wframe-larger-than.

I don't think that's true.

Quite the reverse, in fact.

Yes, the *dynamic* stack size is doubled due to KASAN, because it will
cause much deeper callchains.

But the individual frames don't grow that much apart from compilers
doing stupid things (ie apparently clang and KASAN_STACK), and if
anything, the deeper dynamic call chains means that the individual
frame size being small is even *more* important, but we do compensate
for the deeper stacks by making THREAD_SIZE_ORDER bigger at least on
x86.

Honestly, I am not even happy with the current "2048 bytes for
64-bit". The excuse has been that 64-bit needs more stack, but all it
ever did was clearly to just allow people to just do bad things.

Because a 1kB stack frame is horrendous even in 64-bit. That's not
"spill some registers" kind of stack frame. That's "put a big
structure on the stack" kind of stack frame regardless of any other
issues.

And no, "but we have 16kB of stack and we'll switch stacks on
interrupts" is not an excuse for one single level to use up 1kB, much
less 2kB.  Does anybody seriously believe that we don't quite normally
have stacks that are easily tens of frames deep?

Without having some true "this is the full callchain" information, the
best we can do is just limit individual stack frames. And 2kB is
*excessive*.

 Linus

Re: [PATCH] Enable '-Werror' by default for all kernel builds

[Linus Torvalds]

On Tue, Sep 7, 2021 at 10:10 AM Linus Torvalds
 wrote:
>
> Do I know why? No. I do note that that code is disgusting.
>
> It's passing one of those structs around by value, for example. That's
> a 72-byte structure that is copied on the stack due to stupid calling
> conventions. Maybe clang generates a few extra temporaries for it as
> part of the function call stack setup? Who knows..

Ooh, yes.

This attached patch is crap - it converts the helper functions to use
const pointers instead of passing the whole structure, but it then
only converts that one file that *uses* them.

So the end result will not compile in general, but you can do

make 
drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_rq_dlg_calc_30.o

and it compiles for me.

And while gcc doesn't care that much - it will apparently either
generate the argument stack every call - clang cares deeply.

The nasty 720-byte stack frame that clang generates turns into just a
320-byte one, and code generation in general looks a *lot* better.

Now, as mentioned, this patch is broken and incomplete. But I really
think the AMD GPU people need to do this. It makes those functions go
from practically unusable to not horribly disgusting.

So Harry/Leo/Alex/Christian and amd-gfx list - can you look into
making this ugly "make one file compile better" patch actually work
properly?

It *looks* lto me ike that code was perhaps written for a C++ compiler
and the helpers have been written as a "pass by reference", and the
arguments used to be

 const display_data_rq_misc_params_st& rq_misc_param

and then the compiler will pass the argument as a pointer. And then it
was converted to C, and the "pass by reference" in the function
declaration was turned into "pass by value", to avoid changing "." to
"->" in the use.

But a '' thing in C++ really is a '*arg' pointer in C, and should
have been done as that.

Of course, it's also possible that that code was simply written by
somebody who didn't understand just *how* horrible it is to pass
structures bigger than a word or two by value.

Do we have a compiler warning for passing big structures by value?

   Linus
 .../display/dc/dml/dcn30/display_rq_dlg_calc_30.c  | 142 ++---
 .../display/dc/dml/dcn30/display_rq_dlg_calc_30.h  |   2 +-
 .../amd/display/dc/dml/display_rq_dlg_helpers.h|  20 +--
 3 files changed, 82 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_rq_dlg_calc_30.c b/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_rq_dlg_calc_30.c
index 0d934fae1c3a..9b5ee53d2de3 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_rq_dlg_calc_30.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/dcn30/display_rq_dlg_calc_30.c
@@ -92,7 +92,7 @@ static void extract_rq_sizing_regs(struct display_mode_lib *mode_lib,
 	const display_data_rq_sizing_params_st rq_sizing)
 {
 	dml_print("DML_DLG: %s: rq_sizing param\n", __func__);
-	print__data_rq_sizing_params_st(mode_lib, rq_sizing);
+	print__data_rq_sizing_params_st(mode_lib, _sizing);
 
 	rq_regs->chunk_size = dml_log2(rq_sizing.chunk_bytes) - 10;
 
@@ -113,28 +113,28 @@ static void extract_rq_sizing_regs(struct display_mode_lib *mode_lib,
 
 static void extract_rq_regs(struct display_mode_lib *mode_lib,
 	display_rq_regs_st *rq_regs,
-	const display_rq_params_st rq_param)
+	const display_rq_params_st *rq_param)
 {
 	unsigned int detile_buf_size_in_bytes = mode_lib->ip.det_buffer_size_kbytes * 1024;
 	unsigned int detile_buf_plane1_addr = 0;
 
-	extract_rq_sizing_regs(mode_lib, &(rq_regs->rq_regs_l), rq_param.sizing.rq_l);
+	extract_rq_sizing_regs(mode_lib, &(rq_regs->rq_regs_l), rq_param->sizing.rq_l);
 
-	rq_regs->rq_regs_l.pte_row_height_linear = dml_floor(dml_log2(rq_param.dlg.rq_l.dpte_row_height),
+	rq_regs->rq_regs_l.pte_row_height_linear = dml_floor(dml_log2(rq_param->dlg.rq_l.dpte_row_height),
 		1) - 3;
 
-	if (rq_param.yuv420) {
-		extract_rq_sizing_regs(mode_lib, &(rq_regs->rq_regs_c), rq_param.sizing.rq_c);
-		rq_regs->rq_regs_c.pte_row_height_linear = dml_floor(dml_log2(rq_param.dlg.rq_c.dpte_row_height),
+	if (rq_param->yuv420) {
+		extract_rq_sizing_regs(mode_lib, &(rq_regs->rq_regs_c), rq_param->sizing.rq_c);
+		rq_regs->rq_regs_c.pte_row_height_linear = dml_floor(dml_log2(rq_param->dlg.rq_c.dpte_row_height),
 			1) - 3;
 	}
 
-	rq_regs->rq_regs_l.swath_height = dml_log2(rq_param.dlg.rq_l.swath_height);
-	rq_regs->rq_regs_c.swath_height = dml_log2(rq_param.dlg.rq_c.swath_height);
+	rq_regs->rq_regs_l.swath_height = dml_log2(rq_param->dlg.rq_l.swath_height);
+	rq_regs->rq_regs_c.swath_height = dml_log2(rq_param->dlg.rq_c.swath_height);
 
 	// FIXME: take the max between luma, chroma chunk size?
 	// okay for now, as we are setting chunk_bytes to 8kb anyways
-	if (rq_param.sizing.rq_l.c

Re: [PATCH] Enable '-Werror' by default for all kernel builds

[Linus Torvalds]

On Tue, Sep 7, 2021 at 8:52 PM Harry Wentland  wrote:
>
> Attached patches fix these x86_64 ones reported by Nick:

Hmm.

You didn't seem to fix up the calling convention for print__xyz(),
which still take those xyz structs as pass-by-value.

Obviously it would be good to do things incrementally, so if that
attached patch was just [1/N] I won't complain..

> I'm also seeing one more that might be more challenging to fix but is nearly 
> at 1024:
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn21/display_mode_vba_21.c:3397:6:
>  error: stack frame size of 1064 bytes in function 
> 'dml21_ModeSupportAndSystemConfigurationFull' [-Werror,-Wframe-larger-than=]

Oh Gods, that function is truly something else..

Is there some reason why it's one humongous function, with the
occasional single-line comment?

Because it really looks to me like pretty much everywhere I see one of
those rare comments, I would go "this part should be a function of its
own", and then there would be one caller fuynction that just calls
each of those sub-functions one after the other.

That would - I think - make the code easier to read, and then it would
also make it very obvious where it magically uses a lot of stack.

My suspicion is actually "nowhere". The stack use is just hugely
spread out, and the compiler has just kept accumulating more spill
variables on the frame with no single big reason.

Yes, I see a couple of local structures:

Pipe myPipe;
HostVM myHostVM;

but more than that I see several function calls that have basically 62
arguments. And I wish I was making that number up. I'm not. That
"CalculatePrefetchSchedule()" call literally has 62 arguments.

But *all* of the top-level loops in that function literally look like
they could - and should - be functions in their own right. Some of
them would be fairly complex even so (ie that code under the comment

//Prefetch Check

would be quite the big function all of its own.

We have a coding style thing:

Documentation/process/coding-style.rst

that says that you should strive to have functions that are "short and
sweet" and fit on one or two screenfuls of text.

That one function from hell is 1832 lines of code.

It really could be improved upon.

   Linus

Re: [git pull] drm fixes for 5.14-rc4

[Linus Torvalds]

This might possibly have been fixed already by the previous drm pull,
but I wanted to report it anyway, just in case.

It happened after an uptime of over a week, so it might not be trivial
to reproduce.

It's a NULL pointer dereference in dc_stream_retain() with the code being

lock xadd %eax,0x390(%rdi) <-- trapping instruction

and that's just the

kref_get(>refcount);

with a NULL 'stream' argument.

  Call Trace:
   dc_resource_state_copy_construct+0x13f/0x190 [amdgpu]
   amdgpu_dm_atomic_commit_tail+0xd5/0x1540 [amdgpu]
   commit_tail+0x97/0x180 [drm_kms_helper]
   process_one_work+0x1df/0x3a0

the oops is followed by a stream of

  [drm:amdgpu_dm_atomic_check [amdgpu]] *ERROR* [CRTC:55:crtc-1]
hw_done or flip_done timed out

and the machine was not usable afterwards.

lspci says this is a

 49:00.0 VGA compatible controller [0300]:
   Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere
   [Radeon RX 470/480/570/570X/580/580X/590]
   [1002:67df] (rev e7) (prog-if 00 [VGA controller])

Full oops in the attachment, but I think the above is all the really
salient details.

   Linus


amd-gpu-ooops
Description: Binary data

Re: [PATCH v3 3/3] drm/amdgpu: Change type of module param `ppfeaturemask` to hexint

[Linus Torvalds]

On Fri, Jul 24, 2020 at 12:54 AM Christian König
 wrote:
>
> But since you already addressed Linus comments and it looks rather clean
> I think I can just push it to drm-misc-next on Monday if nobody objects
> over the weekend.

Yeah, no objections from me.

Add a note to it to the pull request, so that when my bird-brain sees
the pull during the next merge window and I've forgotten this thread,
I don't go "why is the drm tree modifying code files"?

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

Re: [PATCH 1/2] moduleparams: Add hex type parameter

[Linus Torvalds]

On Thu, Jul 2, 2020 at 7:42 AM Christian König  wrote:
>
> I'm just not sure how well this is received upstream because it only
> covers u32
>
> On the other hand that is probably also the most used.

Not necessarily true. I'd argue that "unsigned long"  is equally
possible for some bit mask (or other hex-likely) type.

So don't call it just "hex". Call it "hexint" (the hex does imply
"unsigned", I feel - showing hex numbers with a sign sounds insane).

That way, if somebody ends up wanting it for unsigned long values,
we're not stuck.

Another option is to just say that hex values always have bit _sizes_.
So "hex32" and "hex64" would also make sense as names to me.

While at it, should the hex numbers always be padded out to the size?
The example Paul used doesn't have that issue (high bit being set).

Bbut often it may make sense to show a 32-bit hex number as "%#08x"
because it really makes things clearer when you're looking at high
bits, say.

It's really hard to tell the difference between "just bit 27 set" and
"just bit 31" set otherwise, and that's not all that uncommon when the
bitmasks are sparse.

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Wed, Dec 18, 2019 at 10:37 AM Jason Gunthorpe  wrote:
>
> I think this is what you are looking for?

I think that with these names, I would have had an easier time reading
the original patch that made me go "Eww", yes.

Of course, now that it's just a rename patch, it's not like the patch
is all that legible, but yeah, I think the naming is saner.

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Wed, Dec 18, 2019 at 6:59 AM Jason Gunthorpe  wrote:
>
> Do you think calling it 'mmn_subscriptions' is clear?

Why do you want that "mmn"?

Guys, the "mmn" part is clear from the _context_. The function name is

When the function name is something like "mmu_interval_read_begin()",
and the filename is "mm/mmu_notifier.c", you do NOT NEED silly
prefixes like "mmn" for local variables.

They add NOTHING.

And they make the code an illegible mess.

Yes, global function names need to be unique, and if they aren't
really core, they want some prefix that explains the context, because
global functions are called from _outside_ the context that explains
them.

But if it's a "struct mmu_interval_notifier" pointer, and it's inside
a file that is all about these pointers, it shouldn't be called
"mmn_xyz".  That's not a name. That's line noise.

So call it a "notifier". Maybe even an "interval_notifier" if you
don't mind the typing. Name it by something _descriptive_. And if you
want.

And "subscriptions" is a lovely name. What does the "mmn" buy you?

Just to clarify: the names I really hated were the local variable
names (and the argument names) that were all entirely within the
context of mm/mmu_notifier.c. Calling something "mmn_mm" is a random
jumble of letters that looks more like you're humming than you're
speaking.

Don't mumble. Speak _clearly_.

The other side of "short names" is that some non-local conventions
exist because they are _so_ global. So if it's just a mm pointer, call
it "mm". We do have some very core concepts in the kernel that
permeate _everything_, and those core things we tend to have very
short names for. So whenever you're working with VM code, you'll see
lots of small names like "mm", "vma", "pte" etc. They aren't exactly
clear, but they are _globally_ something you read and learn when you
work on the Linux VM code.

That's very diofferent from "mmn" - the "mmn" thing isn't some global
shorthand, it is just a local abomination.

So "notifier_mm" makes sense - it's the mm for a notifier. But
"mmn_notifier" does not, because "mmn" only makes sense in a local
context, and in that local context it's not any new information at
all.

See the difference? Two shorthands, but one makes sense and adds
information, while the other is just unnecessary and pointless and
doesn't add anything at all.

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Sat, Nov 30, 2019 at 10:03 AM Linus Torvalds
 wrote:
>
> I'll try to figure the code out, but my initial reaction was "yeah,
> not in my VM".

Why is it ok to sometimes do

WRITE_ONCE(mni->invalidate_seq, cur_seq);

(to pair with the unlocked READ_ONCE), and sometimes then do

mni->invalidate_seq = mmn_mm->invalidate_seq;

My initial guess was that latter is only done at initialization time,
but at least in one case it's done *after* the mni has been added to
the mmn_mm (oh, how I despise those names - I can only repeat: WTF?).

See __mmu_interval_notifier_insert() in the
mmn_mm->active_invalidate_ranges case.

I'm guessing that it doesn't matter, because when inserting the
notifier the sequence number is presumably not used until after the
insertion (and any use though mmn_mm is protected by the
mmn_mm->lock), but it still looks odd to me.

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Mon, Nov 25, 2019 at 12:42 PM Jason Gunthorpe  wrote:
>
> Here is this batch of hmm updates, I think we are nearing the end of this
> project for now, although I suspect there will be some more patches related to
> hmm_range_fault() in the next cycle.

I've ended up pulling this, but I'm not entirely happy with the code.
You've already seen the comments on it in the earlier replies.

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Mon, Nov 25, 2019 at 12:42 PM Jason Gunthorpe  wrote:
>
> You will probably be most interested in the patch "mm/mmu_notifier: add an
> interval tree notifier".

I'm trying to read that patch, and I'm completely unable to by the
absolutely *horrid* variable names.

There are zero excuses for names like "mmn_mm". WTF?

I'll try to figure the code out, but my initial reaction was "yeah,
not in my VM".

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

Re: [GIT PULL] Please pull hmm changes

[Linus Torvalds]

On Tue, Jul 9, 2019 at 12:24 PM Jason Gunthorpe  wrote:
>
> I'm sending it early as it is now a dependency for several patches in
> mm's quilt.

.. but I waited to merge it until I had time to review it more
closely, because I expected the review to be painful.

I'm happy to say that I was overly pessimistic, and that instead of
finding things to hate, I found it all looking good.

Particularly the whole "use reference counts properly, so that
lifetimes make sense and all those nasty cases can't happen" parts.

It's all merged, just waiting for the test-build to verify that I
didn't miss anything (well, at least nothing obvious).

  Linus

Re: 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-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: 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 00/13] mmu_notifier kill invalidate_page callback

[Linus Torvalds]

On Tue, Aug 29, 2017 at 4:54 PM, Jérôme Glisse  wrote:
>
> Note this is barely tested. I intend to do more testing of next few days
> but i do not have access to all hardware that make use of the mmu_notifier
> API.

Thanks for doing this.

> First 2 patches convert existing call of mmu_notifier_invalidate_page()
> to mmu_notifier_invalidate_range() and bracket those call with call to
> mmu_notifier_invalidate_range_start()/end().

Ok, those two patches are a bit more complex than I was hoping for,
but not *too* bad.

And the final end result certainly looks nice:

>  16 files changed, 74 insertions(+), 214 deletions(-)

Yeah, removing all those invalidate_page() notifiers certainly makes
for a nice patch.

And I actually think you missed some more lines that can now be
removed: kvm_arch_mmu_notifier_invalidate_page() should no longer be
needed either, so you can remove all of those too (most of them are
empty inline functions, but x86 has one that actually does something.

So there's an added 30 or so dead lines that should be removed in the
kvm patch, I think.

But from a _very_ quick read-through this looks fine. But it obviously
needs testing.

People - *especially* the people who saw issues under KVM - can you
try out Jérôme's patch-series? I aded some people to the cc, the full
series is on lkml. Jérôme - do you have a git branch for people to
test that they could easily pull and try out?

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