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

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.

Qualcomm devices in engineering mode provides a mechanism for
generating full system ramdumps for post mortem debugging. But in some
cases it's however not feasible to capture the entire content of RAM.
The minidump mechanism provides the means for selecting region should
be included in the ramdump. The solution supports extracting the
ramdump/minidump produced either over USB or stored to an attached
storage device.

The core of minidump feature is part of Qualcomm's boot firmware code.
It initializes shared memory (SMEM), which is a part of DDR and
allocates a small section of it to minidump table i.e also called
global table of content (G-ToC). Each subsystem (APSS, ADSP, ...) has
their own table of segments to be included in the minidump, all
references from a descriptor in SMEM (G-ToC). Each segment/region has
some details like name, physical address and it's size etc. and it
could be anywhere scattered in the DDR.

qcom_minidump(core or frontend) driver adds the capability to add inux
region to be dumped as part of ram dump collection. It provides
appropriate symbol to register/unregister client regions.

To simplify post mortem debugging, it creates and maintain an ELF
header as first region that gets updated upon registration
of a new region.

Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
---
 drivers/soc/qcom/Kconfig                  |  13 +
 drivers/soc/qcom/Makefile                 |   1 +
 drivers/soc/qcom/qcom_minidump.c          | 603 ++++++++++++++++++++++++++++++
 drivers/soc/qcom/qcom_minidump_internal.h |  10 +
 drivers/soc/qcom/smem.c                   |  18 +
 include/linux/soc/qcom/smem.h             |   2 +
 include/soc/qcom/qcom_minidump.h          |  33 ++
 7 files changed, 680 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_minidump.c

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

On 09/09/2023 22:16, Mukesh Ojha wrote:
> +/**
> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> +	struct minidump *md;
> +	int ret;
> +
> +	md = qcom_smem_minidump_ready();
> +	if (!md)
> +		return -EPROBE_DEFER;
> +
> +	if (!qcom_minidump_valid_region(region))
> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_register(md, region);
> +	if (ret)
> +		goto unlock;
> +
> +	qcom_md_update_elfheader(md, region);
> +unlock:
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);

NAK, there is no user for this.

Drop all exports from minidump drivers. Your upstream driver *must not*
expose stuff to your vendor drivers.

Best regards,
Krzysztof

On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
> On 09/09/2023 22:16, Mukesh Ojha wrote:
>> +/**
>> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
>> + * @region: minidump region.
>> + *
>> + * Return: On success, it returns 0 and negative error value on failure.
>> + */
>> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
>> +{
>> +	struct minidump *md;
>> +	int ret;
>> +
>> +	md = qcom_smem_minidump_ready();
>> +	if (!md)
>> +		return -EPROBE_DEFER;
>> +
>> +	if (!qcom_minidump_valid_region(region))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&md->md_lock);
>> +	ret = qcom_md_region_register(md, region);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	qcom_md_update_elfheader(md, region);
>> +unlock:
>> +	mutex_unlock(&md->md_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
> 
> NAK, there is no user for this.
> 
> Drop all exports from minidump drivers. Your upstream driver *must not*
> expose stuff to your vendor drivers.

Do we not expect that upstream drivers would want to register?
Mind you, in the downstream code the following was a bad limitation:
#define MAX_NUM_OF_SS           10

On 11.09.2023 21:09, Jeff Johnson wrote:
> On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, Mukesh Ojha wrote:
>>> +/**
>>> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
>>> + * @region: minidump region.
>>> + *
>>> + * Return: On success, it returns 0 and negative error value on failure.
>>> + */
>>> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
>>> +{
>>> +    struct minidump *md;
>>> +    int ret;
>>> +
>>> +    md = qcom_smem_minidump_ready();
>>> +    if (!md)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    if (!qcom_minidump_valid_region(region))
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&md->md_lock);
>>> +    ret = qcom_md_region_register(md, region);
>>> +    if (ret)
>>> +        goto unlock;
>>> +
>>> +    qcom_md_update_elfheader(md, region);
>>> +unlock:
>>> +    mutex_unlock(&md->md_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
>>
>> NAK, there is no user for this.
>>
>> Drop all exports from minidump drivers. Your upstream driver *must not*
>> expose stuff to your vendor drivers.
> 
> Do we not expect that upstream drivers would want to register?
> Mind you, in the downstream code the following was a bad limitation:
> #define MAX_NUM_OF_SS           10
No, Krzysztof meant that you are not allowed to export symbols
without immediately providing a user for them - meaning if the
functions are not going to be used upstream, this change will
not be accepted.

Konrad

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
>

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

On 9/12/2023 2:56 PM, Mukesh Ojha wrote:
> 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?

Got the reason, thanks.

-Mukesh

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

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
>

On 9/12/2023 12:39 AM, Jeff Johnson wrote:
> On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, Mukesh Ojha wrote:
>>> +/**
>>> + * qcom_minidump_region_register() - Register region in APSS 
>>> Minidump table.
>>> + * @region: minidump region.
>>> + *
>>> + * Return: On success, it returns 0 and negative error value on 
>>> failure.
>>> + */
>>> +int qcom_minidump_region_register(const struct qcom_minidump_region 
>>> *region)
>>> +{
>>> +    struct minidump *md;
>>> +    int ret;
>>> +
>>> +    md = qcom_smem_minidump_ready();
>>> +    if (!md)
>>> +        return -EPROBE_DEFER;
>>> +
>>> +    if (!qcom_minidump_valid_region(region))
>>> +        return -EINVAL;
>>> +
>>> +    mutex_lock(&md->md_lock);
>>> +    ret = qcom_md_region_register(md, region);
>>> +    if (ret)
>>> +        goto unlock;
>>> +
>>> +    qcom_md_update_elfheader(md, region);
>>> +unlock:
>>> +    mutex_unlock(&md->md_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
>>
>> NAK, there is no user for this.
>>
>> Drop all exports from minidump drivers. Your upstream driver *must not*
>> expose stuff to your vendor drivers.
> 
> Do we not expect that upstream drivers would want to register?

As of current version of patch, there is no user. Let's avoid till
any upstream QCOM driver uses it .


> Mind you, in the downstream code the following was a bad limitation:
> #define MAX_NUM_OF_SS           10

I don't think, there is any problem with above macro, instead there is
restriction on total number of APSS region can be registered.

#define MAX_NUM_ENTRIES	  201

-Mukesh
> 
>