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.
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...
Best regards,
Krzysztof
