Re: [PATCH v4 1/3] soc: qcom: ubwc: Fix link error when QCOM_SMEM=n

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch adds `depends on QCOM_SMEM` to `QCOM_UBWC_CONFIG` but misses that `VIDEO_QCOM_IRIS` unconditionally selects it, causing an unmet dependency warning and leaving the original linker error unfixed.
- [High] The conditional select in `DRM_MSM` creates a configuration where a built-in driver calls a function from a module, resulting in a linker error.
--

--- Patch [1]: [PATCH v4 1/3] soc: qcom: ubwc: Fix link error when QCOM_SMEM=n ---
commit a54f8c4ddffc0b9c1fbde97cc8f98242588a42bf
Author: Daniel Baluta <[email protected]>

soc: qcom: ubwc: Fix link error when QCOM_SMEM=n

DRM_MSM unconditionally selects QCOM_UBWC_CONFIG, which calls SMEM APIs,
causing a link error on non-Qcom platforms (e.g. SOC_IMX5). Make
QCOM_UBWC_CONFIG depend on QCOM_SMEM to make the requirement explicit, and
guard the select in DRM_MSM with ARCH_QCOM && QCOM_SMEM.

> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index 09469d56513b0..1670dbc9464c6 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -22,7 +22,7 @@ config DRM_MSM
>  	select TMPFS
>  	select QCOM_SCM
>  	select QCOM_PAS
> -	select QCOM_UBWC_CONFIG
> +	select QCOM_UBWC_CONFIG if ARCH_QCOM && QCOM_SMEM

[Severity: High]
Can this conditional select create a new configuration where a built-in driver
fails to link against a module?

If we compile with CONFIG_DRM_MSM=y, CONFIG_ARCH_QCOM=y, and
CONFIG_QCOM_SMEM=m, the select evaluates to 'm', forcing
CONFIG_QCOM_UBWC_CONFIG=m.

Because IS_ENABLED(CONFIG_QCOM_UBWC_CONFIG) evaluates to true for both 'y'
and 'm', built-in code like msm_mdss_init() in drivers/gpu/drm/msm/msm_mdss.c
will attempt to call the external qcom_ubwc_config_get_data() function:

include/linux/soc/qcom/ubwc.h:
#if IS_ENABLED(CONFIG_QCOM_UBWC_CONFIG)
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void);
...

At link time, vmlinux will fail to resolve this function because it resides
in the ubwc_config module.

>  	select WANT_DEV_COREDUMP
>  	select SND_SOC_HDMI_CODEC if SND_SOC
>  	select SYNC_FILE

[ ... ]

> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index fd4d4ecd2df0f..e0629e9328c87 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -321,6 +321,7 @@ config QCOM_QMI_HELPERS
>  
>  config QCOM_UBWC_CONFIG
>  	tristate
> +	depends on QCOM_SMEM

[Severity: High]
Does this leave the linker error unfixed when CONFIG_VIDEO_QCOM_IRIS=y?

Looking at drivers/media/platform/qcom/iris/Kconfig, it unconditionally
selects QCOM_UBWC_CONFIG:

config VIDEO_QCOM_IRIS
    ...
    select QCOM_UBWC_CONFIG

If we compile with CONFIG_COMPILE_TEST=y, CONFIG_VIDEO_QCOM_IRIS=y, and
CONFIG_QCOM_SMEM=n, the unconditional select will force QCOM_UBWC_CONFIG to y
despite the new dependency.

This produces an unmet dependency warning and attempts to link ubwc_config.c,
which fails with the exact undefined reference to 'qcom_smem_*' APIs that
this patch intended to fix.

>  	help
>  	  Most Qualcomm SoCs feature a number of Universal Bandwidth Compression
>  	  (UBWC) engines across various IP blocks, which need to be initialized

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