* [PATCH] drm/i915: Add flex arrays to struct i915_syncmap
@ 2024-02-08 18:13 Erick Archer
2024-02-08 18:39 ` Gustavo A. R. Silva
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Erick Archer @ 2024-02-08 18:13 UTC (permalink / raw)
To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, Gustavo A. R. Silva
Cc: Erick Archer, intel-gfx, dri-devel, linux-kernel, linux-hardening
The "struct i915_syncmap" uses a dynamically sized set of trailing
elements. It can use an "u32" array or a "struct i915_syncmap *"
array.
So, use the preferred way in the kernel declaring flexible arrays [1].
Because there are two possibilities for the trailing arrays, it is
necessary to declare a union and use the DECLARE_FLEX_ARRAY macro.
The comment can be removed as the union is now clear enough.
Also, avoid the open-coded arithmetic in the memory allocator functions
[2] using the "struct_size" macro.
Moreover, refactor the "__sync_seqno" and "__sync_child" functions due
to now it is possible to use the union members added to the structure.
This way, it is also possible to avoid the open-coded arithmetic in
pointers.
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
Signed-off-by: Erick Archer <erick.archer@gmx.com>
---
drivers/gpu/drm/i915/i915_syncmap.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c
index 60404dbb2e9f..df6437c37373 100644
--- a/drivers/gpu/drm/i915/i915_syncmap.c
+++ b/drivers/gpu/drm/i915/i915_syncmap.c
@@ -75,13 +75,10 @@ struct i915_syncmap {
unsigned int height;
unsigned int bitmap;
struct i915_syncmap *parent;
- /*
- * Following this header is an array of either seqno or child pointers:
- * union {
- * u32 seqno[KSYNCMAP];
- * struct i915_syncmap *child[KSYNCMAP];
- * };
- */
+ union {
+ DECLARE_FLEX_ARRAY(u32, seqno);
+ DECLARE_FLEX_ARRAY(struct i915_syncmap *, child);
+ };
};
/**
@@ -99,13 +96,13 @@ void i915_syncmap_init(struct i915_syncmap **root)
static inline u32 *__sync_seqno(struct i915_syncmap *p)
{
GEM_BUG_ON(p->height);
- return (u32 *)(p + 1);
+ return p->seqno;
}
static inline struct i915_syncmap **__sync_child(struct i915_syncmap *p)
{
GEM_BUG_ON(!p->height);
- return (struct i915_syncmap **)(p + 1);
+ return p->child;
}
static inline unsigned int
@@ -200,7 +197,7 @@ __sync_alloc_leaf(struct i915_syncmap *parent, u64 id)
{
struct i915_syncmap *p;
- p = kmalloc(sizeof(*p) + KSYNCMAP * sizeof(u32), GFP_KERNEL);
+ p = kmalloc(struct_size(p, seqno, KSYNCMAP), GFP_KERNEL);
if (unlikely(!p))
return NULL;
@@ -282,7 +279,7 @@ static noinline int __sync_set(struct i915_syncmap **root, u64 id, u32 seqno)
unsigned int above;
/* Insert a join above the current layer */
- next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next),
+ next = kzalloc(struct_size(next, child, KSYNCMAP),
GFP_KERNEL);
if (unlikely(!next))
return -ENOMEM;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add flex arrays to struct i915_syncmap
2024-02-08 18:13 [PATCH] drm/i915: Add flex arrays to struct i915_syncmap Erick Archer
@ 2024-02-08 18:39 ` Gustavo A. R. Silva
2024-02-10 7:25 ` Kees Cook
2024-02-12 12:01 ` Tvrtko Ursulin
2 siblings, 0 replies; 4+ messages in thread
From: Gustavo A. R. Silva @ 2024-02-08 18:39 UTC (permalink / raw)
To: Erick Archer, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, David Airlie, Daniel Vetter, Gustavo A. R. Silva
Cc: intel-gfx, dri-devel, linux-kernel, linux-hardening
On 2/8/24 12:13, Erick Archer wrote:
> The "struct i915_syncmap" uses a dynamically sized set of trailing
> elements. It can use an "u32" array or a "struct i915_syncmap *"
> array.
>
> So, use the preferred way in the kernel declaring flexible arrays [1].
> Because there are two possibilities for the trailing arrays, it is
> necessary to declare a union and use the DECLARE_FLEX_ARRAY macro.
>
> The comment can be removed as the union is now clear enough.
>
> Also, avoid the open-coded arithmetic in the memory allocator functions
> [2] using the "struct_size" macro.
>
> Moreover, refactor the "__sync_seqno" and "__sync_child" functions due
> to now it is possible to use the union members added to the structure.
> This way, it is also possible to avoid the open-coded arithmetic in
> pointers.
>
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Signed-off-by: Erick Archer <erick.archer@gmx.com>
Nice transformation!
LGTM:
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
> ---
> drivers/gpu/drm/i915/i915_syncmap.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c
> index 60404dbb2e9f..df6437c37373 100644
> --- a/drivers/gpu/drm/i915/i915_syncmap.c
> +++ b/drivers/gpu/drm/i915/i915_syncmap.c
> @@ -75,13 +75,10 @@ struct i915_syncmap {
> unsigned int height;
> unsigned int bitmap;
> struct i915_syncmap *parent;
> - /*
> - * Following this header is an array of either seqno or child pointers:
> - * union {
> - * u32 seqno[KSYNCMAP];
> - * struct i915_syncmap *child[KSYNCMAP];
> - * };
> - */
> + union {
> + DECLARE_FLEX_ARRAY(u32, seqno);
> + DECLARE_FLEX_ARRAY(struct i915_syncmap *, child);
> + };
> };
>
> /**
> @@ -99,13 +96,13 @@ void i915_syncmap_init(struct i915_syncmap **root)
> static inline u32 *__sync_seqno(struct i915_syncmap *p)
> {
> GEM_BUG_ON(p->height);
> - return (u32 *)(p + 1);
> + return p->seqno;
> }
>
> static inline struct i915_syncmap **__sync_child(struct i915_syncmap *p)
> {
> GEM_BUG_ON(!p->height);
> - return (struct i915_syncmap **)(p + 1);
> + return p->child;
> }
>
> static inline unsigned int
> @@ -200,7 +197,7 @@ __sync_alloc_leaf(struct i915_syncmap *parent, u64 id)
> {
> struct i915_syncmap *p;
>
> - p = kmalloc(sizeof(*p) + KSYNCMAP * sizeof(u32), GFP_KERNEL);
> + p = kmalloc(struct_size(p, seqno, KSYNCMAP), GFP_KERNEL);
> if (unlikely(!p))
> return NULL;
>
> @@ -282,7 +279,7 @@ static noinline int __sync_set(struct i915_syncmap **root, u64 id, u32 seqno)
> unsigned int above;
>
> /* Insert a join above the current layer */
> - next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next),
> + next = kzalloc(struct_size(next, child, KSYNCMAP),
> GFP_KERNEL);
> if (unlikely(!next))
> return -ENOMEM;
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add flex arrays to struct i915_syncmap
2024-02-08 18:13 [PATCH] drm/i915: Add flex arrays to struct i915_syncmap Erick Archer
2024-02-08 18:39 ` Gustavo A. R. Silva
@ 2024-02-10 7:25 ` Kees Cook
2024-02-12 12:01 ` Tvrtko Ursulin
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2024-02-10 7:25 UTC (permalink / raw)
To: Erick Archer
Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
David Airlie, Daniel Vetter, Gustavo A. R. Silva, intel-gfx,
dri-devel, linux-kernel, linux-hardening
On Thu, Feb 08, 2024 at 07:13:18PM +0100, Erick Archer wrote:
> The "struct i915_syncmap" uses a dynamically sized set of trailing
> elements. It can use an "u32" array or a "struct i915_syncmap *"
> array.
>
> So, use the preferred way in the kernel declaring flexible arrays [1].
> Because there are two possibilities for the trailing arrays, it is
> necessary to declare a union and use the DECLARE_FLEX_ARRAY macro.
>
> The comment can be removed as the union is now clear enough.
>
> Also, avoid the open-coded arithmetic in the memory allocator functions
> [2] using the "struct_size" macro.
>
> Moreover, refactor the "__sync_seqno" and "__sync_child" functions due
> to now it is possible to use the union members added to the structure.
> This way, it is also possible to avoid the open-coded arithmetic in
> pointers.
>
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Signed-off-by: Erick Archer <erick.archer@gmx.com>
> ---
> drivers/gpu/drm/i915/i915_syncmap.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c
> index 60404dbb2e9f..df6437c37373 100644
> --- a/drivers/gpu/drm/i915/i915_syncmap.c
> +++ b/drivers/gpu/drm/i915/i915_syncmap.c
> @@ -75,13 +75,10 @@ struct i915_syncmap {
> unsigned int height;
> unsigned int bitmap;
> struct i915_syncmap *parent;
> - /*
> - * Following this header is an array of either seqno or child pointers:
> - * union {
> - * u32 seqno[KSYNCMAP];
> - * struct i915_syncmap *child[KSYNCMAP];
> - * };
> - */
> + union {
> + DECLARE_FLEX_ARRAY(u32, seqno);
> + DECLARE_FLEX_ARRAY(struct i915_syncmap *, child);
> + };
This is a new code pattern for me! Trailing arrays of different element
sizes but with a fixed element count. :)
I hope when __counted_by is expanded to take expressions we can add a
literal. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Add flex arrays to struct i915_syncmap
2024-02-08 18:13 [PATCH] drm/i915: Add flex arrays to struct i915_syncmap Erick Archer
2024-02-08 18:39 ` Gustavo A. R. Silva
2024-02-10 7:25 ` Kees Cook
@ 2024-02-12 12:01 ` Tvrtko Ursulin
2 siblings, 0 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2024-02-12 12:01 UTC (permalink / raw)
To: Erick Archer, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
David Airlie, Daniel Vetter, Gustavo A. R. Silva
Cc: intel-gfx, dri-devel, linux-kernel, linux-hardening
On 08/02/2024 18:13, Erick Archer wrote:
> The "struct i915_syncmap" uses a dynamically sized set of trailing
> elements. It can use an "u32" array or a "struct i915_syncmap *"
> array.
>
> So, use the preferred way in the kernel declaring flexible arrays [1].
> Because there are two possibilities for the trailing arrays, it is
> necessary to declare a union and use the DECLARE_FLEX_ARRAY macro.
>
> The comment can be removed as the union is now clear enough.
>
> Also, avoid the open-coded arithmetic in the memory allocator functions
> [2] using the "struct_size" macro.
>
> Moreover, refactor the "__sync_seqno" and "__sync_child" functions due
> to now it is possible to use the union members added to the structure.
> This way, it is also possible to avoid the open-coded arithmetic in
> pointers.
>
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#zero-length-and-one-element-arrays [1]
> Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2]
> Signed-off-by: Erick Archer <erick.archer@gmx.com>
Looks good to me too so I've pushed it to drm-intel-gt-next, thanks!
Regards,
Tvrtko
> ---
> drivers/gpu/drm/i915/i915_syncmap.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_syncmap.c b/drivers/gpu/drm/i915/i915_syncmap.c
> index 60404dbb2e9f..df6437c37373 100644
> --- a/drivers/gpu/drm/i915/i915_syncmap.c
> +++ b/drivers/gpu/drm/i915/i915_syncmap.c
> @@ -75,13 +75,10 @@ struct i915_syncmap {
> unsigned int height;
> unsigned int bitmap;
> struct i915_syncmap *parent;
> - /*
> - * Following this header is an array of either seqno or child pointers:
> - * union {
> - * u32 seqno[KSYNCMAP];
> - * struct i915_syncmap *child[KSYNCMAP];
> - * };
> - */
> + union {
> + DECLARE_FLEX_ARRAY(u32, seqno);
> + DECLARE_FLEX_ARRAY(struct i915_syncmap *, child);
> + };
> };
>
> /**
> @@ -99,13 +96,13 @@ void i915_syncmap_init(struct i915_syncmap **root)
> static inline u32 *__sync_seqno(struct i915_syncmap *p)
> {
> GEM_BUG_ON(p->height);
> - return (u32 *)(p + 1);
> + return p->seqno;
> }
>
> static inline struct i915_syncmap **__sync_child(struct i915_syncmap *p)
> {
> GEM_BUG_ON(!p->height);
> - return (struct i915_syncmap **)(p + 1);
> + return p->child;
> }
>
> static inline unsigned int
> @@ -200,7 +197,7 @@ __sync_alloc_leaf(struct i915_syncmap *parent, u64 id)
> {
> struct i915_syncmap *p;
>
> - p = kmalloc(sizeof(*p) + KSYNCMAP * sizeof(u32), GFP_KERNEL);
> + p = kmalloc(struct_size(p, seqno, KSYNCMAP), GFP_KERNEL);
> if (unlikely(!p))
> return NULL;
>
> @@ -282,7 +279,7 @@ static noinline int __sync_set(struct i915_syncmap **root, u64 id, u32 seqno)
> unsigned int above;
>
> /* Insert a join above the current layer */
> - next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next),
> + next = kzalloc(struct_size(next, child, KSYNCMAP),
> GFP_KERNEL);
> if (unlikely(!next))
> return -ENOMEM;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-02-12 12:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-08 18:13 [PATCH] drm/i915: Add flex arrays to struct i915_syncmap Erick Archer
2024-02-08 18:39 ` Gustavo A. R. Silva
2024-02-10 7:25 ` Kees Cook
2024-02-12 12:01 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).