[PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Colin King]

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

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Zhenyu Wang]

[-- 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 --]

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joe Perches]

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?

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Zhenyu 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 --]

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

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

RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Wang, Zhi A]

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=

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

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

RE: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Wang, Zhi A]

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=

Re: [PATCH][drm-next] drm/i915/gvt: ensure -ve return value is handled correctly

[Joonas Lahtinen]

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