linux-kernel - Re: [PATCH v2 0/3] drm/komeda: Convert logging to drm

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: <aTmkOCE5M7fJeuPo@e142607>
Date: Wed, 10 Dec 2025 16:47:52 +0000
From: Liviu Dudau <liviu.dudau@....com>
To: Rahul Kumar <rk0006818@...il.com>
Cc: maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/3] drm/komeda: Convert logging to drm_* helpers

Hi Rahul,

Appologies for the amount of time it took me to get to review the series,
I hope this is a one-off instance.

On Tue, Nov 18, 2025 at 04:29:31PM +0530, Rahul Kumar wrote:
> This series converts Komeda logging from the legacy DRM_ERROR/WARN/INFO()
> and DRM_DEBUG() macros to the modern drm_*() helpers. The drm_*() helpers
> take a struct drm_device * and allow the DRM core to include device
> information in the log output. This improves readability and brings the
> driver in line with current DRM logging guidelines.
> 
> To support this conversion, a small Komeda internal header
> (komeda_drv.h) is introduced to provide access to struct komeda_drv
> where needed. No functional changes are intended.

Here are some comments for the general direction of the patches that can
help you improve the series.

First, you don't need to expose the structure komeda_drv, as that is meant
to be internal to komeda_drv.c. What you can do is take inspiration from the
function following the definition of the structure and add a new accessor,
something like this:

--8<------------------------
+struct komeda_kms_dev *dev_to_kmsdev(struct device *dev)
+{
+	struct komeda_drv *mdrv = dev_get_drvdata(dev);
+
+	return mdrv ? mdrv->kms : NULL;
+}
-->8------------------------

Then you can reduce the boilerplate code that you have spread through your
patches to just calling this function to get the komeda_kms_dev. Alternatively,
you can make the function to return a struct drm_device *.

If you read through the code and try to understand its structure you will
realise that there is a top-down organisation of concepts, from a drm_device
down to a komeda_dev that is trying to abstract the hardware differences
between D71 and future Arm Display IP. But some functions, like komeda_pipeline_dump()
are only used in a limited scope, which means you could add the drm_device pointer
to the function signature because komeda_crtc_add() is the only caller of that
function and it already has drm_device pointer, so no point into going through hoops
to get it inside komeda_pipeline_dump().

Another thing that you can do with the series is to be comprehensive and
remove ALL calls to DRM_ERROR/WARN/INFO/DEBUG() where appropriate (leaving
the probe call path untouched as the drm_device is not valid yet).
komeda_pipeline_state.c, komeda_wb_connector.c and komeda_framebuffer.c
should at minimum be cleaned up as well.

There is a third thing that can be done, but that's not on you. komeda
driver is a bit too verbose with the error and debug messages and I could
just go and remove them as they don't add too much information unless
you're doing bring up on a new platform.

Best regards,
Liviu

> 
> Changes in v2:
> - Corrected the use of dev_get_drvdata(); it returns struct komeda_drv *,
>   not struct komeda_kms_dev *.
> - Added komeda_drv.h to make struct komeda_drv available for logging
>   conversion.
> - Split the series into 3 small patches as requested.
> 
> Rahul Kumar (3):
>   drm/komeda: Add komeda_drv.h to make struct komeda_drv available
>   drm/komeda: Convert logging in komeda_pipeline.c to drm_* with
>   drm_device parameter
>   drm/komeda: Convert logging in d71_dev.c to drm_* with drm_device
>   parameter
> 
>  .../gpu/drm/arm/display/komeda/d71/d71_dev.c  | 24 +++++---
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  6 +-
>  .../gpu/drm/arm/display/komeda/komeda_drv.h   | 24 ++++++++
>  .../drm/arm/display/komeda/komeda_pipeline.c  | 61 +++++++++++++------
>  4 files changed, 84 insertions(+), 31 deletions(-)
>  create mode 100644 drivers/gpu/drm/arm/display/komeda/komeda_drv.h
> 
> -- 
> 2.43.0
>