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?
> + 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
> + }
> +
> + 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
> + 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?
> + 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?
> + md = NULL;
That's useless assignment.
> +
> + 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.
Best regards,
Krzysztof
