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 <[email protected]> >>>>>> >>>>>> 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 <[email protected]> >>>>>> >>>>>> --- >>>>>> 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
