| Message ID | 20180921215505.14634-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: Change status's type in aux_reply_transaction_data | expand |
On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns when one enumerated type is implicitly converted to another. > > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: > implicit conversion from enumeration type 'enum > aux_channel_operation_result' to different enumeration type 'enum > aux_transaction_reply' [-Wenum-conversion] > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: > warning: implicit conversion from enumeration type 'enum > aux_channel_operation_result' to different enumeration type 'enum > aux_transaction_reply' [-Wenum-conversion] > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I think the enum is actually wrong here. I think the correct fix would be: - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; The identifiers are so similar, my guess was that it was easy to mix them up. This looks like an actual bug to me, since the identifiers have different values between the 2 different enums. > > Instead of implicitly or explicitly converting between types, just > change status to type uint8_t (since its max size is 255) which avoids > this construct altogether. > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > index 05c8c31d8b31..97e1d4d19263 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > @@ -79,7 +79,7 @@ enum aux_transaction_reply { > }; > > struct aux_reply_transaction_data { > - enum aux_transaction_reply status; > + uint8_t status; > uint32_t length; > uint8_t *data; > }; > -- > 2.19.0 >
On Mon, Sep 24, 2018 at 03:07:16PM -0700, Nick Desaulniers wrote: > On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns when one enumerated type is implicitly converted to another. > > > > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: > > implicit conversion from enumeration type 'enum > > aux_channel_operation_result' to different enumeration type 'enum > > aux_transaction_reply' [-Wenum-conversion] > > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: > > warning: implicit conversion from enumeration type 'enum > > aux_channel_operation_result' to different enumeration type 'enum > > aux_transaction_reply' [-Wenum-conversion] > > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I think the enum is actually wrong here. I think the correct fix would be: > > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; > > The identifiers are so similar, my guess was that it was easy to mix > them up. This looks like an actual bug to me, since the identifiers > have different values between the 2 different enums. > Hmmm interesting... I will be happy to send a v2 with your suggestion if one of the maintainers could confirm that to be the case (given DRM code is rather dense). Thanks for the review! Nathan > > > > Instead of implicitly or explicitly converting between types, just > > change status to type uint8_t (since its max size is 255) which avoids > > this construct altogether. > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > > index 05c8c31d8b31..97e1d4d19263 100644 > > --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > > +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h > > @@ -79,7 +79,7 @@ enum aux_transaction_reply { > > }; > > > > struct aux_reply_transaction_data { > > - enum aux_transaction_reply status; > > + uint8_t status; > > uint32_t length; > > uint8_t *data; > > }; > > -- > > 2.19.0 > > > > > -- > Thanks, > ~Nick Desaulniers
On 2018-09-24 06:22 PM, Nathan Chancellor wrote: > On Mon, Sep 24, 2018 at 03:07:16PM -0700, Nick Desaulniers wrote: >> On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor >> <natechancellor@gmail.com> wrote: >>> >>> Clang warns when one enumerated type is implicitly converted to another. >>> >>> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: >>> implicit conversion from enumeration type 'enum >>> aux_channel_operation_result' to different enumeration type 'enum >>> aux_transaction_reply' [-Wenum-conversion] >>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: >>> warning: implicit conversion from enumeration type 'enum >>> aux_channel_operation_result' to different enumeration type 'enum >>> aux_transaction_reply' [-Wenum-conversion] >>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> I think the enum is actually wrong here. I think the correct fix would be: >> >> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; >> >> The identifiers are so similar, my guess was that it was easy to mix >> them up. This looks like an actual bug to me, since the identifiers >> have different values between the 2 different enums. >> > > Hmmm interesting... I will be happy to send a v2 with your suggestion if > one of the maintainers could confirm that to be the case (given DRM code > is rather dense). > Nick is correct. We should keep the enum but assign AUX_TRANSACTION_REPLY_HPD_DISCON in dce_aux.c and aux_engine_dce110.c. Thanks for spotting this. Harry > Thanks for the review! > Nathan > >>> >>> Instead of implicitly or explicitly converting between types, just >>> change status to type uint8_t (since its max size is 255) which avoids >>> this construct altogether. >>> >>> Reported-by: Nick Desaulniers <ndesaulniers@google.com> >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >>> --- >>> drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h >>> index 05c8c31d8b31..97e1d4d19263 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h >>> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h >>> @@ -79,7 +79,7 @@ enum aux_transaction_reply { >>> }; >>> >>> struct aux_reply_transaction_data { >>> - enum aux_transaction_reply status; >>> + uint8_t status; >>> uint32_t length; >>> uint8_t *data; >>> }; >>> -- >>> 2.19.0 >>> >> >> >> -- >> Thanks, >> ~Nick Desaulniers
On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote: > > Clang warns when one enumerated type is implicitly converted to another. > > > > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: > > implicit conversion from enumeration type 'enum > > aux_channel_operation_result' to different enumeration type 'enum > > aux_transaction_reply' [-Wenum-conversion] > > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: > > warning: implicit conversion from enumeration type 'enum > > aux_channel_operation_result' to different enumeration type 'enum > > aux_transaction_reply' [-Wenum-conversion] > > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > The current enum is incorrect, it should be from aux_transaction_reply, > > so use AUX_TRANSACTION_REPLY_HPD_DISCON. > > > > Reported-by: Nick Desaulniers <ndesaulniers@google.com> > > Suggested-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > --- > > > > v1 -> v2: > > > > * Rather than change status to an integer, use the proper enumerated > > type from aux_transaction reply as suggested by Nick and confirmed > > by Henry. > > Sigh Harry, sorry... Thanks for the patch, Nathan. Don't worry about sending a v3 over this; its below the commit line so this detail wont get committed. > > > > > drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +- > > .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c > > index 3f5b2e6f7553..aaeb7faac0c4 100644 > > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c > > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c > > @@ -312,7 +312,7 @@ static void process_channel_reply( > > > > /* in case HPD is LOW, exit AUX transaction */ > > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) { > > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; > > return; > > } > > > > diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c > > index 8eee8ace1259..59c3ed43d609 100644 > > --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c > > +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c > > @@ -346,7 +346,7 @@ static void process_channel_reply( > > > > /* in case HPD is LOW, exit AUX transaction */ > > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) { > > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; > > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; > > return; > > } > > > > -- > > 2.19.0 > >
On 2018-09-27 02:11 PM, Nick Desaulniers wrote: > On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: >> >> On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote: >>> Clang warns when one enumerated type is implicitly converted to another. >>> >>> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: >>> implicit conversion from enumeration type 'enum >>> aux_channel_operation_result' to different enumeration type 'enum >>> aux_transaction_reply' [-Wenum-conversion] >>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: >>> warning: implicit conversion from enumeration type 'enum >>> aux_channel_operation_result' to different enumeration type 'enum >>> aux_transaction_reply' [-Wenum-conversion] >>> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> The current enum is incorrect, it should be from aux_transaction_reply, >>> so use AUX_TRANSACTION_REPLY_HPD_DISCON. >>> >>> Reported-by: Nick Desaulniers <ndesaulniers@google.com> >>> Suggested-by: Nick Desaulniers <ndesaulniers@google.com> >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Reviewed-by: Harry Wentland <harry.wentland@amd.com> Pulling it in through amd-staging-drm-next. > >>> --- >>> >>> v1 -> v2: >>> >>> * Rather than change status to an integer, use the proper enumerated >>> type from aux_transaction reply as suggested by Nick and confirmed >>> by Henry. >> >> Sigh Harry, sorry... > No worries. You're not the first to get that wrong. :) Harry > Thanks for the patch, Nathan. Don't worry about sending a v3 over > this; its below the commit line so this detail wont get committed. > >> >>> >>> drivers/gpu/drm/amd/display/dc/dce/dce_aux.c | 2 +- >>> .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c >>> index 3f5b2e6f7553..aaeb7faac0c4 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c >>> +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c >>> @@ -312,7 +312,7 @@ static void process_channel_reply( >>> >>> /* in case HPD is LOW, exit AUX transaction */ >>> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) { >>> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; >>> return; >>> } >>> >>> diff --git a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c >>> index 8eee8ace1259..59c3ed43d609 100644 >>> --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c >>> +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c >>> @@ -346,7 +346,7 @@ static void process_channel_reply( >>> >>> /* in case HPD is LOW, exit AUX transaction */ >>> if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) { >>> - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; >>> + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON; >>> return; >>> } >>> >>> -- >>> 2.19.0 >>> > > >
diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h index 05c8c31d8b31..97e1d4d19263 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h @@ -79,7 +79,7 @@ enum aux_transaction_reply { }; struct aux_reply_transaction_data { - enum aux_transaction_reply status; + uint8_t status; uint32_t length; uint8_t *data; };
Clang warns when one enumerated type is implicitly converted to another. drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning: implicit conversion from enumeration type 'enum aux_channel_operation_result' to different enumeration type 'enum aux_transaction_reply' [-Wenum-conversion] reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19: warning: implicit conversion from enumeration type 'enum aux_channel_operation_result' to different enumeration type 'enum aux_transaction_reply' [-Wenum-conversion] reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON; ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Instead of implicitly or explicitly converting between types, just change status to type uint8_t (since its max size is 255) which avoids this construct altogether. Reported-by: Nick Desaulniers <ndesaulniers@google.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)