* Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c @ 2019-03-20 0:54 Nathan Chancellor 2019-03-20 1:31 ` Pan, Xinhui 0 siblings, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2019-03-20 0:54 UTC (permalink / raw) To: Alex Deucher, Christian König, David (ChunMing) Zhou, xinhui pan Cc: amd-gfx, dri-devel, linux-kernel, clang-built-linux Hi all, The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)") introduces the following Clang warnings: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = head->block, ~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .error_type = head->type, ~~~~~~^~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = head->block, ~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .error_type = head->type, ~~~~~~^~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = info->head.block, ~~~~~~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .inject_error_type = info->head.type, ~~~~~~~~~~~^~~~ 6 warnings generated. Normally, I would sent a fix for this myself but I am not entirely sure why these two enumerated types exist when one would do since they have the same values minus the prefix. In fact, the ta_ras_{block,error_type} values are never used aside from being defined. Some clarification would be appreciated. Thank you, Nathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 0:54 Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c Nathan Chancellor @ 2019-03-20 1:31 ` Pan, Xinhui 2019-03-20 4:34 ` Nathan Chancellor 0 siblings, 1 reply; 10+ messages in thread From: Pan, Xinhui @ 2019-03-20 1:31 UTC (permalink / raw) To: Nathan Chancellor, Deucher, Alexander, Koenig, Christian, Zhou, David(ChunMing) Cc: amd-gfx, dri-devel, linux-kernel, clang-built-linux these two enumerated types are same for now. both of them might change in the future. I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. Thanks xinhui -----Original Message----- From: Nathan Chancellor <natechancellor@gmail.com> Sent: 2019年3月20日 8:54 To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c Hi all, The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)") introduces the following Clang warnings: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = head->block, ~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .error_type = head->type, ~~~~~~^~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = head->block, ~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .error_type = head->type, ~~~~~~^~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] .block_id = info->head.block, ~~~~~~~~~~~^~~~~ drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] .inject_error_type = info->head.type, ~~~~~~~~~~~^~~~ 6 warnings generated. Normally, I would sent a fix for this myself but I am not entirely sure why these two enumerated types exist when one would do since they have the same values minus the prefix. In fact, the ta_ras_{block,error_type} values are never used aside from being defined. Some clarification would be appreciated. Thank you, Nathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 1:31 ` Pan, Xinhui @ 2019-03-20 4:34 ` Nathan Chancellor 2019-03-20 9:37 ` Koenig, Christian 0 siblings, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2019-03-20 4:34 UTC (permalink / raw) To: Pan, Xinhui Cc: Deucher, Alexander, Koenig, Christian, Zhou, David(ChunMing), amd-gfx, dri-devel, linux-kernel, clang-built-linux On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > these two enumerated types are same for now. both of them might change in the future. > > I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. > > Thanks > xinhui > > > -----Original Message----- > From: Nathan Chancellor <natechancellor@gmail.com> > Sent: 2019年3月20日 8:54 > To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com> > Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com > Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > > Hi all, > > The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)") introduces the following Clang > warnings: > > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > .block_id = head->block, > ~~~~~~^~~~~ > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > .error_type = head->type, > ~~~~~~^~~~ > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > .block_id = head->block, > ~~~~~~^~~~~ > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > .error_type = head->type, > ~~~~~~^~~~ > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > .block_id = info->head.block, > ~~~~~~~~~~~^~~~~ > drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > .inject_error_type = info->head.type, > ~~~~~~~~~~~^~~~ > 6 warnings generated. > > Normally, I would sent a fix for this myself but I am not entirely sure why these two enumerated types exist when one would do since they have the same values minus the prefix. In fact, the ta_ras_{block,error_type} values are never used aside from being defined. Some clarification would be appreciated. > > Thank you, > Nathan Hi Xinhui, Yes, explicitly casting these six spots to int would resolve this warning. Thank you for the quick response! Nathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 4:34 ` Nathan Chancellor @ 2019-03-20 9:37 ` Koenig, Christian [not found] ` <63518f1f-b808-77b0-aac6-ee1ece669c4b-5C7GfCeVMHo@public.gmane.org> 2019-03-20 17:19 ` Nick Desaulniers 0 siblings, 2 replies; 10+ messages in thread From: Koenig, Christian @ 2019-03-20 9:37 UTC (permalink / raw) To: Nathan Chancellor, Pan, Xinhui Cc: Zhou, David(ChunMing), linux-kernel-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, clang-built-linux-/JYPxA39Uh5TLH3MbocFFw, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Deucher, Alexander Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: >> these two enumerated types are same for now. both of them might change in the future. >> >> I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. >> >> Thanks >> xinhui >> >> >> -----Original Message----- >> From: Nathan Chancellor <natechancellor@gmail.com> >> Sent: 2019年3月20日 8:54 >> To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com> >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c >> >> Hi all, >> >> The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)") introduces the following Clang >> warnings: >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> .block_id = head->block, >> ~~~~~~^~~~~ >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> .error_type = head->type, >> ~~~~~~^~~~ >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> .block_id = head->block, >> ~~~~~~^~~~~ >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> .error_type = head->type, >> ~~~~~~^~~~ >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> .block_id = info->head.block, >> ~~~~~~~~~~~^~~~~ >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> .inject_error_type = info->head.type, >> ~~~~~~~~~~~^~~~ >> 6 warnings generated. >> >> Normally, I would sent a fix for this myself but I am not entirely sure why these two enumerated types exist when one would do since they have the same values minus the prefix. In fact, the ta_ras_{block,error_type} values are never used aside from being defined. Some clarification would be appreciated. >> >> Thank you, >> Nathan > Hi Xinhui, > > Yes, explicitly casting these six spots to int would resolve this > warning. Another question is if it is such a good idea to just silence the warning? Maybe add a amdgpu_ras_error_to_ta() helper to do this casting? When the enums drift away from each other then we can still add warnings to that helper to make sure we don't accidentally cast invalid values around. Regards, Christian. > > Thank you for the quick response! > Nathan _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <63518f1f-b808-77b0-aac6-ee1ece669c4b-5C7GfCeVMHo@public.gmane.org>]
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c [not found] ` <63518f1f-b808-77b0-aac6-ee1ece669c4b-5C7GfCeVMHo@public.gmane.org> @ 2019-03-20 14:58 ` Stephen Hines 2019-03-20 15:32 ` Stephen Hines 2019-03-20 15:44 ` Nathan Chancellor 0 siblings, 2 replies; 10+ messages in thread From: Stephen Hines @ 2019-03-20 14:58 UTC (permalink / raw) To: Koenig, Christian Cc: Zhou, David(ChunMing), Pan, Xinhui, linux-kernel-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, clang-built-linux-/JYPxA39Uh5TLH3MbocFFw, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Deucher, Alexander, Nathan Chancellor [-- Attachment #1.1: Type: text/plain, Size: 5453 bytes --] Why are there 2 different enums for this same thing at all? By casting, you are reducing type safety in the kernel, which can cause bugs later (should the two enums diverge in encoding). In my opinion, the proper solution is to remove one of the enums or provide an explicit helper that does the conversion (along with assertions for handling any unexpected cases). The helper function should not be doing a direct cast, since a bug could change the integer value of one (or both) of these enums so that they don't match up. Thanks, Steve On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian <Christian.Koenig-5C7GfCeVMHo@public.gmane.org> wrote: > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > >> these two enumerated types are same for now. both of them might change > in the future. > >> > >> I have not used clang, but would .block_id = (int)head->block fix > your warning? If such change is acceptable, I can make one then. > >> > >> Thanks > >> xinhui > >> > >> > >> -----Original Message----- > >> From: Nathan Chancellor <natechancellor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > >> Sent: 2019年3月20日 8:54 > >> To: Deucher, Alexander <Alexander.Deucher-5C7GfCeVMHo@public.gmane.org>; Koenig, Christian < > Christian.Koenig-5C7GfCeVMHo@public.gmane.org>; Zhou, David(ChunMing) <David1.Zhou-5C7GfCeVMHo@public.gmane.org>; > Pan, Xinhui <Xinhui.Pan-5C7GfCeVMHo@public.gmane.org> > >> Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org; > linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; clang-built-linux-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org > >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > >> > >> Hi all, > >> > >> The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add > amdgpu_ras.c to support ras (v2)") introduces the following Clang > >> warnings: > >> > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_block' to different > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > >> .block_id = head->block, > >> ~~~~~~^~~~~ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > >> .error_type = head->type, > >> ~~~~~~^~~~ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_block' to different > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > >> .block_id = head->block, > >> ~~~~~~^~~~~ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > >> .error_type = head->type, > >> ~~~~~~^~~~ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_block' to different > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > >> .block_id = info->head.block, > >> ~~~~~~~~~~~^~~~~ > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > >> .inject_error_type = info->head.type, > >> ~~~~~~~~~~~^~~~ > >> 6 warnings generated. > >> > >> Normally, I would sent a fix for this myself but I am not entirely sure > why these two enumerated types exist when one would do since they have the > same values minus the prefix. In fact, the ta_ras_{block,error_type} values > are never used aside from being defined. Some clarification would be > appreciated. > >> > >> Thank you, > >> Nathan > > Hi Xinhui, > > > > Yes, explicitly casting these six spots to int would resolve this > > warning. > > Another question is if it is such a good idea to just silence the warning? > > Maybe add a amdgpu_ras_error_to_ta() helper to do this casting? > > When the enums drift away from each other then we can still add warnings > to that helper to make sure we don't accidentally cast invalid values > around. > > Regards, > Christian. > > > > > Thank you for the quick response! > > Nathan > > -- > You received this message because you are subscribed to the Google Groups > "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to clang-built-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To post to this group, send email to clang-built-linux-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org > To view this discussion on the web visit > https://groups.google.com/d/msgid/clang-built-linux/63518f1f-b808-77b0-aac6-ee1ece669c4b%40amd.com > . > For more options, visit https://groups.google.com/d/optout. > [-- Attachment #1.2: Type: text/html, Size: 7557 bytes --] [-- Attachment #2: Type: text/plain, Size: 153 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 14:58 ` Stephen Hines @ 2019-03-20 15:32 ` Stephen Hines 2019-03-20 15:44 ` Nathan Chancellor 1 sibling, 0 replies; 10+ messages in thread From: Stephen Hines @ 2019-03-20 15:32 UTC (permalink / raw) To: Koenig, Christian Cc: Nathan Chancellor, Pan, Xinhui, Deucher, Alexander, Zhou, David(ChunMing), amd-gfx, dri-devel, linux-kernel, clang-built-linux Resending as plain-text to rest of list. Steve On Wed, Mar 20, 2019 at 7:58 AM Stephen Hines <srhines@google.com> wrote: > > Why are there 2 different enums for this same thing at all? By casting, you are reducing type safety in the kernel, which can cause bugs later (should the two enums diverge in encoding). In my opinion, the proper solution is to remove one of the enums or provide an explicit helper that does the conversion (along with assertions for handling any unexpected cases). The helper function should not be doing a direct cast, since a bug could change the integer value of one (or both) of these enums so that they don't match up. > > Thanks, > Steve > > On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: >> >> Am 20.03.19 um 05:34 schrieb Nathan Chancellor: >> > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: >> >> these two enumerated types are same for now. both of them might change in the future. >> >> >> >> I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. >> >> >> >> Thanks >> >> xinhui >> >> >> >> >> >> -----Original Message----- >> >> From: Nathan Chancellor <natechancellor@gmail.com> >> >> Sent: 2019年3月20日 8:54 >> >> To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com> >> >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com >> >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c >> >> >> >> Hi all, >> >> >> >> The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add amdgpu_ras.c to support ras (v2)") introduces the following Clang >> >> warnings: >> >> >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> >> .block_id = head->block, >> >> ~~~~~~^~~~~ >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> >> .error_type = head->type, >> >> ~~~~~~^~~~ >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> >> .block_id = head->block, >> >> ~~~~~~^~~~~ >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> >> .error_type = head->type, >> >> ~~~~~~^~~~ >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit conversion from enumeration type 'enum amdgpu_ras_block' to different enumeration type 'enum ta_ras_block' [-Wenum-conversion] >> >> .block_id = info->head.block, >> >> ~~~~~~~~~~~^~~~~ >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit conversion from enumeration type 'enum amdgpu_ras_error_type' to different enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] >> >> .inject_error_type = info->head.type, >> >> ~~~~~~~~~~~^~~~ >> >> 6 warnings generated. >> >> >> >> Normally, I would sent a fix for this myself but I am not entirely sure why these two enumerated types exist when one would do since they have the same values minus the prefix. In fact, the ta_ras_{block,error_type} values are never used aside from being defined. Some clarification would be appreciated. >> >> >> >> Thank you, >> >> Nathan >> > Hi Xinhui, >> > >> > Yes, explicitly casting these six spots to int would resolve this >> > warning. >> >> Another question is if it is such a good idea to just silence the warning? >> >> Maybe add a amdgpu_ras_error_to_ta() helper to do this casting? >> >> When the enums drift away from each other then we can still add warnings >> to that helper to make sure we don't accidentally cast invalid values >> around. >> >> Regards, >> Christian. >> >> > >> > Thank you for the quick response! >> > Nathan >> >> -- >> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. >> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. >> To post to this group, send email to clang-built-linux@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/63518f1f-b808-77b0-aac6-ee1ece669c4b%40amd.com. >> For more options, visit https://groups.google.com/d/optout. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 14:58 ` Stephen Hines 2019-03-20 15:32 ` Stephen Hines @ 2019-03-20 15:44 ` Nathan Chancellor 2019-03-21 6:58 ` Pan, Xinhui 1 sibling, 1 reply; 10+ messages in thread From: Nathan Chancellor @ 2019-03-20 15:44 UTC (permalink / raw) To: Stephen Hines Cc: Koenig, Christian, Pan, Xinhui, Deucher, Alexander, Zhou, David(ChunMing), amd-gfx, dri-devel, linux-kernel, clang-built-linux On Wed, Mar 20, 2019 at 07:58:09AM -0700, Stephen Hines wrote: > Why are there 2 different enums for this same thing at all? By casting, you > are reducing type safety in the kernel, which can cause bugs later (should > the two enums diverge in encoding). In my opinion, the proper solution is > to remove one of the enums or provide an explicit helper that does the > conversion (along with assertions for handling any unexpected cases). The > helper function should not be doing a direct cast, since a bug could change > the integer value of one (or both) of these enums so that they don't match > up. > > Thanks, > Steve > Indeed, I would suggest something like this (if this was to be a build time error, this would need to be a macro instead of a function): diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c index a71668b8a7d0..451478cf4f35 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c @@ -541,13 +541,13 @@ int amdgpu_ras_feature_enable(struct amdgpu_device *adev, if (!enable) { info.disable_features = (struct ta_ras_disable_features_input) { - .block_id = head->block, - .error_type = head->type, + .block_id = amdgpu_ras_block_to_ta(head->block), + .error_type = amdgpu_ras_error_to_ta(head->type), }; } else { info.enable_features = (struct ta_ras_enable_features_input) { - .block_id = head->block, - .error_type = head->type, + .block_id = amdgpu_ras_block_to_ta(head->block), + .error_type = amdgpu_ras_error_to_ta(head->type), }; } @@ -647,8 +647,8 @@ int amdgpu_ras_error_inject(struct amdgpu_device *adev, { struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head); struct ta_ras_trigger_error_input block_info = { - .block_id = info->head.block, - .inject_error_type = info->head.type, + .block_id = amdgpu_ras_block_to_ta(info->head.block), + .inject_error_type = amdgpu_ras_error_to_ta(info->head.type), .sub_block_index = info->head.sub_block_index, .address = info->address, .value = info->value, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h index 7a35316baab0..c8576ab6e057 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h @@ -197,6 +197,64 @@ static inline int amdgpu_ras_reset_gpu(struct amdgpu_device *adev, return 0; } +static inline enum ta_ras_block +amdgpu_ras_block_to_ta(enum amdgpu_ras_block block) +{ + switch (block) { + case AMDGPU_RAS_BLOCK__UMC: + return TA_RAS_BLOCK__UMC; + case AMDGPU_RAS_BLOCK__SDMA: + return TA_RAS_BLOCK__SDMA; + case AMDGPU_RAS_BLOCK__GFX: + return TA_RAS_BLOCK__GFX; + case AMDGPU_RAS_BLOCK__MMHUB: + return TA_RAS_BLOCK__MMHUB; + case AMDGPU_RAS_BLOCK__ATHUB: + return TA_RAS_BLOCK__ATHUB; + case AMDGPU_RAS_BLOCK__PCIE_BIF: + return TA_RAS_BLOCK__PCIE_BIF; + case AMDGPU_RAS_BLOCK__HDP: + return TA_RAS_BLOCK__HDP; + case AMDGPU_RAS_BLOCK__XGMI_WAFL: + return TA_RAS_BLOCK__XGMI_WAFL; + case AMDGPU_RAS_BLOCK__DF: + return TA_RAS_BLOCK__DF; + case AMDGPU_RAS_BLOCK__SMN: + return TA_RAS_BLOCK__SMN; + case AMDGPU_RAS_BLOCK__SEM: + return TA_RAS_BLOCK__SEM; + case AMDGPU_RAS_BLOCK__MP0: + return TA_RAS_BLOCK__MP0; + case AMDGPU_RAS_BLOCK__MP1: + return TA_RAS_BLOCK__MP1; + case AMDGPU_RAS_BLOCK__FUSE: + return TA_RAS_BLOCK__FUSE; + default: + WARN(1, "amdgpu_ras_block_to_ta fell through with a value of %d\n", block); + return TA_RAS_BLOCK__UMC; + } +} + +static inline enum ta_ras_error_type +amdgpu_ras_error_to_ta(enum amdgpu_ras_error_type error) +{ + switch (error) { + case AMDGPU_RAS_ERROR__NONE: + return TA_RAS_ERROR__NONE; + case AMDGPU_RAS_ERROR__PARITY: + return TA_RAS_ERROR__PARITY; + case AMDGPU_RAS_ERROR__SINGLE_CORRECTABLE: + return TA_RAS_ERROR__SINGLE_CORRECTABLE; + case AMDGPU_RAS_ERROR__MULTI_UNCORRECTABLE: + return TA_RAS_ERROR__MULTI_UNCORRECTABLE; + case AMDGPU_RAS_ERROR__POISON: + return TA_RAS_ERROR__POISON; + default: + WARN(1, "amdgpu_ras_error_to_ta fell through with a value of %d\n", error); + return TA_RAS_ERROR__NONE; + } +} + /* called in ip_init and ip_fini */ int amdgpu_ras_init(struct amdgpu_device *adev); void amdgpu_ras_post_init(struct amdgpu_device *adev); > On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian <Christian.Koenig@amd.com> > wrote: > > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > > >> these two enumerated types are same for now. both of them might change > > in the future. > > >> > > >> I have not used clang, but would .block_id = (int)head->block fix > > your warning? If such change is acceptable, I can make one then. > > >> > > >> Thanks > > >> xinhui > > >> > > >> > > >> -----Original Message----- > > >> From: Nathan Chancellor <natechancellor@gmail.com> > > >> Sent: 2019年3月20日 8:54 > > >> To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian < > > Christian.Koenig@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; > > Pan, Xinhui <Xinhui.Pan@amd.com> > > >> Cc: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; > > linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com > > >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > > >> > > >> Hi all, > > >> > > >> The introduction of this file in commit dbd249c24427 ("drm/amdgpu: add > > amdgpu_ras.c to support ras (v2)") introduces the following Clang > > >> warnings: > > >> > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to different > > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = head->block, > > >> ~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > > >> .error_type = head->type, > > >> ~~~~~~^~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to different > > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = head->block, > > >> ~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > > >> .error_type = head->type, > > >> ~~~~~~^~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to different > > enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = info->head.block, > > >> ~~~~~~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to different > > enumeration type 'enum ta_ras_error_type' [-Wenum-conversion] > > >> .inject_error_type = info->head.type, > > >> ~~~~~~~~~~~^~~~ > > >> 6 warnings generated. > > >> > > >> Normally, I would sent a fix for this myself but I am not entirely sure > > why these two enumerated types exist when one would do since they have the > > same values minus the prefix. In fact, the ta_ras_{block,error_type} values > > are never used aside from being defined. Some clarification would be > > appreciated. > > >> > > >> Thank you, > > >> Nathan > > > Hi Xinhui, > > > > > > Yes, explicitly casting these six spots to int would resolve this > > > warning. > > > > Another question is if it is such a good idea to just silence the warning? > > > > Maybe add a amdgpu_ras_error_to_ta() helper to do this casting? > > > > When the enums drift away from each other then we can still add warnings > > to that helper to make sure we don't accidentally cast invalid values > > around. > > > > Regards, > > Christian. > > > > > > > > Thank you for the quick response! > > > Nathan > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Clang Built Linux" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to clang-built-linux+unsubscribe@googlegroups.com. > > To post to this group, send email to clang-built-linux@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/clang-built-linux/63518f1f-b808-77b0-aac6-ee1ece669c4b%40amd.com > > . > > For more options, visit https://groups.google.com/d/optout. > > ^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 15:44 ` Nathan Chancellor @ 2019-03-21 6:58 ` Pan, Xinhui 0 siblings, 0 replies; 10+ messages in thread From: Pan, Xinhui @ 2019-03-21 6:58 UTC (permalink / raw) To: Nathan Chancellor, Stephen Hines Cc: Zhou, David(ChunMing), linux-kernel-u79uwXL29TY76Z2rM5mHXA, dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, clang-built-linux-/JYPxA39Uh5TLH3MbocFFw, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Deucher, Alexander, Koenig, Christian I am going to apply your fix patch in my branch. Thanks xinhui -----Original Message----- From: Nathan Chancellor <natechancellor@gmail.com> Sent: 2019年3月20日 23:45 To: Stephen Hines <srhines@google.com> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Zhou, David(ChunMing) <David1.Zhou@amd.com>; amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com Subject: Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c On Wed, Mar 20, 2019 at 07:58:09AM -0700, Stephen Hines wrote: > Why are there 2 different enums for this same thing at all? By > casting, you are reducing type safety in the kernel, which can cause > bugs later (should the two enums diverge in encoding). In my opinion, > the proper solution is to remove one of the enums or provide an > explicit helper that does the conversion (along with assertions for > handling any unexpected cases). The helper function should not be > doing a direct cast, since a bug could change the integer value of one > (or both) of these enums so that they don't match up. > > Thanks, > Steve > Indeed, I would suggest something like this (if this was to be a build time error, this would need to be a macro instead of a function): diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c index a71668b8a7d0..451478cf4f35 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c @@ -541,13 +541,13 @@ int amdgpu_ras_feature_enable(struct amdgpu_device *adev, if (!enable) { info.disable_features = (struct ta_ras_disable_features_input) { - .block_id = head->block, - .error_type = head->type, + .block_id = amdgpu_ras_block_to_ta(head->block), + .error_type = amdgpu_ras_error_to_ta(head->type), }; } else { info.enable_features = (struct ta_ras_enable_features_input) { - .block_id = head->block, - .error_type = head->type, + .block_id = amdgpu_ras_block_to_ta(head->block), + .error_type = amdgpu_ras_error_to_ta(head->type), }; } @@ -647,8 +647,8 @@ int amdgpu_ras_error_inject(struct amdgpu_device *adev, { struct ras_manager *obj = amdgpu_ras_find_obj(adev, &info->head); struct ta_ras_trigger_error_input block_info = { - .block_id = info->head.block, - .inject_error_type = info->head.type, + .block_id = amdgpu_ras_block_to_ta(info->head.block), + .inject_error_type = amdgpu_ras_error_to_ta(info->head.type), .sub_block_index = info->head.sub_block_index, .address = info->address, .value = info->value, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h index 7a35316baab0..c8576ab6e057 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h @@ -197,6 +197,64 @@ static inline int amdgpu_ras_reset_gpu(struct amdgpu_device *adev, return 0; } +static inline enum ta_ras_block +amdgpu_ras_block_to_ta(enum amdgpu_ras_block block) { + switch (block) { + case AMDGPU_RAS_BLOCK__UMC: + return TA_RAS_BLOCK__UMC; + case AMDGPU_RAS_BLOCK__SDMA: + return TA_RAS_BLOCK__SDMA; + case AMDGPU_RAS_BLOCK__GFX: + return TA_RAS_BLOCK__GFX; + case AMDGPU_RAS_BLOCK__MMHUB: + return TA_RAS_BLOCK__MMHUB; + case AMDGPU_RAS_BLOCK__ATHUB: + return TA_RAS_BLOCK__ATHUB; + case AMDGPU_RAS_BLOCK__PCIE_BIF: + return TA_RAS_BLOCK__PCIE_BIF; + case AMDGPU_RAS_BLOCK__HDP: + return TA_RAS_BLOCK__HDP; + case AMDGPU_RAS_BLOCK__XGMI_WAFL: + return TA_RAS_BLOCK__XGMI_WAFL; + case AMDGPU_RAS_BLOCK__DF: + return TA_RAS_BLOCK__DF; + case AMDGPU_RAS_BLOCK__SMN: + return TA_RAS_BLOCK__SMN; + case AMDGPU_RAS_BLOCK__SEM: + return TA_RAS_BLOCK__SEM; + case AMDGPU_RAS_BLOCK__MP0: + return TA_RAS_BLOCK__MP0; + case AMDGPU_RAS_BLOCK__MP1: + return TA_RAS_BLOCK__MP1; + case AMDGPU_RAS_BLOCK__FUSE: + return TA_RAS_BLOCK__FUSE; + default: + WARN(1, "amdgpu_ras_block_to_ta fell through with a value of %d\n", block); + return TA_RAS_BLOCK__UMC; + } +} + +static inline enum ta_ras_error_type +amdgpu_ras_error_to_ta(enum amdgpu_ras_error_type error) { + switch (error) { + case AMDGPU_RAS_ERROR__NONE: + return TA_RAS_ERROR__NONE; + case AMDGPU_RAS_ERROR__PARITY: + return TA_RAS_ERROR__PARITY; + case AMDGPU_RAS_ERROR__SINGLE_CORRECTABLE: + return TA_RAS_ERROR__SINGLE_CORRECTABLE; + case AMDGPU_RAS_ERROR__MULTI_UNCORRECTABLE: + return TA_RAS_ERROR__MULTI_UNCORRECTABLE; + case AMDGPU_RAS_ERROR__POISON: + return TA_RAS_ERROR__POISON; + default: + WARN(1, "amdgpu_ras_error_to_ta fell through with a value of %d\n", error); + return TA_RAS_ERROR__NONE; + } +} + /* called in ip_init and ip_fini */ int amdgpu_ras_init(struct amdgpu_device *adev); void amdgpu_ras_post_init(struct amdgpu_device *adev); > On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian > <Christian.Koenig@amd.com> > wrote: > > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > > >> these two enumerated types are same for now. both of them might > > >> change > > in the future. > > >> > > >> I have not used clang, but would .block_id = (int)head->block > > >> fix > > your warning? If such change is acceptable, I can make one then. > > >> > > >> Thanks > > >> xinhui > > >> > > >> > > >> -----Original Message----- > > >> From: Nathan Chancellor <natechancellor@gmail.com> > > >> Sent: 2019年3月20日 8:54 > > >> To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, > > >> Christian < > > Christian.Koenig@amd.com>; Zhou, David(ChunMing) > > <David1.Zhou@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com> > > >> Cc: amd-gfx@lists.freedesktop.org; > > >> dri-devel@lists.freedesktop.org; > > linux-kernel@vger.kernel.org; clang-built-linux@googlegroups.com > > >> Subject: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c > > >> > > >> Hi all, > > >> > > >> The introduction of this file in commit dbd249c24427 > > >> ("drm/amdgpu: add > > amdgpu_ras.c to support ras (v2)") introduces the following Clang > > >> warnings: > > >> > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:544:23: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to > > different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = head->block, > > >> ~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:545:24: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to > > different enumeration type 'enum ta_ras_error_type' > > [-Wenum-conversion] > > >> .error_type = head->type, > > >> ~~~~~~^~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:549:23: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to > > different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = head->block, > > >> ~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:550:24: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to > > different enumeration type 'enum ta_ras_error_type' > > [-Wenum-conversion] > > >> .error_type = head->type, > > >> ~~~~~~^~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:650:26: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_block' to > > different enumeration type 'enum ta_ras_block' [-Wenum-conversion] > > >> .block_id = info->head.block, > > >> ~~~~~~~~~~~^~~~~ > > >> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:651:35: warning: implicit > > conversion from enumeration type 'enum amdgpu_ras_error_type' to > > different enumeration type 'enum ta_ras_error_type' > > [-Wenum-conversion] > > >> .inject_error_type = info->head.type, > > >> ~~~~~~~~~~~^~~~ > > >> 6 warnings generated. > > >> > > >> Normally, I would sent a fix for this myself but I am not > > >> entirely sure > > why these two enumerated types exist when one would do since they > > have the same values minus the prefix. In fact, the > > ta_ras_{block,error_type} values are never used aside from being > > defined. Some clarification would be appreciated. > > >> > > >> Thank you, > > >> Nathan > > > Hi Xinhui, > > > > > > Yes, explicitly casting these six spots to int would resolve this > > > warning. > > > > Another question is if it is such a good idea to just silence the warning? > > > > Maybe add a amdgpu_ras_error_to_ta() helper to do this casting? > > > > When the enums drift away from each other then we can still add > > warnings to that helper to make sure we don't accidentally cast > > invalid values around. > > > > Regards, > > Christian. > > > > > > > > Thank you for the quick response! > > > Nathan > > > > -- > > You received this message because you are subscribed to the Google > > Groups "Clang Built Linux" group. > > To unsubscribe from this group and stop receiving emails from it, > > send an email to clang-built-linux+unsubscribe@googlegroups.com. > > To post to this group, send email to clang-built-linux@googlegroups.com. > > To view this discussion on the web visit > > https://groups.google.com/d/msgid/clang-built-linux/63518f1f-b808-77 > > b0-aac6-ee1ece669c4b%40amd.com > > . > > For more options, visit https://groups.google.com/d/optout. > > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 9:37 ` Koenig, Christian [not found] ` <63518f1f-b808-77b0-aac6-ee1ece669c4b-5C7GfCeVMHo@public.gmane.org> @ 2019-03-20 17:19 ` Nick Desaulniers 2019-03-21 4:44 ` Alex Deucher 1 sibling, 1 reply; 10+ messages in thread From: Nick Desaulniers @ 2019-03-20 17:19 UTC (permalink / raw) To: Koenig, Christian Cc: Nathan Chancellor, Pan, Xinhui, Deucher, Alexander, Zhou, David(ChunMing), amd-gfx, dri-devel, linux-kernel, clang-built-linux On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian <Christian.Koenig@amd.com> wrote: > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > >> these two enumerated types are same for now. both of them might change in the future. Please consider if the YAGNI principle applies here. https://martinfowler.com/bliki/Yagni.html > >> > >> I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. My preference on solutions, in order: 1. One enum (this is the simplest most type safe solution). Add another enum when you actually need it. Otherwise, YAGNI. 2. Safe casting function (like the one Nathan supplied, maybe with WARN_ONCE rather than WARN). This ensures that at least if the types diverge you get a runtime warning. A compile-time warning would be preferred, but I haven't taken the time to think through how that might be implemented. 3. Cast to int (this has been used in other places throughout the kernel, but provides the weakest type safety and doesn't catch future divergence). 4. Disabling the warning. (I almost never advocate for this). > Another question is if it is such a good idea to just silence the warning? For the kernel, it seems that each maintainer can choose what to apply to their subsystem. I would recommend against disabling additional warnings that aren't disable kernel-wide for most cases. -Wenum-conversion has spotted many bugs. While the enums in question today are not different, they MIGHT eventually diverge and lead to bugs, like the others we've found and fixed throughout the kernel. So I would recommend fixing now, and be insulated in the future. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 2019-03-20 17:19 ` Nick Desaulniers @ 2019-03-21 4:44 ` Alex Deucher 0 siblings, 0 replies; 10+ messages in thread From: Alex Deucher @ 2019-03-21 4:44 UTC (permalink / raw) To: Nick Desaulniers Cc: Koenig, Christian, Zhou, David(ChunMing), Pan, Xinhui, linux-kernel, dri-devel, clang-built-linux, amd-gfx, Deucher, Alexander, Nathan Chancellor On Wed, Mar 20, 2019 at 4:49 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian > <Christian.Koenig@amd.com> wrote: > > > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > > On Wed, Mar 20, 2019 at 01:31:27AM +0000, Pan, Xinhui wrote: > > >> these two enumerated types are same for now. both of them might change in the future. > > Please consider if the YAGNI principle applies here. > https://martinfowler.com/bliki/Yagni.html > > > >> > > >> I have not used clang, but would .block_id = (int)head->block fix your warning? If such change is acceptable, I can make one then. > > My preference on solutions, in order: > 1. One enum (this is the simplest most type safe solution). Add > another enum when you actually need it. Otherwise, YAGNI. It make sense to have two enums. One is a firmware interface that is only used by some asics and the other is for the general driver interface (non-asic specific for the ras features. > 2. Safe casting function (like the one Nathan supplied, maybe with > WARN_ONCE rather than WARN). This ensures that at least if the types > diverge you get a runtime warning. A compile-time warning would be > preferred, but I haven't taken the time to think through how that > might be implemented. I'd prefer this one. Alex > 3. Cast to int (this has been used in other places throughout the > kernel, but provides the weakest type safety and doesn't catch future > divergence). > 4. Disabling the warning. (I almost never advocate for this). > > > Another question is if it is such a good idea to just silence the warning? > > For the kernel, it seems that each maintainer can choose what to apply > to their subsystem. I would recommend against disabling additional > warnings that aren't disable kernel-wide for most cases. > -Wenum-conversion has spotted many bugs. While the enums in question > today are not different, they MIGHT eventually diverge and lead to > bugs, like the others we've found and fixed throughout the kernel. So > I would recommend fixing now, and be insulated in the future. > > -- > Thanks, > ~Nick Desaulniers > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-03-21 6:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-20 0:54 Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c Nathan Chancellor 2019-03-20 1:31 ` Pan, Xinhui 2019-03-20 4:34 ` Nathan Chancellor 2019-03-20 9:37 ` Koenig, Christian [not found] ` <63518f1f-b808-77b0-aac6-ee1ece669c4b-5C7GfCeVMHo@public.gmane.org> 2019-03-20 14:58 ` Stephen Hines 2019-03-20 15:32 ` Stephen Hines 2019-03-20 15:44 ` Nathan Chancellor 2019-03-21 6:58 ` Pan, Xinhui 2019-03-20 17:19 ` Nick Desaulniers 2019-03-21 4:44 ` Alex Deucher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).