Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 9/12/2023 3:24 PM, Krzysztof Kozlowski wrote:
On 12/09/2023 11:26, Mukesh Ojha wrote:

+		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.

OK, but then the entire code is poorly readable. First, any cleanup of
qcom_apss_md_table_init() should be named similarly, e.g.
qcom_apss_md_table_clean() or qcom_apss_md_table_exit() or whatever
seems feasible.

ACK on this.


Second, shouldn't writing to shared memory be the last step? Step which
cannot fail and there is no cleanup afterwards (like
platform_set_drvdata)? I don't enjoy looking at this interface...

It can be done, if i shift adding elf header as first thing to first
caller of qcom_minidump_region_register() but then i would have to remove qcom_ramoops_minidump() from this probe in 11/17 patch.

-Mukesh



Best regards,
Krzysztof




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux