From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
To make sure the correct settings for a given DRAM configuration get
applied, attempt to retrieve that data from SMEM (which happens to be
what the BSP kernel does, albeit with through convoluted means of the
bootloader altering the DT with this data).
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
I'm not sure about this approach - perhaps a global variable storing
the selected config, which would then be non-const would be better?
---
drivers/soc/qcom/ubwc_config.c | 69 ++++++++++++++++++++++++++----------------
1 file changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/soc/qcom/ubwc_config.c b/drivers/soc/qcom/ubwc_config.c
index 1c25aaf55e52..21bb444dc27c 100644
--- a/drivers/soc/qcom/ubwc_config.c
+++ b/drivers/soc/qcom/ubwc_config.c
@@ -11,12 +11,13 @@
#include <linux/platform_device.h>
#include <linux/soc/qcom/ubwc.h>
+#include <linux/soc/qcom/smem.h>
-static const struct qcom_ubwc_cfg_data no_ubwc_data = {
+static struct qcom_ubwc_cfg_data no_ubwc_data = {
/* no UBWC, no HBB */
};
-static const struct qcom_ubwc_cfg_data kaanapali_data = {
+static struct qcom_ubwc_cfg_data kaanapali_data = {
.ubwc_enc_version = UBWC_6_0,
.ubwc_dec_version = UBWC_6_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -26,7 +27,7 @@ static const struct qcom_ubwc_cfg_data kaanapali_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data msm8937_data = {
+static struct qcom_ubwc_cfg_data msm8937_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_1_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -35,7 +36,7 @@ static const struct qcom_ubwc_cfg_data msm8937_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data msm8998_data = {
+static struct qcom_ubwc_cfg_data msm8998_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_1_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -44,12 +45,12 @@ static const struct qcom_ubwc_cfg_data msm8998_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data qcm2290_data = {
+static struct qcom_ubwc_cfg_data qcm2290_data = {
/* no UBWC */
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sa8775p_data = {
+static struct qcom_ubwc_cfg_data sa8775p_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL3,
@@ -58,7 +59,7 @@ static const struct qcom_ubwc_cfg_data sa8775p_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sar2130p_data = {
+static struct qcom_ubwc_cfg_data sar2130p_data = {
.ubwc_enc_version = UBWC_3_0, /* 4.0.2 in hw */
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -68,7 +69,7 @@ static const struct qcom_ubwc_cfg_data sar2130p_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc7180_data = {
+static struct qcom_ubwc_cfg_data sc7180_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -77,7 +78,7 @@ static const struct qcom_ubwc_cfg_data sc7180_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sc7280_data = {
+static struct qcom_ubwc_cfg_data sc7280_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -87,7 +88,7 @@ static const struct qcom_ubwc_cfg_data sc7280_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc8180x_data = {
+static struct qcom_ubwc_cfg_data sc8180x_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -96,7 +97,7 @@ static const struct qcom_ubwc_cfg_data sc8180x_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sc8280xp_data = {
+static struct qcom_ubwc_cfg_data sc8280xp_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -106,7 +107,7 @@ static const struct qcom_ubwc_cfg_data sc8280xp_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sdm670_data = {
+static struct qcom_ubwc_cfg_data sdm670_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -114,7 +115,7 @@ static const struct qcom_ubwc_cfg_data sdm670_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sdm845_data = {
+static struct qcom_ubwc_cfg_data sdm845_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -122,7 +123,7 @@ static const struct qcom_ubwc_cfg_data sdm845_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sm6115_data = {
+static struct qcom_ubwc_cfg_data sm6115_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -132,7 +133,7 @@ static const struct qcom_ubwc_cfg_data sm6115_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6125_data = {
+static struct qcom_ubwc_cfg_data sm6125_data = {
.ubwc_enc_version = UBWC_1_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL1 |
@@ -141,7 +142,7 @@ static const struct qcom_ubwc_cfg_data sm6125_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6150_data = {
+static struct qcom_ubwc_cfg_data sm6150_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -149,7 +150,7 @@ static const struct qcom_ubwc_cfg_data sm6150_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm6350_data = {
+static struct qcom_ubwc_cfg_data sm6350_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -158,7 +159,7 @@ static const struct qcom_ubwc_cfg_data sm6350_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm7150_data = {
+static struct qcom_ubwc_cfg_data sm7150_data = {
.ubwc_enc_version = UBWC_2_0,
.ubwc_dec_version = UBWC_2_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -166,7 +167,7 @@ static const struct qcom_ubwc_cfg_data sm7150_data = {
.highest_bank_bit = 14,
};
-static const struct qcom_ubwc_cfg_data sm8150_data = {
+static struct qcom_ubwc_cfg_data sm8150_data = {
.ubwc_enc_version = UBWC_3_0,
.ubwc_dec_version = UBWC_3_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -174,7 +175,7 @@ static const struct qcom_ubwc_cfg_data sm8150_data = {
.highest_bank_bit = 15,
};
-static const struct qcom_ubwc_cfg_data sm8250_data = {
+static struct qcom_ubwc_cfg_data sm8250_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -185,7 +186,7 @@ static const struct qcom_ubwc_cfg_data sm8250_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8350_data = {
+static struct qcom_ubwc_cfg_data sm8350_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_0,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -196,7 +197,7 @@ static const struct qcom_ubwc_cfg_data sm8350_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8550_data = {
+static struct qcom_ubwc_cfg_data sm8550_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -207,7 +208,7 @@ static const struct qcom_ubwc_cfg_data sm8550_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data sm8750_data = {
+static struct qcom_ubwc_cfg_data sm8750_data = {
.ubwc_enc_version = UBWC_5_0,
.ubwc_dec_version = UBWC_5_0,
.ubwc_swizzle = 6,
@@ -217,7 +218,7 @@ static const struct qcom_ubwc_cfg_data sm8750_data = {
.macrotile_mode = true,
};
-static const struct qcom_ubwc_cfg_data x1e80100_data = {
+static struct qcom_ubwc_cfg_data x1e80100_data = {
.ubwc_enc_version = UBWC_4_0,
.ubwc_dec_version = UBWC_4_3,
.ubwc_swizzle = UBWC_SWIZZLE_ENABLE_LVL2 |
@@ -301,14 +302,30 @@ static const struct of_device_id qcom_ubwc_configs[] __maybe_unused = {
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
- const struct qcom_ubwc_cfg_data *data;
+ struct qcom_ubwc_cfg_data *data;
+ int hbb;
- data = of_machine_get_match_data(qcom_ubwc_configs);
+ if (!qcom_smem_is_available())
+ return ERR_PTR(-EPROBE_DEFER);
+
+ /* Discard the const qualifier, but still return a const pointer to consumers */
+ data = (struct qcom_ubwc_cfg_data *)of_machine_get_match_data(qcom_ubwc_configs);
if (!data) {
pr_err("Couldn't find UBWC config data for this platform!\n");
return ERR_PTR(-EINVAL);
}
+ hbb = qcom_smem_dram_get_hbb();
+ if (hbb == -ENODATA) {
+ /* Lack of HBB data is OK - it was only introduced later */
+ return data;
+ } else if (hbb < 0) {
+ pr_err("Couldn't get HBB data from SMEM: %d\n", hbb);
+ return ERR_PTR(hbb);
+ }
+
+ data->highest_bank_bit = hbb;
+
return data;
}
EXPORT_SYMBOL_GPL(qcom_ubwc_config_get_data);
--
2.52.0On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote: > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > To make sure the correct settings for a given DRAM configuration get > applied, attempt to retrieve that data from SMEM (which happens to be > what the BSP kernel does, albeit with through convoluted means of the > bootloader altering the DT with this data). > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > --- > I'm not sure about this approach - perhaps a global variable storing > the selected config, which would then be non-const would be better? I'd prefer if const data was const, split HBB to a separate API. -- With best wishes Dmitry
On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote: > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote: > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > > > To make sure the correct settings for a given DRAM configuration get > > applied, attempt to retrieve that data from SMEM (which happens to be > > what the BSP kernel does, albeit with through convoluted means of the > > bootloader altering the DT with this data). > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > > > --- > > I'm not sure about this approach - perhaps a global variable storing > > the selected config, which would then be non-const would be better? > > I'd prefer if const data was const, split HBB to a separate API. > I agree, but I'd prefer to avoid a separate API for it. Instead I'd like to either return the struct by value (after updating the hbb), but we then loose the ability to return errors, or by changing the signature to: int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data) This costs us an additional 16 bytes in each client (as the pointer is replaced with the data), but I think it's a cleaner API. Regards, Bjorn
On 1/8/26 6:49 PM, Bjorn Andersson wrote:
> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> To make sure the correct settings for a given DRAM configuration get
>>> applied, attempt to retrieve that data from SMEM (which happens to be
>>> what the BSP kernel does, albeit with through convoluted means of the
>>> bootloader altering the DT with this data).
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>
>>> ---
>>> I'm not sure about this approach - perhaps a global variable storing
>>> the selected config, which would then be non-const would be better?
>>
>> I'd prefer if const data was const, split HBB to a separate API.
>>
>
> I agree, but I'd prefer to avoid a separate API for it.
>
> Instead I'd like to either return the struct by value (after updating
> the hbb), but we then loose the ability to return errors, or by changing
> the signature to:
>
> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>
> This costs us an additional 16 bytes in each client (as the pointer is
> replaced with the data), but I think it's a cleaner API.
I don't see how that's much better than
static const qcom_ubwc_cfg_data foobar_ubwc_data = {
...
};
static qcom_ubwc_cfg __data;
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
...
if (__data)
return data;
__data = of_machine_get_match_data()
...
hbb = ...;
__data->hbb = hbb;
return data; //still returns a constptr
}
Since we essentially do the same thing, except we save space and rest
assured the various client drivers don't mess with the data they receive
Konrad
On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote: > On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote: > > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote: > > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > > > > > To make sure the correct settings for a given DRAM configuration get > > > applied, attempt to retrieve that data from SMEM (which happens to be > > > what the BSP kernel does, albeit with through convoluted means of the > > > bootloader altering the DT with this data). > > > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> > > > > > > --- > > > I'm not sure about this approach - perhaps a global variable storing > > > the selected config, which would then be non-const would be better? > > > > I'd prefer if const data was const, split HBB to a separate API. > > > > I agree, but I'd prefer to avoid a separate API for it. > > Instead I'd like to either return the struct by value (after updating > the hbb), but we then loose the ability to return errors, or by changing > the signature to: > > int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data) > > This costs us an additional 16 bytes in each client (as the pointer is > replaced with the data), but I think it's a cleaner API. What about: const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb) I really want to keep the data as const and, as important, use it as a const pointer. > > Regards, > Bjorn -- With best wishes Dmitry
On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> > On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > >
> > > > To make sure the correct settings for a given DRAM configuration get
> > > > applied, attempt to retrieve that data from SMEM (which happens to be
> > > > what the BSP kernel does, albeit with through convoluted means of the
> > > > bootloader altering the DT with this data).
> > > >
> > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > >
> > > > ---
> > > > I'm not sure about this approach - perhaps a global variable storing
> > > > the selected config, which would then be non-const would be better?
> > >
> > > I'd prefer if const data was const, split HBB to a separate API.
> > >
> >
> > I agree, but I'd prefer to avoid a separate API for it.
> >
> > Instead I'd like to either return the struct by value (after updating
> > the hbb), but we then loose the ability to return errors, or by changing
> > the signature to:
> >
> > int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >
> > This costs us an additional 16 bytes in each client (as the pointer is
> > replaced with the data), but I think it's a cleaner API.
>
> What about:
>
> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
>
> I really want to keep the data as const and, as important, use it as a
> const pointer.
>
I guess the question is what are you actually trying to achive; my goal
was to keep the base data constant, but I'm guessing that you also want
to retain the "const" classifier in the client's context struct (e.g.
the "mdss" member in struct dpu_kms)
If we're returning the data by value, there's no way for you to mark
it as "const" in the calling code's context object (as by definition you
shouldn't be able to change the value after initializing the object).
You also can't return the data by value and then track it by reference -
as that value lives on the stack. This has the benefit of making the
lifecycle of that object clear (it lives in each client) - but perhaps
not a goal of ours...
How come the ubwc config is const but the hbb isn't?
If we want both the per-target data to remain const and data in the
client's context to be carrying the const qualifier, the one solution I
can see is:
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
{
const struct qcom_ubwc_cfg_data *data;
static struct qcom_ubwc_cfg_data cfg;
int hbb;
...
data = of_machine_get_match_data(qcom_ubwc_configs);
...
hbb = qcom_smem_dram_get_hbb();
...
cfg = *data;
cfg.highest_bank_bit = hbb;
return &cfg;
}
But we'd need to deal with the race in cfg assignment...
Regards,
Bjorn
> >
> > Regards,
> > Bjorn
>
> --
> With best wishes
> Dmitry
On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> > On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> > > On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> > > > On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> > > > > From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > >
> > > > > To make sure the correct settings for a given DRAM configuration get
> > > > > applied, attempt to retrieve that data from SMEM (which happens to be
> > > > > what the BSP kernel does, albeit with through convoluted means of the
> > > > > bootloader altering the DT with this data).
> > > > >
> > > > > Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > >
> > > > > ---
> > > > > I'm not sure about this approach - perhaps a global variable storing
> > > > > the selected config, which would then be non-const would be better?
> > > >
> > > > I'd prefer if const data was const, split HBB to a separate API.
> > > >
> > >
> > > I agree, but I'd prefer to avoid a separate API for it.
> > >
> > > Instead I'd like to either return the struct by value (after updating
> > > the hbb), but we then loose the ability to return errors, or by changing
> > > the signature to:
> > >
> > > int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> > >
> > > This costs us an additional 16 bytes in each client (as the pointer is
> > > replaced with the data), but I think it's a cleaner API.
> >
> > What about:
> >
> > const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> >
> > I really want to keep the data as const and, as important, use it as a
> > const pointer.
> >
>
> I guess the question is what are you actually trying to achive; my goal
> was to keep the base data constant, but I'm guessing that you also want
> to retain the "const" classifier in the client's context struct (e.g.
> the "mdss" member in struct dpu_kms)
>
> If we're returning the data by value, there's no way for you to mark
> it as "const" in the calling code's context object (as by definition you
> shouldn't be able to change the value after initializing the object).
And I, of course, misssed one star:
const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
This leaks the knowledge that HBB is slightly different kind of property
than the rest of UBWC data.
>
> You also can't return the data by value and then track it by reference -
> as that value lives on the stack. This has the benefit of making the
> lifecycle of that object clear (it lives in each client) - but perhaps
> not a goal of ours...
>
> How come the ubwc config is const but the hbb isn't?
>
>
> If we want both the per-target data to remain const and data in the
> client's context to be carrying the const qualifier, the one solution I
> can see is:
>
> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> {
> const struct qcom_ubwc_cfg_data *data;
> static struct qcom_ubwc_cfg_data cfg;
> int hbb;
>
> ...
>
> data = of_machine_get_match_data(qcom_ubwc_configs);
> ...
>
> hbb = qcom_smem_dram_get_hbb();
> ...
>
> cfg = *data;
> cfg.highest_bank_bit = hbb;
>
> return &cfg;
> }
>
> But we'd need to deal with the race in cfg assignment...
static struct qcom_ubwc_cfg_data *cfg;
static DEFINE_MUTEX(cfg_mutex);
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;
data = of_machine_get_match_data(qcom_ubwc_configs);
if (!data)
return ERR_PTR(-ENOMEM);
hbb = qcom_smem_dram_get_hbb();
if (hbb = -ENODATA)
hbb = 15; /* I think it was default */
else if (hbb < 0)
return ERR_PTR(hbb);
cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
if (!cfg)
return ERR_PTR(-ENOMEM);
cfg->highest_bank_bit = hbb;
return cfg;
}
This potentially leaks sizeof(*data) memory if the module gets removed.
Granted that all users also use qcom_ubwc_config_get_data() symbol, it
should be safe to kfree(cfg) on module removal.
--
With best wishes
Dmitry
On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
>> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
>>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
>>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
>>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> To make sure the correct settings for a given DRAM configuration get
>>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
>>>>>> what the BSP kernel does, albeit with through convoluted means of the
>>>>>> bootloader altering the DT with this data).
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> ---
>>>>>> I'm not sure about this approach - perhaps a global variable storing
>>>>>> the selected config, which would then be non-const would be better?
>>>>>
>>>>> I'd prefer if const data was const, split HBB to a separate API.
>>>>>
>>>>
>>>> I agree, but I'd prefer to avoid a separate API for it.
>>>>
>>>> Instead I'd like to either return the struct by value (after updating
>>>> the hbb), but we then loose the ability to return errors, or by changing
>>>> the signature to:
>>>>
>>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
>>>>
>>>> This costs us an additional 16 bytes in each client (as the pointer is
>>>> replaced with the data), but I think it's a cleaner API.
>>>
>>> What about:
>>>
>>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
>>>
>>> I really want to keep the data as const and, as important, use it as a
>>> const pointer.
>>>
>>
>> I guess the question is what are you actually trying to achive; my goal
>> was to keep the base data constant, but I'm guessing that you also want
>> to retain the "const" classifier in the client's context struct (e.g.
>> the "mdss" member in struct dpu_kms)
>>
>> If we're returning the data by value, there's no way for you to mark
>> it as "const" in the calling code's context object (as by definition you
>> shouldn't be able to change the value after initializing the object).
>
> And I, of course, misssed one star:
>
> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
>
> This leaks the knowledge that HBB is slightly different kind of property
> than the rest of UBWC data.
>
>>
>> You also can't return the data by value and then track it by reference -
>> as that value lives on the stack. This has the benefit of making the
>> lifecycle of that object clear (it lives in each client) - but perhaps
>> not a goal of ours...
>>
>> How come the ubwc config is const but the hbb isn't?
>>
>>
>> If we want both the per-target data to remain const and data in the
>> client's context to be carrying the const qualifier, the one solution I
>> can see is:
>>
>> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
>> {
>> const struct qcom_ubwc_cfg_data *data;
>> static struct qcom_ubwc_cfg_data cfg;
>> int hbb;
>>
>> ...
>>
>> data = of_machine_get_match_data(qcom_ubwc_configs);
>> ...
>>
>> hbb = qcom_smem_dram_get_hbb();
>> ...
>>
>> cfg = *data;
>> cfg.highest_bank_bit = hbb;
>>
>> return &cfg;
>> }
>>
>> But we'd need to deal with the race in cfg assignment...
>
> static struct qcom_ubwc_cfg_data *cfg;
> static DEFINE_MUTEX(cfg_mutex);
> 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;
>
> data = of_machine_get_match_data(qcom_ubwc_configs);
> if (!data)
> return ERR_PTR(-ENOMEM);
>
> hbb = qcom_smem_dram_get_hbb();
> if (hbb = -ENODATA)
> hbb = 15; /* I think it was default */
> else if (hbb < 0)
> return ERR_PTR(hbb);
>
> cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> if (!cfg)
> return ERR_PTR(-ENOMEM);
>
> cfg->highest_bank_bit = hbb;
>
> return cfg;
> }
>
> This potentially leaks sizeof(*data) memory if the module gets removed.
> Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> should be safe to kfree(cfg) on module removal.
I really don't understand why you'd want a separate API for hbb, if
hbb is already available from the larger struct *and* if a driver needs
to know about the value of hbb, it really needs to know about all the
other values as well
Konrad
On Tue, Jan 13, 2026 at 04:31:15PM +0100, Konrad Dybcio wrote:
> On 1/10/26 11:45 AM, Dmitry Baryshkov wrote:
> > On Fri, Jan 09, 2026 at 11:50:46AM -0600, Bjorn Andersson wrote:
> >> On Fri, Jan 09, 2026 at 05:21:10AM +0200, Dmitry Baryshkov wrote:
> >>> On Thu, Jan 08, 2026 at 11:49:54AM -0600, Bjorn Andersson wrote:
> >>>> On Thu, Jan 08, 2026 at 04:45:49PM +0200, Dmitry Baryshkov wrote:
> >>>>> On Thu, Jan 08, 2026 at 03:21:51PM +0100, Konrad Dybcio wrote:
> >>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>
> >>>>>> To make sure the correct settings for a given DRAM configuration get
> >>>>>> applied, attempt to retrieve that data from SMEM (which happens to be
> >>>>>> what the BSP kernel does, albeit with through convoluted means of the
> >>>>>> bootloader altering the DT with this data).
> >>>>>>
> >>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>
> >>>>>> ---
> >>>>>> I'm not sure about this approach - perhaps a global variable storing
> >>>>>> the selected config, which would then be non-const would be better?
> >>>>>
> >>>>> I'd prefer if const data was const, split HBB to a separate API.
> >>>>>
> >>>>
> >>>> I agree, but I'd prefer to avoid a separate API for it.
> >>>>
> >>>> Instead I'd like to either return the struct by value (after updating
> >>>> the hbb), but we then loose the ability to return errors, or by changing
> >>>> the signature to:
> >>>>
> >>>> int qcom_ubwc_config_get_data(struct qcom_ubwc_cfg_data *data)
> >>>>
> >>>> This costs us an additional 16 bytes in each client (as the pointer is
> >>>> replaced with the data), but I think it's a cleaner API.
> >>>
> >>> What about:
> >>>
> >>> const struct qcom_ubwc_cfg_data qcom_ubwc_config_get_data(u32 *hbb)
> >>>
> >>> I really want to keep the data as const and, as important, use it as a
> >>> const pointer.
> >>>
> >>
> >> I guess the question is what are you actually trying to achive; my goal
> >> was to keep the base data constant, but I'm guessing that you also want
> >> to retain the "const" classifier in the client's context struct (e.g.
> >> the "mdss" member in struct dpu_kms)
> >>
> >> If we're returning the data by value, there's no way for you to mark
> >> it as "const" in the calling code's context object (as by definition you
> >> shouldn't be able to change the value after initializing the object).
> >
> > And I, of course, misssed one star:
> >
> > const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(u32 *hbb)
> >
> > This leaks the knowledge that HBB is slightly different kind of property
> > than the rest of UBWC data.
> >
> >>
> >> You also can't return the data by value and then track it by reference -
> >> as that value lives on the stack. This has the benefit of making the
> >> lifecycle of that object clear (it lives in each client) - but perhaps
> >> not a goal of ours...
> >>
> >> How come the ubwc config is const but the hbb isn't?
> >>
> >>
> >> If we want both the per-target data to remain const and data in the
> >> client's context to be carrying the const qualifier, the one solution I
> >> can see is:
> >>
> >> const struct qcom_ubwc_cfg_data *qcom_ubwc_config_get_data(void)
> >> {
> >> const struct qcom_ubwc_cfg_data *data;
> >> static struct qcom_ubwc_cfg_data cfg;
> >> int hbb;
> >>
> >> ...
> >>
> >> data = of_machine_get_match_data(qcom_ubwc_configs);
> >> ...
> >>
> >> hbb = qcom_smem_dram_get_hbb();
> >> ...
> >>
> >> cfg = *data;
> >> cfg.highest_bank_bit = hbb;
> >>
> >> return &cfg;
> >> }
> >>
> >> But we'd need to deal with the race in cfg assignment...
> >
> > static struct qcom_ubwc_cfg_data *cfg;
> > static DEFINE_MUTEX(cfg_mutex);
> > 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;
> >
> > data = of_machine_get_match_data(qcom_ubwc_configs);
> > if (!data)
> > return ERR_PTR(-ENOMEM);
> >
> > hbb = qcom_smem_dram_get_hbb();
> > if (hbb = -ENODATA)
> > hbb = 15; /* I think it was default */
> > else if (hbb < 0)
> > return ERR_PTR(hbb);
> >
> > cfg = kmemdup(data, sizeof(*data), GFP_KERNEL);
> > if (!cfg)
> > return ERR_PTR(-ENOMEM);
> >
> > cfg->highest_bank_bit = hbb;
> >
> > return cfg;
> > }
> >
> > This potentially leaks sizeof(*data) memory if the module gets removed.
> > Granted that all users also use qcom_ubwc_config_get_data() symbol, it
> > should be safe to kfree(cfg) on module removal.
>
> I really don't understand why you'd want a separate API for hbb, if
> hbb is already available from the larger struct *and* if a driver needs
> to know about the value of hbb, it really needs to know about all the
> other values as well
Please take another look, qcom_ubwc_config_get_data() is the only public
API, qcom_smem_dram_get_hbb() is an internal API.
My goal is to keep having UBWC db which keeps const data and which which
also returns a const pointer.
--
With best wishes
Dmitry
© 2016 - 2026 Red Hat, Inc.