No longer produce error messages when dphy is deferred

[PATCH 2/2] drm/rockchip: dsi: Don't log errors on deferred dphy

Posted by Dragan Simic 2 weeks, 1 day ago

Deferred driver probing shouldn't result in errors or warnings being logged,
because their presence in the kernel log provides no value and may actually
cause false impression that some issues exist.  Thus, let's no longer produce
error messages when getting the dphy results in deferred probing.

This prevents misleading error messages like the following one, which was
observed on a Pine64 PineTab2, from appearing in the kernel log.  To make
matters worse, the following error message was observed appearing multiple
times in the kernel log of a single PineTab2 boot:

  dw-mipi-dsi-rockchip fe060000.dsi: [drm:dw_mipi_dsi_rockchip_probe \
  [rockchipdrm]] *ERROR* failed to get mipi dphy: -517

At the same time, make the adjusted logged message a bit more consistent with
the other logged messages by capitalizing its first word.

Reported-by: Diederik de Haas <didi.debian@cknow.org>
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
 drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
index f451e70efbdd..ffa7f2bc640d 100644
--- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
@@ -1387,7 +1387,8 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
 	dsi->phy = devm_phy_optional_get(dev, "dphy");
 	if (IS_ERR(dsi->phy)) {
 		ret = PTR_ERR(dsi->phy);
-		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "Failed to get mipi dphy: %d\n", ret);
 		return ret;
 	}

Re: [PATCH 2/2] drm/rockchip: dsi: Don't log errors on deferred dphy

Posted by Sebastian Reichel 2 weeks, 1 day ago

Hi,

On Fri, Nov 08, 2024 at 02:53:58PM +0100, Dragan Simic wrote:
> Deferred driver probing shouldn't result in errors or warnings being logged,
> because their presence in the kernel log provides no value and may actually
> cause false impression that some issues exist.  Thus, let's no longer produce
> error messages when getting the dphy results in deferred probing.
> 
> This prevents misleading error messages like the following one, which was
> observed on a Pine64 PineTab2, from appearing in the kernel log.  To make
> matters worse, the following error message was observed appearing multiple
> times in the kernel log of a single PineTab2 boot:
> 
>   dw-mipi-dsi-rockchip fe060000.dsi: [drm:dw_mipi_dsi_rockchip_probe \
>   [rockchipdrm]] *ERROR* failed to get mipi dphy: -517
> 
> At the same time, make the adjusted logged message a bit more consistent with
> the other logged messages by capitalizing its first word.
> 
> Reported-by: Diederik de Haas <didi.debian@cknow.org>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---

From include/drm/drm_print.h:

 * DRM_DEV_ERROR() - Error output.
 *
 * NOTE: this is deprecated in favor of drm_err() or dev_err().

The recommended way to do this nowadays looks like this:

return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi dphy");

That will not print anything for -EPROBE_DEFER, but capture
the reason and make it available through
/sys/kernel/debug/devices_deferred if the device never probes.

Greetings,

-- Sebastian

>  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> index f451e70efbdd..ffa7f2bc640d 100644
> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> @@ -1387,7 +1387,8 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
>  	dsi->phy = devm_phy_optional_get(dev, "dphy");
>  	if (IS_ERR(dsi->phy)) {
>  		ret = PTR_ERR(dsi->phy);
> -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "Failed to get mipi dphy: %d\n", ret);
>  		return ret;
>  	}
>

Re: [PATCH 2/2] drm/rockchip: dsi: Don't log errors on deferred dphy

Posted by Dragan Simic 2 weeks, 1 day ago

Hello Sebastian,

On 2024-11-08 15:08, Sebastian Reichel wrote:
> On Fri, Nov 08, 2024 at 02:53:58PM +0100, Dragan Simic wrote:
>> Deferred driver probing shouldn't result in errors or warnings being 
>> logged,
>> because their presence in the kernel log provides no value and may 
>> actually
>> cause false impression that some issues exist.  Thus, let's no longer 
>> produce
>> error messages when getting the dphy results in deferred probing.
>> 
>> This prevents misleading error messages like the following one, which 
>> was
>> observed on a Pine64 PineTab2, from appearing in the kernel log.  To 
>> make
>> matters worse, the following error message was observed appearing 
>> multiple
>> times in the kernel log of a single PineTab2 boot:
>> 
>>   dw-mipi-dsi-rockchip fe060000.dsi: [drm:dw_mipi_dsi_rockchip_probe \
>>   [rockchipdrm]] *ERROR* failed to get mipi dphy: -517
>> 
>> At the same time, make the adjusted logged message a bit more 
>> consistent with
>> the other logged messages by capitalizing its first word.
>> 
>> Reported-by: Diederik de Haas <didi.debian@cknow.org>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
> 
> From include/drm/drm_print.h:
> 
>  * DRM_DEV_ERROR() - Error output.
>  *
>  * NOTE: this is deprecated in favor of drm_err() or dev_err().
> 
> The recommended way to do this nowadays looks like this:
> 
> return dev_err_probe(dev, PTR_ERR(dsi->phy), "Failed to get mipi 
> dphy");
> 
> That will not print anything for -EPROBE_DEFER, but capture
> the reason and make it available through
> /sys/kernel/debug/devices_deferred if the device never probes.

Thanks for your quick response!  As already discussed with Heiko,
I'll move forward with implementing a complete file-level conversion.
At first, I thought that a partial bugfix would be beneficial, [1]
but now I agree that performing a complete file-level coversion is
the way to go. [2]

I've got to admit, I love seeing that DRM_DEV_ERROR() is deprecated,
because I've never been a big fan of the format of the messages
it produces.

[1] 
https://lore.kernel.org/dri-devel/3734f6a5424e3537d717c587a058fc85@manjaro.org/
[2] 
https://lore.kernel.org/dri-devel/047164cc6e88dcbc7701cb0e28d564db@manjaro.org/

>>  drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c 
>> b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> index f451e70efbdd..ffa7f2bc640d 100644
>> --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
>> @@ -1387,7 +1387,8 @@ static int dw_mipi_dsi_rockchip_probe(struct 
>> platform_device *pdev)
>>  	dsi->phy = devm_phy_optional_get(dev, "dphy");
>>  	if (IS_ERR(dsi->phy)) {
>>  		ret = PTR_ERR(dsi->phy);
>> -		DRM_DEV_ERROR(dev, "failed to get mipi dphy: %d\n", ret);
>> +		if (ret != -EPROBE_DEFER)
>> +			DRM_DEV_ERROR(dev, "Failed to get mipi dphy: %d\n", ret);
>>  		return ret;
>>  	}
>>