| 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
| ||
|
Message-ID: <0e54f70a-0b07-4ead-96fb-add2bbdaf787@cherry.de>
Date: Tue, 4 Mar 2025 12:46:59 +0100
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Heiko Stuebner <heiko@...ech.de>
Cc: andy.yan@...k-chips.com, maarten.lankhorst@...ux.intel.com,
mripard@...nel.org, tzimmermann@...e.de, dri-devel@...ts.freedesktop.org,
linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
linux-kernel@...r.kernel.org, Heiko Stuebner <heiko.stuebner@...rry.de>
Subject: Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on
probe deferral
Hi Heiko,
On 3/1/25 6:35 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@...rry.de>
>
> Commit 52d11c863ac9 ("drm/rockchip: lvds: do not print scary message when
> probing defer") already started hiding scary messages that are not relevant
> if the requested supply just returned EPROBE_DEFER, but there are more
> possible sources - like the phy.
>
> So modernize the whole logging in the probe path by replacing the
> remaining deprecated DRM_DEV_ERROR with appropriate dev_err(_probe)
> and drm_err calls.
>
> The distinction here is that all messages talking about mishaps of the
> lvds element use dev_err(_probe) while messages caused by interaction
> with the main Rockchip drm-device use drm_err.
>
> Signed-off-by: Heiko Stuebner <heiko.stuebner@...rry.de>
> ---
> drivers/gpu/drm/rockchip/rockchip_lvds.c | 61 ++++++++++--------------
> 1 file changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index ecfae8d5da89..d8b2007123fa 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -453,10 +453,9 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
> return PTR_ERR(lvds->regs);
>
> lvds->pclk = devm_clk_get_prepared(lvds->dev, "pclk_lvds");
> - if (IS_ERR(lvds->pclk)) {
> - DRM_DEV_ERROR(lvds->dev, "could not get or prepare pclk_lvds\n");
> - return PTR_ERR(lvds->pclk);
> - }
> + if (IS_ERR(lvds->pclk))
> + return dev_err_probe(lvds->dev, PTR_ERR(lvds->pclk),
> + "could not get or prepare pclk_lvds\n");
>
> lvds->pins = devm_kzalloc(lvds->dev, sizeof(*lvds->pins),
> GFP_KERNEL);
> @@ -465,14 +464,14 @@ static int rk3288_lvds_probe(struct platform_device *pdev,
>
> lvds->pins->p = devm_pinctrl_get(lvds->dev);
> if (IS_ERR(lvds->pins->p)) {
> - DRM_DEV_ERROR(lvds->dev, "no pinctrl handle\n");
> + dev_err(lvds->dev, "no pinctrl handle\n");
> devm_kfree(lvds->dev, lvds->pins);
> lvds->pins = NULL;
> } else {
> lvds->pins->default_state =
> pinctrl_lookup_state(lvds->pins->p, "lcdc");
> if (IS_ERR(lvds->pins->default_state)) {
> - DRM_DEV_ERROR(lvds->dev, "no default pinctrl state\n");
> + dev_err(lvds->dev, "no default pinctrl state\n");
> devm_kfree(lvds->dev, lvds->pins);
> lvds->pins = NULL;
Should those be dev_err if they are not breaking anything? After all,
the device will actually probe? Maybe dev_warn would be more appropriate?
Maybe a separate patch since we had DRM_DEV_ERROR already, so switching
to dev_err in one and then lowering the log level in a second would make
"more" sense?
> }
> @@ -547,11 +546,10 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>
> lvds->drm_dev = drm_dev;
> port = of_graph_get_port_by_id(dev->of_node, 1);
> - if (!port) {
> - DRM_DEV_ERROR(dev,
> - "can't found port point, please init lvds panel port!\n");
> - return -EINVAL;
> - }
> + if (!port)
> + return dev_err_probe(dev, -EINVAL,
> + "can't found port point, please init lvds panel port!\n");
> +
> for_each_child_of_node(port, endpoint) {
> child_count++;
> of_property_read_u32(endpoint, "reg", &endpoint_id);
> @@ -563,8 +561,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> }
> }
> if (!child_count) {
> - DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> - ret = -EINVAL;
> + ret = dev_err_probe(dev, -EINVAL, "lvds port does not have any children\n");
> goto err_put_port;
> } else if (ret) {
> dev_err_probe(dev, ret, "failed to find panel and bridge node\n");
> @@ -581,8 +578,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> lvds->output = rockchip_lvds_name_to_output(name);
>
> if (lvds->output < 0) {
> - DRM_DEV_ERROR(dev, "invalid output type [%s]\n", name);
> - ret = lvds->output;
> + ret = dev_err_probe(dev, lvds->output, "invalid output type [%s]\n", name);
> goto err_put_remote;
> }
>
> @@ -593,7 +589,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> lvds->format = rockchip_lvds_name_to_format(name);
>
> if (lvds->format < 0) {
> - DRM_DEV_ERROR(dev, "invalid data-mapping format [%s]\n", name);
> + dev_err(dev, "invalid data-mapping format [%s]\n", name);
> ret = lvds->format;
nipitck:
ret = dev_err_probe(dev, lvds->format, "invalid data-mapping format
[%s]\n", name);
> goto err_put_remote;
> }
> @@ -604,8 +600,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>
> ret = drm_simple_encoder_init(drm_dev, encoder, DRM_MODE_ENCODER_LVDS);
> if (ret < 0) {
> - DRM_DEV_ERROR(drm_dev->dev,
> - "failed to initialize encoder: %d\n", ret);
> + drm_err(drm_dev,
> + "failed to initialize encoder: %d\n", ret);
> goto err_put_remote;
> }
>
> @@ -618,8 +614,8 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> &rockchip_lvds_connector_funcs,
> DRM_MODE_CONNECTOR_LVDS);
> if (ret < 0) {
> - DRM_DEV_ERROR(drm_dev->dev,
> - "failed to initialize connector: %d\n", ret);
> + drm_err(drm_dev,
> + "failed to initialize connector: %d\n", ret);
> goto err_free_encoder;
> }
>
> @@ -633,9 +629,9 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>
> connector = drm_bridge_connector_init(lvds->drm_dev, encoder);
> if (IS_ERR(connector)) {
> - DRM_DEV_ERROR(drm_dev->dev,
> - "failed to initialize bridge connector: %pe\n",
> - connector);
> + drm_err(drm_dev,
> + "failed to initialize bridge connector: %pe\n",
> + connector);
> ret = PTR_ERR(connector);
> goto err_free_encoder;
> }
> @@ -643,8 +639,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>
> ret = drm_connector_attach_encoder(connector, encoder);
> if (ret < 0) {
> - DRM_DEV_ERROR(drm_dev->dev,
> - "failed to attach encoder: %d\n", ret);
> + drm_err(drm_dev, "failed to attach encoder: %d\n", ret);
> goto err_free_connector;
> }
>
> @@ -706,24 +701,20 @@ static int rockchip_lvds_probe(struct platform_device *pdev)
>
> lvds->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
> "rockchip,grf");
> - if (IS_ERR(lvds->grf)) {
> - DRM_DEV_ERROR(dev, "missing rockchip,grf property\n");
> - return PTR_ERR(lvds->grf);
> - }
> + if (IS_ERR(lvds->grf))
> + return dev_err_probe(dev, PTR_ERR(lvds->grf), "missing rockchip,grf property\n");
>
> ret = lvds->soc_data->probe(pdev, lvds);
> - if (ret) {
> - DRM_DEV_ERROR(dev, "Platform initialization failed\n");
> - return ret;
> - }
> + if (ret)
> + return dev_err_probe(dev, ret, "Platform initialization failed\n");
>
> dev_set_drvdata(dev, lvds);
>
> ret = component_add(&pdev->dev, &rockchip_lvds_component_ops);
> if (ret < 0)
> - DRM_DEV_ERROR(dev, "failed to add component\n");
> + return dev_err_probe(dev, ret, "failed to add component\n");
>
Should this rather be drm_error? I believe this is related to the
Rockchip DRM main device?
Otherwise looks good to me,
Cheers,
Quentin
Powered by blists - more mailing lists