Re: [PATCH] drm/amdgpu: fix suspend/resume hang regression
Am 01.03.22 um 07:26 schrieb Qiang Yu:
Regression has been reported that suspend/resume may hang with
the previous vm ready check commit:
https://gitlab.freedesktop.org/drm/amd/-/issues/1915#note_1278198
So bring back the evicted list check as a temp fix.
Fixes: cc8dd2cc1a97 ("drm/amdgpu: check vm ready by amdgpu_vm->evicting flag")
Signed-off-by: Qiang Yu
Reviewed-by: Christian König
Cc:
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2cd9f1a2e5fa..fc4563cf2828 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -779,7 +779,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
amdgpu_vm_eviction_lock(vm);
ret = !vm->evicting;
amdgpu_vm_eviction_unlock(vm);
- return ret;
+
+ return ret && list_empty(>evicted);
}
/**
[PATCH] drm/amdgpu: Move CAP firmware loading to the beginning of PSP firmware list
[Why]
As PSP needs to verify the signature, CAP firmware must be loaded first when
PSP loads firmwares.
Otherwise, when DFC feature is enabled, CP firmwares would be loaded failed.
[ 1149.160480] [drm] MM table gpu addr = 0x800022f000, cpu addr =
a62afcea.
[ 1149.209874] [drm] failed to load ucode CP_CE(0x8)
[ 1149.209878] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[ 1149.215914] [drm] failed to load ucode CP_PFP(0x9)
[ 1149.215917] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[ 1149.221941] [drm] failed to load ucode CP_ME(0xA)
[ 1149.221944] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[ 1149.228082] [drm] failed to load ucode CP_MEC1(0xB)
[ 1149.228085] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[ 1149.234209] [drm] failed to load ucode CP_MEC2(0xD)
[ 1149.234212] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[ 1149.242379] [drm] failed to load ucode VCN(0x1C)
[ 1149.242382] [drm] psp gfx command LOAD_IP_FW(0x6) failed and response status
is (0x0007)
[How]
Move CAP UCODE ID to the beginning of AMDGPU_UCODE_ID enum list.
Signed-off-by: Yifan Zha
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
index 428f4df184d0..40dffbac85a0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
@@ -343,7 +343,8 @@ union amdgpu_firmware_header {
* fw loading support
*/
enum AMDGPU_UCODE_ID {
- AMDGPU_UCODE_ID_SDMA0 = 0,
+ AMDGPU_UCODE_ID_CAP = 0,
+ AMDGPU_UCODE_ID_SDMA0,
AMDGPU_UCODE_ID_SDMA1,
AMDGPU_UCODE_ID_SDMA2,
AMDGPU_UCODE_ID_SDMA3,
@@ -378,7 +379,6 @@ enum AMDGPU_UCODE_ID {
AMDGPU_UCODE_ID_VCN0_RAM,
AMDGPU_UCODE_ID_VCN1_RAM,
AMDGPU_UCODE_ID_DMCUB,
- AMDGPU_UCODE_ID_CAP,
AMDGPU_UCODE_ID_MAXIMUM,
};
--
2.25.1
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 22:13 schrieb James Bottomley:
On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote:
Am 28.02.22 um 21:42 schrieb James Bottomley:
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König
wrote:
[SNIP]
Anybody have any ideas?
I think we should look at the use cases why code is touching
(pos)
after the loop.
Just from skimming over the patches to change this and experience
with the drivers/subsystems I help to maintain I think the
primary pattern looks something like this:
list_for_each_entry(entry, head, member) {
if (some_condition_checking(entry))
break;
}
do_something_with(entry);
Actually, we usually have a check to see if the loop found
anything, but in that case it should something like
if (list_entry_is_head(entry, head, member)) {
return with error;
}
do_somethin_with(entry);
Suffice? The list_entry_is_head() macro is designed to cope with
the bogus entry on head problem.
That will work and is also what people already do.
The key problem is that we let people do the same thing over and
over again with slightly different implementations.
Out in the wild I've seen at least using a separate variable, using
a bool to indicate that something was found and just assuming that
the list has an entry.
The last case is bogus and basically what can break badly.
Yes, I understand that. I'm saying we should replace that bogus checks
of entry->something against some_value loop termination condition with
the list_entry_is_head() macro. That should be a one line and fairly
mechanical change rather than the explosion of code changes we seem to
have in the patch series.
Yes, exactly that's my thinking as well.
Christian.
James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 28 Feb 2022 16:41:04 -0800 Linus Torvalds wrote:
> 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
I presume the goal is that we can do this without changing existing
code? Otherwise actually moving the iterator into the loop body would
be an option, by creating a different hidden variable:
#define list_iter(head) \
for (struct list head *_l = (head)->next; _l != (head); _l = _l->next)
#define list_iter_entry(var, member)\
list_entry(_l, typeof(*var), member)
list_iter(>a_head) {
struct entry *e = list_iter_entry(e, a_member);
/* use e->... */
}
Or we can slide into soft insanity and exploit one of Kees'es tricks
to encode the type of the entries "next to" the head:
#define LIST_HEAD_MEM(name, type) \
union { \
struct list_head name; \
type *name ## _entry; \
}
struct entry {
struct list_head a_member;
};
struct parent {
LIST_HEAD_MEM(a_head, struct entry);
};
#define list_for_each_magic(pos, head, member) \
for (typeof(**(head ## _entry)) *pos = list_first_entry(head,
typeof(**(head ## _entry)), member); \
>member != (head);\
pos = list_next_entry(pos, member))
list_for_each_magic(e, >a_head, a_member) {
/* use e->... */
}
I'll show myself out...
[PATCH] drm/amdgpu: fix suspend/resume hang regression
Regression has been reported that suspend/resume may hang with
the previous vm ready check commit:
https://gitlab.freedesktop.org/drm/amd/-/issues/1915#note_1278198
So bring back the evicted list check as a temp fix.
Fixes: cc8dd2cc1a97 ("drm/amdgpu: check vm ready by amdgpu_vm->evicting flag")
Signed-off-by: Qiang Yu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 2cd9f1a2e5fa..fc4563cf2828 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -779,7 +779,8 @@ bool amdgpu_vm_ready(struct amdgpu_vm *vm)
amdgpu_vm_eviction_lock(vm);
ret = !vm->evicting;
amdgpu_vm_eviction_unlock(vm);
- return ret;
+
+ return ret && list_empty(>evicted);
}
/**
--
2.25.1
Re: [pull] amdgpu, amdkfd drm-next-5.18
On Tue, 1 Mar 2022 at 03:11, Alex Deucher wrote: > > On Mon, Feb 28, 2022 at 1:55 AM Dave Airlie wrote: > > > > On Sat, 26 Feb 2022 at 04:35, Alex Deucher > > wrote: > > > > > > Hi Dave, Daniel, > > > > > > New stuff for 5.18. > > > > > > The following changes since commit > > > b63c54d978236dd6014cf2ffba96d626e97c915c: > > > > > > drm/amdkfd: Use proper enum in pm_unmap_queues_v9() (2022-02-17 > > > 15:59:06 -0500) > > > > > > are available in the Git repository at: > > > > > > https://gitlab.freedesktop.org/agd5f/linux.git > > > tags/amd-drm-next-5.18-2022-02-25 > > > > > > for you to fetch changes up to 111aeed25ec6bf4d5b4a7b4cb5654f002ba9f795: > > > > > > drm/amdgpu: add gfxoff support for smu 13.0.5 (2022-02-25 11:51:18 > > > -0500) > > > > > > > > > amd-drm-next-5.18-2022-02-25: > > > > > > amdgpu: > > > - Raven2 suspend/resume fix > > > - SDMA 5.2.6 updates > > > - VCN 3.1.2 updates > > > - SMU 13.0.5 updates > > > - DCN 3.1.5 updates > > > - Virtual display fixes > > > - SMU code cleanup > > > - Harvest fixes > > > - Expose benchmark tests via debugfs > > > - Drop no longer relevant gart aperture tests > > > - More RAS restructuring > > > - W=1 fixes > > > - PSR rework > > > - DP/VGA adapter fixes > > > - DP MST fixes > > > - GPUVM eviction fix > > > - GPU reset debugfs register dumping support > > > > (this time keeping cc). > > > > ^ this seems to conflict with the removal of reset_sem or something in > > that area. > > > > Can you trial merge this to drm-next and send a fixup patch I should > > apply with it? > > reset_sem moved from adev->reset_sem to adev->reset_domain->sem. See > the attached diff. I also pushed a sample merge if that is helpful: > https://gitlab.freedesktop.org/agd5f/linux/-/commits/drm-next-amd-drm-next-merge-5.18 > > Thanks! > Excellent thanks Alex. Dave.
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 10:20:28AM -0800, Joe Perches wrote: > On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > > > a multi-line indent gets curly braces for readability even though > > it's not required by C. And then both sides would get curly braces. > > That's more your personal preference than a coding style guideline. > It's a USB patch. I thought Greg prefered it that way. Greg has some specific style things which he likes and I have adopted and some I pretend not to see. regards, dan carpenter
[PATCH] drm/amdgpu: enable gfx clock gating control for GC 10.3.7
Enable gfx cg gate/ungate control for GC 10.3.7. Signed-off-by: Prike Liang --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 90158289cd30..fd7ded7799e2 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -8445,6 +8445,7 @@ static int gfx_v10_0_set_clockgating_state(void *handle, case IP_VERSION(10, 3, 5): case IP_VERSION(10, 3, 6): case IP_VERSION(10, 3, 3): + case IP_VERSION(10, 3, 7): gfx_v10_0_update_gfx_clock_gating(adev, state == AMD_CG_STATE_GATE); break; -- 2.17.1
RE: [PATCH] drm/amdgpu: Use IP versions in convert_tiling_flags_to_modifier()
Reviewed-by: Guchun Chen
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Tuesday, March 1, 2022 6:16 AM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander
Subject: [PATCH] drm/amdgpu: Use IP versions in
convert_tiling_flags_to_modifier()
Rather than checking the asic_type.
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 448e9b46417c..dd70c85b8205 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -691,9 +691,9 @@ static int convert_tiling_flags_to_modifier(struct
amdgpu_framebuffer *afb)
return -EINVAL;
}
- if (adev->asic_type >= CHIP_SIENNA_CICHLID)
+ if (adev->ip_versions[GC_HWIP][0] >= IP_VERSION(10, 3, 0))
version = AMD_FMT_MOD_TILE_VER_GFX10_RBPLUS;
- else if (adev->family == AMDGPU_FAMILY_NV)
+ else if (adev->ip_versions[GC_HWIP][0] >= IP_VERSION(10, 0, 0))
version = AMD_FMT_MOD_TILE_VER_GFX10;
else
version = AMD_FMT_MOD_TILE_VER_GFX9; @@ -787,7 +787,7
@@ static int convert_tiling_flags_to_modifier(struct amdgpu_framebuffer *afb)
if (adev->family >= AMDGPU_FAMILY_NV) {
int extra_pipe = 0;
- if (adev->asic_type >=
CHIP_SIENNA_CICHLID &&
+ if ((adev->ip_versions[GC_HWIP][0] >=
IP_VERSION(10, 3, 0)) &&
pipes == packers && pipes > 1)
extra_pipe = 1;
--
2.35.1
Re: [PATCH] mm: split vm_normal_pages for LRU and non-LRU handling
On 2022-02-28 15:34, Alex Sierra wrote:
DEVICE_COHERENT pages introduce a subtle distinction in the way
"normal" pages can be used by various callers throughout the kernel.
They behave like normal pages for purposes of mapping in CPU page
tables, and for COW. But they do not support LRU lists, NUMA
migration or THP.
Should have mentioned KSM here as well for completeness.
Therefore we split vm_normal_page into two
functions vm_normal_any_page and vm_normal_lru_page. The latter will
only return pages that can be put on an LRU list and that support
NUMA migration and THP.
We also introduced a FOLL_LRU flag that adds the same behaviour to
follow_page and related APIs, to allow callers to specify that they
expect to put pages on an LRU list.
Signed-off-by: Alex Sierra
Acked-by: Felix Kuehling
FWIW. Full disclosure, Alex and I worked on this together, but it's a
bit like the blind leading the blind. ;) It's mostly untested at this
point. Alex is working on adding tests for get_user_pages of
DEVICE_COHERENT pages without FOLL_LONGTERM to test_hmm and also a test
for COW of DEVICE_COHERENT pages.
A few more nit-picks inline.
---
fs/proc/task_mmu.c | 12 +-
include/linux/mm.h | 53 -
mm/gup.c| 10 +
mm/hmm.c| 2 +-
mm/huge_memory.c| 2 +-
mm/khugepaged.c | 8 +++
mm/ksm.c| 4 ++--
mm/madvise.c| 4 ++--
mm/memcontrol.c | 2 +-
mm/memory.c | 38 ++--
mm/mempolicy.c | 4 ++--
mm/migrate.c| 2 +-
mm/migrate_device.c | 2 +-
mm/mlock.c | 6 ++---
mm/mprotect.c | 2 +-
15 files changed, 85 insertions(+), 66 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 18f8c3acbb85..4274128fbb4c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -519,7 +519,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
struct page *page = NULL;
if (pte_present(*pte)) {
- page = vm_normal_page(vma, addr, *pte);
+ page = vm_normal_any_page(vma, addr, *pte);
} else if (is_swap_pte(*pte)) {
swp_entry_t swpent = pte_to_swp_entry(*pte);
@@ -705,7 +705,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long hmask,
struct page *page = NULL;
if (pte_present(*pte)) {
- page = vm_normal_page(vma, addr, *pte);
+ page = vm_normal_any_page(vma, addr, *pte);
} else if (is_swap_pte(*pte)) {
swp_entry_t swpent = pte_to_swp_entry(*pte);
@@ -1059,7 +1059,7 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
return false;
if (likely(!test_bit(MMF_HAS_PINNED, >vm_mm->flags)))
return false;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_any_page(vma, addr, pte);
if (!page)
return false;
return page_maybe_dma_pinned(page);
@@ -1172,7 +1172,7 @@ static int clear_refs_pte_range(pmd_t *pmd, unsigned long
addr,
if (!pte_present(ptent))
continue;
- page = vm_normal_page(vma, addr, ptent);
+ page = vm_normal_any_page(vma, addr, ptent);
if (!page)
continue;
@@ -1383,7 +1383,7 @@ static pagemap_entry_t pte_to_pagemap_entry(struct pagemapread *pm,
if (pm->show_pfn)
frame = pte_pfn(pte);
flags |= PM_PRESENT;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_any_page(vma, addr, pte);
if (pte_soft_dirty(pte))
flags |= PM_SOFT_DIRTY;
if (pte_uffd_wp(pte))
@@ -1761,7 +1761,7 @@ static struct page *can_gather_numa_stats(pte_t pte,
struct vm_area_struct *vma,
if (!pte_present(pte))
return NULL;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_lru_page(vma, addr, pte);
if (!page)
return NULL;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ff9f149ca201..8c9f87151d93 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -593,8 +593,8 @@ struct vm_operations_struct {
unsigned long addr);
#endif
/*
-* Called by vm_normal_page() for special PTEs to find the
-* page for @addr. This is useful if the default behavior
+* Called by vm_normal_x_page() for special PTEs to find the
I'd use vm_normal_*_page in these comments to avoid confusion, because
vm_normal_x_page is actually a valid symbol name, which doesn't exist.
+* page for @addr. This is useful if the default behavior
* (using pte_page()) would not find the correct page.
*/
struct page *(*find_special_page)(struct vm_area_struct
RE: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
[AMD Official Use Only] > -Original Message- > From: Lukas Wunner > Sent: Monday, February 28, 2022 16:33 > To: Bjorn Helgaas > Cc: Limonciello, Mario ; Mika Westerberg > ; Michael Jamet > ; open list:PCI SUBSYSTEM p...@vger.kernel.org>; open list:THUNDERBOLT DRIVER u...@vger.kernel.org>; Yehezkel Bernat ; open > list:DRM DRIVERS ; open list:X86 > PLATFORM DRIVERS ; Andreas > Noever ; open list:RADEON and AMDGPU > DRM DRIVERS ; open list:DRM DRIVER FOR > NVIDIA GEFORCE/QUADRO GPUS ; Bjorn > Helgaas ; Deucher, Alexander > > Subject: Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI > core > > On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > > facing" > > > > > assumption above. Not having a Thunderbolt spec, I have no idea > how > > > > > you deal with that. > > > > > > > > You can download the spec here: > [...] > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > > Version 1.0.pdf". > > > > > > The spec has Host_Router_indication (bits 18-19) as meaning external > facing. > > > I'll respin the patch 3 for using that. > > > > Thanks, please include the spec citation when you do. And probably > > the URL, because it's not at all obvious how the casual reader would > > get from "is_thunderbolt" to a recent add-on to the USB4 spec. > > PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, > hence there's no connection between "is_thunderbolt" and the USB4 spec. > > It's a proprietary VSEC used by Intel and the only way to recognize > pre-USB4 Thunderbolt devices that I know of. Its ID is also > different from the DVSEC IDs given in the above-mentioned spec. > > Thanks, The USB4 DVSEC spec makes comments about DVSEC_ID of 0x8086 and also DVSEC VENDOR_ID of 0x8086. Is that not also present on the Intel TBT3 controllers? My interpretation of this (and Mika's comment) was that rather than looking at the Intel VSEC we should look at the USB4 DVSEC to detect the Intel TBT3 controllers.
Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 04:13:44PM -0600, Bjorn Helgaas wrote: > On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > > facing" > > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > > you deal with that. > > > > > > You can download the spec here: [...] > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > > Version 1.0.pdf". > > > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > > I'll respin the patch 3 for using that. > > Thanks, please include the spec citation when you do. And probably > the URL, because it's not at all obvious how the casual reader would > get from "is_thunderbolt" to a recent add-on to the USB4 spec. PCI_VSEC_ID_INTEL_TBT is not mentioned at all in the USB4 spec, hence there's no connection between "is_thunderbolt" and the USB4 spec. It's a proprietary VSEC used by Intel and the only way to recognize pre-USB4 Thunderbolt devices that I know of. Its ID is also different from the DVSEC IDs given in the above-mentioned spec. Thanks, Lukas
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 23:59 +0200, Mike Rapoport wrote:
>
> On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley <
> james.bottom...@hansenpartnership.com> wrote:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian König 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.
> > > >
> > > > Anybody have any ideas?
> > >
> > > I think we should look at the use cases why code is touching
> > > (pos) after the loop.
> > >
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > >
> > > list_for_each_entry(entry, head, member) {
> > > if (some_condition_checking(entry))
> > > break;
> > > }
> > > do_something_with(entry);
> >
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> >
> > if (list_entry_is_head(entry, head, member)) {
> >return with error;
> > }
> > do_somethin_with(entry);
> >
> > Suffice? The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
>
> Won't suffice because the end goal of this work is to limit scope of
> entry only to loop. Hence the need for additional variable.
Well, yes, but my objection is more to the size of churn than the
desire to do loop local. I'm not even sure loop local is possible,
because it's always annoyed me that for (int i = 0; ... in C++ defines
i in the outer scope not the loop scope, which is why I never use it.
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.
> Besides, there are no guarantees that people won't
> do_something_with(entry) without the check or won't compare entry to
> NULL to check if the loop finished with break or not.
I get the wider goal, but we have to patch the problem cases now and a
simple one-liner is better than a larger patch that may or may not work
if we ever achieve the local definition or value poisoning idea. I'm
also fairly certain coccinelle can come up with a use without checking
for loop completion semantic patch which we can add to 0day.
James
Re: [PATCH v2] drm/amdgpu: Fix realloc of ptr
Applied. Thanks!
Alex
On Mon, Feb 28, 2022 at 5:55 AM Christian König
wrote:
>
> Am 27.02.22 um 16:33 schrieb t...@redhat.com:
> > From: Tom Rix
> >
> > Clang static analysis reports this error
> > amdgpu_debugfs.c:1690:9: warning: 1st function call
> >argument is an uninitialized value
> >tmp = krealloc_array(tmp, i + 1,
> > ^~~
> >
> > realloc uses tmp, so tmp can not be garbage.
> > And the return needs to be checked.
> >
> > Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
> > Signed-off-by: Tom Rix
>
> Yeah, stuff I missed because of the long review. I was already wondering
> what semantics krealloc_array is following for freeing up the pointer on
> error.
>
> Reviewed-by: Christian König
>
> Thanks,
> Christian.
>
> > ---
> > v2:
> >use 'new' to hold/check the ralloc return
> >fix commit log mistake on ralloc freeing to using input ptr
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > index 9eb9b440bd438..2f4f8c5618d81 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> > @@ -1676,7 +1676,7 @@ static ssize_t
> > amdgpu_reset_dump_register_list_write(struct file *f,
> > {
> > struct amdgpu_device *adev = (struct amdgpu_device
> > *)file_inode(f)->i_private;
> > char reg_offset[11];
> > - uint32_t *tmp;
> > + uint32_t *new, *tmp = NULL;
> > int ret, i = 0, len = 0;
> >
> > do {
> > @@ -1687,7 +1687,12 @@ static ssize_t
> > amdgpu_reset_dump_register_list_write(struct file *f,
> > goto error_free;
> > }
> >
> > - tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t),
> > GFP_KERNEL);
> > + new = krealloc_array(tmp, i + 1, sizeof(uint32_t),
> > GFP_KERNEL);
> > + if (!new) {
> > + ret = -ENOMEM;
> > + goto error_free;
> > + }
> > + tmp = new;
> > if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
> > ret = -EINVAL;
> > goto error_free;
>
[PATCH] drm/amdgpu: Use IP versions in convert_tiling_flags_to_modifier()
Rather than checking the asic_type.
Signed-off-by: Alex Deucher
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 448e9b46417c..dd70c85b8205 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -691,9 +691,9 @@ static int convert_tiling_flags_to_modifier(struct
amdgpu_framebuffer *afb)
return -EINVAL;
}
- if (adev->asic_type >= CHIP_SIENNA_CICHLID)
+ if (adev->ip_versions[GC_HWIP][0] >= IP_VERSION(10, 3, 0))
version = AMD_FMT_MOD_TILE_VER_GFX10_RBPLUS;
- else if (adev->family == AMDGPU_FAMILY_NV)
+ else if (adev->ip_versions[GC_HWIP][0] >= IP_VERSION(10, 0, 0))
version = AMD_FMT_MOD_TILE_VER_GFX10;
else
version = AMD_FMT_MOD_TILE_VER_GFX9;
@@ -787,7 +787,7 @@ static int convert_tiling_flags_to_modifier(struct
amdgpu_framebuffer *afb)
if (adev->family >= AMDGPU_FAMILY_NV) {
int extra_pipe = 0;
- if (adev->asic_type >=
CHIP_SIENNA_CICHLID &&
+ if ((adev->ip_versions[GC_HWIP][0] >=
IP_VERSION(10, 3, 0)) &&
pipes == packers && pipes > 1)
extra_pipe = 1;
--
2.35.1
Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
On Mon, Feb 28, 2022 at 03:33:13PM +, Limonciello, Mario wrote: > [AMD Official Use Only] > > > > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > > facing" > > > assumption above. Not having a Thunderbolt spec, I have no idea how > > > you deal with that. > > > > You can download the spec here: > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > > w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202 > > 6.zipdata=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64 > > 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0 > > %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC > > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000& > > amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3Dre > > served=0 > > > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > > Version 1.0.pdf". > > The spec has Host_Router_indication (bits 18-19) as meaning external facing. > I'll respin the patch 3 for using that. Thanks, please include the spec citation when you do. And probably the URL, because it's not at all obvious how the casual reader would get from "is_thunderbolt" to a recent add-on to the USB4 spec. Bjorn
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:56, Christian König wrote:
>
>
>
> Am 28.02.22 um 21:42 schrieb James Bottomley:
>> On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>>> Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König
wrote:
[SNIP]
Anybody have any ideas?
>>> I think we should look at the use cases why code is touching (pos)
>>> after the loop.
>>>
>>> Just from skimming over the patches to change this and experience
>>> with the drivers/subsystems I help to maintain I think the primary
>>> pattern looks something like this:
>>>
>>> list_for_each_entry(entry, head, member) {
>>> if (some_condition_checking(entry))
>>> break;
>>> }
>>> do_something_with(entry);
There are other cases where the list iterator variable is used after the loop
Some examples:
- list_for_each_entry_continue() and list_for_each_entry_from().
- (although very rare) the head is actually of the correct struct type.
(ppc440spe_get_group_entry(): drivers/dma/ppc4xx/adma.c:1436)
- to use pos->list for example for list_add_tail():
(add_static_vm_early(): arch/arm/mm/ioremap.c:107)
If the scope of the list iterator is limited those still need fixing in a
different way.
>>
>> Actually, we usually have a check to see if the loop found anything,
>> but in that case it should something like
>>
>> if (list_entry_is_head(entry, head, member)) {
>> return with error;
>> }
>> do_somethin_with(entry);
>>
>> Suffice? The list_entry_is_head() macro is designed to cope with the
>> bogus entry on head problem.
>
> That will work and is also what people already do.
>
> The key problem is that we let people do the same thing over and over again
> with slightly different implementations.
>
> Out in the wild I've seen at least using a separate variable, using a bool to
> indicate that something was found and just assuming that the list has an
> entry.
>
> The last case is bogus and basically what can break badly.
>
> If we would have an unified macro which search for an entry combined with
> automated reporting on patches to use that macro I think the potential to
> introduce such issues will already go down massively without auditing tons of
> existing code.
Having a unified way to do the same thing would indeed be great.
>
> Regards,
> Christian.
>
>>
>> James
>>
>>
>
- Jakob
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:14:44PM -0800, Linus Torvalds wrote: > 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. The new "pos" has not yet been declared, so this has to refer to the outer "pos", it cannot be the inner one. Because it hasn't been declared yet :-) Compare this to typeof (pos) pos = pos; where that last "pos" *does* refer to the newly declared one: that declaration has already been done! (So this code is UB btw, 6.3.2.1/2). > 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". It is perfectly well-defined. Well, it would be good if we (GCC) would document it does work, and if someone tested it on LLVM as well. But it is really hard to implement it to *not* work :-) > 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... It is very pretty! Needs a comment though :-) Segher
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On February 28, 2022 10:42:53 PM GMT+02:00, James Bottomley
wrote:
>On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
>> Am 28.02.22 um 20:56 schrieb 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)
>>
>> Yes, completely agree.
>>
>> > 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?
>>
>> I think we should look at the use cases why code is touching (pos)
>> after the loop.
>>
>> Just from skimming over the patches to change this and experience
>> with the drivers/subsystems I help to maintain I think the primary
>> pattern looks something like this:
>>
>> list_for_each_entry(entry, head, member) {
>> if (some_condition_checking(entry))
>> break;
>> }
>> do_something_with(entry);
>
>
>Actually, we usually have a check to see if the loop found anything,
>but in that case it should something like
>
>if (list_entry_is_head(entry, head, member)) {
>return with error;
>}
>do_somethin_with(entry);
>
>Suffice? The list_entry_is_head() macro is designed to cope with the
>bogus entry on head problem.
Won't suffice because the end goal of this work is to limit scope of entry only
to loop. Hence the need for additional variable.
Besides, there are no guarantees that people won't do_something_with(entry)
without the check or won't compare entry to NULL to check if the loop finished
with break or not.
>James
--
Sincerely yours,
Mike
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 21:10, Linus Torvalds > wrote: > > 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. The goal of this is to get compiler warnings right? This would indeed be great. Changing the list_for_each_entry() macro first will break all of those cases (e.g. the ones using 'list_entry_is_head()). 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. > > Here - can somebody poke holes in this "work of art" patch? With this you are no longer able to set the 'outer' pos within the list iterator loop body or am I missing something? Like this it stays uninitialized but you'll probably want to set it from within the loop. You would then yet again need a variable with another name to use after the loop. I fail to see how this will make most of the changes in this patch obsolete (if that was the intention). > > Linus > - Jakob
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 3:45 PM James Bottomley
wrote:
> ...
> > Just from skimming over the patches to change this and experience
> > with the drivers/subsystems I help to maintain I think the primary
> > pattern looks something like this:
> >
> > list_for_each_entry(entry, head, member) {
> > if (some_condition_checking(entry))
> > break;
> > }
> > do_something_with(entry);
>
>
> Actually, we usually have a check to see if the loop found anything,
> but in that case it should something like
>
> if (list_entry_is_head(entry, head, member)) {
> return with error;
> }
> do_somethin_with(entry);
Borrowing from c++, perhaps an explicit end should be used:
if (list_entry_not_end(entry))
do_somethin_with(entry)
It is modelled after c++ and the 'while(begin != end) {}' pattern.
Jeff
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
> Am 28.02.22 um 20:56 schrieb 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)
>
> Yes, completely agree.
>
> > 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?
>
> I think we should look at the use cases why code is touching (pos)
> after the loop.
>
> Just from skimming over the patches to change this and experience
> with the drivers/subsystems I help to maintain I think the primary
> pattern looks something like this:
>
> list_for_each_entry(entry, head, member) {
> if (some_condition_checking(entry))
> break;
> }
> do_something_with(entry);
Actually, we usually have a check to see if the loop found anything,
but in that case it should something like
if (list_entry_is_head(entry, head, member)) {
return with error;
}
do_somethin_with(entry);
Suffice? The list_entry_is_head() macro is designed to cope with the
bogus entry on head problem.
James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, 2022-02-28 at 21:56 +0100, Christian König wrote:
>
> Am 28.02.22 um 21:42 schrieb James Bottomley:
> > On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
> > > Am 28.02.22 um 20:56 schrieb Linus Torvalds:
> > > > On Mon, Feb 28, 2022 at 4:19 AM Christian König
> > > > wrote:
> > > > [SNIP]
> > > > Anybody have any ideas?
> > > I think we should look at the use cases why code is touching
> > > (pos)
> > > after the loop.
> > >
> > > Just from skimming over the patches to change this and experience
> > > with the drivers/subsystems I help to maintain I think the
> > > primary pattern looks something like this:
> > >
> > > list_for_each_entry(entry, head, member) {
> > > if (some_condition_checking(entry))
> > > break;
> > > }
> > > do_something_with(entry);
> >
> > Actually, we usually have a check to see if the loop found
> > anything, but in that case it should something like
> >
> > if (list_entry_is_head(entry, head, member)) {
> > return with error;
> > }
> > do_somethin_with(entry);
> >
> > Suffice? The list_entry_is_head() macro is designed to cope with
> > the bogus entry on head problem.
>
> That will work and is also what people already do.
>
> The key problem is that we let people do the same thing over and
> over again with slightly different implementations.
>
> Out in the wild I've seen at least using a separate variable, using
> a bool to indicate that something was found and just assuming that
> the list has an entry.
>
> The last case is bogus and basically what can break badly.
Yes, I understand that. I'm saying we should replace that bogus checks
of entry->something against some_value loop termination condition with
the list_entry_is_head() macro. That should be a one line and fairly
mechanical change rather than the explosion of code changes we seem to
have in the patch series.
James
[PATCH 2/2] drm/amd/display: move FPU code from dcn10 to dml/dcn10 folder
FPU operations in dcn10 was already moved to dml folder via calcs code.
However, dcn1_0_ip and dcn_1_0_soc with FPU componentd remains on dcn10.
Following previous changes to isolate FPU, this patch creates dcn10_fpu
files to isolate FPU-specific code and moves those structs to it.
Signed-off-by: Melissa Wen
---
.../drm/amd/display/dc/dcn10/dcn10_resource.c | 62 -
.../drm/amd/display/dc/dcn10/dcn10_resource.h | 4 +
drivers/gpu/drm/amd/display/dc/dml/Makefile | 2 +
.../drm/amd/display/dc/dml/dcn10/dcn10_fpu.c | 124 ++
.../drm/amd/display/dc/dml/dcn10/dcn10_fpu.h | 30 +
5 files changed, 160 insertions(+), 62 deletions(-)
create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn10/dcn10_fpu.c
create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn10/dcn10_fpu.h
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
index 858b72149897..ac96242cc474 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
@@ -70,68 +70,6 @@
#include "dce/dce_aux.h"
#include "dce/dce_i2c.h"
-const struct _vcs_dpi_ip_params_st dcn1_0_ip = {
- .rob_buffer_size_kbytes = 64,
- .det_buffer_size_kbytes = 164,
- .dpte_buffer_size_in_pte_reqs_luma = 42,
- .dpp_output_buffer_pixels = 2560,
- .opp_output_buffer_lines = 1,
- .pixel_chunk_size_kbytes = 8,
- .pte_enable = 1,
- .pte_chunk_size_kbytes = 2,
- .meta_chunk_size_kbytes = 2,
- .writeback_chunk_size_kbytes = 2,
- .line_buffer_size_bits = 589824,
- .max_line_buffer_lines = 12,
- .IsLineBufferBppFixed = 0,
- .LineBufferFixedBpp = -1,
- .writeback_luma_buffer_size_kbytes = 12,
- .writeback_chroma_buffer_size_kbytes = 8,
- .max_num_dpp = 4,
- .max_num_wb = 2,
- .max_dchub_pscl_bw_pix_per_clk = 4,
- .max_pscl_lb_bw_pix_per_clk = 2,
- .max_lb_vscl_bw_pix_per_clk = 4,
- .max_vscl_hscl_bw_pix_per_clk = 4,
- .max_hscl_ratio = 4,
- .max_vscl_ratio = 4,
- .hscl_mults = 4,
- .vscl_mults = 4,
- .max_hscl_taps = 8,
- .max_vscl_taps = 8,
- .dispclk_ramp_margin_percent = 1,
- .underscan_factor = 1.10,
- .min_vblank_lines = 14,
- .dppclk_delay_subtotal = 90,
- .dispclk_delay_subtotal = 42,
- .dcfclk_cstate_latency = 10,
- .max_inter_dcn_tile_repeaters = 8,
- .can_vstartup_lines_exceed_vsync_plus_back_porch_lines_minus_one = 0,
- .bug_forcing_LC_req_same_size_fixed = 0,
-};
-
-const struct _vcs_dpi_soc_bounding_box_st dcn1_0_soc = {
- .sr_exit_time_us = 9.0,
- .sr_enter_plus_exit_time_us = 11.0,
- .urgent_latency_us = 4.0,
- .writeback_latency_us = 12.0,
- .ideal_dram_bw_after_urgent_percent = 80.0,
- .max_request_size_bytes = 256,
- .downspread_percent = 0.5,
- .dram_page_open_time_ns = 50.0,
- .dram_rw_turnaround_time_ns = 17.5,
- .dram_return_buffer_per_channel_bytes = 8192,
- .round_trip_ping_latency_dcfclk_cycles = 128,
- .urgent_out_of_order_return_per_channel_bytes = 256,
- .channel_interleave_bytes = 256,
- .num_banks = 8,
- .num_chans = 2,
- .vmm_page_size_bytes = 4096,
- .dram_clock_change_latency_us = 17.0,
- .writeback_dram_clock_change_latency_us = 23.0,
- .return_bus_width_bytes = 64,
-};
-
#ifndef mmDP0_DP_DPHY_INTERNAL_CTRL
#define mmDP0_DP_DPHY_INTERNAL_CTRL 0x210f
#define mmDP0_DP_DPHY_INTERNAL_CTRL_BASE_IDX2
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.h
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.h
index 633025ccb870..bf8e33cd8147 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.h
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.h
@@ -27,6 +27,7 @@
#define __DC_RESOURCE_DCN10_H__
#include "core_types.h"
+#include "dml/dcn10/dcn10_fpu.h"
#define TO_DCN10_RES_POOL(pool)\
container_of(pool, struct dcn10_resource_pool, base)
@@ -35,6 +36,9 @@ struct dc;
struct resource_pool;
struct _vcs_dpi_display_pipe_params_st;
+extern struct _vcs_dpi_ip_params_st dcn1_0_ip;
+extern struct _vcs_dpi_soc_bounding_box_st dcn1_0_soc;
+
struct dcn10_resource_pool {
struct resource_pool base;
};
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index b16c492593e2..6b7f8b62a56f 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -58,6 +58,7 @@ CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o :=
$(dml_ccflags)
ifdef CONFIG_DRM_AMD_DC_DCN
CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_ccflags)
+CFLAGS_$(AMDDALPATH)/dc/dml/dcn10/dcn10_fpu.o := $(dml_ccflags)
CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/dcn20_fpu.o := $(dml_ccflags)
[PATCH 1/2] drm/amd/display: move FPU operations from dcn21 to dml/dcn20 folder
dml/dcn20_fpu file centralizes all DCN2x functions that require FPU access.
Therefore, this patch moves FPU-related code from dcn21 to dcn20_fpu. These
include:
- dcn21_populate_dml_pipes_from_context()
- dcn21_validate_bandwidth_fp() and related:
- dcn21_calculate_wm(),
- patch_bounding_box(),
- calculate_wm_set_for_vlevel()
- renaming update_bw_bounding_box() to dcn21_update_bw_bounding_box(), move
to dcn20_fpu with related static function construct_low_pstate_lvl()
Also, make dcn21_fast_validate_bw() public in dcn21_resource as it is called
by dcn21_validate_bandwidth_fp() now in dcn20_fpu.
Reuse dcn20_fpu_adjust_dppclk() in dcn21_fast_validate_bw() as it isolates
the same FPU operation.
Include dchubbub.h as it is required in dcn21_populate_dml_pipes_from_context()
Signed-off-by: Melissa Wen
---
drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 25 -
.../drm/amd/display/dc/dcn21/dcn21_resource.c | 566 +-
.../drm/amd/display/dc/dcn21/dcn21_resource.h | 11 +
.../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 538 -
.../drm/amd/display/dc/dml/dcn20/dcn20_fpu.h | 9 +
5 files changed, 571 insertions(+), 578 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
index bb8c95141082..0dc06e428999 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
@@ -5,31 +5,6 @@
DCN21 = dcn21_init.o dcn21_hubp.o dcn21_hubbub.o dcn21_resource.o \
dcn21_hwseq.o dcn21_link_encoder.o dcn21_dccg.o
-ifdef CONFIG_X86
-CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
-endif
-
-ifdef CONFIG_PPC64
-CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -maltivec
-endif
-
-ifdef CONFIG_CC_IS_GCC
-ifeq ($(call cc-ifversion, -lt, 0701, y), y)
-IS_OLD_GCC = 1
-endif
-endif
-
-ifdef CONFIG_X86
-ifdef IS_OLD_GCC
-# Stack alignment mismatch, proceed with caution.
-# GCC < 7.1 cannot compile code using `double` and -mpreferred-stack-boundary=3
-# (8B stack alignment).
-CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -mpreferred-stack-boundary=4
-else
-CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -msse2
-endif
-endif
-
AMD_DAL_DCN21 = $(addprefix $(AMDDALPATH)/dc/dcn21/,$(DCN21))
AMD_DISPLAY_FILES += $(AMD_DAL_DCN21)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
index c1cd1a8ff1d7..612732656772 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn21/dcn21_resource.c
@@ -34,6 +34,7 @@
#include "resource.h"
#include "include/irq_service_interface.h"
#include "dcn20/dcn20_resource.h"
+#include "dcn21/dcn21_resource.h"
#include "dml/dcn20/dcn20_fpu.h"
@@ -89,230 +90,6 @@
#include "dce/dmub_psr.h"
#include "dce/dmub_abm.h"
-#define DC_LOGGER_INIT(logger)
-
-
-struct _vcs_dpi_ip_params_st dcn2_1_ip = {
- .odm_capable = 1,
- .gpuvm_enable = 1,
- .hostvm_enable = 1,
- .gpuvm_max_page_table_levels = 1,
- .hostvm_max_page_table_levels = 4,
- .hostvm_cached_page_table_levels = 2,
- .num_dsc = 3,
- .rob_buffer_size_kbytes = 168,
- .det_buffer_size_kbytes = 164,
- .dpte_buffer_size_in_pte_reqs_luma = 44,
- .dpte_buffer_size_in_pte_reqs_chroma = 42,//todo
- .dpp_output_buffer_pixels = 2560,
- .opp_output_buffer_lines = 1,
- .pixel_chunk_size_kbytes = 8,
- .pte_enable = 1,
- .max_page_table_levels = 4,
- .pte_chunk_size_kbytes = 2,
- .meta_chunk_size_kbytes = 2,
- .min_meta_chunk_size_bytes = 256,
- .writeback_chunk_size_kbytes = 2,
- .line_buffer_size_bits = 789504,
- .is_line_buffer_bpp_fixed = 0,
- .line_buffer_fixed_bpp = 0,
- .dcc_supported = true,
- .max_line_buffer_lines = 12,
- .writeback_luma_buffer_size_kbytes = 12,
- .writeback_chroma_buffer_size_kbytes = 8,
- .writeback_chroma_line_buffer_width_pixels = 4,
- .writeback_max_hscl_ratio = 1,
- .writeback_max_vscl_ratio = 1,
- .writeback_min_hscl_ratio = 1,
- .writeback_min_vscl_ratio = 1,
- .writeback_max_hscl_taps = 12,
- .writeback_max_vscl_taps = 12,
- .writeback_line_buffer_luma_buffer_size = 0,
- .writeback_line_buffer_chroma_buffer_size = 14643,
- .cursor_buffer_size = 8,
- .cursor_chunk_size = 2,
- .max_num_otg = 4,
- .max_num_dpp = 4,
- .max_num_wb = 1,
- .max_dchub_pscl_bw_pix_per_clk = 4,
- .max_pscl_lb_bw_pix_per_clk = 2,
- .max_lb_vscl_bw_pix_per_clk = 4,
- .max_vscl_hscl_bw_pix_per_clk = 4,
- .max_hscl_ratio = 4,
- .max_vscl_ratio = 4,
- .hscl_mults = 4,
- .vscl_mults = 4,
- .max_hscl_taps = 8,
- .max_vscl_taps = 8,
- .dispclk_ramp_margin_percent = 1,
- .underscan_factor = 1.10,
- .min_vblank_lines = 32, //
[PATCH 0/2] isolate FPU code from dcn10 and dcn21 to dml folder
Continuing the work of isolating FPU code from DCN drivers, this patchset moves FPU-specific operations from dcn10 and dcn21 to dml folder. I move FPU code from dcn21 to dml/dcn20_fpu since there is a documentation in dcn20_fpu.c that states dcn20_fpu centralizes: `all DCN20 and DCN2.1 (DCN2x) functions that require FPU access` Also, there isn't a dcn10_fpu in dml/dcn10 folder, therefore, I create related files to isolate FPU structs there. This patchset depends on previous patch to isolate FPU code from dcn20 driver: https://patchwork.freedesktop.org/series/100487/ Melissa Wen (2): drm/amd/display: move FPU operations from dcn21 to dml/dcn20 folder drm/amd/display: move FPU code from dcn10 to dml/dcn10 folder .../drm/amd/display/dc/dcn10/dcn10_resource.c | 62 -- .../drm/amd/display/dc/dcn10/dcn10_resource.h | 4 + drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 25 - .../drm/amd/display/dc/dcn21/dcn21_resource.c | 566 +- .../drm/amd/display/dc/dcn21/dcn21_resource.h | 11 + drivers/gpu/drm/amd/display/dc/dml/Makefile | 2 + .../drm/amd/display/dc/dml/dcn10/dcn10_fpu.c | 124 .../drm/amd/display/dc/dml/dcn10/dcn10_fpu.h | 30 + .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 538 - .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.h | 9 + 10 files changed, 731 insertions(+), 640 deletions(-) create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn10/dcn10_fpu.c create mode 100644 drivers/gpu/drm/amd/display/dc/dml/dcn10/dcn10_fpu.h -- 2.34.1
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 21:42 schrieb James Bottomley:
On Mon, 2022-02-28 at 21:07 +0100, Christian König wrote:
Am 28.02.22 um 20:56 schrieb Linus Torvalds:
On Mon, Feb 28, 2022 at 4:19 AM Christian König
wrote:
[SNIP]
Anybody have any ideas?
I think we should look at the use cases why code is touching (pos)
after the loop.
Just from skimming over the patches to change this and experience
with the drivers/subsystems I help to maintain I think the primary
pattern looks something like this:
list_for_each_entry(entry, head, member) {
if (some_condition_checking(entry))
break;
}
do_something_with(entry);
Actually, we usually have a check to see if the loop found anything,
but in that case it should something like
if (list_entry_is_head(entry, head, member)) {
return with error;
}
do_somethin_with(entry);
Suffice? The list_entry_is_head() macro is designed to cope with the
bogus entry on head problem.
That will work and is also what people already do.
The key problem is that we let people do the same thing over and over
again with slightly different implementations.
Out in the wild I've seen at least using a separate variable, using a
bool to indicate that something was found and just assuming that the
list has an entry.
The last case is bogus and basically what can break badly.
If we would have an unified macro which search for an entry combined
with automated reporting on patches to use that macro I think the
potential to introduce such issues will already go down massively
without auditing tons of existing code.
Regards,
Christian.
James
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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
On Mon, 2022-02-28 at 20:16 +, Matthew Wilcox wrote: > On Mon, Feb 28, 2022 at 12:10:24PM -0800, 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. > > Then we can never use -Wshadow ;-( I'd love to be able to turn it on; > it catches real bugs. > I was just going to say the same thing... 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? johannes
[PATCH] mm: split vm_normal_pages for LRU and non-LRU handling
DEVICE_COHERENT pages introduce a subtle distinction in the way
"normal" pages can be used by various callers throughout the kernel.
They behave like normal pages for purposes of mapping in CPU page
tables, and for COW. But they do not support LRU lists, NUMA
migration or THP. Therefore we split vm_normal_page into two
functions vm_normal_any_page and vm_normal_lru_page. The latter will
only return pages that can be put on an LRU list and that support
NUMA migration and THP.
We also introduced a FOLL_LRU flag that adds the same behaviour to
follow_page and related APIs, to allow callers to specify that they
expect to put pages on an LRU list.
Signed-off-by: Alex Sierra
---
fs/proc/task_mmu.c | 12 +-
include/linux/mm.h | 53 -
mm/gup.c| 10 +
mm/hmm.c| 2 +-
mm/huge_memory.c| 2 +-
mm/khugepaged.c | 8 +++
mm/ksm.c| 4 ++--
mm/madvise.c| 4 ++--
mm/memcontrol.c | 2 +-
mm/memory.c | 38 ++--
mm/mempolicy.c | 4 ++--
mm/migrate.c| 2 +-
mm/migrate_device.c | 2 +-
mm/mlock.c | 6 ++---
mm/mprotect.c | 2 +-
15 files changed, 85 insertions(+), 66 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 18f8c3acbb85..4274128fbb4c 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -519,7 +519,7 @@ static void smaps_pte_entry(pte_t *pte, unsigned long addr,
struct page *page = NULL;
if (pte_present(*pte)) {
- page = vm_normal_page(vma, addr, *pte);
+ page = vm_normal_any_page(vma, addr, *pte);
} else if (is_swap_pte(*pte)) {
swp_entry_t swpent = pte_to_swp_entry(*pte);
@@ -705,7 +705,7 @@ static int smaps_hugetlb_range(pte_t *pte, unsigned long
hmask,
struct page *page = NULL;
if (pte_present(*pte)) {
- page = vm_normal_page(vma, addr, *pte);
+ page = vm_normal_any_page(vma, addr, *pte);
} else if (is_swap_pte(*pte)) {
swp_entry_t swpent = pte_to_swp_entry(*pte);
@@ -1059,7 +1059,7 @@ static inline bool pte_is_pinned(struct vm_area_struct
*vma, unsigned long addr,
return false;
if (likely(!test_bit(MMF_HAS_PINNED, >vm_mm->flags)))
return false;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_any_page(vma, addr, pte);
if (!page)
return false;
return page_maybe_dma_pinned(page);
@@ -1172,7 +1172,7 @@ static int clear_refs_pte_range(pmd_t *pmd, unsigned long
addr,
if (!pte_present(ptent))
continue;
- page = vm_normal_page(vma, addr, ptent);
+ page = vm_normal_any_page(vma, addr, ptent);
if (!page)
continue;
@@ -1383,7 +1383,7 @@ static pagemap_entry_t pte_to_pagemap_entry(struct
pagemapread *pm,
if (pm->show_pfn)
frame = pte_pfn(pte);
flags |= PM_PRESENT;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_any_page(vma, addr, pte);
if (pte_soft_dirty(pte))
flags |= PM_SOFT_DIRTY;
if (pte_uffd_wp(pte))
@@ -1761,7 +1761,7 @@ static struct page *can_gather_numa_stats(pte_t pte,
struct vm_area_struct *vma,
if (!pte_present(pte))
return NULL;
- page = vm_normal_page(vma, addr, pte);
+ page = vm_normal_lru_page(vma, addr, pte);
if (!page)
return NULL;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ff9f149ca201..8c9f87151d93 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -593,8 +593,8 @@ struct vm_operations_struct {
unsigned long addr);
#endif
/*
-* Called by vm_normal_page() for special PTEs to find the
-* page for @addr. This is useful if the default behavior
+* Called by vm_normal_x_page() for special PTEs to find the
+* page for @addr. This is useful if the default behavior
* (using pte_page()) would not find the correct page.
*/
struct page *(*find_special_page)(struct vm_area_struct *vma,
@@ -1781,7 +1781,9 @@ static inline bool can_do_mlock(void) { return false; }
extern int user_shm_lock(size_t, struct ucounts *);
extern void user_shm_unlock(size_t, struct ucounts *);
-struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
+struct page *vm_normal_any_page(struct vm_area_struct *vma, unsigned long addr,
+pte_t pte);
+struct page *vm_normal_lru_page(struct vm_area_struct *vma, unsigned long addr,
pte_t pte);
struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr,
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:10:24PM -0800, 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. Then we can never use -Wshadow ;-( I'd love to be able to turn it on; it catches real bugs. > +#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
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
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
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
Am 28.02.22 um 20:56 schrieb 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)
Yes, completely agree.
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?
I think we should look at the use cases why code is touching (pos) after
the loop.
Just from skimming over the patches to change this and experience with
the drivers/subsystems I help to maintain I think the primary pattern
looks something like this:
list_for_each_entry(entry, head, member) {
if (some_condition_checking(entry))
break;
}
do_something_with(entry);
So the solution should probably not be to change all those use cases to
use more temporary variables, but rather to add a list_find_entry(...,
condition) macro and consistently use that one instead.
Regards,
Christian.
Linus
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
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: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > a multi-line indent gets curly braces for readability even though > it's not required by C. And then both sides would get curly braces. That's more your personal preference than a coding style guideline.
Re: [PATCH v2] drm/amdgpu: check vm ready by amdgpu_vm->evicting flag
https://gitlab.freedesktop.org/drm/amd/-/issues/1922 2022-03-01 1:26 GMT+08:00, youling257 : > this patch cause suspend to disk resume amdgpu hang, > [drm:amdgpu_job_timedout [amdgpu]] *ERROR* ring gfx timeout, signaled > seq=8330, emitted seq=8332 > > https://gitlab.freedesktop.org/drm/amd/-/issues/1915 >
Re: [PATCH v2] drm/amdgpu: check vm ready by amdgpu_vm->evicting flag
this patch cause suspend to disk resume amdgpu hang, [drm:amdgpu_job_timedout [amdgpu]] *ERROR* ring gfx timeout, signaled seq=8330, emitted seq=8332 https://gitlab.freedesktop.org/drm/amd/-/issues/1915
[PATCH] drm/amdgpu: Set correct DMA mask for aldebaran
Aldebaran has 48-bit physical address support
Signed-off-by: Harish Kasiviswanathan
---
drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 412e44af1608..0765c163b355 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -1675,12 +1675,17 @@ static int gmc_v9_0_sw_init(void *handle)
*/
adev->gmc.mc_mask = 0xULL; /* 48 bit MC */
- r = dma_set_mask_and_coherent(adev->dev, DMA_BIT_MASK(44));
+ if (adev->ip_versions[GC_HWIP][0] == IP_VERSION(9, 4, 2)) {
+ r = dma_set_mask_and_coherent(adev->dev, DMA_BIT_MASK(48));
+ adev->need_swiotlb = drm_need_swiotlb(48);
+ } else {
+ r = dma_set_mask_and_coherent(adev->dev, DMA_BIT_MASK(44));
+ adev->need_swiotlb = drm_need_swiotlb(44);
+ }
if (r) {
printk(KERN_WARNING "amdgpu: No suitable DMA available.\n");
return r;
}
- adev->need_swiotlb = drm_need_swiotlb(44);
r = gmc_v9_0_mc_init(adev);
if (r)
--
2.25.1
Re: [pull] amdgpu, amdkfd drm-next-5.18
On Mon, Feb 28, 2022 at 1:55 AM Dave Airlie wrote:
>
> On Sat, 26 Feb 2022 at 04:35, Alex Deucher wrote:
> >
> > Hi Dave, Daniel,
> >
> > New stuff for 5.18.
> >
> > The following changes since commit b63c54d978236dd6014cf2ffba96d626e97c915c:
> >
> > drm/amdkfd: Use proper enum in pm_unmap_queues_v9() (2022-02-17 15:59:06
> > -0500)
> >
> > are available in the Git repository at:
> >
> > https://gitlab.freedesktop.org/agd5f/linux.git
> > tags/amd-drm-next-5.18-2022-02-25
> >
> > for you to fetch changes up to 111aeed25ec6bf4d5b4a7b4cb5654f002ba9f795:
> >
> > drm/amdgpu: add gfxoff support for smu 13.0.5 (2022-02-25 11:51:18 -0500)
> >
> >
> > amd-drm-next-5.18-2022-02-25:
> >
> > amdgpu:
> > - Raven2 suspend/resume fix
> > - SDMA 5.2.6 updates
> > - VCN 3.1.2 updates
> > - SMU 13.0.5 updates
> > - DCN 3.1.5 updates
> > - Virtual display fixes
> > - SMU code cleanup
> > - Harvest fixes
> > - Expose benchmark tests via debugfs
> > - Drop no longer relevant gart aperture tests
> > - More RAS restructuring
> > - W=1 fixes
> > - PSR rework
> > - DP/VGA adapter fixes
> > - DP MST fixes
> > - GPUVM eviction fix
> > - GPU reset debugfs register dumping support
>
> (this time keeping cc).
>
> ^ this seems to conflict with the removal of reset_sem or something in
> that area.
>
> Can you trial merge this to drm-next and send a fixup patch I should
> apply with it?
reset_sem moved from adev->reset_sem to adev->reset_domain->sem. See
the attached diff. I also pushed a sample merge if that is helpful:
https://gitlab.freedesktop.org/agd5f/linux/-/commits/drm-next-amd-drm-next-merge-5.18
Thanks!
Alex
>
> Dave.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index dfeed6410f79..426b63e4f1f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1651,23 +1651,23 @@ static ssize_t amdgpu_reset_dump_register_list_read(struct file *f,
return 0;
memset(reg_offset, 0, 12);
- ret = down_read_killable(>reset_sem);
+ ret = down_read_killable(>reset_domain->sem);
if (ret)
return ret;
for (i = 0; i < adev->num_regs; i++) {
sprintf(reg_offset, "0x%x\n", adev->reset_dump_reg_list[i]);
- up_read(>reset_sem);
+ up_read(>reset_domain->sem);
if (copy_to_user(buf + len, reg_offset, strlen(reg_offset)))
return -EFAULT;
len += strlen(reg_offset);
- ret = down_read_killable(>reset_sem);
+ ret = down_read_killable(>reset_domain->sem);
if (ret)
return ret;
}
- up_read(>reset_sem);
+ up_read(>reset_domain->sem);
*pos += len;
return len;
@@ -1699,13 +1699,13 @@ static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
i++;
} while (len < size);
- ret = down_write_killable(>reset_sem);
+ ret = down_write_killable(>reset_domain->sem);
if (ret)
goto error_free;
swap(adev->reset_dump_reg_list, tmp);
adev->num_regs = i;
- up_write(>reset_sem);
+ up_write(>reset_domain->sem);
ret = size;
error_free:
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5396a9f1865f..ca854626a108 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4721,7 +4721,7 @@ static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev)
uint32_t reg_value;
int i;
- lockdep_assert_held(>reset_sem);
+ lockdep_assert_held(>reset_domain->sem);
dump_stack();
for (i = 0; i < adev->num_regs; i++) {
RE: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
[AMD Official Use Only] > > On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external- > facing" > > assumption above. Not having a Thunderbolt spec, I have no idea how > > you deal with that. > > You can download the spec here: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > w.usb.org%2Fsites%2Fdefault%2Ffiles%2FUSB4%2520Specification%2520202 > 6.zipdata=04%7C01%7Cmario.limonciello%40amd.com%7Ca26e64 > 7a4acf4e7681d308d9faa358fd%7C3dd8961fe4884e608e11a82d994e183d%7C0 > %7C0%7C637816402472428689%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC > 4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000& > amp;sdata=HSDqx%2BHzSnczTZxaBij8sgqvJSS8ajtjCzZd2CPSbR4%3Dre > served=0 > > Inside the archive there is also the DVSEC spec with name "USB4 DVSEC > Version 1.0.pdf". The spec has Host_Router_indication (bits 18-19) as meaning external facing. I'll respin the patch 3 for using that.
RE: [drm/selftests] 39ec47bbfd: kernel_BUG_at_drivers/gpu/drm/drm_buddy.c
[AMD Official Use Only]
Hi Christian,
I will check
Thanks,
Arun
-Original Message-
From: Koenig, Christian
Sent: Monday, February 28, 2022 4:29 PM
To: kernel test robot ; Paneer Selvam, Arunpravin
Cc: 0day robot ; Matthew Auld ; LKML
; l...@lists.01.org;
dri-de...@lists.freedesktop.org; intel-...@lists.freedesktop.org;
amd-gfx@lists.freedesktop.org; tzimmerm...@suse.de; Deucher, Alexander
Subject: Re: [drm/selftests] 39ec47bbfd:
kernel_BUG_at_drivers/gpu/drm/drm_buddy.c
Arun can you take a look at that one here?
It looks like a real problem to me and not just a potential false negative like
the other issue.
Thanks,
Christian.
Am 27.02.22 um 16:18 schrieb kernel test robot:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 39ec47bbfd5dd3cea0b711ee9f1acdca37399c86 ("[PATCH v2 2/7]
> drm/selftests: add drm buddy alloc limit testcase")
> url:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2F0day-ci%2Flinux%2Fcommits%2FArunpravin%2Fdrm-selftests-Move-i
> 915-buddy-selftests-into-drm%2F20220223-015043data=04%7C01%7Cchri
> stian.koenig%40amd.com%7C3101ff318a994e6eaf5f08d9fa0481ea%7C3dd8961fe4
> 884e608e11a82d994e183d%7C0%7C0%7C637815719552700496%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C3000sdata=sKvsDtHufRMfSO14HdmHxvNsJiPyDZVDXCFUpWTDwFI%3D
> ;reserved=0 patch link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> .kernel.org%2Fdri-devel%2F20220222174845.2175-2-Arunpravin.PaneerSelva
> m%40amd.comdata=04%7C01%7Cchristian.koenig%40amd.com%7C3101ff318a
> 994e6eaf5f08d9fa0481ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63
> 7815719552700496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV
> 2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=aWG4x27aMLcOySO
> UkHbLQ1NL9L8t8AF4dgXux65IIP8%3Dreserved=0
>
> in testcase: boot
>
> on test machine: qemu-system-x86_64 -enable-kvm -cpu Icelake-Server
> -smp 4 -m 16G
>
> caused below changes (please refer to attached dmesg/kmsg for entire
> log/backtrace):
>
>
> +---+++
> | | be9e8c6c00 |
> | 39ec47bbfd |
> +---+++
> | boot_successes| 14 | 0
> |
> | boot_failures | 0 | 16
> |
> | UBSAN:shift-out-of-bounds_in_include/linux/log2.h | 0 | 16
> |
> | kernel_BUG_at_drivers/gpu/drm/drm_buddy.c | 0 | 16
> |
> | invalid_opcode:#[##] | 0 | 16
> |
> | EIP:drm_buddy_init| 0 | 16
> |
> | Kernel_panic-not_syncing:Fatal_exception | 0 | 16
> |
> +---+++
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot
>
>
> [ 68.124177][T1] UBSAN: shift-out-of-bounds in
> include/linux/log2.h:67:13
> [ 68.125333][T1] shift exponent 4294967295 is too large for 32-bit type
> 'long unsigned int'
> [ 68.126563][T1] CPU: 0 PID: 1 Comm: swapper Not tainted
> 5.17.0-rc2-00311-g39ec47bbfd5d #2
> [ 68.127758][T1] Call Trace:
> [ 68.128187][ T1] dump_stack_lvl (lib/dump_stack.c:108) [ 68.128793][
> T1] dump_stack (lib/dump_stack.c:114) [ 68.129331][ T1] ubsan_epilogue
> (lib/ubsan.c:152) [ 68.129958][ T1]
> __ubsan_handle_shift_out_of_bounds.cold
> (arch/x86/include/asm/smap.h:85) [ 68.130791][ T1] ?
> drm_block_alloc+0x28/0x80 [ 68.131582][ T1] ? rcu_read_lock_sched_held
> (kernel/rcu/update.c:125) [ 68.132215][ T1] ? kmem_cache_alloc
> (include/trace/events/kmem.h:54 mm/slab.c:3501) [ 68.132878][ T1] ?
> mark_free+0x2e/0x80 [ 68.133524][ T1] drm_buddy_init.cold
> (include/linux/log2.h:67 drivers/gpu/drm/drm_buddy.c:131) [
> 68.134145][ T1] ? test_drm_cmdline_init
> (drivers/gpu/drm/selftests/test-drm_buddy.c:87)
> [ 68.134770][ T1] igt_buddy_alloc_limit
> (drivers/gpu/drm/selftests/test-drm_buddy.c:30)
> [ 68.135472][ T1] ? vprintk_default (kernel/printk/printk.c:2257) [
> 68.136057][ T1] ? test_drm_cmdline_init
> (drivers/gpu/drm/selftests/test-drm_buddy.c:87)
> [ 68.136812][ T1] test_drm_buddy_init
> (drivers/gpu/drm/selftests/drm_selftest.c:77
> drivers/gpu/drm/selftests/test-drm_buddy.c:95)
> [ 68.137475][ T1] do_one_initcall (init/main.c:1300) [ 68.138111][ T1]
> ? parse_args (kernel/params.c:609 kernel/params.c:146
> kernel/params.c:188) [ 68.138717][ T1] do_basic_setup
> (init/main.c:1372 init/main.c:1389 init/main.c:1408) [ 68.139366][ T1]
> kernel_init_freeable (init/main.c:1617) [ 68.140040][ T1] ? rest_init
> (init/main.c:1494) [ 68.140634][ T1] kernel_init (init/main.c:1504) [
> 68.141155][ T1]
Re: kernel amdgpu problems
i dont have currently a setup with non as module compiled but when i do, im sure it tries to load em right, they were just not in the initrd ... ill check when i be on it, somewhen not now cheers On Mon, Feb 28, 2022 at 12:50 PM Christian König < ckoenig.leichtzumer...@gmail.com> wrote: > Am 28.02.22 um 07:43 schrieb Alex fxmbsw7 Ratchev: > > > > On Mon, Feb 28, 2022 at 12:22 AM Mike Lothian wrote: > >> On Sat, 26 Feb 2022 at 10:01, Alex fxmbsw7 Ratchev >> wrote: >> > >> > i have observed at least two major problems of using amdgpu >> > >> > 1. cant be built-in instead of module, display stays blank >> >> Are you remembering to add in the firmware to the kernel image too? >> > > i copied all from official linux-firmware.gif to /lib/firmware > > > That's not sufficient, you need to tell the kernel explicitly which > firmware will be needed. > > In other words try mentioning everything in /lib/firmware/amdgpu as > CONFIG_EXTRA_FIRMWARE. > > Regards, > Christian. > > >> There's a good guide at >> https://wiki.gentoo.org/wiki/AMDGPU#Known_firmware_blobs >> >> I've successfully used AMDGPU builtin on Tonga, Raven, Renoir and Navy >> Flounder systems >> >> I've never used kexec so have no suggestions for that >> > > it may be back then when i compiled it inline not as module i didnt > include firmware in the initrd so that may been that > > about kexec, i dunno, X dri doesnt work after first kexec, after second > screen at all is blank > > driver init problems .. > > cheers.. > > >
RE: [PATCH 00/14] DC Patches Feburary 26, 2022
[Public] Hi all, This week this patchset was tested on the following systems: Lenovo Thinkpad T14s Gen2 with AMD Ryzen 5 5650U, with the following display types: eDP 1080p 60hz, 4k 60hz (via USB-C to DP/HDMI), 1440p 144hz (via USB-C to DP/HDMI), 1680*1050 60hz (via USB-C to DP and then DP to DVI/VGA) Sapphire Pulse RX5700XT with the following display types: 4k 60hz (via DP/HDMI), 1440p 144hz (via DP/HDMI), 1680*1050 60hz (via DP to DVI/VGA) Reference AMD RX6800 with the following display types: 4k 60hz (via DP/HDMI and USB-C to DP/HDMI), 1440p 144hz (via USB-C to DP/HDMI and USB-C to DP/HDMI), 1680*1050 60hz (via DP to DVI/VGA) Included testing using a Startech DP 1.4 MST hub at 2x 4k 60hz, and 3x 1080p 60hz on all systems. Also tested DSC via USB-C to DP DSC Hub with 3x 4k 60hz on Ryzen 9 5900h and Ryzen 5 4500u. Tested on Ubuntu 20.04.3 with Kernel Version 5.13 and ChromeOS Tested-by: Daniel Wheeler Thank you, Dan Wheeler Technologist | AMD SW Display -- 1 Commerce Valley Dr E, Thornhill, ON L3T 7X6 Facebook | Twitter | amd.com -Original Message- From: amd-gfx On Behalf Of Alan Liu Sent: February 26, 2022 6:41 PM To: amd-gfx@lists.freedesktop.org Cc: Wang, Chao-kai (Stylon) ; Liu, HaoPing (Alan) ; Li, Sun peng (Leo) ; Wentland, Harry ; Zhuo, Qingqing (Lillian) ; Siqueira, Rodrigo ; Li, Roman ; Chiu, Solomon ; Pillai, Aurabindo ; Lin, Wayne ; Lakha, Bhawanpreet ; Gutierrez, Agustin ; Kotarac, Pavle Subject: [PATCH 00/14] DC Patches Feburary 26, 2022 This DC patchset brings improvements in multiple areas. In summary, we have: * Removing legacy invalid code. * Fixes in DC, DCE110. * Enhancements in DMUB. * Improvements on DCN10, DCN31. * Firmware promotion. drm/amd/display: 3.2.175 This version brings along following fixes: - Remove invalid RDPCS Programming in DAL - Make functional resource functions non-static - Reset VIC if HDMI_VIC is present - Add frame alternate 3D & restrict HW packed on dongles - Reg to turn on/off PSR Power seq in FSM - Modify plane removal sequence to avoid hangs - Pass HostVM enable flag into DCN3.1 DML - DC Validation failures - Program OPP before ODM - Refactor fixed VS w/a for PHY tests - Pass deep sleep disabled allow info to dmub fw - Refine the EDID override - [FW Promotion] Release 0.0.106.0 - Add verify_link_cap back for hdmi Anthony Koo (1): drm/amd/display: [FW Promotion] Release 0.0.106.0 Aric Cyr (1): drm/amd/display: 3.2.175 Charlene Liu (1): drm/amd/display: add verify_link_cap back for hdmi Chris Park (1): drm/amd/display: Reset VIC if HDMI_VIC is present Dillon Varone (2): drm/amd/display: Add frame alternate 3D & restrict HW packed on dongles drm/amd/display: Modify plane removal sequence to avoid hangs. Hansen Dsouza (1): drm/amd/display: Remove invalid RDPCS Programming in DAL Michael Strauss (1): drm/amd/display: Pass HostVM enable flag into DCN3.1 DML Nicholas Kazlauskas (1): drm/amd/display: Make functional resource functions non-static Robin Chen (1): drm/amd/display: Pass deep sleep disabled allow info to dmub fw Shah, Dharati (1): drm/amd/display: Adding a dc_debug option and dmub setting to use PHY FSM for PSR Shen, George (1): drm/amd/display: Refactor fixed VS w/a for PHY tests Wesley Chalmers (1): drm/amd/display: Program OPP before ODM jinzh (1): drm/amd/display: refine the EDID override drivers/gpu/drm/amd/display/dc/core/dc_link.c | 14 +- .../gpu/drm/amd/display/dc/core/dc_link_dp.c | 16 -- .../gpu/drm/amd/display/dc/core/dc_resource.c | 2 + drivers/gpu/drm/amd/display/dc/dc.h | 4 +- drivers/gpu/drm/amd/display/dc/dce/dmub_psr.c | 1 + .../display/dc/dce110/dce110_hw_sequencer.c | 49 ++- .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 5 +- .../gpu/drm/amd/display/dc/dcn20/dcn20_hubp.c | 21 ++-- .../display/dc/dcn31/dcn31_dio_link_encoder.c | 9 .../drm/amd/display/dc/dcn31/dcn31_resource.c | 5 +- .../drm/amd/display/dc/dcn31/dcn31_resource.h | 5 ++ .../amd/display/dc/dcn315/dcn315_resource.c | 1 + drivers/gpu/drm/amd/display/dc/dm_helpers.h | 3 ++ .../gpu/drm/amd/display/dmub/inc/dmub_cmd.h | 12 +++-- 14 files changed, 83 insertions(+), 64 deletions(-) -- 2.25.1
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
> On 28. Feb 2022, at 12:20, Greg KH wrote: > > On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: >> If the list does not contain the expected element, the value of >> list_for_each_entry() iterator will not point to a valid structure. >> To avoid type confusion in such case, the list iterator >> scope will be limited to list_for_each_entry() loop. >> >> In preparation to limiting scope of a list iterator to the list traversal >> loop, use a dedicated pointer to point to the found element. >> Determining if an element was found is then simply checking if >> the pointer is != NULL. >> >> Signed-off-by: Jakob Koschel >> --- >> arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- >> drivers/scsi/scsi_transport_sas.c| 17 - >> drivers/thermal/thermal_core.c | 38 ++-- >> drivers/usb/gadget/configfs.c| 22 ++-- >> drivers/usb/gadget/udc/max3420_udc.c | 11 +--- >> drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- >> drivers/usb/mtu3/mtu3_gadget.c | 11 +--- >> drivers/usb/musb/musb_gadget.c | 11 +--- >> drivers/vfio/mdev/mdev_core.c| 11 +--- >> 9 files changed, 88 insertions(+), 50 deletions(-) > > The drivers/usb/ portion of this patch should be in patch 1/X, right? I kept them separate since it's a slightly different case. Using 'ptr' instead of '>other_member'. Regardless, I'll split this commit up as you mentioned. > > Also, you will have to split these up per-subsystem so that the > different subsystem maintainers can take these in their trees. Thanks for the feedback. To clarify I understand you correctly: I should do one patch set per-subsystem with every instance/(file?) fixed as a separate commit? If I understand correctly, I'll repost accordingly. > > thanks, > > greg k-h thanks, Jakob Koschel
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
> On 28. Feb 2022, at 12:24, Dan Carpenter wrote:
>
> On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
>> diff --git a/drivers/usb/gadget/udc/at91_udc.c
>> b/drivers/usb/gadget/udc/at91_udc.c
>> index 9040a0561466..0fd0307bc07b 100644
>> --- a/drivers/usb/gadget/udc/at91_udc.c
>> +++ b/drivers/usb/gadget/udc/at91_udc.c
>> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
>> at91_ep *ep)
>> if (list_empty (>queue))
>> seq_printf(s, "\t(queue empty)\n");
>>
>> -else list_for_each_entry (req, >queue, queue) {
>> -unsignedlength = req->req.actual;
>> +else
>> +list_for_each_entry(req, >queue, queue) {
>> +unsigned intlength = req->req.actual;
>>
>> -seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> ->req, length,
>> -req->req.length, req->req.buf);
>> -}
>> +seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> +>req, length,
>> +req->req.length, req->req.buf);
>> +}
>
> Don't make unrelated white space changes. It just makes the patch
> harder to review. As you're writing the patch make note of any
> additional changes and do them later in a separate patch.
>
> Also a multi-line indent gets curly braces for readability even though
> it's not required by C. And then both sides would get curly braces.
>
>> spin_unlock_irqrestore(>lock, flags);
>> }
>>
>> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void
>> *unused)
>>
>> if (udc->enabled && udc->vbus) {
>> proc_ep_show(s, >ep[0]);
>> -list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
>> +list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
>
> Another unrelated change.
>
>> if (ep->ep.desc)
>> proc_ep_show(s, ep);
>> }
>
>
> [ snip ]
Thanks for pointing out, I'll remove the changes here and note them down
to send them separately.
>
>> diff --git a/drivers/usb/gadget/udc/net2272.c
>> b/drivers/usb/gadget/udc/net2272.c
>> index 7c38057dcb4a..bb59200f1596 100644
>> --- a/drivers/usb/gadget/udc/net2272.c
>> +++ b/drivers/usb/gadget/udc/net2272.c
>> @@ -926,7 +926,8 @@ static int
>> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> {
>> struct net2272_ep *ep;
>> -struct net2272_request *req;
>> +struct net2272_request *req = NULL;
>> +struct net2272_request *tmp;
>> unsigned long flags;
>> int stopped;
>>
>> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
>> *_req)
>> ep->stopped = 1;
>>
>> /* make sure it's still queued on this endpoint */
>> -list_for_each_entry(req, >queue, queue) {
>> -if (>req == _req)
>> +list_for_each_entry(tmp, >queue, queue) {
>> +if (>req == _req) {
>> +req = tmp;
>> break;
>> +}
>> }
>> -if (>req != _req) {
>> +if (!req) {
>> ep->stopped = stopped;
>> spin_unlock_irqrestore(>dev->lock, flags);
>> return -EINVAL;
>> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
>> *_req)
>> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
>> net2272_done(ep, req, -ECONNRESET);
>> }
>> -req = NULL;
>
> Another unrelated change. These are all good changes but send them as
> separate patches.
You are referring to the req = NULL, right?
I've changed the use of 'req' in the same function and assumed that I can
just remove the unnecessary statement. But if it's better to do separately
I'll do that.
>
>> ep->stopped = stopped;
>>
>> spin_unlock_irqrestore(>dev->lock, flags);
>
> regards,
> dan carpenter
thanks,
Jakob Koschel
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote: > >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request > >> *_req) > >>dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name); > >>net2272_done(ep, req, -ECONNRESET); > >>} > >> - req = NULL; > > > > Another unrelated change. These are all good changes but send them as > > separate patches. > > You are referring to the req = NULL, right? Yes. > > I've changed the use of 'req' in the same function and assumed that I can > just remove the unnecessary statement. But if it's better to do separately > I'll do that. > These are all changes which made me pause during my review to figure out why they were necessary. The line between what is a related part of a patch is a bit vague and some maintainers will ask you to add or subtract from a patch depending on their individual tastes. I don't really have an exact answer, but I felt like this patch needs to be subtracted from. Especially if there is a whole chunk of the patch which can be removed, then to me, that obviously should be in a different patch. regards, dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c
> index 4ee578b181da..a8cbd90db9d2 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
> * connected to a remote device is a port, so ports must be formed on
> * all devices with phys if they're connected to anything.
> */
> -void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
> +void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
_phy is an unfortunate name.
> {
> mutex_lock(>phy_list_mutex);
> - if (unlikely(!list_empty(>port_siblings))) {
> + if (unlikely(!list_empty(&_phy->port_siblings))) {
> /* make sure we're already on this port */
> + struct sas_phy *phy = NULL;
Maybe call this port_phy?
> struct sas_phy *tmp;
>
> list_for_each_entry(tmp, >phy_list, port_siblings)
> - if (tmp == phy)
> + if (tmp == _phy) {
> + phy = tmp;
> break;
> + }
> /* If this trips, you added a phy that was already
>* part of a different port */
> - if (unlikely(tmp != phy)) {
> + if (unlikely(!phy)) {
> dev_printk(KERN_ERR, >dev, "trying to add phy %s
> fails: it's already part of another port\n",
> -dev_name(>dev));
> +dev_name(&_phy->dev));
> BUG();
> }
> } else {
> - sas_port_create_link(port, phy);
> - list_add_tail(>port_siblings, >phy_list);
> + sas_port_create_link(port, _phy);
> + list_add_tail(&_phy->port_siblings, >phy_list);
> port->num_phys++;
> }
> mutex_unlock(>phy_list_mutex);
regards,
dan carpenter
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
On Mon, Feb 28, 2022 at 12:08:22PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/infiniband/hw/hfi1/tid_rdma.c
> b/drivers/infiniband/hw/hfi1/tid_rdma.c
> index 2a7abf7a1f7f..a069847b56aa 100644
> --- a/drivers/infiniband/hw/hfi1/tid_rdma.c
> +++ b/drivers/infiniband/hw/hfi1/tid_rdma.c
> @@ -1239,7 +1239,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> struct hfi1_ctxtdata *rcd = flow->req->rcd;
> struct hfi1_devdata *dd = rcd->dd;
> u32 ngroups, pageidx = 0;
> - struct tid_group *group = NULL, *used;
> + struct tid_group *group = NULL, *used, *tmp;
> u8 use;
>
> flow->tnode_cnt = 0;
> @@ -1248,13 +1248,15 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
> goto used_list;
>
> /* First look at complete groups */
> - list_for_each_entry(group, >tid_group_list.list, list) {
> - kern_add_tid_node(flow, rcd, "complete groups", group,
> - group->size);
> + list_for_each_entry(tmp, >tid_group_list.list, list) {
> + kern_add_tid_node(flow, rcd, "complete groups", tmp,
> + tmp->size);
>
> - pageidx += group->size;
> - if (!--ngroups)
> + pageidx += tmp->size;
> + if (!--ngroups) {
> + group = tmp;
> break;
> + }
> }
>
> if (pageidx >= flow->npagesets)
> @@ -1277,7 +1279,7 @@ static int kern_alloc_tids(struct tid_rdma_flow *flow)
>* However, if we are at the head, we have reached the end of the
^
>* complete groups list from the first loop above
^^
>*/
Originally this code tested for an open code list_is_head() so the
comment made sense, but it's out of date now. Just delete it.
> - if (group && >list == >tid_group_list.list)
> + if (!group)
> goto bail_eagain;
> group = list_prepare_entry(group, >tid_group_list.list,
> list);
regards,
dan carpenter
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
Am 28.02.22 um 12:08 schrieb Jakob Koschel:
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
We explicitly have the list_entry_is_head() macro to test after a loop
if the element pointer points to the head of the list instead of a valid
list entry.
So at least from my side I absolutely don't think that this is a good idea.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.
Since when do we actually want to do this?
Take this code here as an example:
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..6c916416decc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,8 @@ 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 *found_encl_mm = NULL;
+ struct sgx_encl_mm *tmp;
/*
* The enclave itself can remove encl_mm. Note, objects can't be moved
@@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier
*mn,
list_for_each_entry(tmp, _mm->encl->mm_list, list) {
if (tmp == encl_mm) {
list_del_rcu(_mm->list);
+ found_encl_mm = tmp;
break;
}
}
spin_unlock(_mm->encl->mm_lock);
- if (tmp == encl_mm) {
+ if (found_encl_mm) {
synchronize_srcu(_mm->encl->srcu);
mmu_notifier_put(mn);
}
I don't think that using the extra variable makes the code in any way
more reliable or easier to read.
Regards,
Christian.
Re: kernel amdgpu problems
Am 28.02.22 um 07:43 schrieb Alex fxmbsw7 Ratchev: On Mon, Feb 28, 2022 at 12:22 AM Mike Lothian wrote: On Sat, 26 Feb 2022 at 10:01, Alex fxmbsw7 Ratchev wrote: > > i have observed at least two major problems of using amdgpu > > 1. cant be built-in instead of module, display stays blank Are you remembering to add in the firmware to the kernel image too? i copied all from official linux-firmware.gif to /lib/firmware That's not sufficient, you need to tell the kernel explicitly which firmware will be needed. In other words try mentioning everything in /lib/firmware/amdgpu as CONFIG_EXTRA_FIRMWARE. Regards, Christian. There's a good guide at https://wiki.gentoo.org/wiki/AMDGPU#Known_firmware_blobs I've successfully used AMDGPU builtin on Tonga, Raven, Renoir and Navy Flounder systems I've never used kexec so have no suggestions for that it may be back then when i compiled it inline not as module i didnt include firmware in the initrd so that may been that about kexec, i dunno, X dri doesnt work after first kexec, after second screen at all is blank driver init problems .. cheers..
[PATCH 4/6] drivers: remove unnecessary use of list iterator variable
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will *always* be a bogus
pointer computed based on the head element.
To avoid type confusion use the actual list head directly instead of last
iterator value.
Signed-off-by: Jakob Koschel
---
drivers/dma/dw-edma/dw-edma-core.c | 4 ++--
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 3 ++-
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/dw-edma/dw-edma-core.c
b/drivers/dma/dw-edma/dw-edma-core.c
index 468d1097a1ec..7883c4831857 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -136,7 +136,7 @@ static void dw_edma_free_burst(struct dw_edma_chunk *chunk)
}
/* Remove the list head */
- kfree(child);
+ kfree(chunk->burst);
chunk->burst = NULL;
}
@@ -156,7 +156,7 @@ static void dw_edma_free_chunk(struct dw_edma_desc *desc)
}
/* Remove the list head */
- kfree(child);
+ kfree(desc->chunk);
desc->chunk = NULL;
}
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 091f36adbbe1..c0ea9dbc4ff6 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -3963,7 +3963,8 @@ static void __i40e_reprogram_flex_pit(struct i40e_pf *pf,
* correctly, the hardware will disable flexible field parsing.
*/
if (!list_empty(flex_pit_list))
- last_offset = list_prev_entry(entry, list)->src_offset + 1;
+ last_offset = list_entry(flex_pit_list->prev,
+struct i40e_flex_pit,
list)->src_offset + 1;
for (; i < 3; i++, last_offset++) {
i40e_write_rx_ctl(>hw,
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index e3874421c4c0..cf5b05860799 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -104,7 +104,7 @@ static void ath6kl_credit_init(struct
ath6kl_htc_credit_info *cred_info,
* it use list_for_each_entry_reverse to walk around the whole ep list.
* Therefore assign this lowestpri_ep_dist after walk around the ep_list
*/
- cred_info->lowestpri_ep_dist = cur_ep_dist->list;
+ cred_info->lowestpri_ep_dist = *ep_list;
WARN_ON(cred_info->cur_free_credits <= 0);
--
2.25.1
[PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element.
Determining if an element was found is then simply checking if
the pointer is != NULL.
Signed-off-by: Jakob Koschel
---
arch/x86/kernel/cpu/sgx/encl.c | 6 +++--
drivers/scsi/scsi_transport_sas.c| 17 -
drivers/thermal/thermal_core.c | 38 ++--
drivers/usb/gadget/configfs.c| 22 ++--
drivers/usb/gadget/udc/max3420_udc.c | 11 +---
drivers/usb/gadget/udc/tegra-xudc.c | 11 +---
drivers/usb/mtu3/mtu3_gadget.c | 11 +---
drivers/usb/musb/musb_gadget.c | 11 +---
drivers/vfio/mdev/mdev_core.c| 11 +---
9 files changed, 88 insertions(+), 50 deletions(-)
diff --git a/arch/x86/kernel/cpu/sgx/encl.c b/arch/x86/kernel/cpu/sgx/encl.c
index 48afe96ae0f0..6c916416decc 100644
--- a/arch/x86/kernel/cpu/sgx/encl.c
+++ b/arch/x86/kernel/cpu/sgx/encl.c
@@ -450,7 +450,8 @@ 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 *found_encl_mm = NULL;
+ struct sgx_encl_mm *tmp;
/*
* The enclave itself can remove encl_mm. Note, objects can't be moved
@@ -460,12 +461,13 @@ static void sgx_mmu_notifier_release(struct mmu_notifier
*mn,
list_for_each_entry(tmp, _mm->encl->mm_list, list) {
if (tmp == encl_mm) {
list_del_rcu(_mm->list);
+ found_encl_mm = tmp;
break;
}
}
spin_unlock(_mm->encl->mm_lock);
- if (tmp == encl_mm) {
+ if (found_encl_mm) {
synchronize_srcu(_mm->encl->srcu);
mmu_notifier_put(mn);
}
diff --git a/drivers/scsi/scsi_transport_sas.c
b/drivers/scsi/scsi_transport_sas.c
index 4ee578b181da..a8cbd90db9d2 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1060,26 +1060,29 @@ EXPORT_SYMBOL(sas_port_get_phy);
* connected to a remote device is a port, so ports must be formed on
* all devices with phys if they're connected to anything.
*/
-void sas_port_add_phy(struct sas_port *port, struct sas_phy *phy)
+void sas_port_add_phy(struct sas_port *port, struct sas_phy *_phy)
{
mutex_lock(>phy_list_mutex);
- if (unlikely(!list_empty(>port_siblings))) {
+ if (unlikely(!list_empty(&_phy->port_siblings))) {
/* make sure we're already on this port */
+ struct sas_phy *phy = NULL;
struct sas_phy *tmp;
list_for_each_entry(tmp, >phy_list, port_siblings)
- if (tmp == phy)
+ if (tmp == _phy) {
+ phy = tmp;
break;
+ }
/* If this trips, you added a phy that was already
* part of a different port */
- if (unlikely(tmp != phy)) {
+ if (unlikely(!phy)) {
dev_printk(KERN_ERR, >dev, "trying to add phy %s
fails: it's already part of another port\n",
- dev_name(>dev));
+ dev_name(&_phy->dev));
BUG();
}
} else {
- sas_port_create_link(port, phy);
- list_add_tail(>port_siblings, >phy_list);
+ sas_port_create_link(port, _phy);
+ list_add_tail(&_phy->port_siblings, >phy_list);
port->num_phys++;
}
mutex_unlock(>phy_list_mutex);
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 82654dc8382b..97198543448b 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -625,24 +625,30 @@ int thermal_zone_bind_cooling_device(struct
thermal_zone_device *tz,
{
struct thermal_instance *dev;
struct thermal_instance *pos;
- struct thermal_zone_device *pos1;
- struct thermal_cooling_device *pos2;
+ struct thermal_zone_device *pos1 = NULL;
+ struct thermal_zone_device *tmp1;
+ struct thermal_cooling_device *pos2 = NULL;
+ struct thermal_cooling_device *tmp2;
unsigned long max_state;
int result, ret;
if (trip >= tz->trips || trip < 0)
return -EINVAL;
- list_for_each_entry(pos1, _tz_list, node) {
- if (pos1 == tz)
+ list_for_each_entry(tmp1,
[PATCH 0/6] Remove usage of list iterator past the loop body
This is the first patch removing several categories of use cases of
the list iterator variable past the loop.
This is follow up to the discussion in:
https://lore.kernel.org/all/20220217184829.1991035-1-jakobkosc...@gmail.com/
As concluded in:
https://lore.kernel.org/all/yhdfeiwi4edth...@kroah.com/
the correct use should be using a separate variable after the loop
and using a 'tmp' variable as the list iterator.
The list iterator will not point to a valid structure after the loop
if no break/goto was hit. Invalid uses of the list iterator variable
can be avoided altogether by simply using a separate pointer to
iterate the list.
Linus and Greg agreed on the following pattern:
- struct gr_request *req;
+ struct gr_request *req = NULL;
+ struct gr_request *tmp;
struct gr_ep *ep;
int ret = 0;
- list_for_each_entry(req, >queue, queue) {
- if (>req == _req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == _req) {
+ req = tmp;
break;
+ }
}
- if (>req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
With gnu89 the list iterator variable cannot yet be declared
within the for loop of the list iterator.
Moving to a more modern version of C would allow defining
the list iterator variable within the macro, limiting
the scope to the loop.
This avoids any incorrect usage past the loop altogether.
This are around 30% of the cases where the iterator
variable is used past the loop (identified with a slightly
modified version of use_after_iter.cocci).
I've decided to split it into at a few patches separated
by similar use cases.
Because the output of get_maintainer.pl was too big,
I included all the found lists and everyone from the
previous discussion.
Jakob Koschel (6):
drivers: usb: remove usage of list iterator past the loop body
treewide: remove using list iterator after loop body as a ptr
treewide: fix incorrect use to determine if list is empty
drivers: remove unnecessary use of list iterator variable
treewide: remove dereference of list iterator after loop body
treewide: remove check of list iterator against head past the loop
body
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +---
arch/powerpc/sysdev/fsl_gtm.c | 4 +-
arch/x86/kernel/cpu/sgx/encl.c| 6 ++-
drivers/block/drbd/drbd_req.c | 45 ---
drivers/counter/counter-chrdev.c | 26 ++-
drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++--
drivers/dma/dw-edma/dw-edma-core.c| 4 +-
drivers/dma/ppc4xx/adma.c | 11 +++--
drivers/firewire/core-transaction.c | 32 +++--
drivers/firewire/sbp2.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +---
drivers/gpu/drm/drm_memory.c | 15 ---
drivers/gpu/drm/drm_mm.c | 17 ---
drivers/gpu/drm/drm_vm.c | 13 +++---
drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 ---
drivers/gpu/drm/i915/gt/intel_ring.c | 15 ---
.../gpu/drm/nouveau/nvkm/subdev/clk/base.c| 11 +++--
.../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++---
drivers/gpu/drm/scheduler/sched_main.c| 14 +++---
drivers/gpu/drm/ttm/ttm_bo.c | 19
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ---
drivers/infiniband/hw/mlx4/main.c | 12 ++---
drivers/media/dvb-frontends/mxl5xx.c | 11 +++--
drivers/media/pci/saa7134/saa7134-alsa.c | 4 +-
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++--
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/intel/i40e/i40e_ethtool.c| 3 +-
.../net/ethernet/qlogic/qede/qede_filter.c| 11 +++--
drivers/net/wireless/ath/ath6kl/htc_mbox.c| 2 +-
.../net/wireless/intel/ipw2x00/libipw_rx.c| 15 ---
drivers/perf/xgene_pmu.c | 13 +++---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ---
drivers/scsi/scsi_transport_sas.c | 17 ---
drivers/scsi/wd719x.c | 12 +++--
drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++
drivers/staging/rtl8192e/rtllib_rx.c | 17 ---
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ---
.../rtl8192u/ieee80211/rtl819x_TSProc.c | 19
drivers/thermal/thermal_core.c| 38 ++--
drivers/usb/gadget/composite.c| 9 ++--
[PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
If the list representing the request queue does not contain the expected
request, the value of list_for_each_entry() iterator will not point to a
valid structure. To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object.
Signed-off-by: Jakob Koschel
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 ++
drivers/usb/gadget/udc/at91_udc.c| 26 ++--
drivers/usb/gadget/udc/atmel_usba_udc.c | 11 ++
drivers/usb/gadget/udc/bdc/bdc_ep.c | 11 +++---
drivers/usb/gadget/udc/fsl_qe_udc.c | 11 ++
drivers/usb/gadget/udc/fsl_udc_core.c| 11 ++
drivers/usb/gadget/udc/goku_udc.c| 11 ++
drivers/usb/gadget/udc/gr_udc.c | 11 ++
drivers/usb/gadget/udc/lpc32xx_udc.c | 11 ++
drivers/usb/gadget/udc/mv_u3d_core.c | 11 ++
drivers/usb/gadget/udc/mv_udc_core.c | 11 ++
drivers/usb/gadget/udc/net2272.c | 12 ++-
drivers/usb/gadget/udc/net2280.c | 11 ++
drivers/usb/gadget/udc/omap_udc.c| 11 ++
drivers/usb/gadget/udc/pxa25x_udc.c | 11 ++
drivers/usb/gadget/udc/s3c-hsudc.c | 11 ++
drivers/usb/gadget/udc/udc-xilinx.c | 11 ++
17 files changed, 128 insertions(+), 75 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..cad874ee4472 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -466,19 +466,22 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep,
struct usb_request *u_req)
{
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub *vhub = ep->vhub;
- struct ast_vhub_req *req;
+ struct ast_vhub_req *req = NULL;
+ struct ast_vhub_req *tmp;
unsigned long flags;
int rc = -EINVAL;
spin_lock_irqsave(>lock, flags);
/* Make sure it's actually queued on this endpoint */
- list_for_each_entry (req, >queue, queue) {
- if (>req == u_req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == u_req) {
+ req = tmp;
break;
+ }
}
- if (>req == u_req) {
+ if (req) {
EPVDBG(ep, "dequeue req @%p active=%d\n",
req, req->active);
if (req->active)
diff --git a/drivers/usb/gadget/udc/at91_udc.c
b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
at91_ep *ep)
if (list_empty (>queue))
seq_printf(s, "\t(queue empty)\n");
- else list_for_each_entry (req, >queue, queue) {
- unsignedlength = req->req.actual;
+ else
+ list_for_each_entry(req, >queue, queue) {
+ unsigned intlength = req->req.actual;
- seq_printf(s, "\treq %p len %d/%d buf %p\n",
- >req, length,
- req->req.length, req->req.buf);
- }
+ seq_printf(s, "\treq %p len %d/%d buf %p\n",
+ >req, length,
+ req->req.length, req->req.buf);
+ }
spin_unlock_irqrestore(>lock, flags);
}
@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
if (udc->enabled && udc->vbus) {
proc_ep_show(s, >ep[0]);
- list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
+ list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
if (ep->ep.desc)
proc_ep_show(s, ep);
}
@@ -704,7 +705,8 @@ static int at91_ep_queue(struct usb_ep *_ep,
static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct at91_ep *ep;
- struct at91_request *req;
+ struct at91_request *req = NULL;
+ struct at91_request *tmp;
unsigned long flags;
struct at91_udc *udc;
@@ -717,11 +719,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct
usb_request *_req)
spin_lock_irqsave(>lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry (req, >queue, queue) {
- if (>req == _req)
+ list_for_each_entry(tmp, >queue, queue) {
+ if (>req == _req) {
+ req = tmp;
break;
+ }
}
- if
[PATCH 5/6] treewide: remove dereference of list iterator after loop body
The list iterator variable will be a bogus pointer if no break was hit.
Dereferencing it could load *any* out-of-bounds/undefined value
making it unsafe to use that in the comparision to determine if the
specific element was found.
This is fixed by using a separate list iterator variable for the loop
and only setting the original variable if a suitable element was found.
Then determing if the element was found is simply checking if the
variable is set.
Signed-off-by: Jakob Koschel
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 11 +++
drivers/scsi/wd719x.c | 12
fs/f2fs/segment.c | 9 ++---
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
index 57199be082fd..c56cd9e59a66 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
@@ -471,20 +471,23 @@ nvkm_pstate_new(struct nvkm_clk *clk, int idx)
static int
nvkm_clk_ustate_update(struct nvkm_clk *clk, int req)
{
- struct nvkm_pstate *pstate;
+ struct nvkm_pstate *pstate = NULL;
+ struct nvkm_pstate *tmp;
int i = 0;
if (!clk->allow_reclock)
return -ENOSYS;
if (req != -1 && req != -2) {
- list_for_each_entry(pstate, >states, head) {
- if (pstate->pstate == req)
+ list_for_each_entry(tmp, >states, head) {
+ if (tmp->pstate == req) {
+ pstate = tmp;
break;
+ }
i++;
}
- if (pstate->pstate != req)
+ if (!pstate)
return -EINVAL;
req = i;
}
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index 1a7947554581..be270ed8e00d 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -684,11 +684,15 @@ static irqreturn_t wd719x_interrupt(int irq, void *dev_id)
case WD719X_INT_SPIDERFAILED:
/* was the cmd completed a direct or SCB command? */
if (regs.bytes.OPC == WD719X_CMD_PROCESS_SCB) {
- struct wd719x_scb *scb;
- list_for_each_entry(scb, >active_scbs, list)
- if (SCB_out == scb->phys)
+ struct wd719x_scb *scb = NULL;
+ struct wd719x_scb *tmp;
+
+ list_for_each_entry(tmp, >active_scbs, list)
+ if (SCB_out == tmp->phys) {
+ scb = tmp;
break;
- if (SCB_out == scb->phys)
+ }
+ if (scb)
wd719x_interrupt_SCB(wd, regs, scb);
else
dev_err(>pdev->dev, "card returned invalid
SCB pointer\n");
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1dabc8244083..a3684385e04a 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -356,16 +356,19 @@ void f2fs_drop_inmem_page(struct inode *inode, struct
page *page)
struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
struct list_head *head = >inmem_pages;
struct inmem_pages *cur = NULL;
+ struct inmem_pages *tmp;
f2fs_bug_on(sbi, !page_private_atomic(page));
mutex_lock(>inmem_lock);
- list_for_each_entry(cur, head, list) {
- if (cur->page == page)
+ list_for_each_entry(tmp, head, list) {
+ if (tmp->page == page) {
+ cur = tmp;
break;
+ }
}
- f2fs_bug_on(sbi, list_empty(head) || cur->page != page);
+ f2fs_bug_on(sbi, !cur);
list_del(>list);
mutex_unlock(>inmem_lock);
--
2.25.1
[PATCH 6/6] treewide: remove check of list iterator against head past the loop body
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
>member == head, using the iterator variable after the loop should
be avoided.
In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element.
Signed-off-by: Jakob Koschel
---
arch/arm/mach-mmp/sram.c | 9 ++--
arch/arm/plat-pxa/ssp.c | 28 +---
drivers/block/drbd/drbd_req.c | 45 ---
drivers/counter/counter-chrdev.c | 26 ++-
drivers/crypto/cavium/nitrox/nitrox_main.c| 11 +++--
drivers/dma/ppc4xx/adma.c | 11 +++--
drivers/firewire/core-transaction.c | 32 +++--
drivers/firewire/sbp2.c | 14 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c| 19 +---
drivers/gpu/drm/drm_memory.c | 15 ---
drivers/gpu/drm/drm_mm.c | 17 ---
drivers/gpu/drm/drm_vm.c | 13 +++---
drivers/gpu/drm/gma500/oaktrail_lvds.c| 9 ++--
drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 ---
drivers/gpu/drm/i915/gt/intel_ring.c | 15 ---
.../gpu/drm/nouveau/nvkm/subdev/fb/ramgk104.c | 13 +++---
drivers/gpu/drm/scheduler/sched_main.c| 14 +++---
drivers/gpu/drm/ttm/ttm_bo.c | 19
drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 22 +
drivers/infiniband/hw/hfi1/tid_rdma.c | 16 ---
drivers/infiniband/hw/mlx4/main.c | 12 ++---
drivers/media/dvb-frontends/mxl5xx.c | 11 +++--
drivers/media/v4l2-core/v4l2-ctrls-api.c | 31 +++--
drivers/misc/mei/interrupt.c | 12 ++---
.../net/ethernet/qlogic/qede/qede_filter.c| 11 +++--
.../net/wireless/intel/ipw2x00/libipw_rx.c| 15 ---
drivers/power/supply/cpcap-battery.c | 11 +++--
drivers/scsi/lpfc/lpfc_bsg.c | 16 ---
drivers/staging/rtl8192e/rtl819x_TSProc.c | 17 +++
drivers/staging/rtl8192e/rtllib_rx.c | 17 ---
.../staging/rtl8192u/ieee80211/ieee80211_rx.c | 15 ---
.../rtl8192u/ieee80211/rtl819x_TSProc.c | 19
drivers/usb/gadget/composite.c| 9 ++--
fs/cifs/smb2misc.c| 10 +++--
fs/proc/kcore.c | 13 +++---
kernel/debug/kdb/kdb_main.c | 36 +--
kernel/power/snapshot.c | 10 +++--
kernel/trace/ftrace.c | 22 +
kernel/trace/trace_eprobe.c | 15 ---
kernel/trace/trace_events.c | 11 ++---
net/9p/trans_xen.c| 11 +++--
net/ipv4/udp_tunnel_nic.c | 10 +++--
net/tipc/name_table.c | 11 +++--
net/tipc/socket.c | 11 +++--
net/xfrm/xfrm_ipcomp.c| 11 +++--
sound/soc/intel/catpt/pcm.c | 13 +++---
sound/soc/sprd/sprd-mcdt.c| 13 +++---
48 files changed, 455 insertions(+), 315 deletions(-)
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;
diff --git a/arch/arm/plat-pxa/ssp.c b/arch/arm/plat-pxa/ssp.c
index 563440315acd..4884a8c0c89b 100644
--- a/arch/arm/plat-pxa/ssp.c
+++ b/arch/arm/plat-pxa/ssp.c
@@ -38,22 +38,20 @@ static LIST_HEAD(ssp_list);
struct ssp_device *pxa_ssp_request(int port, const char *label)
{
struct ssp_device *ssp = NULL;
+ struct ssp_device *tmp;
mutex_lock(_lock);
- list_for_each_entry(ssp, _list, node) {
- if (ssp->port_id == port && ssp->use_count == 0) {
- ssp->use_count++;
- ssp->label = label;
+ list_for_each_entry(tmp,
[PATCH v2] drm/amdgpu: Fix realloc of ptr
From: Tom Rix
Clang static analysis reports this error
amdgpu_debugfs.c:1690:9: warning: 1st function call
argument is an uninitialized value
tmp = krealloc_array(tmp, i + 1,
^~~
realloc uses tmp, so tmp can not be garbage.
And the return needs to be checked.
Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
Signed-off-by: Tom Rix
---
v2:
use 'new' to hold/check the ralloc return
fix commit log mistake on ralloc freeing to using input ptr
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 9eb9b440bd438..2f4f8c5618d81 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1676,7 +1676,7 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
{
struct amdgpu_device *adev = (struct amdgpu_device
*)file_inode(f)->i_private;
char reg_offset[11];
- uint32_t *tmp;
+ uint32_t *new, *tmp = NULL;
int ret, i = 0, len = 0;
do {
@@ -1687,7 +1687,12 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
goto error_free;
}
- tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ new = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ if (!new) {
+ ret = -ENOMEM;
+ goto error_free;
+ }
+ tmp = new;
if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
ret = -EINVAL;
goto error_free;
--
2.26.3
Re: [PATCH 6/6] treewide: remove check of list iterator against head past the loop body
This is a bit more work (and a lot more noise), but I'd prefer if this were split into as many patches as there are components. I'm not going to review the parts of the patches that don't concern me, and if something turns out to be a problem later one (it shouldn't but one never knows) it'll be much easier to revert or put the blame on an individual smaller commit than on this... With that being said, ultimately I don't care that much and will leave that to people who do :) Jakob Koschel wrote on Mon, Feb 28, 2022 at 12:08:22PM +0100: > net/9p/trans_xen.c| 11 +++-- This 9p change looks good to me. Reviewed-by: Dominique Martinet # 9p -- Dominique
Re: [PATCH 2/6] treewide: remove using list iterator after loop body as a ptr
On Mon, Feb 28, 2022 at 12:08:18PM +0100, Jakob Koschel wrote: > If the list does not contain the expected element, the value of > list_for_each_entry() iterator will not point to a valid structure. > To avoid type confusion in such case, the list iterator > scope will be limited to list_for_each_entry() loop. > > In preparation to limiting scope of a list iterator to the list traversal > loop, use a dedicated pointer to point to the found element. > Determining if an element was found is then simply checking if > the pointer is != NULL. > > Signed-off-by: Jakob Koschel > --- > arch/x86/kernel/cpu/sgx/encl.c | 6 +++-- > drivers/scsi/scsi_transport_sas.c| 17 - > drivers/thermal/thermal_core.c | 38 ++-- > drivers/usb/gadget/configfs.c| 22 ++-- > drivers/usb/gadget/udc/max3420_udc.c | 11 +--- > drivers/usb/gadget/udc/tegra-xudc.c | 11 +--- > drivers/usb/mtu3/mtu3_gadget.c | 11 +--- > drivers/usb/musb/musb_gadget.c | 11 +--- > drivers/vfio/mdev/mdev_core.c| 11 +--- > 9 files changed, 88 insertions(+), 50 deletions(-) The drivers/usb/ portion of this patch should be in patch 1/X, right? Also, you will have to split these up per-subsystem so that the different subsystem maintainers can take these in their trees. thanks, greg k-h
[PATCH 3/6] treewide: fix incorrect use to determine if list is empty
The list iterator value will *always* be set by list_for_each_entry().
It is incorrect to assume that the iterator value will be NULL if the
list is empty.
Instead of checking the pointer it should be checked if
the list is empty.
In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL
on the break, it is set to the correct value and leaving it
NULL if no element was found.
Signed-off-by: Jakob Koschel
---
arch/powerpc/sysdev/fsl_gtm.c| 4 ++--
drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++--
drivers/perf/xgene_pmu.c | 13 +++--
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_gtm.c b/arch/powerpc/sysdev/fsl_gtm.c
index 8963eaffb1b7..39186ad6b3c3 100644
--- a/arch/powerpc/sysdev/fsl_gtm.c
+++ b/arch/powerpc/sysdev/fsl_gtm.c
@@ -86,7 +86,7 @@ static LIST_HEAD(gtms);
*/
struct gtm_timer *gtm_get_timer16(void)
{
- struct gtm *gtm = NULL;
+ struct gtm *gtm;
int i;
list_for_each_entry(gtm, , list_node) {
@@ -103,7 +103,7 @@ struct gtm_timer *gtm_get_timer16(void)
spin_unlock_irq(>lock);
}
- if (gtm)
+ if (!list_empty())
return ERR_PTR(-EBUSY);
return ERR_PTR(-ENODEV);
}
diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c
b/drivers/media/pci/saa7134/saa7134-alsa.c
index fb24d2ed3621..d3cde05a6eba 100644
--- a/drivers/media/pci/saa7134/saa7134-alsa.c
+++ b/drivers/media/pci/saa7134/saa7134-alsa.c
@@ -1214,7 +1214,7 @@ static int alsa_device_exit(struct saa7134_dev *dev)
static int saa7134_alsa_init(void)
{
- struct saa7134_dev *dev = NULL;
+ struct saa7134_dev *dev;
saa7134_dmasound_init = alsa_device_init;
saa7134_dmasound_exit = alsa_device_exit;
@@ -1229,7 +1229,7 @@ static int saa7134_alsa_init(void)
alsa_device_init(dev);
}
- if (dev == NULL)
+ if (list_empty(_devlist))
pr_info("saa7134 ALSA: no saa7134 cards found\n");
return 0;
diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
index 2b6d476bd213..e255f9e665d1 100644
--- a/drivers/perf/xgene_pmu.c
+++ b/drivers/perf/xgene_pmu.c
@@ -1460,7 +1460,8 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu
*xgene_pmu,
struct hw_pmu_info *inf;
void __iomem *dev_csr;
struct resource res;
- struct resource_entry *rentry;
+ struct resource_entry *rentry = NULL;
+ struct resource_entry *tmp;
int enable_bit;
int rc;
@@ -1475,16 +1476,16 @@ xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu
*xgene_pmu,
return NULL;
}
- list_for_each_entry(rentry, _list, node) {
- if (resource_type(rentry->res) == IORESOURCE_MEM) {
- res = *rentry->res;
- rentry = NULL;
+ list_for_each_entry(tmp, _list, node) {
+ if (resource_type(tmp->res) == IORESOURCE_MEM) {
+ res = *tmp->res;
+ rentry = tmp;
break;
}
}
acpi_dev_free_resource_list(_list);
- if (rentry) {
+ if (!rentry) {
dev_err(dev, "PMU type %d: No memory resource found\n", type);
return NULL;
}
--
2.25.1
RE: [PATCH] drm/amdgpu: Fix realloc of ptr
From: t...@redhat.com
> Sent: 26 February 2022 15:59
>
> From: Tom Rix
>
> Clang static analysis reports this error
> amdgpu_debugfs.c:1690:9: warning: 1st function call
> argument is an uninitialized value
> tmp = krealloc_array(tmp, i + 1,
> ^~~
>
> realloc will free tmp, so tmp can not be garbage.
> And the return needs to be checked.
Are you sure?
A quick check seems to show that krealloc() behaves the same
way as libc realloc() and the pointer isn't freed on failure.
David
> Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 9eb9b440bd438..159b97c0b4ebc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1676,7 +1676,7 @@ static ssize_t
> amdgpu_reset_dump_register_list_write(struct file *f,
> {
> struct amdgpu_device *adev = (struct amdgpu_device
> *)file_inode(f)->i_private;
> char reg_offset[11];
> - uint32_t *tmp;
> + uint32_t *tmp = NULL;
> int ret, i = 0, len = 0;
>
> do {
> @@ -1688,6 +1688,10 @@ static ssize_t
> amdgpu_reset_dump_register_list_write(struct file *f,
> }
>
> tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
> + if (!tmp) {
> + ret = -ENOMEM;
> + goto error_free;
> + }
> if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
> ret = -EINVAL;
> goto error_free;
> --
> 2.26.3
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
[PATCH] drm/amdgpu: Fix realloc of ptr
From: Tom Rix
Clang static analysis reports this error
amdgpu_debugfs.c:1690:9: warning: 1st function call
argument is an uninitialized value
tmp = krealloc_array(tmp, i + 1,
^~~
realloc will free tmp, so tmp can not be garbage.
And the return needs to be checked.
Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
Signed-off-by: Tom Rix
---
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 9eb9b440bd438..159b97c0b4ebc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1676,7 +1676,7 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
{
struct amdgpu_device *adev = (struct amdgpu_device
*)file_inode(f)->i_private;
char reg_offset[11];
- uint32_t *tmp;
+ uint32_t *tmp = NULL;
int ret, i = 0, len = 0;
do {
@@ -1688,6 +1688,10 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
}
tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto error_free;
+ }
if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
ret = -EINVAL;
goto error_free;
--
2.26.3
Re: [PATCH v5 3/7] PCI: Drop the `is_thunderbolt` attribute from PCI core
Hi Bjorn, On Fri, Feb 25, 2022 at 11:42:24AM -0600, Bjorn Helgaas wrote: > That would just leave the "PCI_VSEC_ID_INTEL_TBT implies external-facing" > assumption above. Not having a Thunderbolt spec, I have no idea how > you deal with that. You can download the spec here: https://www.usb.org/sites/default/files/USB4%20Specification%202026.zip Inside the archive there is also the DVSEC spec with name "USB4 DVSEC Version 1.0.pdf".
amdgpu: sometimes black screen
Hi, I've Gigabyte Radeon RX Vega 64 and sometimes when booting it doesn't give any signal to monitor over DisplayPort. Rebooting usually helps and it works fine after. Not sure if it's hardware or software issue. In dmesg only suspicious thing I see is many [drm] perform_link_training_with_retries: Link training attempt 3 of 4 failed Any ideas? Also is there some way to force reload/turn on signal so I don't need to reboot? [0.640199] ACPI: bus type drm_connector registered [0.654114] efifb: probing for efifb [0.654180] efifb: No BGRT, not showing boot graphics [0.654181] efifb: framebuffer at 0x8000, using 3072k, total 3072k [0.654183] efifb: mode is 1024x768x32, linelength=4096, pages=1 [0.654184] efifb: scrolling: redraw [0.654185] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 [0.654400] fbcon: Deferring console take-over [0.654407] fb0: EFI VGA frame buffer device [3.413771] fbcon: Taking over console [3.721845] [drm] amdgpu kernel modesetting enabled. [3.722127] amdgpu: Ignoring ACPI CRAT on non-APU system [3.722132] amdgpu: Virtual CRAT table created for CPU [3.722294] amdgpu: Topology: Add CPU node [3.722558] amdgpu :44:00.0: runtime IRQ mapping not provided by arch [3.722589] fb0: switching to amdgpu from EFI VGA [3.724054] amdgpu :44:00.0: vgaarb: deactivate vga console [3.724419] [drm] initializing kernel modesetting (VEGA10 0x1002:0x687F 0x1002:0x6B76 0xC1). [3.724432] amdgpu :44:00.0: amdgpu: Trusted Memory Zone (TMZ) feature not supported [3.724453] [drm] register mmio base: 0x9F00 [3.724455] [drm] register mmio size: 524288 [3.724483] [drm] add ip block number 0 [3.724486] [drm] add ip block number 1 [3.724488] [drm] add ip block number 2 [3.724489] [drm] add ip block number 3 [3.724490] [drm] add ip block number 4 [3.724492] [drm] add ip block number 5 [3.724493] [drm] add ip block number 6 [3.724494] [drm] add ip block number 7 [3.724495] [drm] add ip block number 8 [3.724497] [drm] add ip block number 9 [3.726255] amdgpu :44:00.0: No more image in the PCI ROM [3.726277] amdgpu :44:00.0: amdgpu: Fetched VBIOS from ROM BAR [3.726280] amdgpu: ATOM BIOS: 113-D0501400-101 [3.730100] [drm] UVD(0) is enabled in VM mode [3.730105] [drm] UVD(0) ENC is enabled in VM mode [3.730107] [drm] VCE enabled in VM mode [3.730460] amdgpu :44:00.0: amdgpu: MEM ECC is not presented. [3.730462] amdgpu :44:00.0: amdgpu: SRAM ECC is not presented. [3.730470] [drm] vm size is 262144 GB, 4 levels, block size is 9-bit, fragment size is 9-bit [3.730487] amdgpu :44:00.0: BAR 2: releasing [mem 0x9000-0x901f 64bit pref] [3.730491] amdgpu :44:00.0: BAR 0: releasing [mem 0x8000-0x8fff 64bit pref] [3.730568] amdgpu :44:00.0: BAR 0: assigned [mem 0x480-0x481 64bit pref] [3.730583] amdgpu :44:00.0: BAR 2: assigned [mem 0x47f-0x47f001f 64bit pref] [3.730648] amdgpu :44:00.0: amdgpu: VRAM: 8176M 0x00F4 - 0x00F5FEFF (8176M used) [3.730651] amdgpu :44:00.0: amdgpu: GART: 512M 0x - 0x1FFF [3.730653] amdgpu :44:00.0: amdgpu: AGP: 267419648M 0x00F8 - 0x [3.730661] [drm] Detected VRAM RAM=8176M, BAR=8192M [3.730662] [drm] RAM width 2048bits HBM [3.731219] [drm] amdgpu: 8176M of VRAM memory ready [3.731223] [drm] amdgpu: 8176M of GTT memory ready. [3.731237] [drm] GART: num cpu pages 131072, num gpu pages 131072 [3.731521] [drm] PCIE GART of 512M enabled. [3.731523] [drm] PTB located at 0x00F40090 [3.737412] amdgpu :44:00.0: amdgpu: PSP runtime database doesn't exist [3.737422] amdgpu: hwmgr_sw_init smu backed is vega10_smu [3.782923] [drm] Found UVD firmware Version: 66.43 Family ID: 17 [3.782983] [drm] PSP loading UVD firmware [3.794121] [drm] Found VCE firmware Version: 57.6 Binary ID: 4 [3.794181] [drm] PSP loading VCE firmware [3.983603] [drm] reserve 0x40 from 0xf5fec0 for PSP TMR [4.043970] [drm] Display Core initialized with v3.2.167! [4.236220] [drm] kiq ring mec 2 pipe 1 q 0 [4.258101] [drm] UVD and UVD ENC initialized successfully. [4.358563] [drm] VCE initialized successfully. [4.360719] kfd kfd: amdgpu: Allocated 3969056 bytes on gart [4.399639] amdgpu: HMM registered 8176MB device memory [4.402935] amdgpu: SRAT table not found [4.402936] amdgpu: Virtual CRAT table created for GPU [4.403647] amdgpu: Topology: Add dGPU node [0x687f:0x1002] [4.403658] kfd kfd: amdgpu: added device 1002:687f [4.403689] amdgpu :44:00.0: amdgpu: SE 4, SH per SE 1, CU per SH 16, active_cu_number 64 [4.403989] amdgpu :44:00.0: amdgpu: ring gfx uses VM inv eng 0 on hub 0 [4.403992] amdgpu :44:00.0: amdgpu: ring comp_1.0.0 uses
Re: [PATCH] drm/amdgpu: Fix realloc of ptr
On 2/26/22 2:21 PM, David Laight wrote:
From: t...@redhat.com
Sent: 26 February 2022 15:59
From: Tom Rix
Clang static analysis reports this error
amdgpu_debugfs.c:1690:9: warning: 1st function call
argument is an uninitialized value
tmp = krealloc_array(tmp, i + 1,
^~~
realloc will free tmp, so tmp can not be garbage.
And the return needs to be checked.
Are you sure?
A quick check seems to show that krealloc() behaves the same
way as libc realloc() and the pointer isn't freed on failure.
I suck, I'll respin the patch
Thanks
Tom
David
Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
Signed-off-by: Tom Rix
---
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 9eb9b440bd438..159b97c0b4ebc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1676,7 +1676,7 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
{
struct amdgpu_device *adev = (struct amdgpu_device
*)file_inode(f)->i_private;
char reg_offset[11];
- uint32_t *tmp;
+ uint32_t *tmp = NULL;
int ret, i = 0, len = 0;
do {
@@ -1688,6 +1688,10 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
}
tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ if (!tmp) {
+ ret = -ENOMEM;
+ goto error_free;
+ }
if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
ret = -EINVAL;
goto error_free;
--
2.26.3
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: kernel amdgpu problems
On Mon, Feb 28, 2022 at 12:22 AM Mike Lothian wrote: > On Sat, 26 Feb 2022 at 10:01, Alex fxmbsw7 Ratchev > wrote: > > > > i have observed at least two major problems of using amdgpu > > > > 1. cant be built-in instead of module, display stays blank > > Are you remembering to add in the firmware to the kernel image too? > i copied all from official linux-firmware.gif to /lib/firmware > > There's a good guide at > https://wiki.gentoo.org/wiki/AMDGPU#Known_firmware_blobs > > I've successfully used AMDGPU builtin on Tonga, Raven, Renoir and Navy > Flounder systems > > I've never used kexec so have no suggestions for that > it may be back then when i compiled it inline not as module i didnt include firmware in the initrd so that may been that about kexec, i dunno, X dri doesnt work after first kexec, after second screen at all is blank driver init problems .. cheers..
Re: [PATCH 3/6] treewide: fix incorrect use to determine if list is empty
On Mon, Feb 28, 2022 at 12:08:19PM +0100, Jakob Koschel wrote: > The list iterator value will *always* be set by list_for_each_entry(). > It is incorrect to assume that the iterator value will be NULL if the > list is empty. > > Instead of checking the pointer it should be checked if > the list is empty. > In acpi_get_pmu_hw_inf() instead of setting the pointer to NULL > on the break, it is set to the correct value and leaving it > NULL if no element was found. > > Signed-off-by: Jakob Koschel > --- > arch/powerpc/sysdev/fsl_gtm.c| 4 ++-- > drivers/media/pci/saa7134/saa7134-alsa.c | 4 ++-- > drivers/perf/xgene_pmu.c | 13 +++-- > 3 files changed, 11 insertions(+), 10 deletions(-) These are all bug fixes. 1) Send them as 3 separate patches. 2) Add Fixes tags. regards, dan carpenter
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/usb/gadget/udc/at91_udc.c
> b/drivers/usb/gadget/udc/at91_udc.c
> index 9040a0561466..0fd0307bc07b 100644
> --- a/drivers/usb/gadget/udc/at91_udc.c
> +++ b/drivers/usb/gadget/udc/at91_udc.c
> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct
> at91_ep *ep)
> if (list_empty (>queue))
> seq_printf(s, "\t(queue empty)\n");
>
> - else list_for_each_entry (req, >queue, queue) {
> - unsignedlength = req->req.actual;
> + else
> + list_for_each_entry(req, >queue, queue) {
> + unsigned intlength = req->req.actual;
>
> - seq_printf(s, "\treq %p len %d/%d buf %p\n",
> - >req, length,
> - req->req.length, req->req.buf);
> - }
> + seq_printf(s, "\treq %p len %d/%d buf %p\n",
> + >req, length,
> + req->req.length, req->req.buf);
> + }
Don't make unrelated white space changes. It just makes the patch
harder to review. As you're writing the patch make note of any
additional changes and do them later in a separate patch.
Also a multi-line indent gets curly braces for readability even though
it's not required by C. And then both sides would get curly braces.
> spin_unlock_irqrestore(>lock, flags);
> }
>
> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
>
> if (udc->enabled && udc->vbus) {
> proc_ep_show(s, >ep[0]);
> - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
> + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
Another unrelated change.
> if (ep->ep.desc)
> proc_ep_show(s, ep);
> }
[ snip ]
> diff --git a/drivers/usb/gadget/udc/net2272.c
> b/drivers/usb/gadget/udc/net2272.c
> index 7c38057dcb4a..bb59200f1596 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -926,7 +926,8 @@ static int
> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct net2272_ep *ep;
> - struct net2272_request *req;
> + struct net2272_request *req = NULL;
> + struct net2272_request *tmp;
> unsigned long flags;
> int stopped;
>
> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> ep->stopped = 1;
>
> /* make sure it's still queued on this endpoint */
> - list_for_each_entry(req, >queue, queue) {
> - if (>req == _req)
> + list_for_each_entry(tmp, >queue, queue) {
> + if (>req == _req) {
> + req = tmp;
> break;
> + }
> }
> - if (>req != _req) {
> + if (!req) {
> ep->stopped = stopped;
> spin_unlock_irqrestore(>dev->lock, flags);
> return -EINVAL;
> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request
> *_req)
> dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
> net2272_done(ep, req, -ECONNRESET);
> }
> - req = NULL;
Another unrelated change. These are all good changes but send them as
separate patches.
> ep->stopped = stopped;
>
> spin_unlock_irqrestore(>dev->lock, flags);
regards,
dan carpenter
Re: [drm/selftests] 39ec47bbfd: kernel_BUG_at_drivers/gpu/drm/drm_buddy.c
Arun can you take a look at that one here?
It looks like a real problem to me and not just a potential false
negative like the other issue.
Thanks,
Christian.
Am 27.02.22 um 16:18 schrieb kernel test robot:
Greeting,
FYI, we noticed the following commit (built with gcc-9):
commit: 39ec47bbfd5dd3cea0b711ee9f1acdca37399c86 ("[PATCH v2 2/7] drm/selftests: add
drm buddy alloc limit testcase")
url:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FArunpravin%2Fdrm-selftests-Move-i915-buddy-selftests-into-drm%2F20220223-015043data=04%7C01%7Cchristian.koenig%40amd.com%7C3101ff318a994e6eaf5f08d9fa0481ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637815719552700496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=sKvsDtHufRMfSO14HdmHxvNsJiPyDZVDXCFUpWTDwFI%3Dreserved=0
patch link:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20220222174845.2175-2-Arunpravin.PaneerSelvam%40amd.comdata=04%7C01%7Cchristian.koenig%40amd.com%7C3101ff318a994e6eaf5f08d9fa0481ea%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637815719552700496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=aWG4x27aMLcOySOUkHbLQ1NL9L8t8AF4dgXux65IIP8%3Dreserved=0
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu Icelake-Server -smp 4 -m
16G
caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
+---+++
| | be9e8c6c00 | 39ec47bbfd |
+---+++
| boot_successes| 14 | 0 |
| boot_failures | 0 | 16 |
| UBSAN:shift-out-of-bounds_in_include/linux/log2.h | 0 | 16 |
| kernel_BUG_at_drivers/gpu/drm/drm_buddy.c | 0 | 16 |
| invalid_opcode:#[##] | 0 | 16 |
| EIP:drm_buddy_init| 0 | 16 |
| Kernel_panic-not_syncing:Fatal_exception | 0 | 16 |
+---+++
If you fix the issue, kindly add following tag
Reported-by: kernel test robot
[ 68.124177][T1] UBSAN: shift-out-of-bounds in include/linux/log2.h:67:13
[ 68.125333][T1] shift exponent 4294967295 is too large for 32-bit type
'long unsigned int'
[ 68.126563][T1] CPU: 0 PID: 1 Comm: swapper Not tainted
5.17.0-rc2-00311-g39ec47bbfd5d #2
[ 68.127758][T1] Call Trace:
[ 68.128187][ T1] dump_stack_lvl (lib/dump_stack.c:108)
[ 68.128793][ T1] dump_stack (lib/dump_stack.c:114)
[ 68.129331][ T1] ubsan_epilogue (lib/ubsan.c:152)
[ 68.129958][ T1] __ubsan_handle_shift_out_of_bounds.cold
(arch/x86/include/asm/smap.h:85)
[ 68.130791][ T1] ? drm_block_alloc+0x28/0x80
[ 68.131582][ T1] ? rcu_read_lock_sched_held (kernel/rcu/update.c:125)
[ 68.132215][ T1] ? kmem_cache_alloc (include/trace/events/kmem.h:54
mm/slab.c:3501)
[ 68.132878][ T1] ? mark_free+0x2e/0x80
[ 68.133524][ T1] drm_buddy_init.cold (include/linux/log2.h:67
drivers/gpu/drm/drm_buddy.c:131)
[ 68.134145][ T1] ? test_drm_cmdline_init
(drivers/gpu/drm/selftests/test-drm_buddy.c:87)
[ 68.134770][ T1] igt_buddy_alloc_limit
(drivers/gpu/drm/selftests/test-drm_buddy.c:30)
[ 68.135472][ T1] ? vprintk_default (kernel/printk/printk.c:2257)
[ 68.136057][ T1] ? test_drm_cmdline_init
(drivers/gpu/drm/selftests/test-drm_buddy.c:87)
[ 68.136812][ T1] test_drm_buddy_init
(drivers/gpu/drm/selftests/drm_selftest.c:77
drivers/gpu/drm/selftests/test-drm_buddy.c:95)
[ 68.137475][ T1] do_one_initcall (init/main.c:1300)
[ 68.138111][ T1] ? parse_args (kernel/params.c:609 kernel/params.c:146
kernel/params.c:188)
[ 68.138717][ T1] do_basic_setup (init/main.c:1372 init/main.c:1389
init/main.c:1408)
[ 68.139366][ T1] kernel_init_freeable (init/main.c:1617)
[ 68.140040][ T1] ? rest_init (init/main.c:1494)
[ 68.140634][ T1] kernel_init (init/main.c:1504)
[ 68.141155][ T1] ret_from_fork (arch/x86/entry/entry_32.S:772)
[ 68.141607][T1]
[ 68.146730][T1] [ cut here ]
[ 68.147460][T1] kernel BUG at drivers/gpu/drm/drm_buddy.c:140!
[ 68.148280][T1] invalid opcode: [#1]
[ 68.148895][T1] CPU: 0 PID: 1 Comm: swapper Not tainted
5.17.0-rc2-00311-g39ec47bbfd5d #2
[ 68.149896][ T1] EIP: drm_buddy_init (drivers/gpu/drm/drm_buddy.c:140
(discriminator 1))
[ 68.149896][ T1] Code: 76 00 b8 ea ff ff ff 8d 65 f4 5b 5e 5f 5d c3 8d 76 00 0f bd
45 d8 75 05 b8 ff ff ff ff 83 c0 21 e9 5e ff ff ff 8d 74 26 00 90 <0f> 0b 8d b6
00 00 00 00 0f 0b
Re: [PATCH v2] drm/amdgpu: Fix realloc of ptr
Am 27.02.22 um 16:33 schrieb t...@redhat.com:
From: Tom Rix
Clang static analysis reports this error
amdgpu_debugfs.c:1690:9: warning: 1st function call
argument is an uninitialized value
tmp = krealloc_array(tmp, i + 1,
^~~
realloc uses tmp, so tmp can not be garbage.
And the return needs to be checked.
Fixes: 5ce5a584cb82 ("drm/amdgpu: add debugfs for reset registers list")
Signed-off-by: Tom Rix
Yeah, stuff I missed because of the long review. I was already wondering
what semantics krealloc_array is following for freeing up the pointer on
error.
Reviewed-by: Christian König
Thanks,
Christian.
---
v2:
use 'new' to hold/check the ralloc return
fix commit log mistake on ralloc freeing to using input ptr
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 9eb9b440bd438..2f4f8c5618d81 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1676,7 +1676,7 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
{
struct amdgpu_device *adev = (struct amdgpu_device
*)file_inode(f)->i_private;
char reg_offset[11];
- uint32_t *tmp;
+ uint32_t *new, *tmp = NULL;
int ret, i = 0, len = 0;
do {
@@ -1687,7 +1687,12 @@ static ssize_t
amdgpu_reset_dump_register_list_write(struct file *f,
goto error_free;
}
- tmp = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ new = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
+ if (!new) {
+ ret = -ENOMEM;
+ goto error_free;
+ }
+ tmp = new;
if (sscanf(reg_offset, "%X %n", [i], ) != 1) {
ret = -EINVAL;
goto error_free;
[PATCH] drm/amdgpu: Refactor mode2 reset logic for v13.0.2
Use IP version and refactor reset logic to apply to a list of devices.
Signed-off-by: Lijo Lazar
Reviewed-by: Hawking Zhang
---
drivers/gpu/drm/amd/amdgpu/aldebaran.c| 66 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_reset.c | 8 +--
2 files changed, 54 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
index a545df4efce1..c6cc493a5486 100644
--- a/drivers/gpu/drm/amd/amdgpu/aldebaran.c
+++ b/drivers/gpu/drm/amd/amdgpu/aldebaran.c
@@ -31,6 +31,17 @@
#include "amdgpu_psp.h"
#include "amdgpu_xgmi.h"
+static bool aldebaran_is_mode2_default(struct amdgpu_reset_control *reset_ctl)
+{
+ struct amdgpu_device *adev = (struct amdgpu_device *)reset_ctl->handle;
+
+ if ((adev->ip_versions[MP1_HWIP][0] == IP_VERSION(13, 0, 2) &&
+adev->gmc.xgmi.connected_to_cpu))
+ return true;
+
+ return false;
+}
+
static struct amdgpu_reset_handler *
aldebaran_get_reset_handler(struct amdgpu_reset_control *reset_ctl,
struct amdgpu_reset_context *reset_context)
@@ -48,7 +59,7 @@ aldebaran_get_reset_handler(struct amdgpu_reset_control
*reset_ctl,
}
}
- if (adev->gmc.xgmi.connected_to_cpu) {
+ if (aldebaran_is_mode2_default(reset_ctl)) {
list_for_each_entry(handler, _ctl->reset_handlers,
handler_list) {
if (handler->reset_method == AMD_RESET_METHOD_MODE2) {
@@ -136,18 +147,31 @@ static int
aldebaran_mode2_perform_reset(struct amdgpu_reset_control *reset_ctl,
struct amdgpu_reset_context *reset_context)
{
- struct amdgpu_device *tmp_adev = NULL;
struct amdgpu_device *adev = (struct amdgpu_device *)reset_ctl->handle;
+ struct amdgpu_device *tmp_adev = NULL;
+ struct list_head reset_device_list;
int r = 0;
dev_dbg(adev->dev, "aldebaran perform hw reset\n");
- if (reset_context->hive == NULL) {
+ if (adev->ip_versions[MP1_HWIP][0] == IP_VERSION(13, 0, 2) &&
+ reset_context->hive == NULL) {
/* Wrong context, return error */
return -EINVAL;
}
- list_for_each_entry(tmp_adev, _context->hive->device_list,
-gmc.xgmi.head) {
+ INIT_LIST_HEAD(_device_list);
+ if (reset_context->hive) {
+ list_for_each_entry (tmp_adev,
+_context->hive->device_list,
+gmc.xgmi.head)
+ list_add_tail(_adev->reset_list,
+ _device_list);
+ } else {
+ list_add_tail(_context->reset_req_dev->reset_list,
+ _device_list);
+ }
+
+ list_for_each_entry (tmp_adev, _device_list, reset_list) {
mutex_lock(_adev->reset_cntl->reset_lock);
tmp_adev->reset_cntl->active_reset = AMD_RESET_METHOD_MODE2;
}
@@ -155,8 +179,7 @@ aldebaran_mode2_perform_reset(struct amdgpu_reset_control
*reset_ctl,
* Mode2 reset doesn't need any sync between nodes in XGMI hive,
instead launch
* them together so that they can be completed asynchronously on
multiple nodes
*/
- list_for_each_entry(tmp_adev, _context->hive->device_list,
-gmc.xgmi.head) {
+ list_for_each_entry (tmp_adev, _device_list, reset_list) {
/* For XGMI run all resets in parallel to speed up the process
*/
if (tmp_adev->gmc.xgmi.num_physical_nodes > 1) {
if (!queue_work(system_unbound_wq,
@@ -174,9 +197,7 @@ aldebaran_mode2_perform_reset(struct amdgpu_reset_control
*reset_ctl,
/* For XGMI wait for all resets to complete before proceed */
if (!r) {
- list_for_each_entry(tmp_adev,
-_context->hive->device_list,
-gmc.xgmi.head) {
+ list_for_each_entry (tmp_adev, _device_list, reset_list) {
if (tmp_adev->gmc.xgmi.num_physical_nodes > 1) {
flush_work(_adev->reset_cntl->reset_work);
r = tmp_adev->asic_reset_res;
@@ -186,8 +207,7 @@ aldebaran_mode2_perform_reset(struct amdgpu_reset_control
*reset_ctl,
}
}
- list_for_each_entry(tmp_adev, _context->hive->device_list,
-gmc.xgmi.head) {
+ list_for_each_entry (tmp_adev, _device_list, reset_list) {
mutex_unlock(_adev->reset_cntl->reset_lock);
tmp_adev->reset_cntl->active_reset = AMD_RESET_METHOD_NONE;
}
@@ -319,16 +339,30 @@ static int
aldebaran_mode2_restore_hwcontext(struct amdgpu_reset_control *reset_ctl,
struct
