| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <nsjzp77f7btdziurip3v6nu5utcwni253mrx6orkefz5mibb3s@cp7c6tv3joxk>
Date: Fri, 9 Jan 2026 12:08:08 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Konrad Dybcio <konradybcio@...nel.org>
Cc: Kees Cook <kees@...nel.org>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>, Rob Clark <robin.clark@....qualcomm.com>,
Sean Paul <sean@...rly.run>, Akhil P Oommen <akhilpo@....qualcomm.com>,
Dmitry Baryshkov <lumag@...nel.org>, Abhinav Kumar <abhinav.kumar@...ux.dev>,
Jessica Zhang <jesszhan0024@...il.com>, Marijn Suijten <marijn.suijten@...ainline.org>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-hardening@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v3 1/3] soc: qcom: smem: Expose DDR data from SMEM
On Thu, Jan 08, 2026 at 03:21:50PM +0100, Konrad Dybcio wrote:
> diff --git a/drivers/soc/qcom/smem_dramc.c b/drivers/soc/qcom/smem_dramc.c
[..]
> +struct ddr_regions_v5 {
> + u32 ddr_region_num; /* We expect this to always be 4 or 6 */
> + u64 ddr_rank0_size;
> + u64 ddr_rank1_size;
> + u64 ddr_cs0_start_addr;
> + u64 ddr_cs1_start_addr;
> + u32 highest_bank_addr_bit;
Aren't all these structs encoded in little endian? __leXX?
> + struct ddr_region_v5 ddr_region[] __counted_by(ddr_region_num);
Was going to joke about this one, but realized that there's a
__counted_by_le()
> +};
> +
> +struct ddr_details_v5 {
> + u8 manufacturer_id;
> + u8 device_type;
> + struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> + struct ddr_freq_plan_v5 ddr_freq_tbl;
> + u8 num_channels;
> + u8 _padding;
> + struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/* V6 */
> +struct ddr_misc_info_v6 {
> + u32 dsf_version;
> + u32 reserved[10];
> +};
> +
> +/* V7 */
> +struct ddr_details_v7 {
> + u8 manufacturer_id;
> + u8 device_type;
> + struct ddr_part_details ddr_params[MAX_CHAN_NUM];
> + struct ddr_freq_plan_v5 ddr_freq_tbl;
> + u8 num_channels;
> + u8 sct_config;
> + struct ddr_regions_v5 ddr_regions;
> +};
> +
> +/**
> + * qcom_smem_dram_get_hbb(): Get the Highest bank address bit
> + *
> + * Context: Check qcom_smem_is_available() before calling this function.
> + * Because __dram * is initialized by smem_dram_parse(), which is in turn
> + * called from * qcom_smem_probe(), __dram will only be NULL if the data
> + * couldn't have been found/interpreted correctly.
> + *
> + * Return: 0 on success, -ENODATA on failure.
Seems more like "highest bank bit on success, -ENODATA on failure.
> + */
> +int qcom_smem_dram_get_hbb(void)
> +{
> + int hbb;
> +
> + if (!__dram)
> + return -ENODATA;
> +
> + hbb = __dram->hbb;
> + if (hbb == 0)
> + return -ENODATA;
> + else if (hbb < DDR_HBB_MIN || hbb > DDR_HBB_MAX)
> + return -EINVAL;
Not really "Invalid argument", -ENODATA is probably better here as well.
> +
> + return hbb;
> +}
> +EXPORT_SYMBOL_GPL(qcom_smem_dram_get_hbb);
> +
[..]
> +/* The structure contains no version field, so we have to perform some guesswork.. */
> +static int smem_dram_infer_struct_version(size_t size)
> +{
> + /* Some early versions provided less bytes of less useful data */
> + if (size < sizeof(struct ddr_details_v3))
> + return -EINVAL;
> +
> + if (size == sizeof(struct ddr_details_v3))
> + return INFO_V3;
> +
> + if (size == sizeof(struct ddr_details_v3)
> + + sizeof(struct ddr_freq_table))
Don't you find it weird to have the + after the wrap?
> + return INFO_V3_WITH_14_FREQS;
> +
> + if (size == sizeof(struct ddr_details_v4))
> + return INFO_V4;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5))
> + return INFO_V5;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_xbl2quantum_smem_data)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V5_WITH_6_REGIONS;
> +
> + if (size == sizeof(struct ddr_details_v5)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V6;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 4 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7;
> +
> + if (size == sizeof(struct ddr_details_v7)
> + + 6 * sizeof(struct ddr_region_v5)
> + + sizeof(struct ddr_misc_info_v6)
> + + sizeof(struct shub_freq_plan_entry))
> + return INFO_V7_WITH_6_REGIONS;
> +
> + return INFO_UNKNOWN;
> +}
> +
[..]
> +
> +struct dentry *smem_dram_parse(struct device *dev)
> +{
> + struct dentry *debugfs_dir;
> + enum ddr_info_version ver;
> + struct smem_dram *dram;
> + size_t actual_size;
> + void *data = NULL;
> +
> + /* No need to check qcom_smem_is_available(), this func is called by the SMEM driver */
> + data = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_DDR_INFO_ID, &actual_size);
> + if (IS_ERR_OR_NULL(data))
> + return ERR_PTR(-ENODATA);
> +
> + ver = smem_dram_infer_struct_version(actual_size);
> + if (ver < 0) {
> + /* Some SoCs don't provide data that's useful for us */
> + return ERR_PTR(-ENODATA);
> + } else if (ver == INFO_UNKNOWN) {
> + /* In other cases, we may not have added support for a newer struct revision */
> + pr_err("Found an unknown type of DRAM info struct (size = %zu)\n", actual_size);
Is there a reason why this isn't dev_err(dev, ...)?
> + return ERR_PTR(-EINVAL);
> + }
> +
> + dram = devm_kzalloc(dev, sizeof(*dram), GFP_KERNEL);
> + if (!dram)
> + return ERR_PTR(-ENOMEM);
> +
> + switch (ver) {
> + case INFO_V3:
> + smem_dram_parse_v3_data(dram, data, false);
> + break;
> + case INFO_V3_WITH_14_FREQS:
> + smem_dram_parse_v3_data(dram, data, true);
> + break;
> + case INFO_V4:
> + smem_dram_parse_v4_data(dram, data);
> + break;
> + case INFO_V5:
> + case INFO_V5_WITH_6_REGIONS:
> + case INFO_V6:
> + smem_dram_parse_v5_data(dram, data);
> + break;
> + case INFO_V7:
> + case INFO_V7_WITH_6_REGIONS:
> + smem_dram_parse_v7_data(dram, data);
> + break;
> + default:
> + return ERR_PTR(-EINVAL);
> + }
> +
> + /* Both the entry and its parent dir will be cleaned up by debugfs_remove_recursive */
> + debugfs_dir = debugfs_create_dir("qcom_smem", NULL);
> + debugfs_create_file("dram_frequencies", 0444, debugfs_dir, dram,
> + &smem_dram_frequencies_fops);
> + debugfs_create_file("hbb", 0444, debugfs_dir, dram, &smem_hbb_fops);
> +
> + /* If there was no failure so far, assign the global variable */
> + __dram = dram;
> +
> + return debugfs_dir;
> +}
> diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
> index f946e3beca21..223cd5090a2a 100644
> --- a/include/linux/soc/qcom/smem.h
> +++ b/include/linux/soc/qcom/smem.h
> @@ -2,6 +2,8 @@
> #ifndef __QCOM_SMEM_H__
> #define __QCOM_SMEM_H__
>
> +#include <linux/platform_device.h>
I'm not able to see why.
Regards,
Bjorn
> +
> #define QCOM_SMEM_HOST_ANY -1
>
> bool qcom_smem_is_available(void);
> @@ -17,4 +19,6 @@ int qcom_smem_get_feature_code(u32 *code);
>
> int qcom_smem_bust_hwspin_lock_by_host(unsigned int host);
>
> +int qcom_smem_dram_get_hbb(void);
> +
> #endif
>
> --
> 2.52.0
>
Powered by blists - more mailing lists