linux-kernel - Re: [PATCH v2 2/2] drm/rockchip: lvds: Hide scary error messages on probe deferral

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] [day] [month] [year] [list]

Message-ID: <9817880.CDJkKcVGEf@diego>
Date: Tue, 04 Mar 2025 13:38:02 +0100
From: Heiko Stübner <heiko@...ech.de>
To: Quentin Schulz <quentin.schulz@...rry.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

Am Dienstag, 4. März 2025, 12:46:59 MEZ schrieb Quentin Schulz:
> > @@ -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?

I did debate a bit whether to ignore here and go directly to dev_warn,
but opted for DRM_DEV_ERROR -> dev_err -> dev_warn, to keep the commit
description intact. Otherwise people looking at this patch alone might ask
if this part was forgotten, when the commit message indicates "all".

So expect an additional patch in v3 :-) .


> > @@ -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);

you're right of course

...

> >   	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?

I would group this more to general device error.
Component_add happens way before the component master binds anything.

drm_err is supposed to also point to the main drm_device as its parameter,
which does not even exist at this point.


Heiko