Quoting maitreye@xxxxxxxxxxxxxx (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@xxxxxxxxxxxxxx> > >> > >> 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@xxxxxxxxxxxxxx> > >> --- > >> 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;