* [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 15:55 ` Colin King
0 siblings, 0 replies; 29+ messages in thread
From: Colin King @ 2017-09-19 15:55 UTC (permalink / raw)
To: fred gao, Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, intel-gvt-dev, intel-gfx, dri-devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
An earlier fix changed the return type from find_bb_size however the
integer return is being assigned to a unsigned int so the -ve error
check will never be detected. Make bb_size an int to fix this.
Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 2c0ccbb817dc..f41cbf664b69 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
struct intel_shadow_bb_entry *entry_obj;
struct intel_vgpu *vgpu = s->vgpu;
unsigned long gma = 0;
- uint32_t bb_size;
+ int bb_size;
void *dst = NULL;
int ret = 0;
--
2.14.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 15:55 ` Colin King
0 siblings, 0 replies; 29+ messages in thread
From: Colin King @ 2017-09-19 15:55 UTC (permalink / raw)
To: fred gao, Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, intel-gvt-dev, intel-gfx, dri-devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
An earlier fix changed the return type from find_bb_size however the
integer return is being assigned to a unsigned int so the -ve error
check will never be detected. Make bb_size an int to fix this.
Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 2c0ccbb817dc..f41cbf664b69 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
struct intel_shadow_bb_entry *entry_obj;
struct intel_vgpu *vgpu = s->vgpu;
unsigned long gma = 0;
- uint32_t bb_size;
+ int bb_size;
void *dst = NULL;
int ret = 0;
--
2.14.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 15:55 ` Colin King
0 siblings, 0 replies; 29+ messages in thread
From: Colin King @ 2017-09-19 15:55 UTC (permalink / raw)
To: fred gao, Zhenyu Wang, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, intel-gvt-dev, intel-gfx, dri-devel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
An earlier fix changed the return type from find_bb_size however the
integer return is being assigned to a unsigned int so the -ve error
check will never be detected. Make bb_size an int to fix this.
Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
index 2c0ccbb817dc..f41cbf664b69 100644
--- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
+++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
@@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
struct intel_shadow_bb_entry *entry_obj;
struct intel_vgpu *vgpu = s->vgpu;
unsigned long gma = 0;
- uint32_t bb_size;
+ int bb_size;
void *dst = NULL;
int ret = 0;
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 29+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 ` Colin King
(?)
(?)
@ 2017-09-19 20:05 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2017-09-19 20:05 UTC (permalink / raw)
To: Colin King; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/gvt: ensure -ve return value is handled correctly
URL : https://patchwork.freedesktop.org/series/30604/
State : success
== Summary ==
Series 30604v1 drm/i915/gvt: ensure -ve return value is handled correctly
https://patchwork.freedesktop.org/api/1.0/series/30604/revisions/1/mbox/
Test gem_exec_suspend:
Subgroup basic-s3:
pass -> INCOMPLETE (fi-kbl-7500u) fdo#102850
Test kms_cursor_legacy:
Subgroup basic-busy-flip-before-cursor-atomic:
pass -> FAIL (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-b:
incomplete -> DMESG-WARN (fi-cfl-s) fdo#102294
Test drv_module_reload:
Subgroup basic-reload:
pass -> DMESG-WARN (fi-glk-1) fdo#102777
fdo#102850 https://bugs.freedesktop.org/show_bug.cgi?id=102850
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777
fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:440s
fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:468s
fi-blb-e6850 total:289 pass:224 dwarn:1 dfail:0 fail:0 skip:64 time:422s
fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:514s
fi-bwr-2160 total:289 pass:184 dwarn:0 dfail:0 fail:0 skip:105 time:279s
fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:495s
fi-byt-j1900 total:289 pass:254 dwarn:1 dfail:0 fail:0 skip:34 time:490s
fi-byt-n2820 total:289 pass:250 dwarn:1 dfail:0 fail:0 skip:38 time:490s
fi-cfl-s total:289 pass:222 dwarn:35 dfail:0 fail:0 skip:32 time:543s
fi-elk-e7500 total:289 pass:230 dwarn:0 dfail:0 fail:0 skip:59 time:416s
fi-glk-1 total:289 pass:259 dwarn:1 dfail:0 fail:0 skip:29 time:567s
fi-hsw-4770 total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:427s
fi-hsw-4770r total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:403s
fi-ilk-650 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:434s
fi-ivb-3520m total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:490s
fi-ivb-3770 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:466s
fi-kbl-7500u total:118 pass:100 dwarn:1 dfail:0 fail:0 skip:16
fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:572s
fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:587s
fi-pnv-d510 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:542s
fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:453s
fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:748s
fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:490s
fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:476s
fi-snb-2520m total:289 pass:251 dwarn:0 dfail:0 fail:0 skip:38 time:572s
fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:1 skip:39 time:412s
bf6ecf6d25c1c45e576643b7d7a65e8b1e6b4f01 drm-tip: 2017y-09m-19d-17h-23m-04s UTC integration manifest
85130ca1c516 drm/i915/gvt: ensure -ve return value is handled correctly
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5752/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 ` Colin King
(?)
@ 2017-09-19 21:46 ` Zhenyu Wang
-1 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-19 21:46 UTC (permalink / raw)
To: Colin King
Cc: fred gao, Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
David Airlie, intel-gvt-dev, intel-gfx, dri-devel,
kernel-janitors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]
On 2017.09.19 16:55:34 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> An earlier fix changed the return type from find_bb_size however the
> integer return is being assigned to a unsigned int so the -ve error
> check will never be detected. Make bb_size an int to fix this.
>
> Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
>
> Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb817dc..f41cbf664b69 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> struct intel_shadow_bb_entry *entry_obj;
> struct intel_vgpu *vgpu = s->vgpu;
> unsigned long gma = 0;
> - uint32_t bb_size;
> + int bb_size;
> void *dst = NULL;
> int ret = 0;
>
Applied this, thanks!
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 21:46 ` Zhenyu Wang
0 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-19 21:46 UTC (permalink / raw)
To: Colin King
Cc: fred gao, David Airlie, intel-gfx, kernel-janitors, linux-kernel,
dri-devel, Rodrigo Vivi, intel-gvt-dev
[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]
On 2017.09.19 16:55:34 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> An earlier fix changed the return type from find_bb_size however the
> integer return is being assigned to a unsigned int so the -ve error
> check will never be detected. Make bb_size an int to fix this.
>
> Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
>
> Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb817dc..f41cbf664b69 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> struct intel_shadow_bb_entry *entry_obj;
> struct intel_vgpu *vgpu = s->vgpu;
> unsigned long gma = 0;
> - uint32_t bb_size;
> + int bb_size;
> void *dst = NULL;
> int ret = 0;
>
Applied this, thanks!
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-19 21:46 ` Zhenyu Wang
0 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-19 21:46 UTC (permalink / raw)
To: Colin King
Cc: fred gao, David Airlie, intel-gfx, kernel-janitors, linux-kernel,
dri-devel, Rodrigo Vivi, intel-gvt-dev
[-- Attachment #1.1: Type: text/plain, Size: 1295 bytes --]
On 2017.09.19 16:55:34 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> An earlier fix changed the return type from find_bb_size however the
> integer return is being assigned to a unsigned int so the -ve error
> check will never be detected. Make bb_size an int to fix this.
>
> Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
>
> Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> index 2c0ccbb817dc..f41cbf664b69 100644
> --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> struct intel_shadow_bb_entry *entry_obj;
> struct intel_vgpu *vgpu = s->vgpu;
> unsigned long gma = 0;
> - uint32_t bb_size;
> + int bb_size;
> void *dst = NULL;
> int ret = 0;
>
Applied this, thanks!
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 15:55 ` Colin King
` (3 preceding siblings ...)
(?)
@ 2017-09-19 23:24 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2017-09-19 23:24 UTC (permalink / raw)
To: Colin King; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/gvt: ensure -ve return value is handled correctly
URL : https://patchwork.freedesktop.org/series/30604/
State : success
== Summary ==
Test perf:
Subgroup polling:
fail -> PASS (shard-hsw) fdo#102252 +1
Test kms_flip:
Subgroup rcs-wf_vblank-vs-dpms:
dmesg-warn -> PASS (shard-hsw)
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
shard-hsw total:2317 pass:1245 dwarn:3 dfail:0 fail:12 skip:1057 time:9651s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5752/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-19 21:46 ` Zhenyu Wang
(?)
@ 2017-09-20 2:35 ` Joe Perches
-1 siblings, 0 replies; 29+ messages in thread
From: Joe Perches @ 2017-09-20 2:35 UTC (permalink / raw)
To: Zhenyu Wang, Colin King
Cc: fred gao, Zhi Wang, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
David Airlie, intel-gvt-dev, intel-gfx, dri-devel,
kernel-janitors, linux-kernel
On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > An earlier fix changed the return type from find_bb_size however the
> > integer return is being assigned to a unsigned int so the -ve error
> > check will never be detected. Make bb_size an int to fix this.
> >
> > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> >
> > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > index 2c0ccbb817dc..f41cbf664b69 100644
> > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > struct intel_shadow_bb_entry *entry_obj;
> > struct intel_vgpu *vgpu = s->vgpu;
> > unsigned long gma = 0;
> > - uint32_t bb_size;
> > + int bb_size;
> > void *dst = NULL;
> > int ret = 0;
> >
>
> Applied this, thanks!
Is it possible for bb_size to be both >= 2g and valid?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-20 2:35 ` Joe Perches
0 siblings, 0 replies; 29+ messages in thread
From: Joe Perches @ 2017-09-20 2:35 UTC (permalink / raw)
To: Zhenyu Wang, Colin King
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, intel-gvt-dev, Zhi Wang
On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > An earlier fix changed the return type from find_bb_size however the
> > integer return is being assigned to a unsigned int so the -ve error
> > check will never be detected. Make bb_size an int to fix this.
> >
> > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> >
> > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > index 2c0ccbb817dc..f41cbf664b69 100644
> > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > struct intel_shadow_bb_entry *entry_obj;
> > struct intel_vgpu *vgpu = s->vgpu;
> > unsigned long gma = 0;
> > - uint32_t bb_size;
> > + int bb_size;
> > void *dst = NULL;
> > int ret = 0;
> >
>
> Applied this, thanks!
Is it possible for bb_size to be both >= 2g and valid?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-20 2:35 ` Joe Perches
0 siblings, 0 replies; 29+ messages in thread
From: Joe Perches @ 2017-09-20 2:35 UTC (permalink / raw)
To: Zhenyu Wang, Colin King
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, intel-gvt-dev, Zhi Wang
On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > An earlier fix changed the return type from find_bb_size however the
> > integer return is being assigned to a unsigned int so the -ve error
> > check will never be detected. Make bb_size an int to fix this.
> >
> > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> >
> > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > index 2c0ccbb817dc..f41cbf664b69 100644
> > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > struct intel_shadow_bb_entry *entry_obj;
> > struct intel_vgpu *vgpu = s->vgpu;
> > unsigned long gma = 0;
> > - uint32_t bb_size;
> > + int bb_size;
> > void *dst = NULL;
> > int ret = 0;
> >
>
> Applied this, thanks!
Is it possible for bb_size to be both >= 2g and valid?
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-20 2:35 ` Joe Perches
(?)
@ 2017-09-20 22:44 ` Zhenyu Wang
-1 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-20 22:44 UTC (permalink / raw)
To: Joe Perches
Cc: Colin King, fred gao, Zhi Wang, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, intel-gvt-dev, intel-gfx, dri-devel,
kernel-janitors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > An earlier fix changed the return type from find_bb_size however the
> > > integer return is being assigned to a unsigned int so the -ve error
> > > check will never be detected. Make bb_size an int to fix this.
> > >
> > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > >
> > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > struct intel_shadow_bb_entry *entry_obj;
> > > struct intel_vgpu *vgpu = s->vgpu;
> > > unsigned long gma = 0;
> > > - uint32_t bb_size;
> > > + int bb_size;
> > > void *dst = NULL;
> > > int ret = 0;
> > >
> >
> > Applied this, thanks!
>
> Is it possible for bb_size to be both >= 2g and valid?
Never be possible in practise and if really that big I think something
is already insane indeed.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-20 22:44 ` Zhenyu Wang
0 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-20 22:44 UTC (permalink / raw)
To: Joe Perches
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, Colin King, intel-gvt-dev,
Zhi Wang
[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]
On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > An earlier fix changed the return type from find_bb_size however the
> > > integer return is being assigned to a unsigned int so the -ve error
> > > check will never be detected. Make bb_size an int to fix this.
> > >
> > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > >
> > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > struct intel_shadow_bb_entry *entry_obj;
> > > struct intel_vgpu *vgpu = s->vgpu;
> > > unsigned long gma = 0;
> > > - uint32_t bb_size;
> > > + int bb_size;
> > > void *dst = NULL;
> > > int ret = 0;
> > >
> >
> > Applied this, thanks!
>
> Is it possible for bb_size to be both >= 2g and valid?
Never be possible in practise and if really that big I think something
is already insane indeed.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-20 22:44 ` Zhenyu Wang
0 siblings, 0 replies; 29+ messages in thread
From: Zhenyu Wang @ 2017-09-20 22:44 UTC (permalink / raw)
To: Joe Perches
Cc: fred gao, intel-gfx, Joonas Lahtinen, kernel-janitors,
linux-kernel, dri-devel, Rodrigo Vivi, Colin King, intel-gvt-dev,
Zhi Wang
[-- Attachment #1.1: Type: text/plain, Size: 1685 bytes --]
On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > An earlier fix changed the return type from find_bb_size however the
> > > integer return is being assigned to a unsigned int so the -ve error
> > > check will never be detected. Make bb_size an int to fix this.
> > >
> > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > >
> > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > struct intel_shadow_bb_entry *entry_obj;
> > > struct intel_vgpu *vgpu = s->vgpu;
> > > unsigned long gma = 0;
> > > - uint32_t bb_size;
> > > + int bb_size;
> > > void *dst = NULL;
> > > int ret = 0;
> > >
> >
> > Applied this, thanks!
>
> Is it possible for bb_size to be both >= 2g and valid?
Never be possible in practise and if really that big I think something
is already insane indeed.
--
Open Source Technology Center, Intel ltd.
$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-20 22:44 ` Zhenyu Wang
(?)
@ 2017-09-21 14:31 ` Joonas Lahtinen
-1 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-21 14:31 UTC (permalink / raw)
To: Zhenyu Wang, Joe Perches
Cc: Colin King, fred gao, Zhi Wang, Jani Nikula, Rodrigo Vivi,
David Airlie, intel-gvt-dev, intel-gfx, dri-devel,
kernel-janitors, linux-kernel
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code
will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-21 14:31 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-21 14:31 UTC (permalink / raw)
To: Zhenyu Wang, Joe Perches
Cc: fred gao, intel-gfx, kernel-janitors, linux-kernel, dri-devel,
Rodrigo Vivi, Colin King, intel-gvt-dev, Zhi Wang
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code
will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-21 14:31 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-21 14:31 UTC (permalink / raw)
To: Zhenyu Wang, Joe Perches
Cc: fred gao, intel-gfx, kernel-janitors, linux-kernel, dri-devel,
Rodrigo Vivi, Colin King, intel-gvt-dev, Zhi Wang
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however the
> > > > integer return is being assigned to a unsigned int so the -ve error
> > > > check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if
the value is completely internal to kernel, we're using GEM_BUG_ON for
these so that our CI will notice breakage. If it's not a driver
internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code
will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-21 14:31 ` Joonas Lahtinen
(?)
@ 2017-09-21 16:17 ` Wang, Zhi A
-1 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-21 16:17 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
Hi Joonas:
Thanks for the introduction. I have been thinking about the possibility of introducing GEM_BUG_ON into GVT-g recently and investigating on it. I'm just a bit confused about the usage between GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly is disabled in a production kernel. In the case of i915, I'm sure it will be enabled in CI test so that it can catch broken code path. Looking into GVT-g, the similar scenario is we enable it in QA test.
Let's say GEM_BUG_ON can do its work very well in QA test but QA test is not fully covered all the condition, then something might be still broken when it comes to the production kernel for user and GEM_BUG_ON will be disabled and will not catch that, I guess.
That's my confusion which scratched my mind during the investigation: If GEM_BUG_ON is not always working, then it looks WARN_ON should always be used.... Expected to learn more about the story behind. :)
Thanks,
Zhi.
-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Joonas Lahtinen
Sent: Thursday, September 21, 2017 5:32 PM
To: Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however
> > > > the integer return is being assigned to a unsigned int so the
> > > > -ve error check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against
> > > > 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if the value is completely internal to kernel, we're using GEM_BUG_ON for these so that our CI will notice breakage. If it's not a driver internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
intel-gvt-dev mailing list
intel-gvt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-21 16:17 ` Wang, Zhi A
0 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-21 16:17 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
SGkgSm9vbmFzOg0KDQpUaGFua3MgZm9yIHRoZSBpbnRyb2R1Y3Rpb24uIEkgaGF2ZSBiZWVuIHRo
aW5raW5nIGFib3V0IHRoZSBwb3NzaWJpbGl0eSBvZiBpbnRyb2R1Y2luZyBHRU1fQlVHX09OIGlu
dG8gR1ZULWcgcmVjZW50bHkgYW5kIGludmVzdGlnYXRpbmcgb24gaXQuIEknbSBqdXN0IGEgYml0
IGNvbmZ1c2VkIGFib3V0IHRoZSB1c2FnZSBiZXR3ZWVuIEdFTV9CVUdfT04gYW5kIFdBUk5fT04u
DQoNCkdFTV9CVUdfT04gaXMgb25seSBlbmFibGVkIHdoZW4ga2VybmVsIGRlYnVnIGlzIGVuYWJs
ZWQsIHdoaWNoIG1vc3RseSBpcyBkaXNhYmxlZCBpbiBhIHByb2R1Y3Rpb24ga2VybmVsLiBJbiB0
aGUgY2FzZSBvZiBpOTE1LCBJJ20gc3VyZSBpdCB3aWxsIGJlIGVuYWJsZWQgaW4gQ0kgdGVzdCBz
byB0aGF0IGl0IGNhbiBjYXRjaCBicm9rZW4gY29kZSBwYXRoLiBMb29raW5nIGludG8gR1ZULWcs
IHRoZSBzaW1pbGFyIHNjZW5hcmlvIGlzIHdlIGVuYWJsZSBpdCBpbiBRQSB0ZXN0Lg0KDQpMZXQn
cyBzYXkgR0VNX0JVR19PTiBjYW4gZG8gaXRzIHdvcmsgdmVyeSB3ZWxsIGluIFFBIHRlc3QgYnV0
IFFBIHRlc3QgaXMgbm90IGZ1bGx5IGNvdmVyZWQgYWxsIHRoZSBjb25kaXRpb24sIHRoZW4gc29t
ZXRoaW5nIG1pZ2h0IGJlIHN0aWxsIGJyb2tlbiB3aGVuIGl0IGNvbWVzIHRvIHRoZSBwcm9kdWN0
aW9uIGtlcm5lbCBmb3IgdXNlciBhbmQgR0VNX0JVR19PTiB3aWxsIGJlIGRpc2FibGVkIGFuZCB3
aWxsIG5vdCBjYXRjaCB0aGF0LCBJIGd1ZXNzLg0KDQpUaGF0J3MgbXkgY29uZnVzaW9uIHdoaWNo
IHNjcmF0Y2hlZCBteSBtaW5kIGR1cmluZyB0aGUgaW52ZXN0aWdhdGlvbjogSWYgR0VNX0JVR19P
TiBpcyBub3QgYWx3YXlzIHdvcmtpbmcsIHRoZW4gaXQgbG9va3MgV0FSTl9PTiBzaG91bGQgYWx3
YXlzIGJlIHVzZWQuLi4uIEV4cGVjdGVkIHRvIGxlYXJuIG1vcmUgYWJvdXQgdGhlIHN0b3J5IGJl
aGluZC4gOikNCg0KVGhhbmtzLA0KWmhpLg0KDQotLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0K
RnJvbTogaW50ZWwtZ3Z0LWRldiBbbWFpbHRvOmludGVsLWd2dC1kZXYtYm91bmNlc0BsaXN0cy5m
cmVlZGVza3RvcC5vcmddIE9uIEJlaGFsZiBPZiBKb29uYXMgTGFodGluZW4NClNlbnQ6IFRodXJz
ZGF5LCBTZXB0ZW1iZXIgMjEsIDIwMTcgNTozMiBQTQ0KVG86IFpoZW55dSBXYW5nIDx6aGVueXV3
QGxpbnV4LmludGVsLmNvbT47IEpvZSBQZXJjaGVzIDxqb2VAcGVyY2hlcy5jb20+DQpDYzogR2Fv
LCBGcmVkIDxmcmVkLmdhb0BpbnRlbC5jb20+OyBEYXZpZCBBaXJsaWUgPGFpcmxpZWRAbGludXgu
aWU+OyBpbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnOyBrZXJuZWwtamFuaXRvcnNAdmdl
ci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBKYW5pIE5pa3VsYSA8
amFuaS5uaWt1bGFAbGludXguaW50ZWwuY29tPjsgZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9w
Lm9yZzsgVml2aSwgUm9kcmlnbyA8cm9kcmlnby52aXZpQGludGVsLmNvbT47IENvbGluIEtpbmcg
PGNvbGluLmtpbmdAY2Fub25pY2FsLmNvbT47IGludGVsLWd2dC1kZXZAbGlzdHMuZnJlZWRlc2t0
b3Aub3JnOyBXYW5nLCBaaGkgQSA8emhpLmEud2FuZ0BpbnRlbC5jb20+DQpTdWJqZWN0OiBSZTog
W1BBVENIXVtkcm0tbmV4dF0gZHJtL2k5MTUvZ3Z0OiBlbnN1cmUgLXZlIHJldHVybiB2YWx1ZSBp
cyBoYW5kbGVkIGNvcnJlY3RseQ0KDQpPbiBUaHUsIDIwMTctMDktMjEgYXQgMDY6NDQgKzA4MDAs
IFpoZW55dSBXYW5nIHdyb3RlOg0KPiBPbiAyMDE3LjA5LjE5IDE5OjM1OjIzIC0wNzAwLCBKb2Ug
UGVyY2hlcyB3cm90ZToNCj4gPiBPbiBXZWQsIDIwMTctMDktMjAgYXQgMDU6NDYgKzA4MDAsIFpo
ZW55dSBXYW5nIHdyb3RlOg0KPiA+ID4gT24gMjAxNy4wOS4xOSAxNjo1NTozNCArMDEwMCwgQ29s
aW4gS2luZyB3cm90ZToNCj4gPiA+ID4gRnJvbTogQ29saW4gSWFuIEtpbmcgPGNvbGluLmtpbmdA
Y2Fub25pY2FsLmNvbT4NCj4gPiA+ID4gDQo+ID4gPiA+IEFuIGVhcmxpZXIgZml4IGNoYW5nZWQg
dGhlIHJldHVybiB0eXBlIGZyb20gZmluZF9iYl9zaXplIGhvd2V2ZXIgDQo+ID4gPiA+IHRoZSBp
bnRlZ2VyIHJldHVybiBpcyBiZWluZyBhc3NpZ25lZCB0byBhIHVuc2lnbmVkIGludCBzbyB0aGUg
DQo+ID4gPiA+IC12ZSBlcnJvciBjaGVjayB3aWxsIG5ldmVyIGJlIGRldGVjdGVkLiBNYWtlIGJi
X3NpemUgYW4gaW50IHRvIGZpeCB0aGlzLg0KPiA+ID4gPiANCj4gPiA+ID4gRGV0ZWN0ZWQgYnkg
Q292ZXJpdHlTY2FuIENJRCMxNDU2ODg2ICgiVW5zaWduZWQgY29tcGFyZWQgYWdhaW5zdCANCj4g
PiA+ID4gMCIpDQo+ID4gPiA+IA0KPiA+ID4gPiBGaXhlczogMWUzMTk3ZDZhZDczICgiZHJtL2k5
MTUvZ3Z0OiBSZWZpbmUgZXJyb3IgaGFuZGxpbmcgZm9yIA0KPiA+ID4gPiBwZXJmb3JtX2JiX3No
YWRvdyIpDQo+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IENvbGluIElhbiBLaW5nIDxjb2xpbi5raW5n
QGNhbm9uaWNhbC5jb20+DQo+ID4gPiA+IC0tLQ0KPiA+ID4gPiAgZHJpdmVycy9ncHUvZHJtL2k5
MTUvZ3Z0L2NtZF9wYXJzZXIuYyB8IDIgKy0NCj4gPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCAxIGlu
c2VydGlvbigrKSwgMSBkZWxldGlvbigtKQ0KPiA+ID4gPiANCj4gPiA+ID4gZGlmZiAtLWdpdCBh
L2RyaXZlcnMvZ3B1L2RybS9pOTE1L2d2dC9jbWRfcGFyc2VyLmMgDQo+ID4gPiA+IGIvZHJpdmVy
cy9ncHUvZHJtL2k5MTUvZ3Z0L2NtZF9wYXJzZXIuYw0KPiA+ID4gPiBpbmRleCAyYzBjY2JiODE3
ZGMuLmY0MWNiZjY2NGI2OSAxMDA2NDQNCj4gPiA+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5
MTUvZ3Z0L2NtZF9wYXJzZXIuYw0KPiA+ID4gPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9n
dnQvY21kX3BhcnNlci5jDQo+ID4gPiA+IEBAIC0xNjI4LDcgKzE2MjgsNyBAQCBzdGF0aWMgaW50
IHBlcmZvcm1fYmJfc2hhZG93KHN0cnVjdCBwYXJzZXJfZXhlY19zdGF0ZSAqcykNCj4gPiA+ID4g
IAlzdHJ1Y3QgaW50ZWxfc2hhZG93X2JiX2VudHJ5ICplbnRyeV9vYmo7DQo+ID4gPiA+ICAJc3Ry
dWN0IGludGVsX3ZncHUgKnZncHUgPSBzLT52Z3B1Ow0KPiA+ID4gPiAgCXVuc2lnbmVkIGxvbmcg
Z21hID0gMDsNCj4gPiA+ID4gLQl1aW50MzJfdCBiYl9zaXplOw0KPiA+ID4gPiArCWludCBiYl9z
aXplOw0KPiA+ID4gPiAgCXZvaWQgKmRzdCA9IE5VTEw7DQo+ID4gPiA+ICAJaW50IHJldCA9IDA7
DQo+ID4gPiA+ICANCj4gPiA+IA0KPiA+ID4gQXBwbGllZCB0aGlzLCB0aGFua3MhDQo+ID4gDQo+
ID4gSXMgaXQgcG9zc2libGUgZm9yIGJiX3NpemUgdG8gYmUgYm90aCA+PSAyZyBhbmQgdmFsaWQ/
DQo+IA0KPiBOZXZlciBiZSBwb3NzaWJsZSBpbiBwcmFjdGlzZSBhbmQgaWYgcmVhbGx5IHRoYXQg
YmlnIEkgdGhpbmsgc29tZXRoaW5nIA0KPiBpcyBhbHJlYWR5IGluc2FuZSBpbmRlZWQuDQoNCkl0
J3MgZ29vZCBpZGVhIHRvIGRvY3VtZW50IHRoZXNlIGFzc3VtcHRpb25zIGFzIFdBUk5fT04ncy4g
SW4gaTkxNSwgaWYgdGhlIHZhbHVlIGlzIGNvbXBsZXRlbHkgaW50ZXJuYWwgdG8ga2VybmVsLCB3
ZSdyZSB1c2luZyBHRU1fQlVHX09OIGZvciB0aGVzZSBzbyB0aGF0IG91ciBDSSB3aWxsIG5vdGlj
ZSBicmVha2FnZS4gSWYgaXQncyBub3QgYSBkcml2ZXIgaW50ZXJuYWwgdmFsdWUgb25seSwgYSBX
QVJOX09OIGlzIHRoZSBhcHByb3ByaWF0ZSBhY3Rpb24uDQoNCk90aGVyd2lzZSB0aGUgaW5mb3Jt
YXRpb24gaXMgbG9zdCBhbmQgdGhlIG5leHQgcGVyc29uIHJlYWRpbmcgdGhlIGNvZGUgd2lsbCBo
YXZlIHRoZSBzYW1lIHF1ZXN0aW9uIGluIG1pbmQuDQoNClJlZ2FyZHMsIEpvb25hcw0KLS0NCkpv
b25hcyBMYWh0aW5lbg0KT3BlbiBTb3VyY2UgVGVjaG5vbG9neSBDZW50ZXINCkludGVsIENvcnBv
cmF0aW9uDQpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXw0K
aW50ZWwtZ3Z0LWRldiBtYWlsaW5nIGxpc3QNCmludGVsLWd2dC1kZXZAbGlzdHMuZnJlZWRlc2t0
b3Aub3JnDQpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2lu
dGVsLWd2dC1kZXYNCg=
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-21 16:17 ` Wang, Zhi A
0 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-21 16:17 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
Hi Joonas:
Thanks for the introduction. I have been thinking about the possibility of introducing GEM_BUG_ON into GVT-g recently and investigating on it. I'm just a bit confused about the usage between GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly is disabled in a production kernel. In the case of i915, I'm sure it will be enabled in CI test so that it can catch broken code path. Looking into GVT-g, the similar scenario is we enable it in QA test.
Let's say GEM_BUG_ON can do its work very well in QA test but QA test is not fully covered all the condition, then something might be still broken when it comes to the production kernel for user and GEM_BUG_ON will be disabled and will not catch that, I guess.
That's my confusion which scratched my mind during the investigation: If GEM_BUG_ON is not always working, then it looks WARN_ON should always be used.... Expected to learn more about the story behind. :)
Thanks,
Zhi.
-----Original Message-----
From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On Behalf Of Joonas Lahtinen
Sent: Thursday, September 21, 2017 5:32 PM
To: Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 06:44 +0800, Zhenyu Wang wrote:
> On 2017.09.19 19:35:23 -0700, Joe Perches wrote:
> > On Wed, 2017-09-20 at 05:46 +0800, Zhenyu Wang wrote:
> > > On 2017.09.19 16:55:34 +0100, Colin King wrote:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > An earlier fix changed the return type from find_bb_size however
> > > > the integer return is being assigned to a unsigned int so the
> > > > -ve error check will never be detected. Make bb_size an int to fix this.
> > > >
> > > > Detected by CoverityScan CID#1456886 ("Unsigned compared against
> > > > 0")
> > > >
> > > > Fixes: 1e3197d6ad73 ("drm/i915/gvt: Refine error handling for
> > > > perform_bb_shadow")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > drivers/gpu/drm/i915/gvt/cmd_parser.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > index 2c0ccbb817dc..f41cbf664b69 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/cmd_parser.c
> > > > @@ -1628,7 +1628,7 @@ static int perform_bb_shadow(struct parser_exec_state *s)
> > > > struct intel_shadow_bb_entry *entry_obj;
> > > > struct intel_vgpu *vgpu = s->vgpu;
> > > > unsigned long gma = 0;
> > > > - uint32_t bb_size;
> > > > + int bb_size;
> > > > void *dst = NULL;
> > > > int ret = 0;
> > > >
> > >
> > > Applied this, thanks!
> >
> > Is it possible for bb_size to be both >= 2g and valid?
>
> Never be possible in practise and if really that big I think something
> is already insane indeed.
It's good idea to document these assumptions as WARN_ON's. In i915, if the value is completely internal to kernel, we're using GEM_BUG_ON for these so that our CI will notice breakage. If it's not a driver internal value only, a WARN_ON is the appropriate action.
Otherwise the information is lost and the next person reading the code will have the same question in mind.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
intel-gvt-dev mailing list
intel-gvt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-21 16:17 ` Wang, Zhi A
(?)
@ 2017-09-22 11:11 ` Joonas Lahtinen
-1 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-22 11:11 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.
WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may
be variations.
User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G",
there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and
how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 11:11 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-22 11:11 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.
WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may
be variations.
User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G",
there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and
how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 11:11 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-22 11:11 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect
ever to happen within the driver. So we often list the function
preconditions as GEM_BUG_ON. It's there for the same reason as the
lockdep_assert_held and KASAN. It's sometimes heavy checks that we
really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious
conditions at the critical command submission path GEM is not
sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that
has the potential to hit it if driver was modified not to respect those
preconditions. So once our testest passes, we can disable the
GEM_BUG_ONs and be confident of the internal driver quality and get the
release performance.
WARN_ON is mostly used for the cases when the hardware is behaving
differently than we expect. We can't remove them as we don't have all
the hardware in the world to test, but we try to exercise them too
through I-G-Ts. The test will often be the subtest that was written to
reproduce the problem with our expectations of hardware in case of
hangs and other bugs. After we've corrected the driver behaviour, or
got a hardware W/A assigned, we keep the test and add a WARN_ON to make
sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may
be variations.
User behaving unexpectedly should never result in WARN_ON (or even
worse, BUG_ON), should always just be debug messages displayed (not to
trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting
system memory or filesystems, so from graphics driver, that's not very
often. Controlled propagation of errors and maybe WARN_ON is always
preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G",
there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by
trying to allocate a huge object for example, and should get rejection
as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if
it is. Either resulting in user reported error if the origin of the
object is outside of kernel <-> hardware. Or a WARN_ON if it's strange
hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and
how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-22 11:11 ` Joonas Lahtinen
(?)
@ 2017-09-22 17:50 ` Wang, Zhi A
-1 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-22 17:50 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
Thanks for the reply. Learned a lot. :)
GEM_BUG_ON is new to me since it wasn't there at the beginning of GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the code and some of them should be GEM_BUG_ON now.
Now I can figure out those differences. We can discuss with our QA to see if they would like to enable I915_GEM_DEBUG and then we can move to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :) Thank you so much. Have a great weekend.
Thanks,
Zhi.
-----Original Message-----
From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
Sent: Friday, September 22, 2017 2:11 PM
To: Wang, Zhi A <zhi.a.wang@intel.com>; Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect ever to happen within the driver. So we often list the function preconditions as GEM_BUG_ON. It's there for the same reason as the lockdep_assert_held and KASAN. It's sometimes heavy checks that we really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious conditions at the critical command submission path GEM is not sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that has the potential to hit it if driver was modified not to respect those preconditions. So once our testest passes, we can disable the GEM_BUG_ONs and be confident of the internal driver quality and get the release performance.
WARN_ON is mostly used for the cases when the hardware is behaving differently than we expect. We can't remove them as we don't have all the hardware in the world to test, but we try to exercise them too through I-G-Ts. The test will often be the subtest that was written to reproduce the problem with our expectations of hardware in case of hangs and other bugs. After we've corrected the driver behaviour, or got a hardware W/A assigned, we keep the test and add a WARN_ON to make sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may be variations.
User behaving unexpectedly should never result in WARN_ON (or even worse, BUG_ON), should always just be debug messages displayed (not to trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting system memory or filesystems, so from graphics driver, that's not very often. Controlled propagation of errors and maybe WARN_ON is always preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G", there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by trying to allocate a huge object for example, and should get rejection as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if it is. Either resulting in user reported error if the origin of the object is outside of kernel <-> hardware. Or a WARN_ON if it's strange hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 17:50 ` Wang, Zhi A
0 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-22 17:50 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
VGhhbmtzIGZvciB0aGUgcmVwbHkuIExlYXJuZWQgYSBsb3QuIDopDQoNCkdFTV9CVUdfT04gaXMg
bmV3IHRvIG1lIHNpbmNlIGl0IHdhc24ndCB0aGVyZSBhdCB0aGUgYmVnaW5uaW5nIG9mIEdWVC1n
IHVwc3RyZWFtLiBJdCBzaG93ZWQgdXAgbGF0ZXIuIFNvIEkgbGVmdCBhIGxvdCBvZiBXQVJOX09O
IGluIHRoZSBjb2RlIGFuZCBzb21lIG9mIHRoZW0gc2hvdWxkIGJlIEdFTV9CVUdfT04gbm93Lg0K
DQpOb3cgSSBjYW4gZmlndXJlIG91dCB0aG9zZSBkaWZmZXJlbmNlcy4gV2UgY2FuIGRpc2N1c3Mg
d2l0aCBvdXIgUUEgdG8gc2VlIGlmIHRoZXkgd291bGQgbGlrZSB0byBlbmFibGUgSTkxNV9HRU1f
REVCVUcgYW5kIHRoZW4gd2UgY2FuIG1vdmUgdG8gR0VNX0JVR19PTiBhbHNvLCBvciBtYXliZSB3
ZSBjYW4gaGF2ZSBhIGRlZGljYXRlZCBHVlRfQlVHX09OLiA6KSBUaGFuayB5b3Ugc28gbXVjaC4g
SGF2ZSBhIGdyZWF0IHdlZWtlbmQuDQoNClRoYW5rcywNClpoaS4NCg0KLS0tLS1PcmlnaW5hbCBN
ZXNzYWdlLS0tLS0NCkZyb206IEpvb25hcyBMYWh0aW5lbiBbbWFpbHRvOmpvb25hcy5sYWh0aW5l
bkBsaW51eC5pbnRlbC5jb21dIA0KU2VudDogRnJpZGF5LCBTZXB0ZW1iZXIgMjIsIDIwMTcgMjox
MSBQTQ0KVG86IFdhbmcsIFpoaSBBIDx6aGkuYS53YW5nQGludGVsLmNvbT47IFpoZW55dSBXYW5n
IDx6aGVueXV3QGxpbnV4LmludGVsLmNvbT47IEpvZSBQZXJjaGVzIDxqb2VAcGVyY2hlcy5jb20+
DQpDYzogR2FvLCBGcmVkIDxmcmVkLmdhb0BpbnRlbC5jb20+OyBEYXZpZCBBaXJsaWUgPGFpcmxp
ZWRAbGludXguaWU+OyBpbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnOyBrZXJuZWwtamFu
aXRvcnNAdmdlci5rZXJuZWwub3JnOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBKYW5p
IE5pa3VsYSA8amFuaS5uaWt1bGFAbGludXguaW50ZWwuY29tPjsgZHJpLWRldmVsQGxpc3RzLmZy
ZWVkZXNrdG9wLm9yZzsgVml2aSwgUm9kcmlnbyA8cm9kcmlnby52aXZpQGludGVsLmNvbT47IENv
bGluIEtpbmcgPGNvbGluLmtpbmdAY2Fub25pY2FsLmNvbT47IGludGVsLWd2dC1kZXZAbGlzdHMu
ZnJlZWRlc2t0b3Aub3JnDQpTdWJqZWN0OiBSZTogW1BBVENIXVtkcm0tbmV4dF0gZHJtL2k5MTUv
Z3Z0OiBlbnN1cmUgLXZlIHJldHVybiB2YWx1ZSBpcyBoYW5kbGVkIGNvcnJlY3RseQ0KDQpPbiBU
aHUsIDIwMTctMDktMjEgYXQgMTY6MTcgKzAwMDAsIFdhbmcsIFpoaSBBIHdyb3RlOg0KPiBIaSBK
b29uYXM6DQo+IA0KPiBUaGFua3MgZm9yIHRoZSBpbnRyb2R1Y3Rpb24uIEkgaGF2ZSBiZWVuIHRo
aW5raW5nIGFib3V0IHRoZSANCj4gcG9zc2liaWxpdHkgb2YgaW50cm9kdWNpbmcgR0VNX0JVR19P
TiBpbnRvIEdWVC1nIHJlY2VudGx5IGFuZCANCj4gaW52ZXN0aWdhdGluZyBvbiBpdC4gSSdtIGp1
c3QgYSBiaXQgY29uZnVzZWQgYWJvdXQgdGhlIHVzYWdlIGJldHdlZW4gDQo+IEdFTV9CVUdfT04g
YW5kIFdBUk5fT04uDQoNCkdFTV9CVUdfT04gaXMgYmFzaWNhbGx5IHRoZXJlIHRvIGNhdGNoIHRo
aW5ncyB0aGF0IHdlIGRvIG5vdCBleHBlY3QgZXZlciB0byBoYXBwZW4gd2l0aGluIHRoZSBkcml2
ZXIuIFNvIHdlIG9mdGVuIGxpc3QgdGhlIGZ1bmN0aW9uIHByZWNvbmRpdGlvbnMgYXMgR0VNX0JV
R19PTi4gSXQncyB0aGVyZSBmb3IgdGhlIHNhbWUgcmVhc29uIGFzIHRoZSBsb2NrZGVwX2Fzc2Vy
dF9oZWxkIGFuZCBLQVNBTi4gSXQncyBzb21ldGltZXMgaGVhdnkgY2hlY2tzIHRoYXQgd2UgcmVh
bGx5IHdhbnQgdG8gcnVuIHdoZW4gZnVuY3Rpb25hbGx5IHZhbGlkYXRpbmcga2VybmVsLg0KDQpH
RU1fQlVHX09OIGJlY2FtZSB0byBleGlzdGVuY2UgYmVjYXVzZSBhZGRpbmcgY2hlY2tzIGZvciBv
YnZpb3VzIGNvbmRpdGlvbnMgYXQgdGhlIGNyaXRpY2FsIGNvbW1hbmQgc3VibWlzc2lvbiBwYXRo
IEdFTSBpcyBub3Qgc3VzdGFpbmFibGUgZm9yIHBlcmZvcm1hbmNlIGluIHByb2R1Y3Rpb24uDQoN
ClRoZSBleHBlY3RhdGlvbiBpcyB0aGF0IGVhY2ggR0VNX0JVR19PTiBoYXMgYSB0ZXN0Y2FzZSBp
biBJLUctVCB0aGF0IGhhcyB0aGUgcG90ZW50aWFsIHRvIGhpdCBpdCBpZiBkcml2ZXIgd2FzIG1v
ZGlmaWVkIG5vdCB0byByZXNwZWN0IHRob3NlIHByZWNvbmRpdGlvbnMuIFNvIG9uY2Ugb3VyIHRl
c3Rlc3QgcGFzc2VzLCB3ZSBjYW4gZGlzYWJsZSB0aGUgR0VNX0JVR19PTnMgYW5kIGJlIGNvbmZp
ZGVudCBvZiB0aGUgaW50ZXJuYWwgZHJpdmVyIHF1YWxpdHkgYW5kIGdldCB0aGUgcmVsZWFzZSBw
ZXJmb3JtYW5jZS4NCg0KV0FSTl9PTiBpcyBtb3N0bHkgdXNlZCBmb3IgdGhlIGNhc2VzIHdoZW4g
dGhlIGhhcmR3YXJlIGlzIGJlaGF2aW5nIGRpZmZlcmVudGx5IHRoYW4gd2UgZXhwZWN0LiBXZSBj
YW4ndCByZW1vdmUgdGhlbSBhcyB3ZSBkb24ndCBoYXZlIGFsbCB0aGUgaGFyZHdhcmUgaW4gdGhl
IHdvcmxkIHRvIHRlc3QsIGJ1dCB3ZSB0cnkgdG8gZXhlcmNpc2UgdGhlbSB0b28gdGhyb3VnaCBJ
LUctVHMuIFRoZSB0ZXN0IHdpbGwgb2Z0ZW4gYmUgdGhlIHN1YnRlc3QgdGhhdCB3YXMgd3JpdHRl
biB0byByZXByb2R1Y2UgdGhlIHByb2JsZW0gd2l0aCBvdXIgZXhwZWN0YXRpb25zIG9mIGhhcmR3
YXJlIGluIGNhc2Ugb2YgaGFuZ3MgYW5kIG90aGVyIGJ1Z3MuIEFmdGVyIHdlJ3ZlIGNvcnJlY3Rl
ZCB0aGUgZHJpdmVyIGJlaGF2aW91ciwgb3IgZ290IGEgaGFyZHdhcmUgVy9BIGFzc2lnbmVkLCB3
ZSBrZWVwIHRoZSB0ZXN0IGFuZCBhZGQgYSBXQVJOX09OIHRvIG1ha2Ugc3VyZSB0aGVyZSB3aWxs
IGJlIG5vIHJlZ3Jlc3Npb24gYmFjayB0byB0aGUgc2FtZSBzaXR1YXRpb24uDQoNClRoaXMgaXMg
YXQgbGVhc3Qgd2hhdCBzaG91bGQgaGFwcGVuLCBnaXZlbiB0aW1lIGNvbnN0cmFpbnRzLCB0aGVy
ZSBtYXkgYmUgdmFyaWF0aW9ucy4NCg0KVXNlciBiZWhhdmluZyB1bmV4cGVjdGVkbHkgc2hvdWxk
IG5ldmVyIHJlc3VsdCBpbiBXQVJOX09OIChvciBldmVuIHdvcnNlLCBCVUdfT04pLCBzaG91bGQg
YWx3YXlzIGp1c3QgYmUgZGVidWcgbWVzc2FnZXMgZGlzcGxheWVkIChub3QgdG8gdHJpZ2dlciB0
aGUgQ0kpIGFuZCBlcnJvcnMgcHJvcGFnYXRlZCBiYWNrIHRvIHVzZXI6DQoNCmh0dHBzOi8vMDEu
b3JnL2xpbnV4Z3JhcGhpY3MvZ2Z4LWRvY3MvZHJtL2dwdS9kcm0tdWFwaS5odG1sI3JlY29tbWVu
ZGVkDQotaW9jdGwtcmV0dXJuLXZhbHVlcw0KDQpCYXJlIEJVR19PTiBzaG91bGQgb25seSBiZSB1
c2VkIHdoZW4gdGhlcmUncyB0aGUgZGFuZ2VyIG9mIGNvcnJ1cHRpbmcgc3lzdGVtIG1lbW9yeSBv
ciBmaWxlc3lzdGVtcywgc28gZnJvbSBncmFwaGljcyBkcml2ZXIsIHRoYXQncyBub3QgdmVyeSBv
ZnRlbi4gQ29udHJvbGxlZCBwcm9wYWdhdGlvbiBvZiBlcnJvcnMgYW5kIG1heWJlIFdBUk5fT04g
aXMgYWx3YXlzIHByZWZlcnJlZCBpZiBwb3NzaWJsZS4NCg0KDQo+IEdFTV9CVUdfT04gaXMgb25s
eSBlbmFibGVkIHdoZW4ga2VybmVsIGRlYnVnIGlzIGVuYWJsZWQsIHdoaWNoIG1vc3RseSANCj4g
aXMgZGlzYWJsZWQgaW4gYSBwcm9kdWN0aW9uIGtlcm5lbC4gSW4gdGhlIGNhc2Ugb2YgaTkxNSwg
SSdtIHN1cmUgaXQgDQo+IHdpbGwgYmUgZW5hYmxlZCBpbiBDSSB0ZXN0IHNvIHRoYXQgaXQgY2Fu
IGNhdGNoIGJyb2tlbiBjb2RlIHBhdGguDQo+IExvb2tpbmcgaW50byBHVlQtZywgdGhlIHNpbWls
YXIgc2NlbmFyaW8gaXMgd2UgZW5hYmxlIGl0IGluIFFBIHRlc3QuDQo+IA0KPiBMZXQncyBzYXkg
R0VNX0JVR19PTiBjYW4gZG8gaXRzIHdvcmsgdmVyeSB3ZWxsIGluIFFBIHRlc3QgYnV0IFFBIHRl
c3QgDQo+IGlzIG5vdCBmdWxseSBjb3ZlcmVkIGFsbCB0aGUgY29uZGl0aW9uLCB0aGVuIHNvbWV0
aGluZyBtaWdodCBiZSBzdGlsbCANCj4gYnJva2VuIHdoZW4gaXQgY29tZXMgdG8gdGhlIHByb2R1
Y3Rpb24ga2VybmVsIGZvciB1c2VyIGFuZCBHRU1fQlVHX09OIA0KPiB3aWxsIGJlIGRpc2FibGVk
IGFuZCB3aWxsIG5vdCBjYXRjaCB0aGF0LCBJIGd1ZXNzLg0KPiANCj4gVGhhdCdzIG15IGNvbmZ1
c2lvbiB3aGljaCBzY3JhdGNoZWQgbXkgbWluZCBkdXJpbmcgdGhlIGludmVzdGlnYXRpb246DQo+
IElmIEdFTV9CVUdfT04gaXMgbm90IGFsd2F5cyB3b3JraW5nLCB0aGVuIGl0IGxvb2tzIFdBUk5f
T04gc2hvdWxkIA0KPiBhbHdheXMgYmUgdXNlZC4uLi4gRXhwZWN0ZWQgdG8gbGVhcm4gbW9yZSBh
Ym91dCB0aGUgc3RvcnkgYmVoaW5kLiA6KQ0KDQpTbyBpZiB0aGUgc2F5aW5nIGlzIHNvbWUgb2Jq
ZWN0IGlzICJuZXZlciBnb2luZyB0byBiZSBiaWdnZXIgdGhhbiAyRyIsIHRoZXJlIHNob3VsZCBi
ZSBlaXRoZXI6DQoNCjEuIEdFTV9CVUdfT04gbGlrZSBhc3NlcnRpb24gZm9yIGl0IGFuZCBhIHRl
c3QgdGhhdCB0cmllcyB0byBoaXQgaXQsIGJ5IHRyeWluZyB0byBhbGxvY2F0ZSBhIGh1Z2Ugb2Jq
ZWN0IGZvciBleGFtcGxlLCBhbmQgc2hvdWxkIGdldCByZWplY3Rpb24gYXMgLUVJTlZBTA0KDQoy
LiBUZXN0IHRvIHNlZSBpZiB0aGUgb2JqZWN0IGlzIGJpZ2dlciwgYW5kIHByb3BhZ2F0ZSBiYWNr
IHRoZSBlcnJvciBpZiBpdCBpcy4gRWl0aGVyIHJlc3VsdGluZyBpbiB1c2VyIHJlcG9ydGVkIGVy
cm9yIGlmIHRoZSBvcmlnaW4gb2YgdGhlIG9iamVjdCBpcyBvdXRzaWRlIG9mIGtlcm5lbCA8LT4g
aGFyZHdhcmUuIE9yIGEgV0FSTl9PTiBpZiBpdCdzIHN0cmFuZ2UgaGFyZHdhcmUgb3Iga2VybmVs
IGRyaXZlciBiZWhhdmlvci4NCg0KWW91IHNob3VsZCBjaG9vc2UgZGVwZW5kaW5nIG9uIGhvdyBv
ZnRlbiB5b3VyIGZ1bmN0aW9uIGdldHMgY2FsbGVkLCBhbmQgaG93IGNyaXRpY2FsIHRoZSBleGVj
dXRpb24gdGltZSBpcy4NCg0KSG9wZWZ1bGx5IHRoaXMgY2xhcmlmaWVkIHRoaW5ncy4NCg0KUmVn
YXJkcywgSm9vbmFzDQotLQ0KSm9vbmFzIExhaHRpbmVuDQpPcGVuIFNvdXJjZSBUZWNobm9sb2d5
IENlbnRlcg0KSW50ZWwgQ29ycG9yYXRpb24NCg=
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-22 17:50 ` Wang, Zhi A
0 siblings, 0 replies; 29+ messages in thread
From: Wang, Zhi A @ 2017-09-22 17:50 UTC (permalink / raw)
To: Joonas Lahtinen, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
Thanks for the reply. Learned a lot. :)
GEM_BUG_ON is new to me since it wasn't there at the beginning of GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the code and some of them should be GEM_BUG_ON now.
Now I can figure out those differences. We can discuss with our QA to see if they would like to enable I915_GEM_DEBUG and then we can move to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :) Thank you so much. Have a great weekend.
Thanks,
Zhi.
-----Original Message-----
From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
Sent: Friday, September 22, 2017 2:11 PM
To: Wang, Zhi A <zhi.a.wang@intel.com>; Zhenyu Wang <zhenyuw@linux.intel.com>; Joe Perches <joe@perches.com>
Cc: Gao, Fred <fred.gao@intel.com>; David Airlie <airlied@linux.ie>; intel-gfx@lists.freedesktop.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org; Jani Nikula <jani.nikula@linux.intel.com>; dri-devel@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>; Colin King <colin.king@canonical.com>; intel-gvt-dev@lists.freedesktop.org
Subject: Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
On Thu, 2017-09-21 at 16:17 +0000, Wang, Zhi A wrote:
> Hi Joonas:
>
> Thanks for the introduction. I have been thinking about the
> possibility of introducing GEM_BUG_ON into GVT-g recently and
> investigating on it. I'm just a bit confused about the usage between
> GEM_BUG_ON and WARN_ON.
GEM_BUG_ON is basically there to catch things that we do not expect ever to happen within the driver. So we often list the function preconditions as GEM_BUG_ON. It's there for the same reason as the lockdep_assert_held and KASAN. It's sometimes heavy checks that we really want to run when functionally validating kernel.
GEM_BUG_ON became to existence because adding checks for obvious conditions at the critical command submission path GEM is not sustainable for performance in production.
The expectation is that each GEM_BUG_ON has a testcase in I-G-T that has the potential to hit it if driver was modified not to respect those preconditions. So once our testest passes, we can disable the GEM_BUG_ONs and be confident of the internal driver quality and get the release performance.
WARN_ON is mostly used for the cases when the hardware is behaving differently than we expect. We can't remove them as we don't have all the hardware in the world to test, but we try to exercise them too through I-G-Ts. The test will often be the subtest that was written to reproduce the problem with our expectations of hardware in case of hangs and other bugs. After we've corrected the driver behaviour, or got a hardware W/A assigned, we keep the test and add a WARN_ON to make sure there will be no regression back to the same situation.
This is at least what should happen, given time constraints, there may be variations.
User behaving unexpectedly should never result in WARN_ON (or even worse, BUG_ON), should always just be debug messages displayed (not to trigger the CI) and errors propagated back to user:
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-uapi.html#recommended
-ioctl-return-values
Bare BUG_ON should only be used when there's the danger of corrupting system memory or filesystems, so from graphics driver, that's not very often. Controlled propagation of errors and maybe WARN_ON is always preferred if possible.
> GEM_BUG_ON is only enabled when kernel debug is enabled, which mostly
> is disabled in a production kernel. In the case of i915, I'm sure it
> will be enabled in CI test so that it can catch broken code path.
> Looking into GVT-g, the similar scenario is we enable it in QA test.
>
> Let's say GEM_BUG_ON can do its work very well in QA test but QA test
> is not fully covered all the condition, then something might be still
> broken when it comes to the production kernel for user and GEM_BUG_ON
> will be disabled and will not catch that, I guess.
>
> That's my confusion which scratched my mind during the investigation:
> If GEM_BUG_ON is not always working, then it looks WARN_ON should
> always be used.... Expected to learn more about the story behind. :)
So if the saying is some object is "never going to be bigger than 2G", there should be either:
1. GEM_BUG_ON like assertion for it and a test that tries to hit it, by trying to allocate a huge object for example, and should get rejection as -EINVAL
2. Test to see if the object is bigger, and propagate back the error if it is. Either resulting in user reported error if the origin of the object is outside of kernel <-> hardware. Or a WARN_ON if it's strange hardware or kernel driver behavior.
You should choose depending on how often your function gets called, and how critical the execution time is.
Hopefully this clarified things.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
2017-09-22 17:50 ` Wang, Zhi A
(?)
@ 2017-09-25 9:32 ` Joonas Lahtinen
-1 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-25 9:32 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Fri, 2017-09-22 at 17:50 +0000, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
>
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
>
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.
GVT_BUG_ON is probably the way to go :)
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-25 9:32 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-25 9:32 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Fri, 2017-09-22 at 17:50 +0000, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
>
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
>
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.
GVT_BUG_ON is probably the way to go :)
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly
@ 2017-09-25 9:32 ` Joonas Lahtinen
0 siblings, 0 replies; 29+ messages in thread
From: Joonas Lahtinen @ 2017-09-25 9:32 UTC (permalink / raw)
To: Wang, Zhi A, Zhenyu Wang, Joe Perches
Cc: Gao, Fred, David Airlie, intel-gfx, kernel-janitors,
linux-kernel, Jani Nikula, dri-devel, Vivi, Rodrigo, Colin King,
intel-gvt-dev
On Fri, 2017-09-22 at 17:50 +0000, Wang, Zhi A wrote:
> Thanks for the reply. Learned a lot. :)
>
> GEM_BUG_ON is new to me since it wasn't there at the beginning of
> GVT-g upstream. It showed up later. So I left a lot of WARN_ON in the
> code and some of them should be GEM_BUG_ON now.
>
> Now I can figure out those differences. We can discuss with our QA to
> see if they would like to enable I915_GEM_DEBUG and then we can move
> to GEM_BUG_ON also, or maybe we can have a dedicated GVT_BUG_ON. :)
> Thank you so much. Have a great weekend.
GVT_BUG_ON is probably the way to go :)
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-09-25 9:32 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-19 15:55 [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly Colin King
2017-09-19 15:55 ` Colin King
2017-09-19 15:55 ` Colin King
2017-09-19 20:05 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-09-19 21:46 ` [PATCH][drm-next] " Zhenyu Wang
2017-09-19 21:46 ` Zhenyu Wang
2017-09-19 21:46 ` Zhenyu Wang
2017-09-20 2:35 ` Joe Perches
2017-09-20 2:35 ` Joe Perches
2017-09-20 2:35 ` Joe Perches
2017-09-20 22:44 ` Zhenyu Wang
2017-09-20 22:44 ` Zhenyu Wang
2017-09-20 22:44 ` Zhenyu Wang
2017-09-21 14:31 ` Joonas Lahtinen
2017-09-21 14:31 ` Joonas Lahtinen
2017-09-21 14:31 ` Joonas Lahtinen
2017-09-21 16:17 ` Wang, Zhi A
2017-09-21 16:17 ` Wang, Zhi A
2017-09-21 16:17 ` Wang, Zhi A
2017-09-22 11:11 ` Joonas Lahtinen
2017-09-22 11:11 ` Joonas Lahtinen
2017-09-22 11:11 ` Joonas Lahtinen
2017-09-22 17:50 ` Wang, Zhi A
2017-09-22 17:50 ` Wang, Zhi A
2017-09-22 17:50 ` Wang, Zhi A
2017-09-25 9:32 ` Joonas Lahtinen
2017-09-25 9:32 ` Joonas Lahtinen
2017-09-25 9:32 ` Joonas Lahtinen
2017-09-19 23:24 ` ✓ Fi.CI.IGT: success for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.