Re: [PATCH 2/9] drm/amdgpu/discovery: Annotate struct ip_hw_instance with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct ip_hw_instance.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Hawking Zhang
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index d1bc7b212520..be4c97a3d7bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -662,7 +662,7 @@ struct ip_hw_instance {
u8 harvest;
int num_base_addresses;
- u32 base_addr[];
+ u32 base_addr[] __counted_by(num_base_addresses);
};
struct ip_hw_id {
Re: [PATCH 7/9] drm/virtio: Annotate struct virtio_gpu_object_array with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
virtio_gpu_object_array.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: David Airlie
Cc: Gerd Hoffmann
Cc: Gurchetan Singh
Cc: Chia-I Wu
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: virtualizat...@lists.linux-foundation.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/virtio/virtgpu_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 8513b671f871..96365a772f77 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -119,7 +119,7 @@ struct virtio_gpu_object_array {
struct ww_acquire_ctx ticket;
struct list_head next;
u32 nents, total;
- struct drm_gem_object *objs[];
+ struct drm_gem_object *objs[] __counted_by(total);
};
struct virtio_gpu_vbuffer;
Re: [PATCH 6/9] drm/vc4: Annotate struct vc4_perfmon with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vc4_perfmon.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Emma Anholt
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/vc4/vc4_drv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index bf66499765fb..ab61e96e7e14 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -76,7 +76,7 @@ struct vc4_perfmon {
* Note that counter values can't be reset, but you can fake a reset by
* destroying the perfmon and creating a new one.
*/
- u64 counters[];
+ u64 counters[] __counted_by(ncounters);
};
struct vc4_dev {
Re: [PATCH 4/9] drm/msm/dpu: Annotate struct dpu_hw_intr with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct dpu_hw_intr.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Rob Clark
Cc: Abhinav Kumar
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Marijn Suijten
Cc: David Airlie
Cc: Daniel Vetter
Cc: Bjorn Andersson
Cc: linux-arm-...@vger.kernel.org
Cc: dri-de...@lists.freedesktop.org
Cc: freedr...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
index dab761e54863..50cf9523d367 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h
@@ -61,7 +61,7 @@ struct dpu_hw_intr {
void (*cb)(void *arg, int irq_idx);
void *arg;
atomic_t count;
- } irq_tbl[];
+ } irq_tbl[] __counted_by(total_irqs);
};
/**
Re: [PATCH 8/9] drm/vmwgfx: Annotate struct vmw_surface_dirty with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct vmw_surface_dirty.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Zack Rusin
Cc: VMware Graphics Reviewers
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 5db403ee8261..2d1d857f99ae 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -77,7 +77,7 @@ struct vmw_surface_offset {
struct vmw_surface_dirty {
struct vmw_surface_cache cache;
u32 num_subres;
- SVGA3dBox boxes[];
+ SVGA3dBox boxes[] __counted_by(num_subres);
};
static void vmw_user_surface_free(struct vmw_resource *res);
Re: [PATCH 5/9] drm/nouveau/pm: Annotate struct nvkm_perfdom with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct nvkm_perfdom.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
index 6ae25d3e7f45..c011227f7052 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/pm/priv.h
@@ -82,7 +82,7 @@ struct nvkm_perfdom {
u8 mode;
u32 clk;
u16 signal_nr;
- struct nvkm_perfsig signal[];
+ struct nvkm_perfsig signal[] __counted_by(signal_nr);
};
struct nvkm_funcdom {
Re: [PATCH 1/9] drm/amd/pm: Annotate struct smu10_voltage_dependency_table with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct
smu10_voltage_dependency_table.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Evan Quan
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: Xiaojian Du
Cc: Huang Rui
Cc: Kevin Wang
Cc: amd-gfx@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
index 808e0ecbe1f0..42adc2a3dcbc 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.h
@@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record {
struct smu10_voltage_dependency_table {
uint32_t count;
- struct smu10_clock_voltage_dependency_record entries[];
+ struct smu10_clock_voltage_dependency_record entries[]
__counted_by(count);
};
struct smu10_clock_voltage_information {
Re: [PATCH 3/9] drm/i915/selftests: Annotate struct perf_series with __counted_by
On 9/22/23 11:32, Kees Cook wrote:
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct perf_series.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: David Airlie
Cc: Daniel Vetter
Cc: Chris Wilson
Cc: John Harrison
Cc: Andi Shyti
Cc: Matthew Brost
Cc: intel-...@lists.freedesktop.org
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Kees Cook
Reviewed-by: Gustavo A. R. Silva
Thanks
--
Gustavo
---
drivers/gpu/drm/i915/selftests/i915_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/selftests/i915_request.c
b/drivers/gpu/drm/i915/selftests/i915_request.c
index a9b79888c193..acae30a04a94 100644
--- a/drivers/gpu/drm/i915/selftests/i915_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_request.c
@@ -1924,7 +1924,7 @@ struct perf_stats {
struct perf_series {
struct drm_i915_private *i915;
unsigned int nengines;
- struct intel_context *ce[];
+ struct intel_context *ce[] __counted_by(nengines);
};
static int cmp_u32(const void *A, const void *B)
Re: [PATCH] drm/amdgpu: replace 1-element arrays with flexible arrays
On 7/12/23 08:12, Alex Deucher wrote: On Wed, Jul 12, 2023 at 8:04 AM Ricardo Cañuelo wrote: UBSAN complains about out-of-bounds array indexes on all 1-element arrays defined on this driver: UBSAN: array-index-out-of-bounds in /home/rcn/work/repos/kernelci/kernelci-core/linux_kernel_mainline/drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/processpptables.c:1249:61 Substitute them with proper flexible arrays. + Gustavo, Paulo I haven't kept up with the flexible arrays stuff. Is this equivalent to a zero sized array? We've been bitten by these kind of changes in In terms of size, yes: the size of each array declaration does not contribute to the overall size of its containing structure. However, in these cases, using the DECLARE_FLEX_ARRAY() helper is not required. Simply removing the '1' from the array declaration will suffice. This helper was created to declare flex-array members in unions, as well as in structs that contain no other members aside from the array. In any case, these changes are not complete, as they're only modifying the struct declaration, hence the size of the struct is affected. Now the rest of the code where these structs are involved should be audited and adjusted to accommodate the change in the sizes of the structs. the past. These structures define the layout of data in a rom image on the board. If the struct size changes, that could lead to errors in the code that deals with these structures. Alex Thanks -- Gustavo
[PATCH][next] drm/amdgpu/discovery: Replace fake flex-arrays with flexible-array members
Zero-length and one-element arrays are deprecated, and we are moving
towards adopting C99 flexible-array members, instead.
Use the DECLARE_FLEX_ARRAY() helper macro to transform zero-length
arrays in a union into flexible-array members. And replace a one-element
array with a C99 flexible-array member.
Address the following warnings found with GCC-13 and
-fstrict-flex-arrays=3 enabled:
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1009:89: warning: array subscript
kk is outside array bounds of ‘uint32_t[0]’ {aka ‘unsigned int[]’}
[-Warray-bounds=]
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1007:94: warning: array subscript
kk is outside array bounds of ‘uint64_t[0]’ {aka ‘long long unsigned int[]’}
[-Warray-bounds=]
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1310:94: warning: array subscript
k is outside array bounds of ‘uint64_t[0]’ {aka ‘long long unsigned int[]’}
[-Warray-bounds=]
drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c:1309:57: warning: array subscript
k is outside array bounds of ‘uint32_t[0]’ {aka ‘unsigned int[]’}
[-Warray-bounds=]
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [1].
This results in no differences in binary output.
Link: https://github.com/KSPP/linux/issues/21
Link: https://github.com/KSPP/linux/issues/193
Link: https://github.com/KSPP/linux/issues/300
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [1]
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/include/discovery.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/discovery.h
b/drivers/gpu/drm/amd/include/discovery.h
index 9181e57887db..f43e29722ef7 100644
--- a/drivers/gpu/drm/amd/include/discovery.h
+++ b/drivers/gpu/drm/amd/include/discovery.h
@@ -122,7 +122,7 @@ typedef struct ip_v3
uint8_t sub_revision : 4; /* HCID Sub-Revision */
uint8_t variant : 4;/* HW variant */
#endif
- uint32_t base_address[1]; /* Base Address list.
Corresponds to the num_base_address field*/
+ uint32_t base_address[];/* Base Address list.
Corresponds to the num_base_address field*/
} ip_v3;
typedef struct ip_v4 {
@@ -140,8 +140,8 @@ typedef struct ip_v4 {
uint8_t sub_revision : 4; /* HCID Sub-Revision */
#endif
union {
- uint32_t base_address[0]; /* 32-bit Base Address
list. Corresponds to the num_base_address field*/
- uint64_t base_address_64[0];/* 64-bit Base Address
list. Corresponds to the num_base_address field*/
+ DECLARE_FLEX_ARRAY(uint32_t, base_address); /* 32-bit Base
Address list. Corresponds to the num_base_address field*/
+ DECLARE_FLEX_ARRAY(uint64_t, base_address_64); /* 64-bit Base
Address list. Corresponds to the num_base_address field*/
} __packed;
} ip_v4;
--
2.34.1
Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members
On Wed, Nov 09, 2022 at 10:20:34PM -0500, Alex Deucher wrote:
> On Wed, Nov 9, 2022 at 8:31 PM Paulo Miguel Almeida
> wrote:
> >
> > On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> > > On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> > >
> > > Adding Alex, Christian and DRM lists to the thread.
> >
> > Thanks so much for your reply Gustavo
> > Yep, that's a good idea.
> >
> > >
> > > > struct _ATOM_INIT_REG_BLOCK {
> > > > USHORT usRegIndexTblSize;/* 0 2 */
> > > > USHORT usRegDataBlkSize; /* 2 2 */
> > > > ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1]; /* 4 3 */
> > > > ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1]; /* 7 8 */
> > >
> > > I didn't find evidence that asRegDataBuf is used anywhere in the
> > > codebase[1].
> > > ...
> > >
> > > ...
> > > As I pointed out above, I don't see asRegDataBuf[] being used in the
> > > codebase (of course it may describe firmware memory layout). Instead,
> > > there is this jump to a block of data past asRegIndexBuf[]:
> > >
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> > > 1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> > > 1445: (ATOM_MEMORY_SETTING_DATA_BLOCK *)
> > > 1446: ((u8 *)reg_block + (2 * sizeof(u16)) +
> > > 1447: le16_to_cpu(reg_block->usRegIndexTblSize));
> > >
> > > So, it seems the one relevant array, from the kernel side, is
> > > asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> > > structure... or if we could try modifying that struct and only have
> > > asRegIndexBuf[] as last member? and then we can transform it into a
> > > flex-array member.
> >
> > I saw that one too. That would be the way it's currently accessing
> > asRegDataBuf member as the existing struct would make asRegDataBuf[0] point
> > to some index within the asRegIndexBuf member (as you probably got it too)
> >
> > you are right... asRegDataBuff isn't used from a static analysis
> > point of view but removing it make the code a bit cryptic, right?
> >
> > That's pickle, ay? :-)
> >
> > >
> > > If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> > > could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> > > in the structure.
> > >
> >
> > Out of curiosity, why both rather than just asRegIndexBuf?
Because if I understand the code and Alex's reply below correctly, both
arrays are being used to describe data of variable size, and both arrays
need to stay. The situation is that it'd be _strange_ to transform just
one of them into a flex-array member and not the other, and at the same
time a flex-array member may only appear as the last member of a
struct, and it's not _great_ to have more than one flex-array member
in a struct --in fact, we can't.
DECLARE_FLEX_ARRAY() was originally designed to have flex-array members
in unions. This is, we can declare multiple flex-array members as long as
they all share the same address. Unfortunately, this is not the case with
asRegIndexBuf[] and asRegDataBuf[], and I don't see[1] DECLARE_FLEX_ARRAY()
working in this case. So, nope, we cannot use it to declare both arrays
and we also cannot have a flex-array in the middle of a struct, so we
cannot use it to declare asRegIndexBuf[] alone. :/
On the other hand, I fail to see how the current state of the code is
problematic for things like -fstrict-flex-arrays, right now. asRegDataBuf[]
is not being used for anything in the kernel, and asRegIndexBuf[] is
correctly being accessed through it's only valid index zero, after which
is decays into a pointer[2].
That struct is definitely not great (I don't love it), but we might try
to explore other cases while we determine how and if we ultimately need
to modify it.
I'll open an issue for this in the bug tracker, so we keep an eye on it.
:)
> >
> > > But first, of course, Alex, Christian, it'd be really great if we can
> > > have your input and feedback. :)
>
> This header describes the layout of memory stored in the ROM on the
> GPU. It's shared between vbios and driver so some parts aren't
> necessarily directly used by the driver. As for what to do about it,
> I'm not sure. This structure stores a set of register offsets
> (asRegIndexBuf) and data values (asRegDataBuf) for specific operations
> (e.g., walk this structure and program register X with value Y. For a
> little background on atombios, it's a
Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members
On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
Adding Alex, Christian and DRM lists to the thread.
> Hi KSPP community,
>
> I've been working on replacing 1-element arrays with flex array members
> on the drm/amdgpu files. I came across one insteresting case which I
> may need to pick your brains to find a solution for it.
>
> The structure below has two fake flexible arrays but I would get an
> error if I try make them both FAM. How should/could I deal with the
> asRegIndexBuf in this case? In theory, DECLARE_FLEX_ARRAY would "work"
> but that doesn't seem to be its intended usage as far I've searched.
> (unless I got it wrong, if that's the case, feel free to set me straight)
>
> Any ideas?
>
> struct _ATOM_INIT_REG_BLOCK {
> USHORT usRegIndexTblSize;/* 0 2 */
> USHORT usRegDataBlkSize; /* 2 2 */
> ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1]; /* 4 3 */
> ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1]; /* 7 8 */
I didn't find evidence that asRegDataBuf is used anywhere in the
codebase[1].
>
> /* size: 15, cachelines: 1, members: 4 */
> /* last cacheline: 15 bytes */
> } __attribute__((__packed__));
Alex, Christian,
It looks like this structure is only being used as a template to populate
instances of struct atom_mc_reg_table[2] and that these[3] are the only
places where asRegIndexBuf[] is being used. Apparently, this array is only
being used to retrieve it's address so that a pointer can jump[4] in chucks
of size sizeof(ATOM_INIT_REG_INDEX_FORMAT):
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1461:
1461: format = (ATOM_INIT_REG_INDEX_FORMAT *)
1462: ((u8 *)format + sizeof(ATOM_INIT_REG_INDEX_FORMAT));
up to (VBIOS_MC_REGISTER_ARRAY_SIZE * sizeof(ATOM_INIT_REG_INDEX_FORMAT))[5],
As I pointed out above, I don't see asRegDataBuf[] being used in the
codebase (of course it may describe firmware memory layout). Instead,
there is this jump to a block of data past asRegIndexBuf[]:
drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
1445: (ATOM_MEMORY_SETTING_DATA_BLOCK *)
1446: ((u8 *)reg_block + (2 * sizeof(u16)) +
1447:le16_to_cpu(reg_block->usRegIndexTblSize));
So, it seems the one relevant array, from the kernel side, is
asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
structure... or if we could try modifying that struct and only have
asRegIndexBuf[] as last member? and then we can transform it into a
flex-array member.
If for any strong reasong we cannot remove asRegDataBuf[] then I think we
could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
in the structure.
But first, of course, Alex, Christian, it'd be really great if we can
have your input and feedback. :)
Thanks!
--
Gustavo
[1] https://elixir.bootlin.com/linux/latest/C/ident/asRegDataBuf
[2]
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c#L1441
[3] https://elixir.bootlin.com/linux/latest/C/ident/asRegIndexBuf
Re: [PATCH][next] drm/amd/display: Fix Wstringop-overflow warnings in dc_link_dp.c
On Thu, Mar 03, 2022 at 12:19:57PM -0600, Gustavo A. R. Silva wrote:
> On Thu, Mar 03, 2022 at 09:43:28AM -0800, Kees Cook wrote:
> > On Thu, Mar 03, 2022 at 11:25:03AM -0600, Gustavo A. R. Silva wrote:
> > > Fix the following Wstringop-overflow warnings when building with GCC-11:
> > >
> > > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17:
> > > warning: ‘dp_decide_lane_settings’ accessing 4 bytes in a region of size
> > > 1 [-Wstringop-overflow=]
> >
> > Can you "show your work" a little more here? I don't actually see the
> > what is getting fixed:
> >
> > enum dc_lane_count {
> > ...
> > LANE_COUNT_FOUR = 4,
> > ...
> > LANE_COUNT_DP_MAX = LANE_COUNT_FOUR
> > };
> >
> > struct link_training_settings {
> > ...
> > union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX];
> > };
> >
> > void dp_hw_to_dpcd_lane_settings(
> > ...
> > union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX])
> > {
> > ...
> > }
> >
> > static enum link_training_result dpia_training_cr_transparent(
> > ...
> > struct link_training_settings *lt_settings)
> > {
> > ...
> > dp_decide_lane_settings(lt_settings, dpcd_lane_adjust,
> > lt_settings->hw_lane_settings,
> > lt_settings->dpcd_lane_settings);
> > ...
> > }
> >
> > Everything looks to be the correct size?
>
> Yep; this fix is similar to the one for intel_pm.c in this
>
> commit e7c6e405e171fb33990a12ecfd14e6500d9e5cf2
>
> where the array size of 8 seems to be fine for all the
> struct members related (pri_latency, spr_latency, cur_latency
> and skl_latency):
>
> drivers/gpu/drm/i915/i915_drv.h:465:struct drm_i915_private {
> ...
>
> drivers/gpu/drm/i915/i915_drv.h-739-struct {
>
> ...
> drivers/gpu/drm/i915/i915_drv.h-745-/* primary */
> drivers/gpu/drm/i915/i915_drv.h-746-u16 pri_latency[5];
> drivers/gpu/drm/i915/i915_drv.h-747-/* sprite */
> drivers/gpu/drm/i915/i915_drv.h-748-u16 spr_latency[5];
> drivers/gpu/drm/i915/i915_drv.h-749-/* cursor */
> drivers/gpu/drm/i915/i915_drv.h-750-u16 cur_latency[5];
> drivers/gpu/drm/i915/i915_drv.h-751-/*
> drivers/gpu/drm/i915/i915_drv.h-752- * Raw watermark memory
> latency values
> drivers/gpu/drm/i915/i915_drv.h-753- * for SKL for all 8 levels
> drivers/gpu/drm/i915/i915_drv.h-754- * in 1us units.
> drivers/gpu/drm/i915/i915_drv.h-755- */
> drivers/gpu/drm/i915/i915_drv.h-756-u16 skl_latency[8];
>
> ...
> drivers/gpu/drm/i915/i915_drv.h-773-} wm;
> ...
> }
and in this case the ilk_wm_max_level() returns the right maximum size for the
corresponding 'struct wm' member:
drivers/gpu/drm/i915/intel_pm.c:2993:int ilk_wm_max_level(const struct
drm_i915_private *dev_priv)
drivers/gpu/drm/i915/intel_pm.c-2994-{
drivers/gpu/drm/i915/intel_pm.c-2995- /* how many WM levels are we expecting
*/
drivers/gpu/drm/i915/intel_pm.c-2996- if (HAS_HW_SAGV_WM(dev_priv))
drivers/gpu/drm/i915/intel_pm.c-2997- return 5;
drivers/gpu/drm/i915/intel_pm.c-2998- else if (DISPLAY_VER(dev_priv) >= 9)
drivers/gpu/drm/i915/intel_pm.c-2999- return 7;
drivers/gpu/drm/i915/intel_pm.c-3000- else if (IS_HASWELL(dev_priv) ||
IS_BROADWELL(dev_priv))
drivers/gpu/drm/i915/intel_pm.c-3001- return 4;
drivers/gpu/drm/i915/intel_pm.c-3002- else if (DISPLAY_VER(dev_priv) >= 6)
drivers/gpu/drm/i915/intel_pm.c-3003- return 3;
drivers/gpu/drm/i915/intel_pm.c-3004- else
drivers/gpu/drm/i915/intel_pm.c-3005- return 2;
drivers/gpu/drm/i915/intel_pm.c-3006-}
drivers/gpu/drm/i915/intel_pm.c:3009:static void intel_print_wm_latency(struct
drm_i915_private *dev_priv,
drivers/gpu/drm/i915/intel_pm.c-3010- const char
*name,
drivers/gpu/drm/i915/intel_pm.c-3011- const u16
wm[])
drivers/gpu/drm/i915/intel_pm.c-3012-{
drivers/gpu/drm/i915/intel_pm.c-3013- int level, max_level =
ilk_wm_max_level(dev_priv);
drivers/gpu/drm/i915/intel_pm.c-3014-
drivers/gpu/drm/i915/intel_pm.c-3015- for (level = 0; level <= max_level;
level++) {
drivers/gpu/drm/i915/intel_pm.c-3016- unsigned int latency =
wm[level];
drivers/gpu/drm/i915/intel_pm.c-3017-
...
}
still GCC warns about this with Wstringop-overread, as it is explained
in commit e7c6e405e171.
--
Gustavo
>
> however GCC warns about accessing bytes beyond the limits, and turning the
> argument declarations into pointers (removing the over-specified array
> size from the argument declaration) silence the warnings.
>
> --
> Gustavo
Re: [PATCH][next] drm/amd/display: Fix Wstringop-overflow warnings in dc_link_dp.c
On Thu, Mar 03, 2022 at 09:43:28AM -0800, Kees Cook wrote:
> On Thu, Mar 03, 2022 at 11:25:03AM -0600, Gustavo A. R. Silva wrote:
> > Fix the following Wstringop-overflow warnings when building with GCC-11:
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17:
> > warning: ‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
> > [-Wstringop-overflow=]
>
> Can you "show your work" a little more here? I don't actually see the
> what is getting fixed:
>
> enum dc_lane_count {
> ...
> LANE_COUNT_FOUR = 4,
> ...
> LANE_COUNT_DP_MAX = LANE_COUNT_FOUR
> };
>
> struct link_training_settings {
> ...
> union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX];
> };
>
> void dp_hw_to_dpcd_lane_settings(
> ...
> union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX])
> {
> ...
> }
>
> static enum link_training_result dpia_training_cr_transparent(
> ...
> struct link_training_settings *lt_settings)
> {
> ...
> dp_decide_lane_settings(lt_settings, dpcd_lane_adjust,
> lt_settings->hw_lane_settings,
> lt_settings->dpcd_lane_settings);
> ...
> }
>
> Everything looks to be the correct size?
Yep; this fix is similar to the one for intel_pm.c in this
commit e7c6e405e171fb33990a12ecfd14e6500d9e5cf2
where the array size of 8 seems to be fine for all the
struct members related (pri_latency, spr_latency, cur_latency
and skl_latency):
drivers/gpu/drm/i915/i915_drv.h:465:struct drm_i915_private {
...
drivers/gpu/drm/i915/i915_drv.h-739-struct {
...
drivers/gpu/drm/i915/i915_drv.h-745-/* primary */
drivers/gpu/drm/i915/i915_drv.h-746-u16 pri_latency[5];
drivers/gpu/drm/i915/i915_drv.h-747-/* sprite */
drivers/gpu/drm/i915/i915_drv.h-748-u16 spr_latency[5];
drivers/gpu/drm/i915/i915_drv.h-749-/* cursor */
drivers/gpu/drm/i915/i915_drv.h-750-u16 cur_latency[5];
drivers/gpu/drm/i915/i915_drv.h-751-/*
drivers/gpu/drm/i915/i915_drv.h-752- * Raw watermark memory latency
values
drivers/gpu/drm/i915/i915_drv.h-753- * for SKL for all 8 levels
drivers/gpu/drm/i915/i915_drv.h-754- * in 1us units.
drivers/gpu/drm/i915/i915_drv.h-755- */
drivers/gpu/drm/i915/i915_drv.h-756-u16 skl_latency[8];
...
drivers/gpu/drm/i915/i915_drv.h-773-} wm;
...
}
however GCC warns about accessing bytes beyond the limits, and turning the
argument declarations into pointers (removing the over-specified array
size from the argument declaration) silence the warnings.
--
Gustavo
[PATCH][next] drm/amd/display: Fix Wstringop-overflow warnings in dc_link_dp.c
Fix the following Wstringop-overflow warnings when building with GCC-11:
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:493:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:388:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:388:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dpia.c:388:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:1491:17: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:2613:25: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_dp.c:2613:25: warning:
‘dp_decide_lane_settings’ accessing 4 bytes in a region of size 1
[-Wstringop-overflow=]
by removing the over-specified array size from the argument declarations.
This helps with the ongoing efforts to globally enable
-Wstringop-overflow.
Link: https://github.com/KSPP/linux/issues/181
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c | 4 ++--
drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
index f11a8d97fb60..81952e07acf6 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
@@ -823,7 +823,7 @@ bool dp_is_interlane_aligned(union
lane_align_status_updated align_status)
void dp_hw_to_dpcd_lane_settings(
const struct link_training_settings *lt_settings,
const struct dc_lane_settings
hw_lane_settings[LANE_COUNT_DP_MAX],
- union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX])
+ union dpcd_training_lane dpcd_lane_settings[])
{
uint8_t lane = 0;
@@ -853,7 +853,7 @@ void dp_decide_lane_settings(
const struct link_training_settings *lt_settings,
const union lane_adjust ln_adjust[LANE_COUNT_DP_MAX],
struct dc_lane_settings hw_lane_settings[LANE_COUNT_DP_MAX],
- union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX])
+ union dpcd_training_lane dpcd_lane_settings[])
{
uint32_t lane;
diff --git a/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
b/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
index ab9939db8cea..0d00da1640a7 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/dc_link_dp.h
@@ -147,12 +147,12 @@ bool dp_is_max_vs_reached(
void dp_hw_to_dpcd_lane_settings(
const struct link_training_settings *lt_settings,
const struct dc_lane_settings hw_lane_settings[LANE_COUNT_DP_MAX],
- union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX]);
+ union dpcd_training_lane dpcd_lane_settings[]);
void dp_decide_lane_settings(
const struct link_training_settings *lt_settings,
const union lane_adjust ln_adjust[LANE_COUNT_DP_MAX],
struct dc_lane_settings hw_lane_settings[LANE_COUNT_DP_MAX],
- union dpcd_training_lane dpcd_lane_settings[LANE_COUNT_DP_MAX]);
+ union dpcd_training_lane dpcd_lane_settings[]);
uint32_t dp_translate_training_aux_read_interval(uint32_t
dpcd_aux_read_interval);
--
2.27.0
[PATCH][next] drm/amd/display: Fix fall-through warning for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix the following warning by replacing a /* fall through */ comment with the new pseudo-keyword macro fallthrough: rivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:672:4: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough] case AUX_TRANSACTION_REPLY_I2C_OVER_AUX_DEFER: ^ Notice that Clang doesn't recognize /* fall through */ comments as implicit fall-through markings, so in order to globally enable -Wimplicit-fallthrough for Clang, these comments need to be replaced with fallthrough; in the whole codebase. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- JFYI: We had thousands of these sorts of warnings and now we are down to just 15 in linux-next. This is one of those last remaining warnings. drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c index 28631714f697..2fb88e54a4bf 100644 --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c @@ -668,7 +668,7 @@ bool dce_aux_transfer_with_retries(struct ddc_service *ddc, /* polling_timeout_period is in us */ defer_time_in_ms += aux110->polling_timeout_period / 1000; ++aux_defer_retries; - /* fall through */ + fallthrough; case AUX_TRANSACTION_REPLY_I2C_OVER_AUX_DEFER: retry_on_defer = true; fallthrough; -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amd/pm: Fix fall-through warning for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix a warning by explicitly adding a break statement instead of letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- JFYI: We had thousands of these sorts of warnings and now we are down to just 22 in linux-next. This is one of those last remaining warnings. :) drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c index 8f71f6a4bb49..43c3f6e755e7 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c +++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu10_hwmgr.c @@ -831,6 +831,7 @@ static int smu10_dpm_force_dpm_level(struct pp_hwmgr *hwmgr, break; case AMD_DPM_FORCED_LEVEL_MANUAL: data->fine_grain_enabled = 1; + break; case AMD_DPM_FORCED_LEVEL_PROFILE_EXIT: default: break; -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon/ni_dpm: Fix booting bug
Hi Alex,
On 5/10/21 16:17, Alex Deucher wrote:
> On Sun, May 9, 2021 at 6:48 PM Gustavo A. R. Silva
> wrote:
[..]
>>
>> Bug:
>> https://lore.kernel.org/dri-devel/3eedbe78-1fbd-4763-a7f3-ac5665e76...@xenosoft.de/
>> Fixes: 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element array
>> with flexible-array member in struct NISLANDS_SMC_SWSTATE")
>> Cc: sta...@vger.kernel.org
>> Reported-by: Christian Zigotzky
>> Tested-by: Christian Zigotzky
>> Link:
>> https://lore.kernel.org/dri-devel/9bb5fcbd-daf5-1669-b3e7-b8624b3c3...@xenosoft.de/
>> Signed-off-by: Gustavo A. R. Silva
>
> This seems like a lot of churn just to use flexible arrays. That
> said, if static checkers are going to keep complaining about single
> element arrays, I don't mind applying these patches since this code is
> not likely to change. Applied. Thanks.
This is not only about the one-element arrays. These fixes (together with
commits
434fb1e7444a and 96e27e8d919e) allow us to fix more than a dozen of these
out-of-bounds
warnings:
drivers/gpu/drm/radeon/ni_dpm.c:2521:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2521 | smc_state->levels[i].dpm2.MaxPS =
| ~^~~
which should be fixed in order to globally enable -Warray-bounds. :)
Thanks!
--
Gustavo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/pm: Fix out-of-bounds bug
Create new structure SISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
and ACPIState.levels are never actually used as flexible arrays. Those
arrays can be used as simple objects of type
SISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
Currently, the code fails because flexible array _levels_ in
struct SISLANDS_SMC_SWSTATE doesn't allow for code that accesses
the first element of initialState.levels and ACPIState.levels
arrays:
drivers/gpu/drm/amd/pm/powerplay/si_dpm.c:
4820: table->initialState.levels[0].mclk.vDLL_CNTL =
4821: cpu_to_be32(si_pi->clock_registers.dll_cntl);
...
5021: table->ACPIState.levels[0].mclk.vDLL_CNTL =
5022: cpu_to_be32(dll_cntl);
because such element cannot be accessed without previously allocating
enough dynamic memory for it to exist (which never actually happens).
So, there is an out-of-bounds bug in this case.
That's why struct SISLANDS_SMC_SWSTATE should only be used as type
for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
created as type for objects initialState, ACPIState and ULVState.
Also, with the change from one-element array to flexible-array member
in commit 0e1aa13ca3ff ("drm/amd/pm: Replace one-element array with
flexible-array in struct SISLANDS_SMC_SWSTATE"), the size of
dpmLevels in struct SISLANDS_SMC_STATETABLE should be fixed to be
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
Fixes: 0e1aa13ca3ff ("drm/amd/pm: Replace one-element array with flexible-array
in struct SISLANDS_SMC_SWSTATE")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 174 +-
.../gpu/drm/amd/pm/powerplay/sislands_smc.h | 34 ++--
2 files changed, 109 insertions(+), 99 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
index 26a5321e621b..15c0b8af376f 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
@@ -4817,70 +4817,70 @@ static int si_populate_smc_initial_state(struct
amdgpu_device *adev,
u32 reg;
int ret;
- table->initialState.levels[0].mclk.vDLL_CNTL =
+ table->initialState.level.mclk.vDLL_CNTL =
cpu_to_be32(si_pi->clock_registers.dll_cntl);
- table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
+ table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(si_pi->clock_registers.mclk_pwrmgt_cntl);
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_ad_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_dq_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_1 =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL_1 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_1);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_2);
- table->initialState.levels[0].mclk.vMPLL_SS =
+ table->initialState.level.mclk.vMPLL_SS =
cpu_to_be32(si_pi->clock_registers.mpll_ss1);
- table->initialState.levels[0].mclk.vMPLL_SS2 =
+ table->initialState.level.mclk.vMPLL_SS2 =
cpu_to_be32(si_pi->clock_registers.mpll_ss2);
- table->initialState.levels[0].mclk.mclk_value =
+ table->initialState.level.mclk.mclk_value =
cpu_to_be32(initial_state->performance_levels[0].mclk);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_2);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_3);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_4);
- table-
Re: [PATCH] Revert "drm/radeon/si_dpm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE"
Hi Alex, I've just sent a couple of fixes for the recent radeon problems: https://lore.kernel.org/lkml/20210509224926.GA31035@embeddedor/ https://lore.kernel.org/lkml/20210509225525.GA32045@embeddedor/ So, there is no need to revert the problematic patches for radeon anymore. Sorry for the inconveniences. Thanks! -- Gustavo On 5/5/21 08:45, Gustavo A. R. Silva wrote: > > > On 5/5/21 08:06, Deucher, Alexander wrote: >> [AMD Public Use] >> >>> -Original Message- >>> From: Gustavo A. R. Silva >>> Sent: Tuesday, May 4, 2021 6:43 PM >>> To: Deucher, Alexander ; amd- >>> g...@lists.freedesktop.org >>> Cc: Gustavo A . R . Silva >>> Subject: Re: [PATCH] Revert "drm/radeon/si_dpm: Replace one-element >>> array with flexible-array in struct SISLANDS_SMC_SWSTATE" >>> >>> Hi, >>> >>> I thought it was this[1] the one causing problems[2]. >> >> They are both causing problems. > > Yeah, I already know why and I'll work out a solution soon. In the meantime, > both > should be reverted. > > These other two[1][2] also seem to have the same issue and should be > reverted, too. > I wonder why no one has reported any problems, yet... in particular regarding > this[2]. > > Thanks > -- > Gustavo > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e1aa13ca3ffdd1e626532a3924ac80686939848 > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4541ea81edde6ce9a1d9be082489aca7e8e7e1dc > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/radeon/si_dpm: Fix SMU power state load
Create new structure SISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
and ACPIState.levels are never actually used as flexible arrays. Those
arrays can be used as simple objects of type
SISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
Currently, the code fails because flexible array _levels_ in
struct SISLANDS_SMC_SWSTATE doesn't allow for code that access
the first element of initialState.levels and ACPIState.levels
arrays:
4353 table->initialState.levels[0].mclk.vDLL_CNTL =
4354 cpu_to_be32(si_pi->clock_registers.dll_cntl);
...
4555 table->ACPIState.levels[0].mclk.vDLL_CNTL =
4556 cpu_to_be32(dll_cntl);
because such element cannot exist without previously allocating
any dynamic memory for it (which never actually happens).
That's why struct SISLANDS_SMC_SWSTATE should only be used as type
for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
created as type for objects initialState, ACPIState and ULVState.
Also, with the change from one-element array to flexible-array member
in commit 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array
with flexible-array in struct SISLANDS_SMC_SWSTATE"), the size of
dpmLevels in struct SISLANDS_SMC_STATETABLE should be fixed to be
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
SISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1583
Fixes: 96e27e8d919e ("drm/radeon/si_dpm: Replace one-element array with
flexible-array in struct SISLANDS_SMC_SWSTATE")
Cc: sta...@vger.kernel.org
Reported-by: Kai-Heng Feng
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/radeon/si_dpm.c | 174 +-
drivers/gpu/drm/radeon/sislands_smc.h | 34 +++--
2 files changed, 109 insertions(+), 99 deletions(-)
diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
index 91bfc4762767..2a8b9680cf6b 100644
--- a/drivers/gpu/drm/radeon/si_dpm.c
+++ b/drivers/gpu/drm/radeon/si_dpm.c
@@ -4350,70 +4350,70 @@ static int si_populate_smc_initial_state(struct
radeon_device *rdev,
u32 reg;
int ret;
- table->initialState.levels[0].mclk.vDLL_CNTL =
+ table->initialState.level.mclk.vDLL_CNTL =
cpu_to_be32(si_pi->clock_registers.dll_cntl);
- table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
+ table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(si_pi->clock_registers.mclk_pwrmgt_cntl);
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_ad_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_dq_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_1 =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL_1 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_1);
- table->initialState.levels[0].mclk.vMPLL_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.mpll_func_cntl_2);
- table->initialState.levels[0].mclk.vMPLL_SS =
+ table->initialState.level.mclk.vMPLL_SS =
cpu_to_be32(si_pi->clock_registers.mpll_ss1);
- table->initialState.levels[0].mclk.vMPLL_SS2 =
+ table->initialState.level.mclk.vMPLL_SS2 =
cpu_to_be32(si_pi->clock_registers.mpll_ss2);
- table->initialState.levels[0].mclk.mclk_value =
+ table->initialState.level.mclk.mclk_value =
cpu_to_be32(initial_state->performance_levels[0].mclk);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_2);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_3);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_4 =
cpu_to_be32(si_pi->clock_registers.cg_spll_func_cntl_4);
- table->initialState.levels[0].sclk.vCG_SPLL_
[PATCH] drm/radeon/ni_dpm: Fix booting bug
Create new structure NISLANDS_SMC_SWSTATE_SINGLE, as initialState.levels
and ACPIState.levels are never actually used as flexible arrays. Those
arrays can be used as simple objects of type
NISLANDS_SMC_HW_PERFORMANCE_LEVEL, instead.
Currently, the code fails because flexible array _levels_ in
struct NISLANDS_SMC_SWSTATE doesn't allow for code that access
the first element of initialState.levels and ACPIState.levels
arrays:
drivers/gpu/drm/radeon/ni_dpm.c:
1690 table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
1691 cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
...
1903: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(mpll_ad_func_cntl);
1904: table->ACPIState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(mpll_ad_func_cntl_2);
because such element cannot exist without previously allocating
any dynamic memory for it (which never actually happens).
That's why struct NISLANDS_SMC_SWSTATE should only be used as type
for object driverState and new struct SISLANDS_SMC_SWSTATE_SINGLE is
created as type for objects initialState, ACPIState and ULVState.
Also, with the change from one-element array to flexible-array member
in commit 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element
array with flexible-array member in struct NISLANDS_SMC_SWSTATE"), the
size of dpmLevels in struct NISLANDS_SMC_STATETABLE should be fixed to
be NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE instead of
NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1.
Bug:
https://lore.kernel.org/dri-devel/3eedbe78-1fbd-4763-a7f3-ac5665e76...@xenosoft.de/
Fixes: 434fb1e7444a ("drm/radeon/nislands_smc.h: Replace one-element array with
flexible-array member in struct NISLANDS_SMC_SWSTATE")
Cc: sta...@vger.kernel.org
Reported-by: Christian Zigotzky
Tested-by: Christian Zigotzky
Link:
https://lore.kernel.org/dri-devel/9bb5fcbd-daf5-1669-b3e7-b8624b3c3...@xenosoft.de/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/radeon/ni_dpm.c | 144 +-
drivers/gpu/drm/radeon/nislands_smc.h | 34 +++---
2 files changed, 94 insertions(+), 84 deletions(-)
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index dd5ef6493723..769f666335ac 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -1687,102 +1687,102 @@ static int ni_populate_smc_initial_state(struct
radeon_device *rdev,
u32 reg;
int ret;
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_AD_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_AD_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.mpll_ad_func_cntl_2);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl);
- table->initialState.levels[0].mclk.vMPLL_DQ_FUNC_CNTL_2 =
+ table->initialState.level.mclk.vMPLL_DQ_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.mpll_dq_func_cntl_2);
- table->initialState.levels[0].mclk.vMCLK_PWRMGT_CNTL =
+ table->initialState.level.mclk.vMCLK_PWRMGT_CNTL =
cpu_to_be32(ni_pi->clock_registers.mclk_pwrmgt_cntl);
- table->initialState.levels[0].mclk.vDLL_CNTL =
+ table->initialState.level.mclk.vDLL_CNTL =
cpu_to_be32(ni_pi->clock_registers.dll_cntl);
- table->initialState.levels[0].mclk.vMPLL_SS =
+ table->initialState.level.mclk.vMPLL_SS =
cpu_to_be32(ni_pi->clock_registers.mpll_ss1);
- table->initialState.levels[0].mclk.vMPLL_SS2 =
+ table->initialState.level.mclk.vMPLL_SS2 =
cpu_to_be32(ni_pi->clock_registers.mpll_ss2);
- table->initialState.levels[0].mclk.mclk_value =
+ table->initialState.level.mclk.mclk_value =
cpu_to_be32(initial_state->performance_levels[0].mclk);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_2 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_2 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_2);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_3 =
+ table->initialState.level.sclk.vCG_SPLL_FUNC_CNTL_3 =
cpu_to_be32(ni_pi->clock_registers.cg_spll_func_cntl_3);
- table->initialState.levels[0].sclk.vCG_SPLL_FUNC_CNTL_4 =
+ table->initialSta
Re: [PATCH] Revert "drm/radeon/si_dpm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE"
On 5/5/21 08:06, Deucher, Alexander wrote: > [AMD Public Use] > >> -Original Message----- >> From: Gustavo A. R. Silva >> Sent: Tuesday, May 4, 2021 6:43 PM >> To: Deucher, Alexander ; amd- >> g...@lists.freedesktop.org >> Cc: Gustavo A . R . Silva >> Subject: Re: [PATCH] Revert "drm/radeon/si_dpm: Replace one-element >> array with flexible-array in struct SISLANDS_SMC_SWSTATE" >> >> Hi, >> >> I thought it was this[1] the one causing problems[2]. > > They are both causing problems. Yeah, I already know why and I'll work out a solution soon. In the meantime, both should be reverted. These other two[1][2] also seem to have the same issue and should be reverted, too. I wonder why no one has reported any problems, yet... in particular regarding this[2]. Thanks -- Gustavo [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e1aa13ca3ffdd1e626532a3924ac80686939848 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4541ea81edde6ce9a1d9be082489aca7e8e7e1dc ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] Revert "drm/radeon/si_dpm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE"
Hi,
I thought it was this[1] the one causing problems[2].
--
Gustavo
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=434fb1e7444a2efc3a4ebd950c7f771ebfcffa31
[2]
https://lore.kernel.org/dri-devel/3eedbe78-1fbd-4763-a7f3-ac5665e76...@xenosoft.de/
On 5/4/21 13:42, Alex Deucher wrote:
> This reverts commit 96e27e8d919e52f30ea6b717e3cb70faa0b102cd.
>
> This causes the SMU to fail to load the power state.
>
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1583
> Signed-off-by: Alex Deucher
> Cc: Gustavo A. R. Silva
> ---
> drivers/gpu/drm/radeon/si_dpm.c | 5 +++--
> drivers/gpu/drm/radeon/sislands_smc.h | 10 +-
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
> index 918609551804..91bfc4762767 100644
> --- a/drivers/gpu/drm/radeon/si_dpm.c
> +++ b/drivers/gpu/drm/radeon/si_dpm.c
> @@ -5250,9 +5250,10 @@ static int si_upload_sw_state(struct radeon_device
> *rdev,
> int ret;
> u32 address = si_pi->state_table_start +
> offsetof(SISLANDS_SMC_STATETABLE, driverState);
> + u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
> + ((new_state->performance_level_count - 1) *
> + sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
> SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
> - size_t state_size = struct_size(smc_state, levels,
> - new_state->performance_level_count);
>
> memset(smc_state, 0, state_size);
>
> diff --git a/drivers/gpu/drm/radeon/sislands_smc.h
> b/drivers/gpu/drm/radeon/sislands_smc.h
> index fbd6589bdab9..966e3a556011 100644
> --- a/drivers/gpu/drm/radeon/sislands_smc.h
> +++ b/drivers/gpu/drm/radeon/sislands_smc.h
> @@ -182,11 +182,11 @@ typedef struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL
> SISLANDS_SMC_HW_PERFORMANCE_LEV
>
> struct SISLANDS_SMC_SWSTATE
> {
> - uint8_t flags;
> - uint8_t levelCount;
> - uint8_t padding2;
> - uint8_t padding3;
> - SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
> +uint8_t flags;
> +uint8_t levelCount;
> +uint8_t padding2;
> +uint8_t padding3;
> +SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
> };
>
> typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: Radeon NI: GIT kernel with the nislands_smc commit doesn't boot on a Freescale P5040 board and P.A.Semi Nemo board
On 4/30/21 10:26, Deucher, Alexander wrote: > [AMD Public Use] > > + Gustavo, amd-gfx > >> -Original Message- >> From: Christian Zigotzky >> Sent: Friday, April 30, 2021 8:00 AM >> To: gustavo...@kernel.org; Deucher, Alexander >> >> Cc: R.T.Dickinson ; Darren Stevens > zone.net>; mad skateman ; linuxppc-dev >> ; Olof Johansson ; >> Maling list - DRI developers ; Michel >> Dänzer ; Christian Zigotzky >> Subject: Radeon NI: GIT kernel with the nislands_smc commit doesn't >> boot on a Freescale P5040 board and P.A.Semi Nemo board >> >> Hello, >> >> The Nemo board (A-EON AmigaOne X1000) [1] and the FSL P5040 Cyrus+ >> board (A-EON AmigaOne X5000) [2] with installed AMD Radeon HD6970 NI >> graphics cards (Cayman XT) [3] don't boot with the latest git kernel >> anymore after the commit "drm/radeon/nislands_smc.h: Replace >> one-element array with flexible-array member in struct NISLANDS_SMC_SWSTATE >> branch" [4]. >> This git kernel boots in a virtual e5500 QEMU machine with a VirtIO-GPU [5]. >> >> I bisected today [6]. >> >> Result: drm/radeon/nislands_smc.h: Replace one-element array with >> flexible-array member in struct NISLANDS_SMC_SWSTATE branch >> (434fb1e7444a2efc3a4ebd950c7f771ebfcffa31) [4] is the first bad commit. >> >> I was able to revert this commit [7] and after a new compiling, the >> kernel boots without any problems on my AmigaOnes. >> >> After that I created a patch for reverting this commit for new git test >> kernels. >> [3] >> >> The kernel compiles and boots with this patch on my AmigaOnes. Please >> find attached the kernel config files. >> >> Please check the first bad commit. I'll have a look. Thanks for the report! -- Gustavo >> >> Thanks, >> Christian >> >> [1] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen. >> wikipedia.org%2Fwiki%2FAmigaOne_X1000data=04%7C01%7Calexand >> er.deucher%40amd.com%7C0622549383fb4320346b08d90bcf7be1%7C3dd89 >> 61fe4884e608e11a82d994e183d%7C0%7C0%7C637553808670161651%7CUnkn >> own%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik >> 1haWwiLCJXVCI6Mn0%3D%7C1000sdata=PNSrApUdMrku20hH7dEKlJJ >> TBi7Qp5JOkqpA4MvKqdE%3Dreserved=0 >> [2] >> https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwiki. >> a miga.org%2Findex.php%3Ftitle%3DX5000data=04%7C01%7Calexander >> .deucher%40amd.com%7C0622549383fb4320346b08d90bcf7be1%7C3dd8961f >> e4884e608e11a82d994e183d%7C0%7C0%7C637553808670161651%7CUnknow >> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha >> WwiLCJXVCI6Mn0%3D%7C1000sdata=B8Uvhs25%2FP3RfnL1AgICN3Y4 >> CEXeCE1yIoi3vvwvGto%3Dreserved=0 >> [3] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforu >> m.hyperion- >> entertainment.com%2Fviewtopic.php%3Ff%3D35%26t%3D4377data= >> 04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346b08d >> 90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63755380 >> 8670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIj >> oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=TokXplD >> Tvg3%2BZMPLCgR1fs%2BN2X9MIfLXLW67MAM2Qsk%3Dreserved=0 >> [4] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit. >> k ernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git% >> 2Fcommit%2F%3Fid%3D434fb1e7444a2efc3a4ebd950c7f771ebfcffa31d >> ata=04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346 >> b08d90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6375 >> 53808670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL >> CJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=JC >> M4xvPEnWdckcTPbQ2Ujv%2FAiMMsFMzzl4Pr%2FRPlcMQ%3Dreserve >> d=0 >> [5] qemu-system-ppc64 -M ppce500 -cpu e5500 -m 1024 -kernel uImage - >> drive format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev >> user,id=mynet0 -device virtio-net-pci,netdev=mynet0 -append "rw >> root=/dev/vda" -device virtio-vga -usb -device usb-ehci,id=ehci >> -device usb- tablet -device virtio-keyboard-pci -smp 4 -vnc :1 [6] >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fforu >> m.hyperion- >> entertainment.com%2Fviewtopic.php%3Fp%3D53074%23p53074data >> =04%7C01%7Calexander.deucher%40amd.com%7C0622549383fb4320346b08 >> d90bcf7be1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6375538 >> 08670161651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ >> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=RXfSlY >> A3bDEFas0%2Fk2vMWsl2l0nuhS2ecjZgSBLc%2Bs4%3Dreserved=0 >> [7] git revert 434fb1e7444a2efc3a4ebd950c7f771ebfcffa3 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH][next] drm/amd/display: Fix sizeof arguments in bw_calcs_init()
On 3/22/21 09:04, Chen, Guchun wrote: > [AMD Public Use] > > Thanks for your patch, Silva. The issue has been fixed by " a5c6007e20e1 > drm/amd/display: fix modprobe failure on vega series". Great. :) Good to know this is already fixed. Thanks! -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amd/display: Fix sizeof arguments in bw_calcs_init()
The wrong sizeof values are currently being used as arguments to
kzalloc().
Fix this by using the right arguments *dceip and *vbios,
correspondingly.
Addresses-Coverity-ID: 1502901 ("Wrong sizeof argument")
Fixes: fca1e079055e ("drm/amd/display/dc/calcs/dce_calcs: Remove some large
variables from the stack")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
index 556ecfabc8d2..1244fcb0f446 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
+++ b/drivers/gpu/drm/amd/display/dc/calcs/dce_calcs.c
@@ -2051,11 +2051,11 @@ void bw_calcs_init(struct bw_calcs_dceip *bw_dceip,
enum bw_calcs_version version = bw_calcs_version_from_asic_id(asic_id);
- dceip = kzalloc(sizeof(dceip), GFP_KERNEL);
+ dceip = kzalloc(sizeof(*dceip), GFP_KERNEL);
if (!dceip)
return;
- vbios = kzalloc(sizeof(vbios), GFP_KERNEL);
+ vbios = kzalloc(sizeof(*vbios), GFP_KERNEL);
if (!vbios) {
kfree(dceip);
return;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH][next] drm/radeon/si_dpm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE
On Fri, Mar 05, 2021 at 02:10:44PM -0500, Alex Deucher wrote: > Applied. Thanks! Awesome. :) Thanks, Alex. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/radeon/si_dpm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct SISLANDS_SMC_SWSTATE, instead of a one-element array, and use
the struct_size() helper to calculate the size for the allocation.
Also, this helps with the ongoing efforts to enable -Warray-bounds by
fixing the following warnings:
drivers/gpu/drm/radeon/si_dpm.c: In function ‘si_convert_power_state_to_smc’:
drivers/gpu/drm/radeon/si_dpm.c:2350:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2350 | smc_state->levels[i].dpm2.MaxPS = (u8)((SISLANDS_DPM2_MAX_PULSE_SKIP
* (max_sclk - min_sclk)) / max_sclk);
| ~^~~
drivers/gpu/drm/radeon/si_dpm.c:2351:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2351 | smc_state->levels[i].dpm2.NearTDPDec = SISLANDS_DPM2_NEAR_TDP_DEC;
| ~^~~
drivers/gpu/drm/radeon/si_dpm.c:2352:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2352 | smc_state->levels[i].dpm2.AboveSafeInc = SISLANDS_DPM2_ABOVE_SAFE_INC;
| ~^~~
drivers/gpu/drm/radeon/si_dpm.c:2353:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2353 | smc_state->levels[i].dpm2.BelowSafeInc = SISLANDS_DPM2_BELOW_SAFE_INC;
| ~^~~
drivers/gpu/drm/radeon/si_dpm.c:2354:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2354 | smc_state->levels[i].dpm2.PwrEfficiencyRatio =
cpu_to_be16(pwr_efficiency_ratio);
| ~^~~
drivers/gpu/drm/radeon/si_dpm.c:5105:20: warning: array subscript 1 is above
array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
5105 | smc_state->levels[i + 1].aT = cpu_to_be32(a_t);
| ~^~~
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/603f9a8f.adlrpmfzzsapzvyq%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/radeon/si_dpm.c | 5 ++---
drivers/gpu/drm/radeon/sislands_smc.h | 10 +-
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
index 91bfc4762767..918609551804 100644
--- a/drivers/gpu/drm/radeon/si_dpm.c
+++ b/drivers/gpu/drm/radeon/si_dpm.c
@@ -5250,10 +5250,9 @@ static int si_upload_sw_state(struct radeon_device *rdev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
diff --git a/drivers/gpu/drm/radeon/sislands_smc.h
b/drivers/gpu/drm/radeon/sislands_smc.h
index 966e3a556011..fbd6589bdab9 100644
--- a/drivers/gpu/drm/radeon/sislands_smc.h
+++ b/drivers/gpu/drm/radeon/sislands_smc.h
@@ -182,11 +182,11 @@ typedef struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL
SISLANDS_SMC_HW_PERFORMANCE_LEV
struct SISLANDS_SMC_SWSTATE
{
-uint8_t flags;
-uint8_t levelCount;
-uint8_t padding2;
-uint8_t padding3;
-SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+ uint8_t flags;
+ uint8_t levelCount;
+ uint8_t padding2;
+ uint8_t padding3;
+ SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct SISLANDS_SMC_SWSTATE SI
[PATCH][next] drm/radeon/nislands_smc.h: Replace one-element array with flexible-array member in struct NISLANDS_SMC_SWSTATE
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Use flexible-array member in struct NISLANDS_SMC_SWSTATE, instead of
one-element array.
Also, this helps with the ongoing efforts to enable -Warray-bounds by
fixing the following warnings:
drivers/gpu/drm/radeon/ni_dpm.c: In function ‘ni_convert_power_state_to_smc’:
drivers/gpu/drm/radeon/ni_dpm.c:2521:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2521 | smc_state->levels[i].dpm2.MaxPS =
| ~^~~
drivers/gpu/drm/radeon/ni_dpm.c:2523:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2523 | smc_state->levels[i].dpm2.NearTDPDec = NISLANDS_DPM2_NEAR_TDP_DEC;
| ~^~~
drivers/gpu/drm/radeon/ni_dpm.c:2524:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2524 | smc_state->levels[i].dpm2.AboveSafeInc = NISLANDS_DPM2_ABOVE_SAFE_INC;
| ~^~~
drivers/gpu/drm/radeon/ni_dpm.c:2525:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2525 | smc_state->levels[i].dpm2.BelowSafeInc = NISLANDS_DPM2_BELOW_SAFE_INC;
| ~^~~
drivers/gpu/drm/radeon/ni_dpm.c:2526:35: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2526 | smc_state->levels[i].stateFlags |=
| ^~
drivers/gpu/drm/radeon/ni_dpm.c:2526:35: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2526 | smc_state->levels[i].stateFlags |=
| ^~
2527 |((i != (state->performance_level_count - 1)) && power_boost_limit) ?
|
2528 |PPSMC_STATEFLAG_POWERBOOST : 0;
|~~
drivers/gpu/drm/radeon/ni_dpm.c:2442:20: warning: array subscript 1 is above
array bounds of ‘NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’ {aka ‘struct
NISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
2442 | smc_state->levels[i + 1].aT = cpu_to_be32(a_t);
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/6023ed54.bfiy+9uz81i6nq19%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/radeon/nislands_smc.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/nislands_smc.h
b/drivers/gpu/drm/radeon/nislands_smc.h
index 3cf8fc0d83f4..7395cb6b3cac 100644
--- a/drivers/gpu/drm/radeon/nislands_smc.h
+++ b/drivers/gpu/drm/radeon/nislands_smc.h
@@ -134,11 +134,11 @@ typedef struct NISLANDS_SMC_HW_PERFORMANCE_LEVEL
NISLANDS_SMC_HW_PERFORMANCE_LEV
struct NISLANDS_SMC_SWSTATE
{
-uint8_t flags;
-uint8_t levelCount;
-uint8_t padding2;
-uint8_t padding3;
-NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+ uint8_t flags;
+ uint8_t levelCount;
+ uint8_t padding2;
+ uint8_t padding3;
+ NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amd/pm: Replace one-element array with flexible-array in struct _ATOM_Vega10_GFXCLK_Dependency_Table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Use flexible-array member in struct _ATOM_Vega10_GFXCLK_Dependency_Table,
instead of one-element array.
Also, this helps with the ongoing efforts to enable -Warray-bounds and
fix the following warning:
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/vega10_hwmgr.c: In function
‘vega10_get_pp_table_entry_callback_func’:
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/vega10_hwmgr.c:3113:30:
warning: array subscript 4 is above array bounds of
‘ATOM_Vega10_GFXCLK_Dependency_Record[1]’ {aka ‘struct
_ATOM_Vega10_GFXCLK_Dependency_Record[1]’} [-Warray-bounds]
3113 | gfxclk_dep_table->entries[4].ulClk;
| ~^~~
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/6023ff3d.wy3ssckgrqpdplvo%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
index c934e9612c1b..9c479bd9a786 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
@@ -161,9 +161,9 @@ typedef struct _ATOM_Vega10_MCLK_Dependency_Record {
} ATOM_Vega10_MCLK_Dependency_Record;
typedef struct _ATOM_Vega10_GFXCLK_Dependency_Table {
-UCHAR ucRevId;
-UCHAR ucNumEntries; /* Number of
entries. */
-ATOM_Vega10_GFXCLK_Dependency_Record entries[1];/* Dynamically
allocate entries. */
+ UCHAR ucRevId;
+ UCHAR ucNumEntries; /* Number of
entries. */
+ ATOM_Vega10_GFXCLK_Dependency_Record entries[]; /* Dynamically
allocate entries. */
} ATOM_Vega10_GFXCLK_Dependency_Table;
typedef struct _ATOM_Vega10_MCLK_Dependency_Table {
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amd/pm: Replace one-element array with flexible-array in struct SISLANDS_SMC_SWSTATE
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct SISLANDS_SMC_SWSTATE, instead of a one-element array, and use
the struct_size() helper to calculate the size for the allocation.
Also, this helps with the ongoing efforts to enable -Warray-bounds and
fix the following warnings:
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:2448:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:2449:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:2450:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:2451:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:2452:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
drivers/gpu/drm/amd/amdgpu/../pm/powerplay/si_dpm.c:5570:20: warning: array
subscript 1 is above array bounds of ‘SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’
{aka ‘struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL[1]’} [-Warray-bounds]
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9/process/deprecated.html#zero-length-and-one-element-arrays
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/109
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/6023be58.sk66l%2fv4vusji5mi%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 6 ++
drivers/gpu/drm/amd/pm/powerplay/sislands_smc.h | 10 +-
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
index afa1711c9620..62291358fb1c 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
@@ -5715,11 +5715,9 @@ static int si_upload_sw_state(struct amdgpu_device *adev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
-
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
ret = si_convert_power_state_to_smc(adev, amdgpu_new_state, smc_state);
diff --git a/drivers/gpu/drm/amd/pm/powerplay/sislands_smc.h
b/drivers/gpu/drm/amd/pm/powerplay/sislands_smc.h
index d2930eceaf3c..0f7554052c90 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/sislands_smc.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/sislands_smc.h
@@ -182,11 +182,11 @@ typedef struct SISLANDS_SMC_HW_PERFORMANCE_LEVEL
SISLANDS_SMC_HW_PERFORMANCE_LEV
struct SISLANDS_SMC_SWSTATE
{
-uint8_t flags;
-uint8_t levelCount;
-uint8_t padding2;
-uint8_t padding3;
-SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+ uint8_t flags;
+ uint8_t levelCount;
+ uint8_t padding2;
+ uint8_t padding3;
+ SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amd/display: Fix potential integer overflow
Fix potential integer overflow by casting actual_calculated_clock_100hz
to u64, in order to give the compiler complete information about the
proper arithmetic to use.
Notice that such variable is used in a context that expects
an expression of type u64 (64 bits, unsigned) and the following
expression is currently being evaluated using 32-bit arithmetic:
actual_calculated_clock_100hz * post_divider
Fixes: 7a03fdf628af ("drm/amd/display: fix 64bit division issue on 32bit OS")
Addresses-Coverity-ID: 1501691 ("Unintentional integer overflow")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
index bc942725b9d8..dec58b3c42e4 100644
--- a/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
+++ b/drivers/gpu/drm/amd/display/dc/dce/dce_clock_source.c
@@ -240,7 +240,7 @@ static bool calc_fb_divider_checking_tolerance(
pll_settings->calculated_pix_clk_100hz =
actual_calculated_clock_100hz;
pll_settings->vco_freq =
- div_u64(actual_calculated_clock_100hz * post_divider,
10);
+ div_u64((u64)actual_calculated_clock_100hz *
post_divider, 10);
return true;
}
return false;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Dec 01, 2020 at 12:52:27AM -0500, Martin K. Petersen wrote: > > Gustavo, > > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > Applied 20-22,54,120-124 to 5.11/scsi-staging, thanks. Awesome! :) Thanks, Martin. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 08:38:46PM +, Mark Brown wrote: > On Fri, 20 Nov 2020 12:21:39 -0600, Gustavo A. R. Silva wrote: > > This series aims to fix almost all remaining fall-through warnings in > > order to enable -Wimplicit-fallthrough for Clang. > > > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > > add multiple break/goto/return/fallthrough statements instead of just > > letting the code fall through to the next case. > > > > [...] > > Applied to > >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git > for-next > > Thanks! > > [1/1] regulator: as3722: Fix fall-through warnings for Clang > commit: b52b417ccac4fae5b1f2ec4f1d46eb91e4493dc5 Thank you, Mark. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 04:03:45PM -0400, Jason Gunthorpe wrote: > On Fri, Nov 20, 2020 at 12:21:39PM -0600, Gustavo A. R. Silva wrote: > > > IB/hfi1: Fix fall-through warnings for Clang > > IB/mlx4: Fix fall-through warnings for Clang > > IB/qedr: Fix fall-through warnings for Clang > > RDMA/mlx5: Fix fall-through warnings for Clang > > I picked these four to the rdma tree, thanks Awesome. :) Thank you, Jason. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 028/141] drm/amd/display: Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 05:45:07PM -0500, Alex Deucher wrote: > On Fri, Nov 20, 2020 at 1:28 PM Gustavo A. R. Silva > wrote: > > > > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple > > warnings by explicitly adding multiple break statements instead of just > > letting the code fall through to the next case. > > > > Link: https://github.com/KSPP/linux/issues/115 > > Signed-off-by: Gustavo A. R. Silva > > Applied. Thanks! Thanks, Alex. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 004/141] drm/amdgpu: Fix fall-through warnings for Clang
On Fri, Nov 20, 2020 at 05:42:38PM -0500, Alex Deucher wrote: > On Fri, Nov 20, 2020 at 1:24 PM Gustavo A. R. Silva > wrote: > > > > In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple > > warnings by explicitly adding multiple break statements instead of just > > letting the code fall through to the next case. > > > > Link: https://github.com/KSPP/linux/issues/115 > > Signed-off-by: Gustavo A. R. Silva > > Applied. Thanks! Thanks, Alex. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, Nov 22, 2020 at 11:53:55AM -0800, James Bottomley wrote: > On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote: > > On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > > > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > > > Please tell me our reward for all this effort isn't a single > > > > > missing error print. > > > > > > > > There were quite literally dozens of logical defects found > > > > by the fallthrough additions. Very few were logging only. > > > > > > So can you give us the best examples (or indeed all of them if > > > someone is keeping score)? hopefully this isn't a US election > > > situation ... > > > > Gustavo? Are you running for congress now? > > > > https://lwn.net/Articles/794944/ > > That's 21 reported fixes of which about 50% seem to produce no change > in code behaviour at all, a quarter seem to have no user visible effect > with the remaining quarter producing unexpected errors on obscure > configuration parameters, which is why no-one really noticed them > before. The really important point here is the number of bugs this has prevented and will prevent in the future. See an example of this, below: https://lore.kernel.org/linux-iio/20190813135802.gb27...@kroah.com/ This work is still relevant, even if the total number of issues/bugs we find in the process is zero (which is not the case). "The sucky thing about doing hard work to deploy hardening is that the result is totally invisible by definition (things not happening) [..]" - Dmitry Vyukov Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On 11/20/20 12:28, Joe Perches wrote: > On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote: >> Hi all, >> >> This series aims to fix almost all remaining fall-through warnings in >> order to enable -Wimplicit-fallthrough for Clang. >> >> In preparation to enable -Wimplicit-fallthrough for Clang, explicitly >> add multiple break/goto/return/fallthrough statements instead of just >> letting the code fall through to the next case. >> >> Notice that in order to enable -Wimplicit-fallthrough for Clang, this >> change[1] is meant to be reverted at some point. So, this patch helps >> to move in that direction. > > This was a bit hard to parse for a second or three. > > Thanks Gustavo. > > How was this change done? I audited case by case in order to determine the best fit for each situation. Depending on the surrounding logic, sometimes it makes more sense a goto or a fallthrough rather than merely a break. Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
Hi,
On 11/20/20 12:53, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote:
>> This series aims to fix almost all remaining fall-through warnings in
>> order to enable -Wimplicit-fallthrough for Clang.
>>
>> In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
>> add multiple break/goto/return/fallthrough statements instead of just
>> letting the code fall through to the next case.
>>
>> Notice that in order to enable -Wimplicit-fallthrough for Clang, this
>> change[1] is meant to be reverted at some point. So, this patch helps
>> to move in that direction.
>>
>> Something important to mention is that there is currently a discrepancy
>> between GCC and Clang when dealing with switch fall-through to empty case
>> statements or to cases that only contain a break/continue/return
>> statement[2][3][4].
>
> Are we sure we want to make this change? Was it discussed before?
>
> Are there any bugs Clangs puritanical definition of fallthrough helped
> find?
>
> IMVHO compiler warnings are supposed to warn about issues that could
> be bugs. Falling through to default: break; can hardly be a bug?!
The justification for this is explained in this same changelog text:
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for
clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for
Clang")
Thanks
--
Gustavo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 078/141] drm/amd/pm: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix a couple of warnings by explicitly adding a break statement instead of letting the code fall through to the next case, and a fallthrough pseudo-keyword as a replacement for a /* fall through */ comment, Notice that Clang doesn't recognize /* fall through */ comments as implicit fall-through markings. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 2 +- drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c index b5986d19dc08..afa1711c9620 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c +++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c @@ -6200,8 +6200,8 @@ static void si_request_link_speed_change_before_state_change(struct amdgpu_devic case AMDGPU_PCIE_GEN2: if (amdgpu_acpi_pcie_performance_request(adev, PCIE_PERF_REQ_PECI_GEN2, false) == 0) break; + fallthrough; #endif - /* fall through */ default: si_pi->force_pcie_gen = si_get_current_pcie_speed(adev); break; diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c b/drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c index c3d2e6dcf62a..7d7d698c7976 100644 --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/polaris10_smumgr.c @@ -2272,6 +2272,7 @@ static int polaris10_update_smc_table(struct pp_hwmgr *hwmgr, uint32_t type) break; case SMU_BIF_TABLE: polaris10_update_bif_smc_table(hwmgr); + break; default: break; } -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 028/141] drm/amd/display: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple warnings by explicitly adding multiple break statements instead of just letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/display/dc/bios/bios_parser.c | 1 + drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c | 2 ++ drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 + 3 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c index ad394aefa5d9..23a373ca94b5 100644 --- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c +++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser.c @@ -1198,6 +1198,7 @@ static enum bp_result bios_parser_get_embedded_panel_info( default: break; } + break; default: break; } diff --git a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c index 29d64e7e304f..fd1e64fa8744 100644 --- a/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c +++ b/drivers/gpu/drm/amd/display/dc/bios/bios_parser2.c @@ -903,6 +903,7 @@ static enum bp_result bios_parser_get_soc_bb_info( break; case 4: result = get_soc_bb_info_v4_4(bp, soc_bb_info); + break; default: break; } @@ -1019,6 +1020,7 @@ static enum bp_result bios_parser_get_embedded_panel_info( default: break; } + break; default: break; } diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c index fec87a2e210c..b9254a87ee73 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c @@ -1052,6 +1052,7 @@ static bool dc_link_detect_helper(struct dc_link *link, return false; } + break; default: break; } -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 004/141] drm/amdgpu: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple warnings by explicitly adding multiple break statements instead of just letting the code fall through to the next case. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 1 + drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 1 + drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 1 + drivers/gpu/drm/amd/amdgpu/vi.c| 1 + 4 files changed, 4 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 3579565e0eab..98ca6b976b6e 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -8398,6 +8398,7 @@ static int gfx_v10_0_set_priv_inst_fault_state(struct amdgpu_device *adev, WREG32_FIELD15(GC, 0, CP_INT_CNTL_RING0, PRIV_INSTR_INT_ENABLE, state == AMDGPU_IRQ_STATE_ENABLE ? 1 : 0); + break; default: break; } diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c index 0d8e203b10ef..e61121629b93 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c @@ -5683,6 +5683,7 @@ static int gfx_v9_0_set_priv_inst_fault_state(struct amdgpu_device *adev, WREG32_FIELD15(GC, 0, CP_INT_CNTL_RING0, PRIV_INSTR_INT_ENABLE, state == AMDGPU_IRQ_STATE_ENABLE ? 1 : 0); + break; default: break; } diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c index 3ebbddb63705..584b99b80c29 100644 --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c @@ -502,6 +502,7 @@ static int gmc_v9_0_vm_fault_interrupt_state(struct amdgpu_device *adev, WREG32(reg, tmp); } } + break; default: break; } diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c index 9bcd0eebc6d7..d56b474b3a21 100644 --- a/drivers/gpu/drm/amd/amdgpu/vi.c +++ b/drivers/gpu/drm/amd/amdgpu/vi.c @@ -1645,6 +1645,7 @@ static int vi_common_set_clockgating_state(void *handle, case CHIP_POLARIS12: case CHIP_VEGAM: vi_common_set_clockgating_state_by_smu(adev, state); + break; default: break; } -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 005/141] drm/radeon: Fix fall-through warnings for Clang
In preparation to enable -Wimplicit-fallthrough for Clang, fix multiple warnings by explicitly adding multiple fallthrough pseudo-keyword macros, as replacement for /* fall through */ comments. Notice that Clang doesn't recognize /* fall through */ comments as implicit fall-through markings. Link: https://github.com/KSPP/linux/issues/115 Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/radeon/ci_dpm.c | 2 +- drivers/gpu/drm/radeon/r300.c | 1 + drivers/gpu/drm/radeon/si_dpm.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/ci_dpm.c b/drivers/gpu/drm/radeon/ci_dpm.c index 886e9959496f..3d0a2e81b2de 100644 --- a/drivers/gpu/drm/radeon/ci_dpm.c +++ b/drivers/gpu/drm/radeon/ci_dpm.c @@ -4860,8 +4860,8 @@ static void ci_request_link_speed_change_before_state_change(struct radeon_devic case RADEON_PCIE_GEN2: if (radeon_acpi_pcie_performance_request(rdev, PCIE_PERF_REQ_PECI_GEN2, false) == 0) break; + fallthrough; #endif - /* fall through */ default: pi->force_pcie_gen = ci_get_current_pcie_speed(rdev); break; diff --git a/drivers/gpu/drm/radeon/r300.c b/drivers/gpu/drm/radeon/r300.c index 73f67bf222e1..213dc49b6322 100644 --- a/drivers/gpu/drm/radeon/r300.c +++ b/drivers/gpu/drm/radeon/r300.c @@ -1162,6 +1162,7 @@ static int r300_packet0_check(struct radeon_cs_parser *p, /* valid register only on RV530 */ if (p->rdev->family == CHIP_RV530) break; + fallthrough; /* fallthrough do not move */ default: goto fail; diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c index d1c73e9db889..d19c08e0ad5a 100644 --- a/drivers/gpu/drm/radeon/si_dpm.c +++ b/drivers/gpu/drm/radeon/si_dpm.c @@ -5748,8 +5748,8 @@ static void si_request_link_speed_change_before_state_change(struct radeon_devic case RADEON_PCIE_GEN2: if (radeon_acpi_pcie_performance_request(rdev, PCIE_PERF_REQ_PECI_GEN2, false) == 0) break; + fallthrough; #endif - /* fall through */ default: si_pi->force_pcie_gen = si_get_current_pcie_speed(rdev); break; -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 000/141] Fix fall-through warnings for Clang
Hi all,
This series aims to fix almost all remaining fall-through warnings in
order to enable -Wimplicit-fallthrough for Clang.
In preparation to enable -Wimplicit-fallthrough for Clang, explicitly
add multiple break/goto/return/fallthrough statements instead of just
letting the code fall through to the next case.
Notice that in order to enable -Wimplicit-fallthrough for Clang, this
change[1] is meant to be reverted at some point. So, this patch helps
to move in that direction.
Something important to mention is that there is currently a discrepancy
between GCC and Clang when dealing with switch fall-through to empty case
statements or to cases that only contain a break/continue/return
statement[2][3][4].
Now that the -Wimplicit-fallthrough option has been globally enabled[5],
any compiler should really warn on missing either a fallthrough annotation
or any of the other case-terminating statements (break/continue/return/
goto) when falling through to the next case statement. Making exceptions
to this introduces variation in case handling which may continue to lead
to bugs, misunderstandings, and a general lack of robustness. The point
of enabling options like -Wimplicit-fallthrough is to prevent human error
and aid developers in spotting bugs before their code is even built/
submitted/committed, therefore eliminating classes of bugs. So, in order
to really accomplish this, we should, and can, move in the direction of
addressing any error-prone scenarios and get rid of the unintentional
fallthrough bug-class in the kernel, entirely, even if there is some minor
redundancy. Better to have explicit case-ending statements than continue to
have exceptions where one must guess as to the right result. The compiler
will eliminate any actual redundancy.
Note that there is already a patch in mainline that addresses almost
40,000 of these issues[6].
I'm happy to carry this whole series in my own tree if people are OK
with it. :)
[1] commit e2079e93f562c ("kbuild: Do not enable -Wimplicit-fallthrough for
clang for now")
[2] ClangBuiltLinux#636
[3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91432
[4] https://godbolt.org/z/xgkvIh
[5] commit a035d552a93b ("Makefile: Globally enable fall-through warning")
[6] commit 4169e889e588 ("include: jhash/signal: Fix fall-through warnings for
Clang")
Thanks!
Gustavo A. R. Silva (141):
afs: Fix fall-through warnings for Clang
ASoC: codecs: Fix fall-through warnings for Clang
cifs: Fix fall-through warnings for Clang
drm/amdgpu: Fix fall-through warnings for Clang
drm/radeon: Fix fall-through warnings for Clang
gfs2: Fix fall-through warnings for Clang
gpio: Fix fall-through warnings for Clang
IB/hfi1: Fix fall-through warnings for Clang
igb: Fix fall-through warnings for Clang
ima: Fix fall-through warnings for Clang
ipv4: Fix fall-through warnings for Clang
ixgbe: Fix fall-through warnings for Clang
media: dvb-frontends: Fix fall-through warnings for Clang
media: usb: dvb-usb-v2: Fix fall-through warnings for Clang
netfilter: Fix fall-through warnings for Clang
nfsd: Fix fall-through warnings for Clang
nfs: Fix fall-through warnings for Clang
qed: Fix fall-through warnings for Clang
qlcnic: Fix fall-through warnings for Clang
scsi: aic7xxx: Fix fall-through warnings for Clang
scsi: aic94xx: Fix fall-through warnings for Clang
scsi: bfa: Fix fall-through warnings for Clang
staging: rtl8723bs: core: Fix fall-through warnings for Clang
staging: vt6655: Fix fall-through warnings for Clang
bnxt_en: Fix fall-through warnings for Clang
ceph: Fix fall-through warnings for Clang
drbd: Fix fall-through warnings for Clang
drm/amd/display: Fix fall-through warnings for Clang
e1000: Fix fall-through warnings for Clang
ext2: Fix fall-through warnings for Clang
ext4: Fix fall-through warnings for Clang
floppy: Fix fall-through warnings for Clang
fm10k: Fix fall-through warnings for Clang
IB/mlx4: Fix fall-through warnings for Clang
IB/qedr: Fix fall-through warnings for Clang
ice: Fix fall-through warnings for Clang
Input: pcspkr - Fix fall-through warnings for Clang
isofs: Fix fall-through warnings for Clang
ixgbevf: Fix fall-through warnings for Clang
kprobes/x86: Fix fall-through warnings for Clang
mm: Fix fall-through warnings for Clang
net: 3c509: Fix fall-through warnings for Clang
net: cassini: Fix fall-through warnings for Clang
net/mlx4: Fix fall-through warnings for Clang
net: mscc: ocelot: Fix fall-through warnings for Clang
netxen_nic: Fix fall-through warnings for Clang
nfp: Fix fall-through warnings for Clang
perf/x86: Fix fall-through warnings for Clang
pinctrl: Fix fall-through warnings for Clang
RDMA/mlx5: Fix fall-through warnings for Clang
reiserfs: Fix fall-through warnings for Clang
security: keys: Fix fall-through warnings for Clang
selinux: Fix fall-through warnings for Clang
target: Fix fall-through warnings for Clang
Re: [PATCH][next] amd/amdgpu_ctx: Use struct_size() helper and kmalloc()
On Fri, Oct 09, 2020 at 04:29:55PM +0200, Christian König wrote: > > > > - entity = kcalloc(1, offsetof(typeof(*entity), > > > > fences[amdgpu_sched_jobs]), > > > > + entity = kmalloc(struct_size(entity, fences, amdgpu_sched_jobs), > > > NAK. You could use kzalloc() here, but kmalloc won't zero initialize the > > > memory which could result in unforeseen consequences. > > Oh I see.. I certainly didn't take that into account. > > > > I'll fix that up and respin. > > Shit happens, we already have a fix for this. Alex merged it and it > immediately broke our testing systems. :/ > So one of our engineers came up with a fix which should already have been > applied. Great. Good to know it's already fixed! :) Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH][next] amd/amdgpu_ctx: Use struct_size() helper and kmalloc()
On Fri, Oct 09, 2020 at 09:08:51AM +0200, Christian König wrote: > Am 08.10.20 um 16:34 schrieb Gustavo A. R. Silva: > > Make use of the new struct_size() helper instead of the offsetof() idiom. > > Also, use kmalloc() instead of kcalloc(). > > > > Signed-off-by: Gustavo A. R. Silva > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > > index c80d8339f58c..5be125f3b92a 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > > @@ -100,7 +100,7 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx > > *ctx, u32 hw_ip, > > enum drm_sched_priority priority; > > int r; > > - entity = kcalloc(1, offsetof(typeof(*entity), > > fences[amdgpu_sched_jobs]), > > + entity = kmalloc(struct_size(entity, fences, amdgpu_sched_jobs), > > NAK. You could use kzalloc() here, but kmalloc won't zero initialize the > memory which could result in unforeseen consequences. Oh I see.. I certainly didn't take that into account. I'll fix that up and respin. Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] amd/amdgpu_ctx: Use struct_size() helper and kmalloc()
Make use of the new struct_size() helper instead of the offsetof() idiom. Also, use kmalloc() instead of kcalloc(). Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c index c80d8339f58c..5be125f3b92a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c @@ -100,7 +100,7 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip, enum drm_sched_priority priority; int r; - entity = kcalloc(1, offsetof(typeof(*entity), fences[amdgpu_sched_jobs]), + entity = kmalloc(struct_size(entity, fences, amdgpu_sched_jobs), GFP_KERNEL); if (!entity) return -ENOMEM; -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH][next] drm/amdgpu: Use struct_size() helper in kmalloc()
Make use of the new struct_size() helper instead of the offsetof() idiom. Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 5da487b64a66..30192dce7775 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -239,8 +239,7 @@ static int amdgpu_amdkfd_remove_eviction_fence(struct amdgpu_bo *bo, if (!old) return 0; - new = kmalloc(offsetof(typeof(*new), shared[old->shared_max]), - GFP_KERNEL); + new = kmalloc(struct_size(new, shared, old->shared_max), GFP_KERNEL); if (!new) return -ENOMEM; -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 00/14] drm/amd/pm: Replace one-element arrays with flexible-array members
On Thu, Oct 08, 2020 at 09:19:47AM +0200, Christian König wrote: > Am 07.10.20 um 18:01 schrieb Gustavo A. R. Silva: > > Hi all, > > > > This series aims to replace one-element arrays with flexible-array > > members. > > > > There is a regular need in the kernel to provide a way to declare having > > a dynamically sized set of trailing elements in a structure. Kernel code > > should always use “flexible array members”[1] for these cases. The older > > style of one-element or zero-length arrays should no longer be used[2]. > > > > Refactor the code according to the use of flexible-array members, instead > > of one-element arrays, and use the struct_size() helper to calculate the > > size for the dynamic memory allocation. > > > > Also, save some heap space in the process. More on this on each individual > > patch. > > Ah! Nice to see that finally be documented and cleaned up. > > Feel free to add an Acked-by: Christian König > > I also know about a case where we don't use struct_size in the DMA-buf code. > > I'm the maintainer of that stuff as well, so be prepared to get patches > thrown at you to clean that up as well. No problem. Feel free to send all of those my way. :) Thanks -- Gustavo > > Thanks, > Christian. > > > > > This series also addresses multiple of the following sorts of warnings: > > > > drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/smu8_hwmgr.c:1515:37: > > warning: array subscript 1 is above array bounds of ‘const struct > > phm_clock_voltage_dependency_record[1]’ [-Warray-bounds] > > > > which, in this case, they are false positives, but nervertheless should be > > fixed in order to enable -Warray-bounds[3][4]. > > > > [1] > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FFlexible_array_memberdata=02%7C01%7Cchristian.koenig%40amd.com%7C5312862a3b8c41838ef508d86ad969c1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637376829947099179sdata=5LEWyR8pYSxmHsqhHiYiOS%2BPPk%2Fm5suOc6H7f5cIBL4%3Dreserved=0 > > [2] > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kernel.org%2Fdoc%2Fhtml%2Fv5.9-rc1%2Fprocess%2Fdeprecated.html%23zero-length-and-one-element-arraysdata=02%7C01%7Cchristian.koenig%40amd.com%7C5312862a3b8c41838ef508d86ad969c1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637376829947099179sdata=wOqxnNkA9FnOI%2BfB3dHn9RU7cqPJ62qqGCK9gsd2i%2Bo%3Dreserved=0 > > [3] > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Flinus%2F44720996e2d79e47d508b0abe99b931a726a3197data=02%7C01%7Cchristian.koenig%40amd.com%7C5312862a3b8c41838ef508d86ad969c1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637376829947099179sdata=x%2BSJeOrQA11HXoTaZEdyLyNWL9rC4GngDyoDMRBUn4M%3Dreserved=0 > > [4] > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F109data=02%7C01%7Cchristian.koenig%40amd.com%7C5312862a3b8c41838ef508d86ad969c1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637376829947099179sdata=48155uVo7AboCdSZfsTP10i2rHfBJctG%2F432lD%2BpfHo%3Dreserved=0 > > > > Gustavo A. R. Silva (14): > >drm/amd/pm: Replace one-element array with flexible-array member > >drm/amd/pm: Replace one-element array with flexible-array member in > > struct vi_dpm_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_clock_array > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_uvd_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_acp_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_phase_shedding_limits_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_vce_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_cac_leakage_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_samu_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_ppt_v1_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_ppt_v1_mm_clock_voltage_dependency_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_ppt_v1_voltage_lookup_table > >drm/amd/pm: Replace one-element array with flexible-array in struct > > phm_ppt_v1_pcie_table >
[PATCH 14/14] drm/amd/pm: Replace one-element array with flexible-array in struct ATOM_Vega10_GFXCLK_Dependency_Table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Use a flexible-array member in struct ATOM_Vega10_GFXCLK_Dependency_Table
instead of a one-element array.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7d61dd.o8jxxi5c6p9fob%2fd%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
index c934e9612c1b..a6968009acc4 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_pptable.h
@@ -163,7 +163,7 @@ typedef struct _ATOM_Vega10_MCLK_Dependency_Record {
typedef struct _ATOM_Vega10_GFXCLK_Dependency_Table {
UCHAR ucRevId;
UCHAR ucNumEntries; /* Number of
entries. */
-ATOM_Vega10_GFXCLK_Dependency_Record entries[1];/* Dynamically
allocate entries. */
+ATOM_Vega10_GFXCLK_Dependency_Record entries[]; /* Dynamically
allocate entries. */
} ATOM_Vega10_GFXCLK_Dependency_Table;
typedef struct _ATOM_Vega10_MCLK_Dependency_Table {
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 13/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_pcie_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_ppt_v1_pcie_table, instead of a one-element array, and use
the struct_size() helper to calculate the size for the allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7db0bc.7xivn4k83f7xw0ug%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
.../drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h| 2 +-
.../powerplay/hwmgr/process_pptables_v1_0.c | 22 ---
.../powerplay/hwmgr/vega10_processpptables.c | 10 +++--
3 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
index e11298cdeb30..729615aff126 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
@@ -103,7 +103,7 @@ typedef struct phm_ppt_v1_pcie_record
phm_ppt_v1_pcie_record;
struct phm_ppt_v1_pcie_table {
uint32_t count;/* Number of
entries. */
- phm_ppt_v1_pcie_record entries[1]; /*
Dynamically allocate count entries. */
+ phm_ppt_v1_pcie_record entries[]; /*
Dynamically allocate count entries. */
};
typedef struct phm_ppt_v1_pcie_table phm_ppt_v1_pcie_table;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
index 426655b9c678..4fa58614e26a 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
@@ -478,7 +478,7 @@ static int get_pcie_table(
PPTable_Generic_SubTable_Header const *ptable
)
{
- uint32_t table_size, i, pcie_count;
+ uint32_t i, pcie_count;
phm_ppt_v1_pcie_table *pcie_table;
struct phm_ppt_v1_information *pp_table_information =
(struct phm_ppt_v1_information *)(hwmgr->pptable);
@@ -491,12 +491,10 @@ static int get_pcie_table(
PP_ASSERT_WITH_CODE((atom_pcie_table->ucNumEntries != 0),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_pcie_record) *
atom_pcie_table->ucNumEntries;
-
- pcie_table = kzalloc(table_size, GFP_KERNEL);
-
- if (pcie_table == NULL)
+ pcie_table = kzalloc(struct_size(pcie_table, entries,
+atom_pcie_table->ucNumEntries),
+GFP_KERNEL);
+ if (!pcie_table)
return -ENOMEM;
/*
@@ -530,12 +528,10 @@ static int get_pcie_table(
PP_ASSERT_WITH_CODE((atom_pcie_table->ucNumEntries != 0),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_pcie_record) *
atom_pcie_table->ucNumEntries;
-
- pcie_table = kzalloc(table_size, GFP_KERNEL);
-
- if (pcie_table == NULL)
+ pcie_table = kzalloc(struct_size(pcie_table, entries,
+atom_pcie_table->ucNumEntries),
+GFP_KERNEL);
+ if (!pcie_table)
return -ENOMEM;
/*
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
index 3d7f915381c8..535404de78a2 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
@@ -784,7 +784,7 @@ static int get_pcie_table(struct pp_hwmgr *hwmgr,
struct phm_ppt_v1_pcie_table **vega10_pcie_table,
const Vega10_PPTable_Generic_SubTable_Header *table)
{
- uint32_t table_size, i, pcie_count;
+ uint32_t i, pcie_count;
struct phm_ppt_v1_pcie_table *pcie_table;
struct phm_ppt_v2_information *table_info =
(struct phm_ppt_v2_information *)(hwmgr->pptable);
@@ -795,12 +795,8 @@ static int get_pcie_table(struct pp_hwmgr *hwmgr,
"Invalid PowerPlay Table!",
return 0);
- table_si
[PATCH 11/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_mm_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_ppt_v1_mm_clock_voltage_dependency_table, instead of a
one-element array, and use the struct_size() helper to calculate the
size for the allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7d61e2.qitvtyg2pvog8bb0%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h| 2 +-
.../amd/pm/powerplay/hwmgr/process_pptables_v1_0.c| 11 ---
.../amd/pm/powerplay/hwmgr/vega10_processpptables.c | 9 +++--
3 files changed, 8 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
index c167083b0872..923cc04e405a 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
@@ -71,7 +71,7 @@ typedef struct phm_ppt_v1_mm_clock_voltage_dependency_record
phm_ppt_v1_mm_clock
struct phm_ppt_v1_mm_clock_voltage_dependency_table {
uint32_t count;
/* Number of entries. */
- phm_ppt_v1_mm_clock_voltage_dependency_record entries[1];
/* Dynamically allocate count entries. */
+ phm_ppt_v1_mm_clock_voltage_dependency_record entries[];
/* Dynamically allocate count entries. */
};
typedef struct phm_ppt_v1_mm_clock_voltage_dependency_table
phm_ppt_v1_mm_clock_voltage_dependency_table;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
index 0725531fbfff..5d8016cd1986 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
@@ -678,19 +678,16 @@ static int get_mm_clock_voltage_table(
const ATOM_Tonga_MM_Dependency_Table * mm_dependency_table
)
{
- uint32_t table_size, i;
+ uint32_t i;
const ATOM_Tonga_MM_Dependency_Record *mm_dependency_record;
phm_ppt_v1_mm_clock_voltage_dependency_table *mm_table;
phm_ppt_v1_mm_clock_voltage_dependency_record *mm_table_record;
PP_ASSERT_WITH_CODE((0 != mm_dependency_table->ucNumEntries),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_mm_clock_voltage_dependency_record)
- * mm_dependency_table->ucNumEntries;
- mm_table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == mm_table)
+ mm_table = kzalloc(struct_size(mm_table, entries,
mm_dependency_table->ucNumEntries),
+ GFP_KERNEL);
+ if (!mm_table)
return -ENOMEM;
mm_table->count = mm_dependency_table->ucNumEntries;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
index 787b23fa25e7..4f6a73a2cf28 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
@@ -344,18 +344,15 @@ static int get_mm_clock_voltage_table(
phm_ppt_v1_mm_clock_voltage_dependency_table **vega10_mm_table,
const ATOM_Vega10_MM_Dependency_Table *mm_dependency_table)
{
- uint32_t table_size, i;
+ uint32_t i;
const ATOM_Vega10_MM_Dependency_Record *mm_dependency_record;
phm_ppt_v1_mm_clock_voltage_dependency_table *mm_table;
PP_ASSERT_WITH_CODE((mm_dependency_table->ucNumEntries != 0),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_mm_clock_voltage_dependency_record) *
- mm_dependency_table->ucNumEntries;
- mm_table = kzalloc(table_size, GFP_KERNEL);
-
+ mm_table = kzalloc(struct_size(mm_table, entries,
mm_dependency_table->ucNumEntries),
+ GFP_KERNEL);
if (!mm_table)
return -ENOMEM;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 12/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_voltage_lookup_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_ppt_v1_voltage_lookup_table, instead of a one-element array,
and use the struct_size() helper to calculate the size for the allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7d61df.jwrffnjxgbjskpop%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h | 2 +-
.../drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c | 10 +++---
.../amd/pm/powerplay/hwmgr/vega10_processpptables.c| 10 +++---
3 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
index 923cc04e405a..e11298cdeb30 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
@@ -86,7 +86,7 @@ typedef struct phm_ppt_v1_voltage_lookup_record
phm_ppt_v1_voltage_lookup_record
struct phm_ppt_v1_voltage_lookup_table {
uint32_t count;
- phm_ppt_v1_voltage_lookup_record entries[1];/* Dynamically allocate
count entries. */
+ phm_ppt_v1_voltage_lookup_record entries[];/* Dynamically allocate
count entries. */
};
typedef struct phm_ppt_v1_voltage_lookup_table phm_ppt_v1_voltage_lookup_table;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
index 5d8016cd1986..426655b9c678 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
@@ -157,7 +157,7 @@ static int get_vddc_lookup_table(
uint32_t max_levels
)
{
- uint32_t table_size, i;
+ uint32_t i;
phm_ppt_v1_voltage_lookup_table *table;
phm_ppt_v1_voltage_lookup_record *record;
ATOM_Tonga_Voltage_Lookup_Record *atom_record;
@@ -165,12 +165,8 @@ static int get_vddc_lookup_table(
PP_ASSERT_WITH_CODE((0 != vddc_lookup_pp_tables->ucNumEntries),
"Invalid CAC Leakage PowerPlay Table!", return 1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_voltage_lookup_record) * max_levels;
-
- table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == table)
+ table = kzalloc(struct_size(table, entries, max_levels), GFP_KERNEL);
+ if (!table)
return -ENOMEM;
table->count = vddc_lookup_pp_tables->ucNumEntries;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
index 4f6a73a2cf28..3d7f915381c8 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
@@ -1040,18 +1040,14 @@ static int get_vddc_lookup_table(
const ATOM_Vega10_Voltage_Lookup_Table *vddc_lookup_pp_tables,
uint32_t max_levels)
{
- uint32_t table_size, i;
+ uint32_t i;
phm_ppt_v1_voltage_lookup_table *table;
PP_ASSERT_WITH_CODE((vddc_lookup_pp_tables->ucNumEntries != 0),
"Invalid SOC_VDDD Lookup Table!", return 1);
- table_size = sizeof(uint32_t) +
- sizeof(phm_ppt_v1_voltage_lookup_record) * max_levels;
-
- table = kzalloc(table_size, GFP_KERNEL);
-
- if (table == NULL)
+ table = kzalloc(struct_size(table, entries, max_levels), GFP_KERNEL);
+ if (!table)
return -ENOMEM;
table->count = vddc_lookup_pp_tables->ucNumEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 10/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_ppt_v1_clock_voltage_dependency_table, instead of a one-element
array, and use the struct_size() helper to calculate the size for the
allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link:
Signed-off-by: Gustavo A. R. Silva
---
.../drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h| 2 +-
.../powerplay/hwmgr/process_pptables_v1_0.c | 31
.../powerplay/hwmgr/vega10_processpptables.c | 50 ++-
3 files changed, 27 insertions(+), 56 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
index c0193e09d58a..c167083b0872 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h
@@ -48,7 +48,7 @@ typedef struct phm_ppt_v1_clock_voltage_dependency_record
phm_ppt_v1_clock_volta
struct phm_ppt_v1_clock_voltage_dependency_table {
uint32_t count;/* Number of
entries. */
- phm_ppt_v1_clock_voltage_dependency_record entries[1]; /*
Dynamically allocate count entries. */
+ phm_ppt_v1_clock_voltage_dependency_record entries[]; /*
Dynamically allocate count entries. */
};
typedef struct phm_ppt_v1_clock_voltage_dependency_table
phm_ppt_v1_clock_voltage_dependency_table;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
index 52188f6cd150..0725531fbfff 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
@@ -367,7 +367,7 @@ static int get_mclk_voltage_dependency_table(
ATOM_Tonga_MCLK_Dependency_Table const *mclk_dep_table
)
{
- uint32_t table_size, i;
+ uint32_t i;
phm_ppt_v1_clock_voltage_dependency_table *mclk_table;
phm_ppt_v1_clock_voltage_dependency_record *mclk_table_record;
ATOM_Tonga_MCLK_Dependency_Record *mclk_dep_record;
@@ -375,12 +375,9 @@ static int get_mclk_voltage_dependency_table(
PP_ASSERT_WITH_CODE((0 != mclk_dep_table->ucNumEntries),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
sizeof(phm_ppt_v1_clock_voltage_dependency_record)
- * mclk_dep_table->ucNumEntries;
-
- mclk_table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == mclk_table)
+ mclk_table = kzalloc(struct_size(mclk_table, entries,
mclk_dep_table->ucNumEntries),
+GFP_KERNEL);
+ if (!mclk_table)
return -ENOMEM;
mclk_table->count = (uint32_t)mclk_dep_table->ucNumEntries;
@@ -410,7 +407,7 @@ static int get_sclk_voltage_dependency_table(
PPTable_Generic_SubTable_Header const *sclk_dep_table
)
{
- uint32_t table_size, i;
+ uint32_t i;
phm_ppt_v1_clock_voltage_dependency_table *sclk_table;
phm_ppt_v1_clock_voltage_dependency_record *sclk_table_record;
@@ -422,12 +419,9 @@ static int get_sclk_voltage_dependency_table(
PP_ASSERT_WITH_CODE((0 != tonga_table->ucNumEntries),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
sizeof(phm_ppt_v1_clock_voltage_dependency_record)
- * tonga_table->ucNumEntries;
-
- sclk_table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == sclk_table)
+ sclk_table = kzalloc(struct_size(sclk_table, entries,
tonga_table->ucNumEntries),
+GFP_KERNEL);
+ if (!sclk_table)
return -ENOMEM;
sclk_table->count = (uint32_t)tonga_table->ucNumEntries;
@@ -454,12 +448,9 @@ static int get_sclk_voltage_dependency_table(
PP_ASSERT_WITH_CODE((0 != polaris_table->ucNumEntries),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
sizeof(phm_ppt_v1_clock_voltage_dependency_record)
- * polaris_table->ucNumEntries;
-
- sclk_table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == sclk_table)
+ sclk_table = kzalloc(struct_size(sclk_table, entries,
polaris_tab
[PATCH 09/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_samu_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_samu_clock_voltage_dependency_table, instead of a one-element array,
and use the struct_size() helper to calculate the size for the allocation.
Also, save some heap space as the original code is multiplying
table->numEntries by sizeof(struct phm_samu_clock_voltage_dependency_table)
when it should have been multiplied it by
sizeof(struct phm_samu_clock_voltage_dependency_record) instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c5d3a.rym4gmzr3e0jezy+%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h| 2 +-
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index 7e0c948a7097..dad703ba0522 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -404,7 +404,7 @@ struct phm_samu_clock_voltage_dependency_record {
struct phm_samu_clock_voltage_dependency_table {
uint8_t count;
- struct phm_samu_clock_voltage_dependency_record entries[1];
+ struct phm_samu_clock_voltage_dependency_record entries[];
};
struct phm_cac_tdp_table {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index e059802d1e25..48d550d26c6a 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1163,15 +1163,12 @@ static int get_samu_clock_voltage_limit_table(struct
pp_hwmgr *hwmgr,
struct phm_samu_clock_voltage_dependency_table **ptable,
const ATOM_PPLIB_SAMClk_Voltage_Limit_Table *table)
{
- unsigned long table_size, i;
+ unsigned long i;
struct phm_samu_clock_voltage_dependency_table *samu_table;
- table_size = sizeof(unsigned long) +
- sizeof(struct phm_samu_clock_voltage_dependency_table) *
- table->numEntries;
-
- samu_table = kzalloc(table_size, GFP_KERNEL);
- if (NULL == samu_table)
+ samu_table = kzalloc(struct_size(samu_table, entries,
table->numEntries),
+GFP_KERNEL);
+ if (!samu_table)
return -ENOMEM;
samu_table->count = table->numEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 08/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_cac_leakage_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_cac_leakage_table, instead of a one-element array,
and use the struct_size() helper to calculate the size for the allocation.
Also, save some heap space as the original code is multiplying
table->ucNumEntries by sizeof(struct phm_cac_leakage_table) when it
should have been multiplied it by sizeof(struct phm_cac_leakage_record)
instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c5d38.it%2fqtjn+659xudo5%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h | 2 +-
.../drm/amd/pm/powerplay/hwmgr/processpptables.c| 13 +
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index b8e33325fac6..7e0c948a7097 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -393,7 +393,7 @@ union phm_cac_leakage_record {
struct phm_cac_leakage_table {
uint32_t count;
- union phm_cac_leakage_record entries[1];
+ union phm_cac_leakage_record entries[];
};
struct phm_samu_clock_voltage_dependency_record {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index 7719f52e6d52..e059802d1e25 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1384,17 +1384,14 @@ static int get_cac_leakage_table(struct pp_hwmgr *hwmgr,
const ATOM_PPLIB_CAC_Leakage_Table *table)
{
struct phm_cac_leakage_table *cac_leakage_table;
- unsigned longtable_size, i;
+ unsigned long i;
- if (hwmgr == NULL || table == NULL || ptable == NULL)
+ if (!hwmgr || !table || !ptable)
return -EINVAL;
- table_size = sizeof(ULONG) +
- (sizeof(struct phm_cac_leakage_table) * table->ucNumEntries);
-
- cac_leakage_table = kzalloc(table_size, GFP_KERNEL);
-
- if (cac_leakage_table == NULL)
+ cac_leakage_table = kzalloc(struct_size(cac_leakage_table, entries,
table->ucNumEntries),
+ GFP_KERNEL);
+ if (!cac_leakage_table)
return -ENOMEM;
cac_leakage_table->count = (ULONG)table->ucNumEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 07/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_vce_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_vce_clock_voltage_dependency_table, instead of a one-element array,
and use the struct_size() helper to calculate the size for the allocation.
Also, save some heap space as the original code is multiplying
table->numEntries by sizeof(struct phm_vce_clock_voltage_dependency_table)
when it should have multiplied it by sizeof(struct
phm_vce_clock_voltage_dependency_record)
instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c5d35.pjtogs3h9khzk6ws%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h| 2 +-
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index ad614e32079e..b8e33325fac6 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -186,7 +186,7 @@ struct phm_acpclock_voltage_dependency_table {
struct phm_vce_clock_voltage_dependency_table {
uint8_t count;
- struct phm_vce_clock_voltage_dependency_record entries[1];
+ struct phm_vce_clock_voltage_dependency_record entries[];
};
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index b2ef76580c6a..7719f52e6d52 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1135,15 +1135,12 @@ static int get_vce_clock_voltage_limit_table(struct
pp_hwmgr *hwmgr,
const ATOM_PPLIB_VCE_Clock_Voltage_Limit_Table *table,
const VCEClockInfoArray*array)
{
- unsigned long table_size, i;
+ unsigned long i;
struct phm_vce_clock_voltage_dependency_table *vce_table = NULL;
- table_size = sizeof(unsigned long) +
- sizeof(struct phm_vce_clock_voltage_dependency_table)
- * table->numEntries;
-
- vce_table = kzalloc(table_size, GFP_KERNEL);
- if (NULL == vce_table)
+ vce_table = kzalloc(struct_size(vce_table, entries, table->numEntries),
+ GFP_KERNEL);
+ if (!vce_table)
return -ENOMEM;
vce_table->count = table->numEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 06/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_phase_shedding_limits_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_phase_shedding_limits_table, instead of a one-element array,
and use the struct_size() helper to calculate the size for the allocation.
Also, save some heap space as the original code is multiplying
ptable->ucNumEntries by sizeof(struct phm_phase_shedding_limits_table)
when it should have multiplied it by sizeof(struct
phm_phase_shedding_limits_record)
instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c5d36.6pstuzp2hrxaz7im%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h | 2 +-
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 12
2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index 361cb1125351..ad614e32079e 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -161,7 +161,7 @@ struct phm_vce_clock_voltage_dependency_record {
struct phm_phase_shedding_limits_table {
uint32_t count;
- struct phm_phase_shedding_limits_record entries[1];
+ struct phm_phase_shedding_limits_record entries[];
};
struct phm_vceclock_voltage_dependency_table {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index a1b198045978..b2ef76580c6a 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1530,16 +1530,12 @@ static int init_phase_shedding_table(struct pp_hwmgr
*hwmgr,
(((unsigned long)powerplay_table4) +
le16_to_cpu(powerplay_table4->usVddcPhaseShedLimitsTableOffset));
struct phm_phase_shedding_limits_table *table;
- unsigned long size, i;
+ unsigned long i;
- size = sizeof(unsigned long) +
- (sizeof(struct phm_phase_shedding_limits_table)
*
- ptable->ucNumEntries);
-
- table = kzalloc(size, GFP_KERNEL);
-
- if (table == NULL)
+ table = kzalloc(struct_size(table, entries,
ptable->ucNumEntries),
+ GFP_KERNEL);
+ if (!table)
return -ENOMEM;
table->count = (unsigned long)ptable->ucNumEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 05/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_acp_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_acp_clock_voltage_dependency_table, instead of a one-element
array, and use the struct_size() helper to calculate the size for the
allocation.
Also, save some heap space as the original code is multiplying
table->numEntries by sizeof(struct phm_acp_clock_voltage_dependency_table)
when it should have multiplied it by
sizeof(phm_acp_clock_voltage_dependency_record)
instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c5d3c.tyfohg%2fa6jycl6zn%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h| 2 +-
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index 2f1886bc5535..361cb1125351 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -150,7 +150,7 @@ struct phm_acp_clock_voltage_dependency_record {
struct phm_acp_clock_voltage_dependency_table {
uint32_t count;
- struct phm_acp_clock_voltage_dependency_record entries[1];
+ struct phm_acp_clock_voltage_dependency_record entries[];
};
struct phm_vce_clock_voltage_dependency_record {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index 305d95c4162d..a1b198045978 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1194,15 +1194,12 @@ static int get_acp_clock_voltage_limit_table(struct
pp_hwmgr *hwmgr,
struct phm_acp_clock_voltage_dependency_table **ptable,
const ATOM_PPLIB_ACPClk_Voltage_Limit_Table *table)
{
- unsigned table_size, i;
+ unsigned long i;
struct phm_acp_clock_voltage_dependency_table *acp_table;
- table_size = sizeof(unsigned long) +
- sizeof(struct phm_acp_clock_voltage_dependency_table) *
- table->numEntries;
-
- acp_table = kzalloc(table_size, GFP_KERNEL);
- if (NULL == acp_table)
+ acp_table = kzalloc(struct_size(acp_table, entries, table->numEntries),
+ GFP_KERNEL);
+ if (!acp_table)
return -ENOMEM;
acp_table->count = (unsigned long)table->numEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 04/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_uvd_clock_voltage_dependency_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_uvd_clock_voltage_dependency_table, instead of a one-element
array, and use the struct_size() helper to calculate the size for the
allocation.
Also, save some heap space as the original code is multiplying
table->numEntries by sizeof(struct phm_uvd_clock_voltage_dependency_table)
when it should have multiplied it by
sizeof(phm_uvd_clock_voltage_dependency_record)
instead.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c433e.pxkc6ksn6hn%2fldhj%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h| 2 +-
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 11 ---
2 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index e84cff09af2d..2f1886bc5535 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -140,7 +140,7 @@ struct phm_uvd_clock_voltage_dependency_record {
struct phm_uvd_clock_voltage_dependency_table {
uint8_t count;
- struct phm_uvd_clock_voltage_dependency_record entries[1];
+ struct phm_uvd_clock_voltage_dependency_record entries[];
};
struct phm_acp_clock_voltage_dependency_record {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index d9bed4df6f65..305d95c4162d 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -1105,15 +1105,12 @@ static int get_uvd_clock_voltage_limit_table(struct
pp_hwmgr *hwmgr,
const ATOM_PPLIB_UVD_Clock_Voltage_Limit_Table *table,
const UVDClockInfoArray *array)
{
- unsigned long table_size, i;
+ unsigned long i;
struct phm_uvd_clock_voltage_dependency_table *uvd_table;
- table_size = sizeof(unsigned long) +
-sizeof(struct phm_uvd_clock_voltage_dependency_table) *
-table->numEntries;
-
- uvd_table = kzalloc(table_size, GFP_KERNEL);
- if (NULL == uvd_table)
+ uvd_table = kzalloc(struct_size(uvd_table, entries, table->numEntries),
+ GFP_KERNEL);
+ if (!uvd_table)
return -ENOMEM;
uvd_table->count = table->numEntries;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 03/14] drm/amd/pm: Replace one-element array with flexible-array in struct phm_clock_array
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_clock_array, instead of a one-element array, and use the
struct_size() helper to calculate the size for the allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c433f.zymd+yuivawihgve%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h| 2 +-
.../amd/pm/powerplay/hwmgr/process_pptables_v1_0.c| 11 ---
.../gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 7 +++
.../amd/pm/powerplay/hwmgr/vega10_processpptables.c | 9 +++--
4 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index d68b547743e6..e84cff09af2d 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -91,7 +91,7 @@ struct phm_set_power_state_input {
struct phm_clock_array {
uint32_t count;
- uint32_t values[1];
+ uint32_t values[];
};
struct phm_clock_voltage_dependency_record {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
index b760f95e7fa7..52188f6cd150 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/process_pptables_v1_0.c
@@ -318,19 +318,16 @@ static int get_valid_clk(
phm_ppt_v1_clock_voltage_dependency_table const
*clk_volt_pp_table
)
{
- uint32_t table_size, i;
+ uint32_t i;
struct phm_clock_array *table;
phm_ppt_v1_clock_voltage_dependency_record *dep_record;
PP_ASSERT_WITH_CODE((0 != clk_volt_pp_table->count),
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(uint32_t) * clk_volt_pp_table->count;
-
- table = kzalloc(table_size, GFP_KERNEL);
-
- if (NULL == table)
+ table = kzalloc(struct_size(table, values, clk_volt_pp_table->count),
+ GFP_KERNEL);
+ if (!table)
return -ENOMEM;
table->count = (uint32_t)clk_volt_pp_table->count;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index d94a7d8e0587..d9bed4df6f65 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -404,12 +404,11 @@ static int get_valid_clk(struct pp_hwmgr *hwmgr,
struct phm_clock_array **ptable,
const struct phm_clock_voltage_dependency_table *table)
{
- unsigned long table_size, i;
+ unsigned long i;
struct phm_clock_array *clock_table;
- table_size = sizeof(unsigned long) + sizeof(unsigned long) *
table->count;
- clock_table = kzalloc(table_size, GFP_KERNEL);
- if (NULL == clock_table)
+ clock_table = kzalloc(struct_size(clock_table, values, table->count),
GFP_KERNEL);
+ if (!clock_table)
return -ENOMEM;
clock_table->count = (unsigned long)table->count;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
index f29af5ca0aa0..e655c04ccdfb 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_processpptables.c
@@ -875,17 +875,14 @@ static int get_valid_clk(
struct phm_clock_array **clk_table,
const phm_ppt_v1_clock_voltage_dependency_table
*clk_volt_pp_table)
{
- uint32_t table_size, i;
+ uint32_t i;
struct phm_clock_array *table;
PP_ASSERT_WITH_CODE(clk_volt_pp_table->count,
"Invalid PowerPlay Table!", return -1);
- table_size = sizeof(uint32_t) +
- sizeof(uint32_t) * clk_volt_pp_table->count;
-
- table = kzalloc(table_size, GFP_KERNEL);
-
+ table = kzalloc(struct_size(table, values, clk_volt_pp_table->count),
+ GFP_KERNEL);
if (!table)
return -ENOMEM;
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 02/14] drm/amd/pm: Replace one-element array with flexible-array member in struct vi_dpm_table
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Use a flexible-array member in struct vi_dpm_table instead of a
one-element array.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link: https://lore.kernel.org/lkml/5f7c433c.ttk9rna+f58kyduy%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index a1dbfd5636e6..d68b547743e6 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -60,7 +60,7 @@ struct vi_dpm_level {
struct vi_dpm_table {
uint32_t count;
- struct vi_dpm_level dpm_level[1];
+ struct vi_dpm_level dpm_level[];
};
#define PCIE_PERF_REQ_REMOVE_REGISTRY 0
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 01/14] drm/amd/pm: Replace one-element array with flexible-array member
There is a regular need in the kernel to provide a way to declare having
a dynamically sized set of trailing elements in a structure. Kernel code
should always use “flexible array members”[1] for these cases. The older
style of one-element or zero-length arrays should no longer be used[2].
Refactor the code according to the use of a flexible-array member in
struct phm_clock_voltage_dependency_table, instead of a one-element
array, and use the struct_size() helper to calculate the size for the
allocation.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2]
https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays
Build-tested-by: kernel test robot
Link:
https://lore.kernel.org/lkml/5f7c295c.8iqp1ifc6oivdq%2f%2f%25...@intel.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/pm/inc/hwmgr.h | 4 ++--
drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c | 9 +++--
drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c | 2 +-
drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c | 5 ++---
4 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
index 3898a95ec28b..a1dbfd5636e6 100644
--- a/drivers/gpu/drm/amd/pm/inc/hwmgr.h
+++ b/drivers/gpu/drm/amd/pm/inc/hwmgr.h
@@ -122,8 +122,8 @@ struct phm_acpclock_voltage_dependency_record {
};
struct phm_clock_voltage_dependency_table {
- uint32_t count;
/* Number of entries. */
- struct phm_clock_voltage_dependency_record entries[1]; /*
Dynamically allocate count entries. */
+ uint32_t count; /*
Number of entries. */
+ struct phm_clock_voltage_dependency_record entries[]; /*
Dynamically allocate count entries. */
};
struct phm_phase_shedding_limits_record {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
index 719597c5d27d..d94a7d8e0587 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/processpptables.c
@@ -377,14 +377,11 @@ static int get_clock_voltage_dependency_table(struct
pp_hwmgr *hwmgr,
const ATOM_PPLIB_Clock_Voltage_Dependency_Table *table)
{
- unsigned long table_size, i;
+ unsigned long i;
struct phm_clock_voltage_dependency_table *dep_table;
- table_size = sizeof(unsigned long) +
- sizeof(struct phm_clock_voltage_dependency_table)
- * table->ucNumEntries;
-
- dep_table = kzalloc(table_size, GFP_KERNEL);
+ dep_table = kzalloc(struct_size(dep_table, entries,
table->ucNumEntries),
+ GFP_KERNEL);
if (NULL == dep_table)
return -ENOMEM;
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
index 35ed47ebaf09..ed9b89980184 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c
@@ -276,7 +276,7 @@ static int smu8_init_dynamic_state_adjustment_rule_settings(
{
struct phm_clock_voltage_dependency_table *table_clk_vlt;
- table_clk_vlt = kzalloc(struct_size(table_clk_vlt, entries, 7),
+ table_clk_vlt = kzalloc(struct_size(table_clk_vlt, entries, 8),
GFP_KERNEL);
if (NULL == table_clk_vlt) {
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c
index 60b5ca974356..b485f8b1d6f2 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu_helper.c
@@ -492,13 +492,12 @@ int phm_get_sclk_for_voltage_evv(struct pp_hwmgr *hwmgr,
*/
int phm_initializa_dynamic_state_adjustment_rule_settings(struct pp_hwmgr
*hwmgr)
{
- uint32_t table_size;
struct phm_clock_voltage_dependency_table *table_clk_vlt;
struct phm_ppt_v1_information *pptable_info = (struct
phm_ppt_v1_information *)(hwmgr->pptable);
/* initialize vddc_dep_on_dal_pwrl table */
- table_size = sizeof(uint32_t) + 4 * sizeof(struct
phm_clock_voltage_dependency_record);
- table_clk_vlt = kzalloc(table_size, GFP_KERNEL);
+ table_clk_vlt = kzalloc(struct_size(table_clk_vlt, entries, 4),
+ GFP_KERNEL);
if (NULL == table_clk_vlt) {
pr_err("Can not allocate space for vddc_dep_on_dal_pwrl! \n");
--
2.27.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 00/14] drm/amd/pm: Replace one-element arrays with flexible-array members
Hi all, This series aims to replace one-element arrays with flexible-array members. There is a regular need in the kernel to provide a way to declare having a dynamically sized set of trailing elements in a structure. Kernel code should always use “flexible array members”[1] for these cases. The older style of one-element or zero-length arrays should no longer be used[2]. Refactor the code according to the use of flexible-array members, instead of one-element arrays, and use the struct_size() helper to calculate the size for the dynamic memory allocation. Also, save some heap space in the process. More on this on each individual patch. This series also addresses multiple of the following sorts of warnings: drivers/gpu/drm/amd/amdgpu/../pm/powerplay/hwmgr/smu8_hwmgr.c:1515:37: warning: array subscript 1 is above array bounds of ‘const struct phm_clock_voltage_dependency_record[1]’ [-Warray-bounds] which, in this case, they are false positives, but nervertheless should be fixed in order to enable -Warray-bounds[3][4]. [1] https://en.wikipedia.org/wiki/Flexible_array_member [2] https://www.kernel.org/doc/html/v5.9-rc1/process/deprecated.html#zero-length-and-one-element-arrays [3] https://git.kernel.org/linus/44720996e2d79e47d508b0abe99b931a726a3197 [4] https://github.com/KSPP/linux/issues/109 Gustavo A. R. Silva (14): drm/amd/pm: Replace one-element array with flexible-array member drm/amd/pm: Replace one-element array with flexible-array member in struct vi_dpm_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_clock_array drm/amd/pm: Replace one-element array with flexible-array in struct phm_uvd_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_acp_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_phase_shedding_limits_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_vce_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_cac_leakage_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_samu_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_mm_clock_voltage_dependency_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_voltage_lookup_table drm/amd/pm: Replace one-element array with flexible-array in struct phm_ppt_v1_pcie_table drm/amd/pm: Replace one-element array with flexible-array in struct ATOM_Vega10_GFXCLK_Dependency_Table drivers/gpu/drm/amd/pm/inc/hwmgr.h| 20 ++--- .../drm/amd/pm/powerplay/hwmgr/hwmgr_ppt.h| 8 +- .../powerplay/hwmgr/process_pptables_v1_0.c | 85 +++--- .../amd/pm/powerplay/hwmgr/processpptables.c | 85 +++--- .../drm/amd/pm/powerplay/hwmgr/smu8_hwmgr.c | 2 +- .../drm/amd/pm/powerplay/hwmgr/smu_helper.c | 5 +- .../amd/pm/powerplay/hwmgr/vega10_pptable.h | 2 +- .../powerplay/hwmgr/vega10_processpptables.c | 88 ++- 8 files changed, 107 insertions(+), 188 deletions(-) -- 2.27.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/smu10: Replace one-element array and use struct_size() helper
On Wed, May 20, 2020 at 09:42:27AM +0200, Christian König wrote: > > > > Signed-off-by: Gustavo A. R. Silva > > Acked-by: Christian König > > May I suggest that we add a section how to correctly do this to > Documentation/process/coding-style.rst or similar document? > That's already on my list. :) > I've seen a bunch of different approaches and some even doesn't work with > some gcc versions and result in a broken binary. > Do you have an example of that one that doesn't work with some GCC versions? It'd be interesting to take a look... Thanks -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/[radeon|amdgpu]: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct SISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdgpu/si_dpm.c | 5 ++---
drivers/gpu/drm/amd/amdgpu/sislands_smc.h | 2 +-
drivers/gpu/drm/radeon/si_dpm.c | 5 ++---
3 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
index c00ba4b23c9a6..0fc56c5bac080 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.c
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.c
@@ -5715,10 +5715,9 @@ static int si_upload_sw_state(struct amdgpu_device *adev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
diff --git a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
index d2930eceaf3c8..a089dbf8f7a93 100644
--- a/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
+++ b/drivers/gpu/drm/amd/amdgpu/sislands_smc.h
@@ -186,7 +186,7 @@ struct SISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+SISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct SISLANDS_SMC_SWSTATE SISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/si_dpm.c b/drivers/gpu/drm/radeon/si_dpm.c
index a167e1c36d243..bab01ca864c63 100644
--- a/drivers/gpu/drm/radeon/si_dpm.c
+++ b/drivers/gpu/drm/radeon/si_dpm.c
@@ -5253,10 +5253,9 @@ static int si_upload_sw_state(struct radeon_device *rdev,
int ret;
u32 address = si_pi->state_table_start +
offsetof(SISLANDS_SMC_STATETABLE, driverState);
- u32 state_size = sizeof(SISLANDS_SMC_SWSTATE) +
- ((new_state->performance_level_count - 1) *
-sizeof(SISLANDS_SMC_HW_PERFORMANCE_LEVEL));
SISLANDS_SMC_SWSTATE *smc_state = _pi->smc_statetable.driverState;
+ size_t state_size = struct_size(smc_state, levels,
+ new_state->performance_level_count);
memset(smc_state, 0, state_size);
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/radeon/dpm: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Reviewed-by: Christian König
Signed-off-by: Gustavo A. R. Silva
---
Changes in v2:
- Use type size_t instead of u16 for state_size variable.
drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
drivers/gpu/drm/radeon/ni_dpm.c | 7 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..abb6345bfae32 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device
*rdev,
struct rv7xx_power_info *pi = rv770_get_pi(rdev);
u16 address = pi->state_table_start +
offsetof(NISLANDS_SMC_STATETABLE, driverState);
- u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
- ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) *
sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+ NISLANDS_SMC_SWSTATE *smc_state;
+ size_t state_size = struct_size(smc_state, levels,
+ NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
int ret;
- NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
+ smc_state = kzalloc(state_size, GFP_KERNEL);
if (smc_state == NULL)
return -ENOMEM;
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
On Fri, May 22, 2020 at 09:00:09AM +0200, Christian König wrote: > > +++ b/drivers/gpu/drm/radeon/ni_dpm.c > > @@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device > > *rdev, > > struct rv7xx_power_info *pi = rv770_get_pi(rdev); > > u16 address = pi->state_table_start + > > offsetof(NISLANDS_SMC_STATETABLE, driverState); > > - u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) + > > - ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) * > > sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL)); > > + NISLANDS_SMC_SWSTATE *smc_state; > > + u16 state_size = struct_size(smc_state, levels, > > + NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE); > > Probably better to use size_t instead of u16 here. With that fixed feel free > to stick my rb on the patch. > Sure thing. I'll send v2, shortly. Thanks, Christian. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/radeon/dpm: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct NISLANDS_SMC_SWSTATE.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdgpu/si_dpm.h | 2 +-
drivers/gpu/drm/radeon/ni_dpm.c | 7 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
index 6b7d292b919f3..bc0be6818e218 100644
--- a/drivers/gpu/drm/amd/amdgpu/si_dpm.h
+++ b/drivers/gpu/drm/amd/amdgpu/si_dpm.h
@@ -781,7 +781,7 @@ struct NISLANDS_SMC_SWSTATE
uint8_t levelCount;
uint8_t padding2;
uint8_t padding3;
-NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[1];
+NISLANDS_SMC_HW_PERFORMANCE_LEVEL levels[];
};
typedef struct NISLANDS_SMC_SWSTATE NISLANDS_SMC_SWSTATE;
diff --git a/drivers/gpu/drm/radeon/ni_dpm.c b/drivers/gpu/drm/radeon/ni_dpm.c
index b57c37ddd164c..7d81dde509dc9 100644
--- a/drivers/gpu/drm/radeon/ni_dpm.c
+++ b/drivers/gpu/drm/radeon/ni_dpm.c
@@ -2685,11 +2685,12 @@ static int ni_upload_sw_state(struct radeon_device
*rdev,
struct rv7xx_power_info *pi = rv770_get_pi(rdev);
u16 address = pi->state_table_start +
offsetof(NISLANDS_SMC_STATETABLE, driverState);
- u16 state_size = sizeof(NISLANDS_SMC_SWSTATE) +
- ((NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE - 1) *
sizeof(NISLANDS_SMC_HW_PERFORMANCE_LEVEL));
+ NISLANDS_SMC_SWSTATE *smc_state;
+ u16 state_size = struct_size(smc_state, levels,
+ NISLANDS_MAX_SMC_PERFORMANCE_LEVELS_PER_SWSTATE);
int ret;
- NISLANDS_SMC_SWSTATE *smc_state = kzalloc(state_size, GFP_KERNEL);
+ smc_state = kzalloc(state_size, GFP_KERNEL);
if (smc_state == NULL)
return -ENOMEM;
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu/smu10: Replace one-element array and use struct_size() helper
The current codebase makes use of one-element arrays in the following
form:
struct something {
int length;
u8 data[1];
};
struct something *instance;
instance = kmalloc(sizeof(*instance) + size, GFP_KERNEL);
instance->length = size;
memcpy(instance->data, source, size);
but the preferred mechanism to declare variable-length types such as
these ones is a flexible array member[1][2], introduced in C99:
struct foo {
int stuff;
struct boo array[];
};
By making use of the mechanism above, we will get a compiler warning
in case the flexible array does not occur last in the structure, which
will help us prevent some kind of undefined behavior bugs from being
inadvertently introduced[3] to the codebase from now on. So, replace
the one-element array with a flexible-array member.
Also, make use of the new struct_size() helper to properly calculate the
size of struct smu10_voltage_dependency_table.
This issue was found with the help of Coccinelle and, audited and fixed
_manually_.
[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://github.com/KSPP/linux/issues/21
[3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c | 6 ++
drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.h | 2 +-
2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
index 246bb9ac557d8..c9cfe90a29471 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
@@ -410,12 +410,10 @@ static int
smu10_get_clock_voltage_dependency_table(struct pp_hwmgr *hwmgr,
struct smu10_voltage_dependency_table **pptable,
uint32_t num_entry, const DpmClock_t
*pclk_dependency_table)
{
- uint32_t table_size, i;
+ uint32_t i;
struct smu10_voltage_dependency_table *ptable;
- table_size = sizeof(uint32_t) + sizeof(struct
smu10_voltage_dependency_table) * num_entry;
- ptable = kzalloc(table_size, GFP_KERNEL);
-
+ ptable = kzalloc(struct_size(ptable, entries, num_entry), GFP_KERNEL);
if (NULL == ptable)
return -ENOMEM;
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.h
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.h
index 1fb296a996f3a..0f969de10fabc 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.h
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.h
@@ -192,7 +192,7 @@ struct smu10_clock_voltage_dependency_record {
struct smu10_voltage_dependency_table {
uint32_t count;
- struct smu10_clock_voltage_dependency_record entries[1];
+ struct smu10_clock_voltage_dependency_record entries[];
};
struct smu10_clock_voltage_information {
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdkfd: Remove logically dead code in kfd_procfs_show
container_of is never null, so the null check on pdd is unnecessary.
If the null check is removed, function kfd_procfs_show()
will always return before reaching "return 0", hence
such return is logically dead. So, remove it, as well.
Addresses-Coverity-ID: 1492793 ("Logically dead code")
Fixes: d4566dee849e ("drm/amdkfd: Track GPU memory utilization per process")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index 598296034b43..63dcd30b2cdc 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -87,14 +87,11 @@ static ssize_t kfd_procfs_show(struct kobject *kobj, struct
attribute *attr,
} else if (strncmp(attr->name, "vram_", 5) == 0) {
struct kfd_process_device *pdd = container_of(attr, struct
kfd_process_device,
attr_vram);
- if (pdd)
- return snprintf(buffer, PAGE_SIZE, "%llu\n",
READ_ONCE(pdd->vram_usage));
+ return snprintf(buffer, PAGE_SIZE, "%llu\n",
READ_ONCE(pdd->vram_usage));
} else {
pr_err("Invalid attribute");
return -EINVAL;
}
-
- return 0;
}
static void kfd_procfs_kobj_release(struct kobject *kobj)
--
2.26.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Fix unsigned variable compared to less than zero
On 11/11/19 11:46, Mikita Lipski wrote:
>
> Thanks for catching it!
>
Glad to help out. :)
> Reviewed-by: Mikita Lipski
>
Thanks
--
Gustavo
>
> On 11.11.2019 12:25, Gustavo A. R. Silva wrote:
>> Currenly, the error check below on variable*vcpi_slots* is always
>> false because it is a uint64_t type variable, hence, the values
>> this variable can hold are never less than zero:
>>
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:
>> 4870 if (dm_new_connector_state->vcpi_slots < 0) {
>> 4871 DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
>> (int)dm_new_connector_stat e->vcpi_slots);
>> 4872 return dm_new_connector_state->vcpi_slots;
>> 4873 }
>>
>> Fix this by making*vcpi_slots* of int type
>>
>> Addresses-Coverity: 1487838 ("Unsigned compared against 0")
>> Fixes: b4c578f08378 ("drm/amd/display: Add MST atomic routines")
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 6db07e9e33ab..a8fc90a927d6 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -403,7 +403,7 @@ struct dm_connector_state {
>> bool underscan_enable;
>> bool freesync_capable;
>> uint8_t abm_level;
>> - uint64_t vcpi_slots;
>> + int vcpi_slots;
>> uint64_t pbn;
>> };
>> -- 2.23.0
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: Fix unsigned variable compared to less than zero
Currenly, the error check below on variable *vcpi_slots* is always
false because it is a uint64_t type variable, hence, the values
this variable can hold are never less than zero:
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:
4870 if (dm_new_connector_state->vcpi_slots < 0) {
4871 DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
(int)dm_new_connector_stat e->vcpi_slots);
4872 return dm_new_connector_state->vcpi_slots;
4873 }
Fix this by making *vcpi_slots* of int type.
Addresses-Coverity: 1487838 ("Unsigned compared against 0")
Fixes: b4c578f08378 ("drm/amd/display: Add MST atomic routines")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 6db07e9e33ab..a8fc90a927d6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -403,7 +403,7 @@ struct dm_connector_state {
bool underscan_enable;
bool freesync_capable;
uint8_t abm_level;
- uint64_t vcpi_slots;
+ int vcpi_slots;
uint64_t pbn;
};
--
2.23.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amdgpu/vcn: use amdgpu_ring_test_helper instead of
On 10/1/19 17:17, Liu, Leo wrote:
> amdgpu_ring_test_ring, so it will determine whether the ring is ready
>
> Signed-off-by: Leo Liu
> Cc: Gustavo A. R. Silva
Acked-by: Gustavo A. R. Silva
> ---
> drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 1 -
> drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c | 21 ++---
> drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 18 ++
> 3 files changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> index 93b3500e522b..b4f84a820a44 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
> @@ -202,7 +202,6 @@ static int vcn_v1_0_hw_init(void *handle)
>
> for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> ring = >vcn.inst->ring_enc[i];
> - ring->sched.ready = true;
> r = amdgpu_ring_test_helper(ring);
> if (r)
> goto done;
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> index 4628fd10a9ec..38f787a560cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_0.c
> @@ -247,30 +247,21 @@ static int vcn_v2_0_hw_init(void *handle)
> adev->nbio.funcs->vcn_doorbell_range(adev, ring->use_doorbell,
>ring->doorbell_index, 0);
>
> - ring->sched.ready = true;
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
>
> for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> ring = >vcn.inst->ring_enc[i];
> - ring->sched.ready = true;
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
> }
>
> ring = >vcn.inst->ring_jpeg;
> - ring->sched.ready = true;
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
>
> done:
> if (!r)
> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> index bf8626e15b09..cc19363f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
> @@ -258,29 +258,23 @@ static int vcn_v2_5_hw_init(void *handle)
> adev->nbio.funcs->vcn_doorbell_range(adev, ring->use_doorbell,
>ring->doorbell_index, j);
>
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
>
> for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
> ring = >vcn.inst[j].ring_enc[i];
> ring->sched.ready = false;
> continue;
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
> }
>
> ring = >vcn.inst[j].ring_jpeg;
> - r = amdgpu_ring_test_ring(ring);
> - if (r) {
> - ring->sched.ready = false;
> + r = amdgpu_ring_test_helper(ring);
> + if (r)
> goto done;
> - }
> }
> done:
> if (!r)
>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
On 10/1/19 17:21, Liu, Leo wrote: OK. So, maybe we can add a comment pointing that out? >>> That could be better. >>> >> Great. I'm glad it's not a bug. I'll write a patch for that so other >> people don't waste time taking a look. > > Thanks, just sent two patches to add comment, and long with the patch to > make VCN ring ready properly. > Awesome. Thank you. I would just add a commit log to this patch: [PATCH 2/2] drm/amdgpu: add a comment to VCN 2.5 encode ring I'd update the subject to: drm/amdgpu: add code comment in vcn_v2_5_hw_init and add this as a commit log: Add a comment to VCN 2.5 encode ring Also, I think it's important to follow the process and CC all the people and lists below: $ scripts/get_maintainer.pl --nokeywords --nogit --nogit-fallback drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c Alex Deucher (supporter:RADEON and AMDGPU DRM DRIVERS) "Christian König" (supporter:RADEON and AMDGPU DRM DRIVERS) "David (ChunMing) Zhou" (supporter:RADEON and AMDGPU DRM DRIVERS) David Airlie (maintainer:DRM DRIVERS) Daniel Vetter (maintainer:DRM DRIVERS) amd-gfx@lists.freedesktop.org (open list:RADEON and AMDGPU DRM DRIVERS) dri-de...@lists.freedesktop.org (open list:DRM DRIVERS) linux-ker...@vger.kernel.org (open list) Thanks -- Gustavo
Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
On 10/1/19 16:46, Liu, Leo wrote:
+ ring->sched.ready = true;
>>> This is redundant. all the sched->ready is initialized as true, please
>>> refer to drm_sched_init()
>>>
>> I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial
>> VCN2.0 support (v2)")
>> that line is also redundant?
>
> Yes.
>
OK.
>
r = amdgpu_ring_test_ring(ring);
if (r) {
ring->sched.ready = false;
@@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle)
for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
ring = >vcn.inst[j].ring_enc[i];
- ring->sched.ready = false;
- continue;
+ ring->sched.ready = true;
>>> Because the VCN 2.5 FW still has issue for encode, so we have it
>>> disabled here.
>>>
>> OK. So, maybe we can add a comment pointing that out?
>
> That could be better.
>
Great. I'm glad it's not a bug. I'll write a patch for that so other
people don't waste time taking a look.
I appreciate your feedback.
Thanks
--
Gustavo
Re: [PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
Hi,
On 10/1/19 16:29, Liu, Leo wrote:
>
> On 2019-10-01 1:16 p.m., Gustavo A. R. Silva wrote:
>> Notice that there is a *continue* statement in the middle of the
>> for loop and that prevents the code below from ever being reached:
>>
>> r = amdgpu_ring_test_ring(ring);
>> if (r) {
>> ring->sched.ready = false;
>> goto done;
>> }
>>
>> Fix this by removing the continue statement and updating ring->sched.ready
>> to true before calling amdgpu_ring_test_ring(ring).
>>
>> Notice that this fix is based on
>> commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)")
>>
>> Addresses-Coverity-ID 1485608 ("Structurally dead code")
>> Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports")
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>>
>> Any feedback is greatly appreciated.
>>
>> drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
>> b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
>> index 395c2259f979..47b0dcd59e13 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
>> @@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle)
>> adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell,
>> ring->doorbell_index, j);
>>
>> +ring->sched.ready = true;
>
> This is redundant. all the sched->ready is initialized as true, please
> refer to drm_sched_init()
>
I see... so in the following commit 1b61de45dfaf ("drm/amdgpu: add initial
VCN2.0 support (v2)")
that line is also redundant?
>
>> r = amdgpu_ring_test_ring(ring);
>> if (r) {
>> ring->sched.ready = false;
>> @@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle)
>>
>> for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
>> ring = >vcn.inst[j].ring_enc[i];
>> -ring->sched.ready = false;
>> -continue;
>> +ring->sched.ready = true;
>
> Because the VCN 2.5 FW still has issue for encode, so we have it
> disabled here.
>
OK. So, maybe we can add a comment pointing that out?
Thanks
--
Gustavo
>
>> r = amdgpu_ring_test_ring(ring);
>> if (r) {
>> ring->sched.ready = false;
>> @@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle)
>> }
>>
>> ring = >vcn.inst[j].ring_jpeg;
>> +ring->sched.ready = true;
>> r = amdgpu_ring_test_ring(ring);
>> if (r) {
>> ring->sched.ready = false;
[PATCH] drm/amdgpu: fix structurally dead code vcn_v2_5_hw_init
Notice that there is a *continue* statement in the middle of the
for loop and that prevents the code below from ever being reached:
r = amdgpu_ring_test_ring(ring);
if (r) {
ring->sched.ready = false;
goto done;
}
Fix this by removing the continue statement and updating ring->sched.ready
to true before calling amdgpu_ring_test_ring(ring).
Notice that this fix is based on
commit 1b61de45dfaf ("drm/amdgpu: add initial VCN2.0 support (v2)")
Addresses-Coverity-ID 1485608 ("Structurally dead code")
Fixes: 28c17d72072b ("drm/amdgpu: add VCN2.5 basic supports")
Signed-off-by: Gustavo A. R. Silva
---
Any feedback is greatly appreciated.
drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
index 395c2259f979..47b0dcd59e13 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v2_5.c
@@ -258,6 +258,7 @@ static int vcn_v2_5_hw_init(void *handle)
adev->nbio_funcs->vcn_doorbell_range(adev, ring->use_doorbell,
ring->doorbell_index, j);
+ ring->sched.ready = true;
r = amdgpu_ring_test_ring(ring);
if (r) {
ring->sched.ready = false;
@@ -266,8 +267,7 @@ static int vcn_v2_5_hw_init(void *handle)
for (i = 0; i < adev->vcn.num_enc_rings; ++i) {
ring = >vcn.inst[j].ring_enc[i];
- ring->sched.ready = false;
- continue;
+ ring->sched.ready = true;
r = amdgpu_ring_test_ring(ring);
if (r) {
ring->sched.ready = false;
@@ -276,6 +276,7 @@ static int vcn_v2_5_hw_init(void *handle)
}
ring = >vcn.inst[j].ring_jpeg;
+ ring->sched.ready = true;
r = amdgpu_ring_test_ring(ring);
if (r) {
ring->sched.ready = false;
--
2.23.0
Re: [PATCH] drm/amdkfd: Fix missing break in switch statement
On 7/22/19 2:10 PM, Alex Deucher wrote:
> On Sun, Jul 21, 2019 at 6:12 PM Gustavo A. R. Silva
> wrote:
>>
>> Add missing break statement in order to prevent the code from falling
>> through to case CHIP_NAVI10.
>>
>> This bug was found thanks to the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Fixes: 14328aa58ce5 ("drm/amdkfd: Add navi10 support to amdkfd. (v3)")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>
> Applied. Thanks!
>
By the way, Alex, I'm planning to add these fixes to my tree. I want
to send a pull-request to Linus for v5.3-rc2 this afternoon. We want
to have the -Wimplicit-fallthrough option globally enabled in v5.3,
and these are some of the last fall-through warnings remaining in
the kernel.
Can I have your Ack or Signed-off-by for all these drm patches?
Thanks!
--
Gustavo
Re: [PATCH] drm/amdgpu/gfx10: Fix missing break in switch statement
On 7/22/19 2:12 PM, Alex Deucher wrote:
> On Sun, Jul 21, 2019 at 6:39 PM Gustavo A. R. Silva
> wrote:
>>
>> Add missing break statement in order to prevent the code from falling
>> through to case AMDGPU_IRQ_STATE_ENABLE.
>>
>> This bug was found thanks to the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Fixes: a644d85a5cd4 ("drm/amdgpu: add gfx v10 implementation (v10)")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>
> Applied. Thanks!
>
Awesome! Glad to help. :)
Thanks
--
Gustavo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdkfd/kfd_mqd_manager_v10: Avoid fall-through warning
In preparation to enabling -Wimplicit-fallthrough, this patch silences
the following warning:
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_mqd_manager_v10.c: In function
‘mqd_manager_init_v10’:
./include/linux/dynamic_debug.h:122:52: warning: this statement may fall
through [-Wimplicit-fallthrough=]
#define __dynamic_func_call(id, fmt, func, ...) do { \
^
./include/linux/dynamic_debug.h:143:2: note: in expansion of macro
‘__dynamic_func_call’
__dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
^~~
./include/linux/dynamic_debug.h:153:2: note: in expansion of macro
‘_dynamic_func_call’
_dynamic_func_call(fmt, __dynamic_pr_debug, \
^~
./include/linux/printk.h:336:2: note: in expansion of macro ‘dynamic_pr_debug’
dynamic_pr_debug(fmt, ##__VA_ARGS__)
^~~~
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_mqd_manager_v10.c:432:3: note: in
expansion of macro ‘pr_debug’
pr_debug("%s@%i\n", __func__, __LINE__);
^~~~
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_mqd_manager_v10.c:433:2: note: here
case KFD_MQD_TYPE_COMPUTE:
^~~~
by removing the call to pr_debug() in KFD_MQD_TYPE_CP:
"The mqd init for CP and COMPUTE will have the same routine." [1]
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
[1] https://lore.kernel.org/lkml/c735a1cc-a545-50fb-44e7-c0ad93ee8...@amd.com/
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
index 4f8a6ffc5775..9cd3eb2d90bd 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
@@ -429,7 +429,6 @@ struct mqd_manager *mqd_manager_init_v10(enum KFD_MQD_TYPE
type,
switch (type) {
case KFD_MQD_TYPE_CP:
- pr_debug("%s@%i\n", __func__, __LINE__);
case KFD_MQD_TYPE_COMPUTE:
pr_debug("%s@%i\n", __func__, __LINE__);
mqd->allocate_mqd = allocate_mqd;
--
2.22.0
Re: [PATCH] drm/amdkfd/kfd_mqd_manager_v10: Fix missing break in switch statement
On 7/22/19 10:58 AM, Deucher, Alexander wrote:
> We need to add a /*fall through */ comment then.
>
It might be better to remove the call to pr_debug() in KFD_MQD_TYPE_CP:
case KFD_MQD_TYPE_CP:
case KFD_MQD_TYPE_COMPUTE:
pr_debug("%s@%i\n", __func__, __LINE__);
mqd->allocate_mqd = allocate_mqd;
Thanks
--
Gustavo
> Alex
>
> From: Liu, Shaoyun
> Sent: Monday, July 22, 2019 11:14 AM
> To: Gustavo A. R. Silva ; Cox, Philip
> ; Oded Gabbay ; Deucher, Alexander
> ; Koenig, Christian ;
> Zhou, David(ChunMing) ; David Airlie ;
> Daniel Vetter
> Cc: amd-gfx@lists.freedesktop.org ;
> dri-de...@lists.freedesktop.org ;
> linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] drm/amdkfd/kfd_mqd_manager_v10: Fix missing break in
> switch statement
>
> This one properly in purpose , The mqd init for CP and COMPUTE will
> have the same routine .
>
> Regard
>
> sshaoyun.liu
>
> On 2019-07-21 6:59 p.m., Gustavo A. R. Silva wrote:
>> Add missing break statement in order to prevent the code from falling
>> through to case KFD_MQD_TYPE_COMPUTE.
>>
>> This bug was found thanks to the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Fixes: 14328aa58ce5 ("drm/amdkfd: Add navi10 support to amdkfd. (v3)")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>> ---
>> drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> index 4f8a6ffc5775..1d8b13ad46f9 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
>> @@ -430,6 +430,7 @@ struct mqd_manager *mqd_manager_init_v10(enum
>> KFD_MQD_TYPE type,
>>switch (type) {
>>case KFD_MQD_TYPE_CP:
>>pr_debug("%s@%i\n", __func__, __LINE__);
>> + break;
>>case KFD_MQD_TYPE_COMPUTE:
>>pr_debug("%s@%i\n", __func__, __LINE__);
>>mqd->allocate_mqd = allocate_mqd;
>
[PATCH] drm/amdkfd/kfd_mqd_manager_v10: Fix missing break in switch statement
Add missing break statement in order to prevent the code from falling
through to case KFD_MQD_TYPE_COMPUTE.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: 14328aa58ce5 ("drm/amdkfd: Add navi10 support to amdkfd. (v3)")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
index 4f8a6ffc5775..1d8b13ad46f9 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_mqd_manager_v10.c
@@ -430,6 +430,7 @@ struct mqd_manager *mqd_manager_init_v10(enum KFD_MQD_TYPE
type,
switch (type) {
case KFD_MQD_TYPE_CP:
pr_debug("%s@%i\n", __func__, __LINE__);
+ break;
case KFD_MQD_TYPE_COMPUTE:
pr_debug("%s@%i\n", __func__, __LINE__);
mqd->allocate_mqd = allocate_mqd;
--
2.22.0
[PATCH] drm/amdgpu/gfx10: Fix missing break in switch statement
Add missing break statement in order to prevent the code from falling
through to case AMDGPU_IRQ_STATE_ENABLE.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: a644d85a5cd4 ("drm/amdgpu: add gfx v10 implementation (v10)")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
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 1675d5837c3c..35e8e29139b1 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -4611,6 +4611,7 @@ gfx_v10_0_set_gfx_eop_interrupt_state(struct
amdgpu_device *adev,
cp_int_cntl = REG_SET_FIELD(cp_int_cntl, CP_INT_CNTL_RING0,
TIME_STAMP_INT_ENABLE, 0);
WREG32(cp_int_cntl_reg, cp_int_cntl);
+ break;
case AMDGPU_IRQ_STATE_ENABLE:
cp_int_cntl = RREG32(cp_int_cntl_reg);
cp_int_cntl = REG_SET_FIELD(cp_int_cntl, CP_INT_CNTL_RING0,
--
2.22.0
[PATCH] drm/amdkfd: Fix missing break in switch statement
Add missing break statement in order to prevent the code from falling
through to case CHIP_NAVI10.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: 14328aa58ce5 ("drm/amdkfd: Add navi10 support to amdkfd. (v3)")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
index 792371442195..4e3fc284f6ac 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
@@ -668,6 +668,7 @@ static int kfd_fill_gpu_cache_info(struct kfd_dev *kdev,
case CHIP_RAVEN:
pcache_info = raven_cache_info;
num_of_cache_types = ARRAY_SIZE(raven_cache_info);
+ break;
case CHIP_NAVI10:
pcache_info = navi10_cache_info;
num_of_cache_types = ARRAY_SIZE(navi10_cache_info);
--
2.22.0
Re: [PATCH] gpu: drm: use struct_size() in kmalloc()
On 5/21/19 3:59 AM, Christian König wrote: > BTW: Is there actually good documentation how to correctly do the variable > length array at end of structure thing in the kernel? > > I do know that I've seen a lot of different variants like array[] array[0] or > array[1] and I have also seen a bunch of gcc versions failing to generate > correct > code for some of them. > > So we should probably nail down how to do things correctly. > A flexible array member is the preferred[1] mechanism: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20190520=76497732932f15e7323dc805e8ea8dc11bb587cf Thanks -- Gustavo [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
Re: [PATCH] gpu: drm: use struct_size() in kmalloc()
On 5/20/19 12:41 PM, Alex Deucher wrote:
> On Fri, May 17, 2019 at 8:43 AM xiaolinkui wrote:
>>
>> Use struct_size() helper to keep code simple.
>>
Again, this is not the reason why this helper was created.
>> Signed-off-by: xiaolinkui
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> This patch results in the following build error:
> DESCEND objtool
> CALLscripts/checksyscalls.sh
> CHK include/generated/compile.h
> CC [M] drivers/gpu/drm/amd/amdgpu/amdgpu_ras.o
> In file included from ./include/linux/kernel.h:15,
> from ./include/linux/list.h:9,
> from ./include/linux/wait.h:7,
> from ./include/linux/wait_bit.h:8,
> from ./include/linux/fs.h:6,
> from ./include/linux/debugfs.h:15,
> from drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:24:
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c: In function ‘amdgpu_ras_init’:
> ./include/linux/build_bug.h:16:45: error: negative width in bit-field
> ‘’
> #define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:(-!!(e)); }))
> ^
> ./include/linux/compiler.h:349:28: note: in expansion of macro
> ‘BUILD_BUG_ON_ZERO’
> #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> ^
> ./include/linux/overflow.h:306:30: note: in expansion of macro
> ‘__must_be_array’
>sizeof(*(p)->member) + __must_be_array((p)->member),\
> ^~~
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1556:16: note: in expansion of
> macro ‘struct_size’
> con = kmalloc(struct_size(con, objs, AMDGPU_RAS_BLOCK_COUNT),
> ^~~
> make[4]: *** [scripts/Makefile.build:276:
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.o] Error 1
> make[4]: *** Waiting for unfinished jobs
> make[3]: *** [scripts/Makefile.build:492: drivers/gpu/drm/amd/amdgpu] Error 2
> make[3]: *** Waiting for unfinished jobs
> make[2]: *** [scripts/Makefile.build:492: drivers/gpu/drm] Error 2
> make[1]: *** [scripts/Makefile.build:492: drivers/gpu] Error 2
> make: *** [Makefile:1042: drivers] Error 2
>
> Alex
>
You continue[1][2] sending these sorts of patches without really understanding
what
you are doing. And you don't even compile them. Why?
--
Gustavo
[1] https://lore.kernel.org/lkml/d83390a9-33be-3d76-3e23-b97f0a05b...@kernel.dk/
[2] https://lore.kernel.org/lkml/b4d33107-75d5-fa18-536e-6d21c96e4...@kernel.dk/
>
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> index 22bd21e..4717a64 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> @@ -1375,8 +1375,7 @@ int amdgpu_ras_init(struct amdgpu_device *adev)
>> if (con)
>> return 0;
>>
>> - con = kmalloc(sizeof(struct amdgpu_ras) +
>> - sizeof(struct ras_manager) * AMDGPU_RAS_BLOCK_COUNT,
>> + con = kmalloc(struct_size(con, objs, AMDGPU_RAS_BLOCK_COUNT),
>> GFP_KERNEL|__GFP_ZERO);
>> if (!con)
>> return -ENOMEM;
>> --
>> 2.7.4
>>
>>
>>
>> ___
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Fix boolean expression in get_surf_rq_param
Hi Harry,
I noticed this patch is already in mainline, but the stable tag
was removed. What is the reason for that if this bug is present
in stable?
Thanks
--
Gustavo
On 1/3/19 3:11 PM, Wentland, Harry wrote:
> On 2019-01-03 2:48 p.m., Gustavo A. R. Silva wrote:
>> Fix boolean expression by using logical AND operator '&&'
>> instead of bitwise operator '&'.
>>
>> This issue was detected with the help of Coccinelle.
>>
>> Fixes: 6d04ee9dc101 ("drm/amd/display: Restructuring and cleaning up DML")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>
> Reviewed-by: Harry Wentland
>
> and applied.
>
> Harry
>
>> ---
>> drivers/gpu/drm/amd/display/dc/dml/dml1_display_rq_dlg_calc.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dml/dml1_display_rq_dlg_calc.c
>> b/drivers/gpu/drm/amd/display/dc/dml/dml1_display_rq_dlg_calc.c
>> index c2037daa8e66..d341b69fdc1a 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dml/dml1_display_rq_dlg_calc.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dml/dml1_display_rq_dlg_calc.c
>> @@ -881,7 +881,7 @@ static void get_surf_rq_param(
>> /* the dpte_group_bytes is reduced for the specific case of vertical
>> * access of a tile surface that has dpte request of 8x1 ptes.
>> */
>> -if (!surf_linear & (log2_dpte_req_height_ptes == 0) & surf_vert)
>> /*reduced, in this case, will have page fault within a group */
>> +if (!surf_linear && (log2_dpte_req_height_ptes == 0) && surf_vert)
>> /*reduced, in this case, will have page fault within a group */
>> rq_sizing_param->dpte_group_bytes = 512;
>> else
>> /*full size */
>>
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Fix boolean expression in get_surf_rq_param
On 3/21/19 10:14 PM, Joe Perches wrote: > On Thu, 2019-03-21 at 22:10 -0500, Gustavo A. R. Silva wrote: >> Hi Harry, >> >> I noticed this patch is already in mainline, but the stable tag >> was removed. What is the reason for that if this bug is present >> in stable? > > It's not a bug, it's just a style issue and > the && use in some compilers it may be slower. > You're right. What might be a bug in some cases is the other way around. Thanks, Joe. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdkfd: Fix unchecked return value
On 3/18/19 1:25 PM, Kuehling, Felix wrote: > Alex already applied an equivalent patch by Colin King (attached for > reference). > Oh, that's great. Good to know. Thanks, Felix. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdkfd: Fix unchecked return value
Assign return value of function amdgpu_bo_sync_wait() to variable ret
for its further check.
Addresses-Coverity-ID: 1443914 ("Logically dead code")
Fixes: c60cd590cb7d ("drm/amdgpu: Replace ttm_bo_wait with amdgpu_bo_sync_wait")
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 1921dec3df7a..fb621abcb006 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -906,7 +906,8 @@ static int init_kfd_vm(struct amdgpu_vm *vm, void
**process_info,
pr_err("validate_pt_pd_bos() failed\n");
goto validate_pd_fail;
}
- amdgpu_bo_sync_wait(vm->root.base.bo, AMDGPU_FENCE_OWNER_KFD, false);
+ ret = amdgpu_bo_sync_wait(vm->root.base.bo, AMDGPU_FENCE_OWNER_KFD,
+ false);
if (ret)
goto wait_pd_fail;
amdgpu_bo_fence(vm->root.base.bo,
--
2.21.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/powerplay: Fix missing break in switch statement
On 3/1/19 5:54 PM, Alex Deucher wrote:
> On Fri, Mar 1, 2019 at 4:51 PM Gustavo A. R. Silva
> wrote:
>>
>> Add missing break statement in order to prevent the code from falling
>> through to case SMU_Discrete_DpmTable.
>>
>> This bug was found thanks to the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Fixes: 34a564eaf528 ("drm/amd/powerplay: implement fw image related smum
>> interface for Polaris.")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Gustavo A. R. Silva
>
> Already fixed:
> https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next-5.2-wip=6feaa4194c18578623565017f95d1b2f6b243567
>
Awesome. :)
I was looking for that link, but I didn't find it. That's why
I sent this patch.
Happy to see it is fixed now.
Thanks, Alex.
--
Gustavo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu/powerplay: Fix missing break in switch statement
Add missing break statement in order to prevent the code from falling
through to case SMU_Discrete_DpmTable.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: 34a564eaf528 ("drm/amd/powerplay: implement fw image related smum
interface for Polaris.")
Cc: sta...@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
index 52abca065764..222fb79d319e 100644
--- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
+++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c
@@ -2330,6 +2330,7 @@ static uint32_t polaris10_get_offsetof(uint32_t type,
uint32_t member)
case DRAM_LOG_BUFF_SIZE:
return offsetof(SMU74_SoftRegisters,
DRAM_LOG_BUFF_SIZE);
}
+ break;
case SMU_Discrete_DpmTable:
switch (member) {
case UvdBootLevel:
--
2.21.0
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu/gfx_v8_0: Mark expected switch fall-through
In preparation to enabling -Wimplicit-fallthrough, mark switch cases where we are expecting to fall through. This patch fixes the following warning: drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c: In function ‘gfx_v8_0_tiling_mode_table_init’: ./include/linux/device.h:1487:2: warning: this statement may fall through [-Wimplicit-fallthrough=] _dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__) ^~~ drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c:3236:3: note: in expansion of macro ‘dev_warn’ dev_warn(adev->dev, ^~~~ drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c:3240:2: note: here case CHIP_CARRIZO: ^~~~ Warning level 3 was used: -Wimplicit-fallthrough=3 This patch is part of the ongoing efforts to enable -Wimplicit-fallthrough. Signed-off-by: Gustavo A. R. Silva --- drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c index b8e50a34bdb3..02955e6e9dd9 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c @@ -3236,6 +3236,7 @@ static void gfx_v8_0_tiling_mode_table_init(struct amdgpu_device *adev) dev_warn(adev->dev, "Unknown chip type (%d) in function gfx_v8_0_tiling_mode_table_init() falling through to CHIP_CARRIZO\n", adev->asic_type); + /* fall through */ case CHIP_CARRIZO: modearray[0] = (ARRAY_MODE(ARRAY_2D_TILED_THIN1) | -- 2.21.0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/powerplay/smu10_hwmgr: use struct_size() in kzalloc()
On 2/19/19 1:51 PM, Alex Deucher wrote:
> On Tue, Feb 19, 2019 at 1:55 PM Gustavo A. R. Silva
> wrote:
>>
>> One of the more common cases of allocation size calculations is finding
>> the size of a structure that has a zero-sized array at the end, along
>> with memory for some number of elements for that array. For example:
>>
>> struct foo {
>> int stuff;
>> struct boo entry[];
>> };
>>
>> size = sizeof(struct foo) + count * sizeof(struct boo);
>> instance = kzalloc(size, GFP_KERNEL);
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can
>> now use the new struct_size() helper:
>>
>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>>
>> Notice that, in this case, variable table_size is not necessary, hence
>> it is removed.
>>
>> This code was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva
>
> Applied this and the smu8 patch. Thanks!
>
Great!
Thanks, Alex.
--
Gustavo
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/powerplay/smu10_hwmgr: use struct_size() in kzalloc()
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
struct boo entry[];
};
size = sizeof(struct foo) + count * sizeof(struct boo);
instance = kzalloc(size, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
Notice that, in this case, variable table_size is not necessary, hence
it is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
index 5273de3c5b98..0ad8fe4a6277 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu10_hwmgr.c
@@ -139,12 +139,10 @@ static int smu10_construct_max_power_limits_table(struct
pp_hwmgr *hwmgr,
static int smu10_init_dynamic_state_adjustment_rule_settings(
struct pp_hwmgr *hwmgr)
{
- uint32_t table_size =
- sizeof(struct phm_clock_voltage_dependency_table) +
- (7 * sizeof(struct phm_clock_voltage_dependency_record));
+ struct phm_clock_voltage_dependency_table *table_clk_vlt;
- struct phm_clock_voltage_dependency_table *table_clk_vlt =
- kzalloc(table_size, GFP_KERNEL);
+ table_clk_vlt = kzalloc(struct_size(table_clk_vlt, entries, 7),
+ GFP_KERNEL);
if (NULL == table_clk_vlt) {
pr_err("Can not allocate memory!\n");
--
2.20.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/powerplay/smu8_hwmgr: use struct_size() in kzalloc()
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct foo {
int stuff;
struct boo entry[];
};
size = sizeof(struct foo) + count * sizeof(struct boo);
instance = kzalloc(size, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
Notice that, in this case, variable table_size is not necessary, hence
it is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
index 553a203ac47c..019d6a206492 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu8_hwmgr.c
@@ -272,12 +272,10 @@ static int
smu8_init_dynamic_state_adjustment_rule_settings(
struct pp_hwmgr *hwmgr,
ATOM_CLK_VOLT_CAPABILITY *disp_voltage_table)
{
- uint32_t table_size =
- sizeof(struct phm_clock_voltage_dependency_table) +
- (7 * sizeof(struct phm_clock_voltage_dependency_record));
+ struct phm_clock_voltage_dependency_table *table_clk_vlt;
- struct phm_clock_voltage_dependency_table *table_clk_vlt =
- kzalloc(table_size, GFP_KERNEL);
+ table_clk_vlt = kzalloc(struct_size(table_clk_vlt, entries, 7),
+ GFP_KERNEL);
if (NULL == table_clk_vlt) {
pr_err("Can not allocate memory!\n");
--
2.20.1
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu/powerplay/polaris10_smumgr: Mark expected switch fall-through
On 2/18/19 4:40 PM, Alex Deucher wrote: > On Fri, Feb 15, 2019 at 1:50 PM Gustavo A. R. Silva > wrote: >> >> In preparation to enabling -Wimplicit-fallthrough, mark switch >> cases where we are expecting to fall through. >> >> Warning level 3 was used: -Wimplicit-fallthrough=3 >> >> This patch is part of the ongoing efforts to enable >> -Wimplicit-fallthrough. >> >> Signed-off-by: Gustavo A. R. Silva >> --- >> drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c >> b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c >> index 52abca065764..92de1bbb2e33 100644 >> --- a/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c >> +++ b/drivers/gpu/drm/amd/powerplay/smumgr/polaris10_smumgr.c >> @@ -2330,6 +2330,7 @@ static uint32_t polaris10_get_offsetof(uint32_t type, >> uint32_t member) >> case DRAM_LOG_BUFF_SIZE: >> return offsetof(SMU74_SoftRegisters, >> DRAM_LOG_BUFF_SIZE); >> } >> + /* fall through */ > > These should be breaks, although I don't think we ever currently hit > this case. I've sent out a patch to fix it and applied the rest of > the radeon and amdgpu patches. Thanks! > Awesome! Thanks, Alex. -- Gustavo ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
