On 9/12/2023 3:24 PM, Krzysztof Kozlowski wrote:

On 12/09/2023 11:26, Mukesh Ojha wrote:

quoted

quoted

quoted

+		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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel