| Message ID | 20210527090421.9172-1-thunder.leizhen@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/1] drm/i915/hdcp: Simplify code in intel_hdcp_auth_downstream() | expand |
On Thu, 27 May 2021, Zhen Lei <thunder.leizhen@huawei.com> wrote: > If intel_hdcp_validate_v_prime() has been successful within the allowed > number of tries, we can directly call drm_dbg_kms() and "goto out" without > jumping out of the loop and repeatedly judging whether the operation is > successful. This can help us reduce an unnecessary if judgment. And it's > a little clearer to read. Generally I think the "happy day scenario" should be at the topmost indentation level and not buried in the ifs with a goto exit. BR, Jani. > > No functional change. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > drivers/gpu/drm/i915/display/intel_hdcp.c | 24 ++++++++++------------- > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c > index d8570e14fe60..c32a854eda66 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdcp.c > +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c > @@ -663,13 +663,13 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) > > ret = shim->read_ksv_fifo(dig_port, num_downstream, ksv_fifo); > if (ret) > - goto err; > + goto out; > > if (drm_hdcp_check_ksvs_revoked(&dev_priv->drm, ksv_fifo, > num_downstream) > 0) { > drm_err(&dev_priv->drm, "Revoked Ksv(s) in ksv_fifo\n"); > ret = -EPERM; > - goto err; > + goto out; > } > > /* > @@ -680,20 +680,16 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) > ret = intel_hdcp_validate_v_prime(connector, shim, > ksv_fifo, num_downstream, > bstatus); > - if (!ret) > - break; > - } > - > - if (i == tries) { > - drm_dbg_kms(&dev_priv->drm, > - "V Prime validation failed.(%d)\n", ret); > - goto err; > + if (!ret) { > + drm_dbg_kms(&dev_priv->drm, > + "HDCP is enabled (%d downstream devices)\n", > + num_downstream); > + goto out; > + } > } > > - drm_dbg_kms(&dev_priv->drm, "HDCP is enabled (%d downstream devices)\n", > - num_downstream); > - ret = 0; > -err: > + drm_dbg_kms(&dev_priv->drm, "V Prime validation failed.(%d)\n", ret); > +out: > kfree(ksv_fifo); > return ret; > } -- Jani Nikula, Intel Open Source Graphics Center
On 2021/5/27 18:04, Jani Nikula wrote: > On Thu, 27 May 2021, Zhen Lei <thunder.leizhen@huawei.com> wrote: >> If intel_hdcp_validate_v_prime() has been successful within the allowed >> number of tries, we can directly call drm_dbg_kms() and "goto out" without >> jumping out of the loop and repeatedly judging whether the operation is >> successful. This can help us reduce an unnecessary if judgment. And it's >> a little clearer to read. > > Generally I think the "happy day scenario" should be at the topmost > indentation level and not buried in the ifs with a goto exit. for (xxx) { if (a == b) return found; } At least this way of writing is common. > > BR, > Jani. > >> >> No functional change. >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> drivers/gpu/drm/i915/display/intel_hdcp.c | 24 ++++++++++------------- >> 1 file changed, 10 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c >> index d8570e14fe60..c32a854eda66 100644 >> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c >> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c >> @@ -663,13 +663,13 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) >> >> ret = shim->read_ksv_fifo(dig_port, num_downstream, ksv_fifo); >> if (ret) >> - goto err; >> + goto out; >> >> if (drm_hdcp_check_ksvs_revoked(&dev_priv->drm, ksv_fifo, >> num_downstream) > 0) { >> drm_err(&dev_priv->drm, "Revoked Ksv(s) in ksv_fifo\n"); >> ret = -EPERM; >> - goto err; >> + goto out; >> } >> >> /* >> @@ -680,20 +680,16 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) >> ret = intel_hdcp_validate_v_prime(connector, shim, >> ksv_fifo, num_downstream, >> bstatus); >> - if (!ret) >> - break; >> - } >> - >> - if (i == tries) { >> - drm_dbg_kms(&dev_priv->drm, >> - "V Prime validation failed.(%d)\n", ret); >> - goto err; >> + if (!ret) { >> + drm_dbg_kms(&dev_priv->drm, >> + "HDCP is enabled (%d downstream devices)\n", >> + num_downstream); >> + goto out; >> + } >> } >> >> - drm_dbg_kms(&dev_priv->drm, "HDCP is enabled (%d downstream devices)\n", >> - num_downstream); >> - ret = 0; >> -err: >> + drm_dbg_kms(&dev_priv->drm, "V Prime validation failed.(%d)\n", ret); >> +out: >> kfree(ksv_fifo); >> return ret; >> } >
On Fri, 28 May 2021, "Leizhen (ThunderTown)" <thunder.leizhen@huawei.com> wrote: > On 2021/5/27 18:04, Jani Nikula wrote: >> On Thu, 27 May 2021, Zhen Lei <thunder.leizhen@huawei.com> wrote: >>> If intel_hdcp_validate_v_prime() has been successful within the allowed >>> number of tries, we can directly call drm_dbg_kms() and "goto out" without >>> jumping out of the loop and repeatedly judging whether the operation is >>> successful. This can help us reduce an unnecessary if judgment. And it's >>> a little clearer to read. >> >> Generally I think the "happy day scenario" should be at the topmost >> indentation level and not buried in the ifs with a goto exit. > > for (xxx) { > if (a == b) > return found; > } > > At least this way of writing is common. Yes, if the loop is abstracted to a separate function. BR, Jani. > > >> >> BR, >> Jani. >> >>> >>> No functional change. >>> >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_hdcp.c | 24 ++++++++++------------- >>> 1 file changed, 10 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c >>> index d8570e14fe60..c32a854eda66 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_hdcp.c >>> +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c >>> @@ -663,13 +663,13 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) >>> >>> ret = shim->read_ksv_fifo(dig_port, num_downstream, ksv_fifo); >>> if (ret) >>> - goto err; >>> + goto out; >>> >>> if (drm_hdcp_check_ksvs_revoked(&dev_priv->drm, ksv_fifo, >>> num_downstream) > 0) { >>> drm_err(&dev_priv->drm, "Revoked Ksv(s) in ksv_fifo\n"); >>> ret = -EPERM; >>> - goto err; >>> + goto out; >>> } >>> >>> /* >>> @@ -680,20 +680,16 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) >>> ret = intel_hdcp_validate_v_prime(connector, shim, >>> ksv_fifo, num_downstream, >>> bstatus); >>> - if (!ret) >>> - break; >>> - } >>> - >>> - if (i == tries) { >>> - drm_dbg_kms(&dev_priv->drm, >>> - "V Prime validation failed.(%d)\n", ret); >>> - goto err; >>> + if (!ret) { >>> + drm_dbg_kms(&dev_priv->drm, >>> + "HDCP is enabled (%d downstream devices)\n", >>> + num_downstream); >>> + goto out; >>> + } >>> } >>> >>> - drm_dbg_kms(&dev_priv->drm, "HDCP is enabled (%d downstream devices)\n", >>> - num_downstream); >>> - ret = 0; >>> -err: >>> + drm_dbg_kms(&dev_priv->drm, "V Prime validation failed.(%d)\n", ret); >>> +out: >>> kfree(ksv_fifo); >>> return ret; >>> } >> > -- Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index d8570e14fe60..c32a854eda66 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -663,13 +663,13 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) ret = shim->read_ksv_fifo(dig_port, num_downstream, ksv_fifo); if (ret) - goto err; + goto out; if (drm_hdcp_check_ksvs_revoked(&dev_priv->drm, ksv_fifo, num_downstream) > 0) { drm_err(&dev_priv->drm, "Revoked Ksv(s) in ksv_fifo\n"); ret = -EPERM; - goto err; + goto out; } /* @@ -680,20 +680,16 @@ int intel_hdcp_auth_downstream(struct intel_connector *connector) ret = intel_hdcp_validate_v_prime(connector, shim, ksv_fifo, num_downstream, bstatus); - if (!ret) - break; - } - - if (i == tries) { - drm_dbg_kms(&dev_priv->drm, - "V Prime validation failed.(%d)\n", ret); - goto err; + if (!ret) { + drm_dbg_kms(&dev_priv->drm, + "HDCP is enabled (%d downstream devices)\n", + num_downstream); + goto out; + } } - drm_dbg_kms(&dev_priv->drm, "HDCP is enabled (%d downstream devices)\n", - num_downstream); - ret = 0; -err: + drm_dbg_kms(&dev_priv->drm, "V Prime validation failed.(%d)\n", ret); +out: kfree(ksv_fifo); return ret; }
If intel_hdcp_validate_v_prime() has been successful within the allowed number of tries, we can directly call drm_dbg_kms() and "goto out" without jumping out of the loop and repeatedly judging whether the operation is successful. This can help us reduce an unnecessary if judgment. And it's a little clearer to read. No functional change. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- drivers/gpu/drm/i915/display/intel_hdcp.c | 24 ++++++++++------------- 1 file changed, 10 insertions(+), 14 deletions(-) -- 2.25.1