Thanks for your time in reviewing this.
On 9/11/2023 4:31 PM, Krzysztof Kozlowski wrote:
On 09/09/2023 22:16, Mukesh Ojha wrote:
Minidump is a best effort mechanism to collect useful and predefined
data for first level of debugging on end user devices running on
Qualcomm SoCs. It is built on the premise that System on Chip (SoC)
or subsystem part of SoC crashes, due to a range of hardware and
software bugs. Hence, the ability to collect accurate data is only
a best-effort. The data collected could be invalid or corrupted,
data collection itself could fail, and so on.
...
+static int qcom_apss_md_table_init(struct minidump *md,
+ struct minidump_subsystem *mdss_toc)
+{
+ struct minidump_ss_data *mdss_data;
+
+ mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
+ if (!mdss_data)
+ return -ENOMEM;
+
+ mdss_data->md_ss_toc = mdss_toc;
+ mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
+ sizeof(struct minidump_region),
+ GFP_KERNEL);
+ if (!mdss_data->md_regions)
+ return -ENOMEM;
+
+ mdss_toc = mdss_data->md_ss_toc;
+ mdss_toc->regions_baseptr = cpu_to_le64(virt_to_phys(mdss_data->md_regions));
+ mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
+ mdss_toc->status = cpu_to_le32(1);
+ mdss_toc->region_count = cpu_to_le32(0);
+
+ /* Tell bootloader not to encrypt the regions of this subsystem */
+ mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
+ mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
+
+ md->apss_data = mdss_data;
+
+ return 0;
+}
+
+static int qcom_apss_minidump_probe(struct platform_device *pdev)
+{
+ struct minidump_global_toc *mdgtoc;
+ struct minidump *md;
+ size_t size;
+ int ret;
+
+ md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);
sizeof(*)
Didn't you get such comments already?
Ok, will fix this, no i have not got such comments as of yet.
Any reason of using this way?
+ if (!md)
+ return -ENOMEM;
+
+ md->dev = &pdev->dev;
+ mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
+ if (IS_ERR(mdgtoc)) {
+ ret = PTR_ERR(mdgtoc);
+ dev_err(md->dev, "Couldn't find minidump smem item: %d\n", ret);
+ return ret;
The syntax is:
return dev_err_probe
ACK.
+ }
+
+ if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+ dev_err(md->dev, "minidump table is not initialized: %d\n", ret);
ret is uninitialized here. Please use automated tools for checking your
code:
coccinelle, smatch and sparse
Thanks.
+ return -EINVAL;
+ }
+
+ mutex_init(&md->md_lock);
+ ret = qcom_apss_md_table_init(md, &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
+ if (ret) {
+ dev_err(md->dev, "apss minidump initialization failed: %d\n", ret);
+ return ret;
+ }
+
+ /* First entry would be ELF header */
+ ret = qcom_md_add_elfheader(md);
+ if (ret) {
+ dev_err(md->dev, "Failed to add elf header: %d\n", ret);
+ memset(md->apss_data->md_ss_toc, 0, sizeof(struct minidump_subsystem));
Why do you need it?
Earlier, i got comment about clearing the SS TOC(subsystem table of
content) which is shared with other SS and it will have stale values.
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, md);
+
+ return ret;
+}
+
+static int qcom_apss_minidump_remove(struct platform_device *pdev)
+{
+ struct minidump *md = platform_get_drvdata(pdev);
+ struct minidump_ss_data *mdss_data;
+
+ mdss_data = md->apss_data;
+ memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct minidump_subsystem));
Why do you need it?
Same as above.
+ md = NULL;
That's useless assignment.
Ok.
+
+ return 0;
+}
+
+static struct platform_driver qcom_minidump_driver = {
+ .probe = qcom_apss_minidump_probe,
+ .remove = qcom_apss_minidump_remove,
+ .driver = {
+ .name = "qcom-minidump-smem",
+ },
+};
+
+module_platform_driver(qcom_minidump_driver);
+
+MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:qcom-minidump-smem");
Add a proper ID table instead of re-inventing it with module aliases.
Ok.
-Mukesh
Best regards,
Krzysztof
