Re: [PATCH v5 3/3] soc: qcom: ubwc: Get HBB from SMEM

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The Kconfig option `QCOM_UBWC_CONFIG` lacks a dependency on `QCOM_SMEM`, leading to linker errors when `QCOM_SMEM` is disabled or built as a module.
- [High] Unconditional probe deferral if SMEM is absent or fails to probe, breaking device initialization (such as display and GPU).
--

--- Patch [3]: [PATCH v5 3/3] soc: qcom: ubwc: Get HBB from SMEM ---
commit 20edaba770542f67d21a0b4400573bdcc45e195f
Author: Konrad Dybcio <[email protected]>

soc: qcom: ubwc: Get HBB from SMEM

This patch attempts to retrieve Highest Bank Bit (HBB) data from SMEM to
ensure correct DRAM configuration settings are applied. It updates the
UBWC configuration retrieval logic to first check for SMEM availability
and dynamically populate the HBB value if present.

> diff --git a/drivers/soc/qcom/ubwc_config.c b/drivers/soc/qcom/ubwc_config.c
> index 0209a02c390f0..23901a4c51f73 100644
> --- a/drivers/soc/qcom/ubwc_config.c
> +++ b/drivers/soc/qcom/ubwc_config.c
[ ... ]
> @@ -168,9 +169,25 @@ static const struct of_device_id qcom_ubwc_configs[] __maybe_unused = {
>  	{ }
>  };
>  
> +static struct qcom_ubwc_cfg_data *cfg;
> +static DEFINE_MUTEX(cfg_mutex);
> +/**
> + * qcom_ubwc_config_get_data() - Retrieve UBWC data for the platform.
> + *
> + * Return: Pointer to valid struct qcom_ubwc_cfg_data on success, negative
> + * errno on failure. Note that this may return EPROBE_DEFER.
> + */
>  const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>  {
>  	const struct qcom_ubwc_cfg_data *data;
> +	int hbb;
> +
> +	guard(mutex)(&cfg_mutex);
> +	if (cfg)
> +		return cfg;
> +
> +	if (!qcom_smem_is_available())
> +		return ERR_PTR(-EPROBE_DEFER);

[Severity: High]
If SMEM is absent on a platform or fails to probe (for example, if the
bootloader didn't initialize it and sets __smem to ERR_PTR(-ENODEV)),
qcom_smem_is_available() will always return false.

Will this cause dependent drivers to endlessly defer probe, completely
bypassing the valid static fallback data in qcom_ubwc_configs?

[Severity: High]
Since this patch introduces calls to qcom_smem_is_available() and
qcom_smem_dram_get_hbb(), which lack static inline stubs for the
!CONFIG_QCOM_SMEM case, could this cause a build failure?

For example, if a kernel is built with CONFIG_DRM_MSM=y (which selects
QCOM_UBWC_CONFIG) and CONFIG_QCOM_SMEM=m (or =n), will this result in
undefined reference errors during the vmlinux linking phase because
QCOM_UBWC_CONFIG lacks a dependency on QCOM_SMEM in drivers/soc/qcom/Kconfig?

>  
>  	data = of_machine_get_match_data(qcom_ubwc_configs);
>  	if (!data) {
> @@ -178,9 +195,30 @@ const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	return data;
> +	hbb = qcom_smem_dram_get_hbb();
> +	if (hbb == -ENODATA) {
[ ... ]

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