linux-kernel - Re: [PATCH 2/3] drm/msm/dp: Clean up logs dp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3a5d2a34-bdc3-ac64-f69a-c7d99ff2f275@linaro.org>
Date:   Sat, 20 May 2023 04:20:20 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Bjorn Andersson <quic_bjorande@...cinc.com>,
        Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Kuogee Hsieh <quic_khsieh@...cinc.com>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Polimera <quic_vpolimer@...cinc.com>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH 2/3] drm/msm/dp: Clean up logs dp_power module

On 15/05/2023 06:02, Bjorn Andersson wrote:
> The clk_bulk API already provides error messages indicating which
> specific clock in the request for which the operation failed, further
> more these errors are associated with the specific DisplayPort
> controller (rather than the shared drm_device). The additional error
> messages int he dp_power module does thereby not provide any benefit.
> 
> While at it, none of the dp_power handles passed to these functions are
> dynamic in nature, so there should not be any need for runtime checking
> them. Drop these as well.

It might have been slightly better to have this split into two patches, 
but as this is a debug/logs only, it's fine with me.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@...cinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_power.c | 62 +++++--------------------------
>   1 file changed, 9 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index c0aaabb03389..031d2eefef07 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -35,25 +35,16 @@ static int dp_power_clk_init(struct dp_power_private *power)
>   	stream = &power->parser->mp[DP_STREAM_PM];
>   
>   	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CORE_PM), rc);
> +	if (rc)
>   		return rc;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	return 0;
>   }
> @@ -121,11 +112,9 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   			mp = &power->parser->mp[DP_CORE_PM];
>   
>   			rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -			if (rc) {
> -				DRM_ERROR("fail to enable clks: %s. err=%d\n",
> -					dp_parser_pm_name(DP_CORE_PM), rc);
> +			if (rc)
>   				return rc;
> -			}
> +
>   			dp_power->core_clks_on = true;
>   		}
>   	}
> @@ -133,10 +122,8 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   	mp = &power->parser->mp[pm_type];
>   	if (enable) {
>   		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -		if (rc) {
> -			DRM_ERROR("failed to enable clks, err: %d\n", rc);
> +		if (rc)
>   			return rc;
> -		}
>   	} else {
>   		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
>   	}
> @@ -162,34 +149,19 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   
>   int dp_power_client_init(struct dp_power *dp_power)
>   {
> -	int rc = 0;
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_enable(&power->pdev->dev);
>   
> -	rc = dp_power_clk_init(power);
> -	if (rc)
> -		DRM_ERROR("failed to init clocks %d\n", rc);
> -
> -	return rc;
> +	return dp_power_clk_init(power);
>   }
>   
>   void dp_power_client_deinit(struct dp_power *dp_power)
>   {
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_disable(&power->pdev->dev);
> @@ -200,25 +172,14 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
>   	int rc = 0;
>   	struct dp_power_private *power = NULL;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_get_sync(&power->pdev->dev);
>   
>   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> -	if (rc) {
> -		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> -		goto exit;
> -	}
> -
> -	return 0;
> +	if (rc)
> +		pm_runtime_put_sync(&power->pdev->dev);
>   
> -exit:
> -	pm_runtime_put_sync(&power->pdev->dev);
>   	return rc;
>   }
>   
> @@ -238,11 +199,6 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
>   	struct dp_power_private *power;
>   	struct dp_power *dp_power;
>   
> -	if (!parser) {
> -		DRM_ERROR("invalid input\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>   	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
>   	if (!power)
>   		return ERR_PTR(-ENOMEM);

-- 
With best wishes
Dmitry