Re: [PATCH v4] drm/msm/dp: add logs across DP driver for ease of debugging

[maitreye@codeaurora.org]

On 2021-07-22 15:09, Stephen Boyd wrote:
Thank you for the comments .
> Quoting maitreye@codeaurora.org (2021-07-22 14:33:43)
>> Thank you Stephen.
>> 
>> On 2021-07-22 13:31, Stephen Boyd wrote:
>> > Quoting maitreye (2021-07-21 16:19:40)
>> >> From: Maitreyee Rao <maitreye@codeaurora.org>
>> >>
>> >> Add trace points across the MSM DP driver to help debug
>> >> interop issues.
>> >>
>> >> Changes in v4:
>> >>  - Changed goto statement and used if else-if
>> >
>> > I think drm likes to see all the changelog here to see patch evolution.
>> >
>> Yes, I will fix this
>> >>
>> >> Signed-off-by: Maitreyee Rao <maitreye@codeaurora.org>
>> >> ---
>> >> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
>> >> b/drivers/gpu/drm/msm/dp/dp_link.c
>> >> index be986da..8c98ab7 100644
>> >> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> >> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> >> @@ -1036,43 +1036,28 @@ int dp_link_process_request(struct dp_link
>> >> *dp_link)
>> >>
>> >>         if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>> >>                 dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
>> >> -               return ret;
>> >>         }
>> >> -
>> >> -       ret = dp_link_process_ds_port_status_change(link);
>> >> -       if (!ret) {
>> >> +       else if (!(ret = dp_link_process_ds_port_status_change(link)))
>> >> {
>> >>                 dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
>> >> -               return ret;
>> >>         }
>> >> -
>> >> -       ret = dp_link_process_link_training_request(link);
>> >> -       if (!ret) {
>> >> +       else if (!(ret = dp_link_process_link_training_request(link)))
>> >> {
>> >>                 dp_link->sink_request |= DP_TEST_LINK_TRAINING;
>> >> -               return ret;
>> >>         }
>> >> -
>> >> -       ret = dp_link_process_phy_test_pattern_request(link);
>> >> -       if (!ret) {
>> >> +       else if (!(ret =
>> >> dp_link_process_phy_test_pattern_request(link))) {
>> >>                 dp_link->sink_request |=
>> >> DP_TEST_LINK_PHY_TEST_PATTERN;
>> >> -               return ret;
>> >> -       }
>> >> -
>> >> -       ret = dp_link_process_link_status_update(link);
>> >> -       if (!ret) {
>> >> +       }
>> >> +       else if (!(ret = dp_link_process_link_status_update(link))) {
>> >
>> > The kernel coding style is to leave the brackets on the same line
>> >
>> >       if (condition) {
>> >
>> >       } else if (conditon) {
>> >
>> >       }
>> >
>> > See Documentation/process/coding-style.rst
>> >
>> Yes, I will fix this
>> 
>> > Also, the if (!(ret = dp_link_...)) style is really hard to read. Maybe
>> > apply this patch before?
>> >
>> > ----8<----
>> > diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
>> > b/drivers/gpu/drm/msm/dp/dp_link.c
>> > index 1195044a7a3b..408cddd90f0f 100644
>> > --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> > +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> > @@ -1027,41 +1027,22 @@ int dp_link_process_request(struct dp_link
>> > *dp_link)
>> >
>> >       if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>> >               dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
>> > -             return ret;
>> > -     }
>> > -
>> > -     ret = dp_link_process_ds_port_status_change(link);
>> > -     if (!ret) {
>> > +     } else if (!dp_link_process_ds_port_status_change(link)) {
>> >               dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
>> > -             return ret;
>> > -     }
>> > -
>> > -     ret = dp_link_process_link_training_request(link);
>> > -     if (!ret) {
>> > +     } else if (!dp_link_process_link_training_request(link)) {
>> >               dp_link->sink_request |= DP_TEST_LINK_TRAINING;
>> > -             return ret;
>> > -     }
>> > -
>> > -     ret = dp_link_process_phy_test_pattern_request(link);
>> > -     if (!ret) {
>> > +     } else if (!dp_link_process_phy_test_pattern_request(link)) {
>> >               dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
>> > -             return ret;
>> > -     }
>> > -
>> > -     ret = dp_link_process_link_status_update(link);
>> > -     if (!ret) {
>> > +     } else if (!dp_link_process_link_status_update(link)) {
>> >               dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
>> > -             return ret;
>> > -     }
>> > -
>> > -     if (dp_link_is_video_pattern_requested(link)) {
>> > -             ret = 0;
>> > -             dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
>> > -     }
>> > +     } else {
>> > +             if (dp_link_is_video_pattern_requested(link))
>> > +                     dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
>> >
>> > -     if (dp_link_is_audio_pattern_requested(link)) {
>> > -             dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
>> > -             return -EINVAL;
>> > +             if (dp_link_is_audio_pattern_requested(link)) {
>> > +                     dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
>> > +                     ret = -EINVAL;
>> > +             }
>> >       }
>> >
>> >       return ret;
>> The reason I did this was to preserve the value of ret as the caller 
>> of
>> the function checks it. Some functions return -EINVAl like in here:
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/dp/dp_link.c#L972
>> , so to check that it would be necessary to get the ret value.
> 
> ret is overwritten multiple times. The logic seems to be if ret is
> not-zero, reassign it, until we get to the end. How about this
> 
> ----8<----
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c 
> b/drivers/gpu/drm/msm/dp/dp_link.c
> index 1195044a7a3b..e59138566c0a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -1027,41 +1027,27 @@ int dp_link_process_request(struct dp_link 
> *dp_link)
> 
>  	if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
>  		dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> -		return ret;
> -	}
> -
> -	ret = dp_link_process_ds_port_status_change(link);
> -	if (!ret) {
> +	} else if (!dp_link_process_ds_port_status_change(link)) {
>  		dp_link->sink_request |= DS_PORT_STATUS_CHANGED;
> -		return ret;
> -	}
> -
> -	ret = dp_link_process_link_training_request(link);
> -	if (!ret) {
> +	} else if (!dp_link_process_link_training_request(link)) {
>  		dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> -		return ret;
> -	}
> -
> -	ret = dp_link_process_phy_test_pattern_request(link);
> -	if (!ret) {
> +	} else if (!dp_link_process_phy_test_pattern_request(link)) {
>  		dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> -		return ret;
> -	}
> -
> -	ret = dp_link_process_link_status_update(link);
> -	if (!ret) {
> -		dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
> -		return ret;
> -	}
> -
> -	if (dp_link_is_video_pattern_requested(link)) {
> -		ret = 0;
> -		dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
> -	}
> -
> -	if (dp_link_is_audio_pattern_requested(link)) {
> -		dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
> -		return -EINVAL;
> +	} else {
> +		ret = dp_link_process_link_status_update(link);
> +		if (!ret) {
> +			dp_link->sink_request |= DP_LINK_STATUS_UPDATED;
> +		} else {
> +			if (dp_link_is_video_pattern_requested(link)) {
> +				ret = 0;
> +				dp_link->sink_request |= DP_TEST_LINK_VIDEO_PATTERN;
> +			}
> +
> +			if (dp_link_is_audio_pattern_requested(link)) {
> +				dp_link->sink_request |= DP_TEST_LINK_AUDIO_PATTERN;
> +				ret = -EINVAL;
> +			}
> +		}
>  	}
> 
>  	return ret;
I do agree, this will solve the issue for over writing ret variable. But 
I can see two potential problems with this, if we get two events at the 
same time or one of the other we might end up processing the two events, 
which won't be the expected behavior. For example, if we get 
dp_link_process_link_status_update and it gives a non zero result and we 
go to the else statement, and get the event for video pattern request, 
we will  end up processing two events. The second problem, is currently 
we only have APIs that return 0 or EINVAL as return values, but if we 
ever in future add an API with a different return value it wont be easy 
to add the behavior. But if these issues seem like they can be ignored , 
then we can go ahead with your suggestions.