Re: [PATCH 11/12] drm/panic: Move panic display code into helper library

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Missing fallback stubs for `drm_panic_helper_init` and `drm_panic_helper_exit` cause link failure when `CONFIG_DRM_PANIC_HELPER` is disabled.
- [Low] `DRM_KMS_HELPER` selects `DRM_PANIC_HELPER` without satisfying its `DRM_TTM` dependency.
--

--- Patch [11]: [PATCH 11/12] drm/panic: Move panic display code into helper library ---
commit 3d718c4f09d4c3ad0b378e7ca17f9b9b1d4d1627
Author: Thomas Zimmermann <[email protected]>

drm/panic: Move panic display code into helper library
    
This commit extracts panic display functionality from drm_panic.c into a
new drm_panic_helper.c file. The KMS helper module is updated to link
this new helper object, and relevant Kconfig and Makefile entries are
added.

> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 323422861e8f6..cdfd72d9a9c5e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -45,6 +45,7 @@ config DRM_MIPI_DSI
>  config DRM_KMS_HELPER
>  	tristate
>  	depends on DRM
> +	select DRM_PANIC_HELPER if DRM_PANIC
>  	select FB_CORE if DRM_FBDEV_EMULATION
>  	help

[ ... ]

> @@ -130,6 +131,10 @@ config DRM_PANIC_SCREEN_QR_VERSION
>  	  Smaller QR code are easier to read, but will contain less debugging
>  	  data. Default is 40.
>  
> +config DRM_PANIC_HELPER
> +	bool
> +	depends on DRM_TTM

[Severity: Low]
Will this cause an unmet direct dependency warning in Kconfig?

DRM_KMS_HELPER selects DRM_PANIC_HELPER, but it doesn't inherit or ensure 
the DRM_TTM dependency is met. If a kernel is configured with DRM_KMS_HELPER
and DRM_PANIC enabled, but without DRM_TTM, it might lead to Kconfig warnings
or a build failure.

> diff --git a/drivers/gpu/drm/drm_kms_helper_common.c b/drivers/gpu/drm/drm_kms_helper_common.c
> index 0c7550c0462b5..b5d5b469b4443 100644
> --- a/drivers/gpu/drm/drm_kms_helper_common.c
> +++ b/drivers/gpu/drm/drm_kms_helper_common.c
> @@ -27,6 +27,20 @@
>  
>  #include <linux/module.h>
>  
> +#include "drm_panic_internal.h"
> +
> +static int __init drm_kms_helper_init(void)
> +{
> +	return drm_panic_helper_init();
> +}
> +module_init(drm_kms_helper_init);

[ ... ]

> diff --git a/drivers/gpu/drm/drm_panic_internal.h b/drivers/gpu/drm/drm_panic_internal.h
> index c68d64c56e156..8aa91e80eb4dc 100644
> --- a/drivers/gpu/drm/drm_panic_internal.h
> +++ b/drivers/gpu/drm/drm_panic_internal.h
> @@ -52,4 +52,8 @@ static inline void drm_panic_lock(struct drm_device *dev, unsigned long flags) {
>  static inline void drm_panic_unlock(struct drm_device *dev, unsigned long flags) {}
>  #endif
>  
> +/* drm_panic_helper.c */
> +int drm_panic_helper_init(void);
> +void drm_panic_helper_exit(void);
> +

[Severity: Low]
Does this cause a link failure when CONFIG_DRM_PANIC_HELPER is disabled?

The calls to drm_panic_helper_init() and drm_panic_helper_exit() in
drm_kms_helper_common.c are unconditional. Without fallback static inline
stubs in drm_panic_internal.h when the feature is disabled, it seems this
will lead to unresolved external references during the linking phase.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11