| Message ID | 20220216083849.91239-1-jiapeng.chong@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: fix unsigned integer to signed assignment | expand |
On Wed, 16 Feb 2022, Jiapeng Chong <jiapeng.chong@linux.alibaba.com> wrote: > Eliminate the follow smatch warning: > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4640 > guc_create_virtual() warn: assigning (-2) to unsigned variable > 've->base.instance'. > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4641 > guc_create_virtual() warn: assigning (-2) to unsigned variable > 've->base.uabi_instance'. > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> The report seems to be valid, but I don't think this is the fix. Where do we even check for invalid instance/uabi_instance in code? BR, Jani. > --- > drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h > index 36365bdbe1ee..dc7cc06c68e7 100644 > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > @@ -328,10 +328,10 @@ struct intel_engine_cs { > intel_engine_mask_t logical_mask; > > u8 class; > - u8 instance; > + s8 instance; > > u16 uabi_class; > - u16 uabi_instance; > + s16 uabi_instance; > > u32 uabi_capabilities; > u32 context_size;
On Wed, Feb 16, 2022 at 11:02:06AM +0200, Jani Nikula wrote: > On Wed, 16 Feb 2022, Jiapeng Chong <jiapeng.chong@linux.alibaba.com> wrote: > > Eliminate the follow smatch warning: > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4640 > > guc_create_virtual() warn: assigning (-2) to unsigned variable > > 've->base.instance'. > > > > drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4641 > > guc_create_virtual() warn: assigning (-2) to unsigned variable > > 've->base.uabi_instance'. > > > > Reported-by: Abaci Robot <abaci@linux.alibaba.com> > > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> > > The report seems to be valid, but I don't think this is the fix. > > Where do we even check for invalid instance/uabi_instance in code? The whole thing seems rather poorly documented as there's a matching uabi struct with __u16's and the negative values are defined right there in the uapi header as well. > > BR, > Jani. > > > > --- > > drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > index 36365bdbe1ee..dc7cc06c68e7 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h > > @@ -328,10 +328,10 @@ struct intel_engine_cs { > > intel_engine_mask_t logical_mask; > > > > u8 class; > > - u8 instance; > > + s8 instance; > > > > u16 uabi_class; > > - u16 uabi_instance; > > + s16 uabi_instance; > > > > u32 uabi_capabilities; > > u32 context_size; > > -- > Jani Nikula, Intel Open Source Graphics Center
On 16/02/2022 09:19, Ville Syrjälä wrote: > On Wed, Feb 16, 2022 at 11:02:06AM +0200, Jani Nikula wrote: >> On Wed, 16 Feb 2022, Jiapeng Chong <jiapeng.chong@linux.alibaba.com> wrote: >>> Eliminate the follow smatch warning: >>> >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4640 >>> guc_create_virtual() warn: assigning (-2) to unsigned variable >>> 've->base.instance'. >>> >>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4641 >>> guc_create_virtual() warn: assigning (-2) to unsigned variable >>> 've->base.uabi_instance'. >>> >>> Reported-by: Abaci Robot <abaci@linux.alibaba.com> >>> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> >> >> The report seems to be valid, but I don't think this is the fix. >> >> Where do we even check for invalid instance/uabi_instance in code? > > The whole thing seems rather poorly documented as there's a matching > uabi struct with __u16's and the negative values are defined right > there in the uapi header as well. Negative ones are exception values to be used in conjunction with the virtual engine uapi (see "DOC: Virtual Engine uAPI" and also comment next to I915_CONTEXT_PARAM_ENGINES). AFAIK assigning negative int to unsigned int is defined and fine. Compiler does warn on comparisons which is why we have: ./gem/i915_gem_busy.c: if (id == (u16)I915_ENGINE_CLASS_INVALID) ./gem/i915_gem_busy.c: if (id == (u16)I915_ENGINE_CLASS_INVALID) ./gem/i915_gem_context.c: if (ci.engine_class == (u16)I915_ENGINE_CLASS_INVALID && ./gem/i915_gem_context.c: ci.engine_instance == (u16)I915_ENGINE_CLASS_INVALID_NONE) So I think no action needed here. Regards, Tvrtko >> >> BR, >> Jani. >> >> >>> --- >>> drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h >>> index 36365bdbe1ee..dc7cc06c68e7 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h >>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h >>> @@ -328,10 +328,10 @@ struct intel_engine_cs { >>> intel_engine_mask_t logical_mask; >>> >>> u8 class; >>> - u8 instance; >>> + s8 instance; >>> >>> u16 uabi_class; >>> - u16 uabi_instance; >>> + s16 uabi_instance; >>> >>> u32 uabi_capabilities; >>> u32 context_size; >> >> -- >> Jani Nikula, Intel Open Source Graphics Center >
On Wed, 16 Feb 2022, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 16/02/2022 09:19, Ville Syrjälä wrote: >> On Wed, Feb 16, 2022 at 11:02:06AM +0200, Jani Nikula wrote: >>> On Wed, 16 Feb 2022, Jiapeng Chong <jiapeng.chong@linux.alibaba.com> wrote: >>>> Eliminate the follow smatch warning: >>>> >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4640 >>>> guc_create_virtual() warn: assigning (-2) to unsigned variable >>>> 've->base.instance'. >>>> >>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4641 >>>> guc_create_virtual() warn: assigning (-2) to unsigned variable >>>> 've->base.uabi_instance'. >>>> >>>> Reported-by: Abaci Robot <abaci@linux.alibaba.com> >>>> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> >>> >>> The report seems to be valid, but I don't think this is the fix. >>> >>> Where do we even check for invalid instance/uabi_instance in code? >> >> The whole thing seems rather poorly documented as there's a matching >> uabi struct with __u16's and the negative values are defined right >> there in the uapi header as well. > > Negative ones are exception values to be used in conjunction with the virtual engine uapi (see "DOC: Virtual Engine uAPI" and also comment next to I915_CONTEXT_PARAM_ENGINES). > > AFAIK assigning negative int to unsigned int is defined and fine. > > Compiler does warn on comparisons which is why we have: > > ./gem/i915_gem_busy.c: if (id == (u16)I915_ENGINE_CLASS_INVALID) > ./gem/i915_gem_busy.c: if (id == (u16)I915_ENGINE_CLASS_INVALID) > ./gem/i915_gem_context.c: if (ci.engine_class == (u16)I915_ENGINE_CLASS_INVALID && > ./gem/i915_gem_context.c: ci.engine_instance == (u16)I915_ENGINE_CLASS_INVALID_NONE) > > So I think no action needed here. We never check instance or uabi_instance members against I915_ENGINE_CLASS_INVALID_VIRTUAL anywhere. BR, Jani. > > Regards, > > Tvrtko > >>> >>> BR, >>> Jani. >>> >>> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h >>>> index 36365bdbe1ee..dc7cc06c68e7 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h >>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h >>>> @@ -328,10 +328,10 @@ struct intel_engine_cs { >>>> intel_engine_mask_t logical_mask; >>>> >>>> u8 class; >>>> - u8 instance; >>>> + s8 instance; >>>> >>>> u16 uabi_class; >>>> - u16 uabi_instance; >>>> + s16 uabi_instance; >>>> >>>> u32 uabi_capabilities; >>>> u32 context_size; >>> >>> -- >>> Jani Nikula, Intel Open Source Graphics Center >>
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 36365bdbe1ee..dc7cc06c68e7 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -328,10 +328,10 @@ struct intel_engine_cs { intel_engine_mask_t logical_mask; u8 class; - u8 instance; + s8 instance; u16 uabi_class; - u16 uabi_instance; + s16 uabi_instance; u32 uabi_capabilities; u32 context_size;
Eliminate the follow smatch warning: drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4640 guc_create_virtual() warn: assigning (-2) to unsigned variable 've->base.instance'. drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:4641 guc_create_virtual() warn: assigning (-2) to unsigned variable 've->base.uabi_instance'. Reported-by: Abaci Robot <abaci@linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com> --- drivers/gpu/drm/i915/gt/intel_engine_types.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)