diff mbox series

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

Message ID 1694290578-17733-7-git-send-email-quic_mojha@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add Qualcomm Minidump kernel driver related support | expand

Commit Message

Mukesh Ojha Sept. 9, 2023, 8:16 p.m. UTC
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

Comments

Krzysztof Kozlowski Sept. 11, 2023, 11:01 a.m. UTC | #1
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
Krzysztof Kozlowski Sept. 11, 2023, 11:07 a.m. UTC | #2
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
Jeff Johnson Sept. 11, 2023, 7:09 p.m. UTC | #3
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
Konrad Dybcio Sept. 11, 2023, 7:33 p.m. UTC | #4
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
Mukesh Ojha Sept. 12, 2023, 9:26 a.m. UTC | #5
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
>
Krzysztof Kozlowski Sept. 12, 2023, 9:54 a.m. UTC | #6
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
Mukesh Ojha Sept. 12, 2023, 9:58 a.m. UTC | #7
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
>>
Mukesh Ojha Sept. 13, 2023, 7:09 a.m. UTC | #8
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
>
Mukesh Ojha Sept. 13, 2023, 3:18 p.m. UTC | #9
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
> 
>
diff mbox series

Patch

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index ff38deac6a7d..4b36d46807bc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -300,4 +300,17 @@  config QCOM_RPROC_MINIDUMP
 	  query predefined minidump segments associated with the remote processor
 	  and check its validity and end up collecting the dump on remote processor
 	  crash during its recovery.
+
+config QCOM_MINIDUMP
+	tristate "QCOM APSS Minidump driver"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on QCOM_SMEM
+	help
+	  This config enables linux core infrastructure for Application
+	  processor subsystem (APSS) minidump collection i.e, it enables
+	  Linux clients drivers to register their internal data structures
+	  and debug messages as part of the apss minidump table and when
+	  the SoC is crashed, these selective regions will be dumped
+	  instead of the entire DDR dump. This saves significant amount
+	  of time and/or storage space.
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index a5fd2fed0923..9c1a409679ec 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,3 +36,4 @@  obj-$(CONFIG_QCOM_ICC_BWMON)	+= icc-bwmon.o
 qcom_ice-objs			+= ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)	+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)	+= qcom_rproc_minidump.o
+obj-$(CONFIG_QCOM_MINIDUMP)		+= qcom_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
new file mode 100644
index 000000000000..86f4d09a7b4e
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -0,0 +1,603 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device.h>
+#include <linux/elf.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/string.h>
+#include <soc/qcom/qcom_minidump.h>
+
+#include "qcom_minidump_internal.h"
+
+/**
+ * struct minidump_ss_data - Minidump subsystem private data
+ * @md_ss_toc	: Application Subsystem TOC pointer
+ * @md_regions	: Application Subsystem region base pointer
+ */
+struct minidump_ss_data {
+	struct minidump_subsystem	*md_ss_toc;
+	struct minidump_region		*md_regions;
+};
+
+/**
+ * struct minidump_elfhdr - Minidump table elf header
+ * @ehdr: elf main header
+ * @shdr: Section header
+ * @phdr: Program header
+ * @elf_offset: Section offset in elf
+ * @strtable_idx: String table current index position
+ */
+struct minidump_elfhdr {
+	struct elfhdr		*ehdr;
+	struct elf_shdr		*shdr;
+	struct elf_phdr		*phdr;
+	size_t			elf_offset;
+	size_t			strtable_idx;
+};
+
+/**
+ * struct minidump - Minidump driver data information
+ * @elf		  : Minidump elf header
+ * @dev		  : Minidump backend device
+ * @max_num_limit : Maximum number of region limit
+ * @apss_data	  : Backend driver's private data pointer
+ */
+struct minidump {
+	struct minidump_elfhdr		elf;
+	struct device			*dev;
+	struct minidump_ss_data		*apss_data;
+	struct mutex			md_lock;
+};
+
+/*
+ * In some of the Old Qualcomm devices, boot firmware statically allocates 300
+ * as total number of supported region (including all co-processors) in
+ * minidump table out of which linux was using 201. In future, this limitation
+ * from boot firmware might get removed by allocating the region dynamically.
+ * So, keep it compatible with older devices, we can keep the current limit for
+ * Linux to 201.
+ */
+#define MAX_NUM_ENTRIES	  201
+#define MAX_STRTBL_SIZE	  (MAX_NUM_ENTRIES * MAX_REGION_NAME_LENGTH)
+
+static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+	struct elf_shdr *eshdr = (struct elf_shdr *)((size_t)ehdr + ehdr->e_shoff);
+
+	return &eshdr[idx];
+}
+
+static struct elf_phdr *elf_phdr_entry_addr(struct elfhdr *ehdr, int idx)
+{
+	struct elf_phdr *ephdr = (struct elf_phdr *)((size_t)ehdr + ehdr->e_phoff);
+
+	return &ephdr[idx];
+}
+
+static char *elf_str_table_start(struct elfhdr *ehdr)
+{
+	struct elf_shdr *eshdr;
+
+	if (ehdr->e_shstrndx == SHN_UNDEF)
+		return NULL;
+
+	eshdr = elf_shdr_entry_addr(ehdr, ehdr->e_shstrndx);
+
+	return (char *)ehdr + eshdr->sh_offset;
+}
+
+static char *elf_lookup_string(struct minidump *md, struct elfhdr *ehdr, int offset)
+{
+	char *strtab = elf_str_table_start(ehdr);
+
+	if (!strtab || (md->elf.strtable_idx < offset))
+		return NULL;
+
+	return strtab + offset;
+}
+
+static unsigned int append_str_to_strtable(struct minidump *md, const char *name)
+{
+	char *strtab = elf_str_table_start(md->elf.ehdr);
+	unsigned int old_idx = md->elf.strtable_idx;
+	unsigned int ret;
+
+	if (!strtab || !name)
+		return 0;
+
+	ret = old_idx;
+	old_idx += strscpy((strtab + old_idx), name, MAX_REGION_NAME_LENGTH);
+	md->elf.strtable_idx = old_idx + 1;
+
+	return ret;
+}
+
+static int qcom_md_clear_elfheader(struct minidump *md,
+				   const struct qcom_minidump_region *region)
+{
+	struct elfhdr *ehdr = md->elf.ehdr;
+	struct elf_shdr *shdr;
+	struct elf_shdr *tmp_shdr;
+	struct elf_phdr *phdr;
+	struct elf_phdr *tmp_phdr;
+	unsigned int phidx;
+	unsigned int shidx;
+	unsigned int len;
+	unsigned int i;
+	char *shname;
+
+	for (i = 0; i < ehdr->e_phnum; i++) {
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		if (phdr->p_paddr == region->phys_addr &&
+		    phdr->p_memsz == region->size)
+			break;
+	}
+
+	if (i == ehdr->e_phnum) {
+		dev_err(md->dev, "Cannot find program header entry in elf\n");
+		return -EINVAL;
+	}
+
+	phidx = i;
+	for (i = 0; i < ehdr->e_shnum; i++) {
+		shdr = elf_shdr_entry_addr(ehdr, i);
+		shname = elf_lookup_string(md, ehdr, shdr->sh_name);
+		if (shname && !strcmp(shname, region->name) &&
+		    shdr->sh_addr == (elf_addr_t)region->virt_addr &&
+		    shdr->sh_size == region->size)
+			break;
+	}
+
+	if (i == ehdr->e_shnum) {
+		dev_err(md->dev, "Cannot find section header entry in elf\n");
+		return -EINVAL;
+	}
+
+	shidx = i;
+	if (shdr->sh_offset != phdr->p_offset) {
+		dev_err(md->dev, "Invalid entry details for region: %s\n", region->name);
+		return -EINVAL;
+	}
+
+	/* Clear name in string table */
+	len = strlen(shname) + 1;
+	memmove(shname, shname + len, md->elf.strtable_idx - shdr->sh_name - len);
+	md->elf.strtable_idx -= len;
+
+	/* Clear program header */
+	tmp_phdr = elf_phdr_entry_addr(ehdr, phidx);
+	for (i = phidx; i < ehdr->e_phnum - 1; i++) {
+		tmp_phdr = elf_phdr_entry_addr(ehdr, i + 1);
+		phdr = elf_phdr_entry_addr(ehdr, i);
+		memcpy(phdr, tmp_phdr, sizeof(struct elf_phdr));
+		phdr->p_offset = phdr->p_offset - region->size;
+	}
+	memset(tmp_phdr, 0, sizeof(struct elf_phdr));
+	ehdr->e_phnum--;
+
+	/* Clear section header */
+	tmp_shdr = elf_shdr_entry_addr(ehdr, shidx);
+	for (i = shidx; i < ehdr->e_shnum - 1; i++) {
+		tmp_shdr = elf_shdr_entry_addr(ehdr, i + 1);
+		shdr = elf_shdr_entry_addr(ehdr, i);
+		memcpy(shdr, tmp_shdr, sizeof(struct elf_shdr));
+		shdr->sh_offset -= region->size;
+		shdr->sh_name -= len;
+	}
+
+	memset(tmp_shdr, 0, sizeof(struct elf_shdr));
+	ehdr->e_shnum--;
+	md->elf.elf_offset -= region->size;
+
+	return 0;
+}
+
+static void qcom_md_update_elfheader(struct minidump *md,
+				     const struct qcom_minidump_region *region)
+{
+	struct elfhdr *ehdr = md->elf.ehdr;
+	struct elf_shdr *shdr;
+	struct elf_phdr *phdr;
+
+	shdr = elf_shdr_entry_addr(ehdr, ehdr->e_shnum++);
+	phdr = elf_phdr_entry_addr(ehdr, ehdr->e_phnum++);
+
+	shdr->sh_type = SHT_PROGBITS;
+	shdr->sh_name = append_str_to_strtable(md, region->name);
+	shdr->sh_addr = (elf_addr_t)region->virt_addr;
+	shdr->sh_size = region->size;
+	shdr->sh_flags = SHF_WRITE;
+	shdr->sh_offset = md->elf.elf_offset;
+	shdr->sh_entsize = 0;
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = md->elf.elf_offset;
+	phdr->p_vaddr = (elf_addr_t)region->virt_addr;
+	phdr->p_paddr = region->phys_addr;
+	phdr->p_filesz = phdr->p_memsz = region->size;
+	phdr->p_flags = PF_R | PF_W;
+	md->elf.elf_offset += shdr->sh_size;
+}
+
+static void qcom_md_add_region(struct minidump_ss_data *mdss_data,
+			       const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	mdr = &mdss_data->md_regions[region_cnt];
+	strscpy(mdr->name, region->name, sizeof(mdr->name));
+	mdr->address = cpu_to_le64(region->phys_addr);
+	mdr->size = cpu_to_le64(region->size);
+	mdr->valid = cpu_to_le32(MINIDUMP_REGION_VALID);
+	region_cnt++;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+}
+
+static int qcom_md_get_region_index(struct minidump_ss_data *mdss_data,
+				    const struct qcom_minidump_region *region)
+{
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int i;
+	unsigned int count;
+
+	count = le32_to_cpu(mdss_toc->region_count);
+	for (i = 0; i < count; i++) {
+		mdr = &mdss_data->md_regions[i];
+		if (!strcmp(mdr->name, region->name))
+			return i;
+	}
+
+	return -ENOENT;
+}
+
+static int qcom_md_region_unregister(struct minidump *md,
+				     const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	struct minidump_region *mdr;
+	unsigned int region_cnt;
+	unsigned int idx;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret < 0) {
+		dev_err(md->dev, "%s region is not present\n", region->name);
+		return ret;
+	}
+
+	idx = ret;
+	mdr = &mdss_data->md_regions[0];
+	region_cnt = le32_to_cpu(mdss_toc->region_count);
+	/*
+	 * Left shift all the regions exist after this removed region
+	 * index by 1 to fill the gap and zero out the last region
+	 * present at the end.
+	 */
+	memmove(&mdr[idx], &mdr[idx + 1],
+		(region_cnt - idx - 1) * sizeof(struct minidump_region));
+	memset(&mdr[region_cnt - 1], 0, sizeof(struct minidump_region));
+	region_cnt--;
+	mdss_toc->region_count = cpu_to_le32(region_cnt);
+
+	return 0;
+}
+
+static int qcom_md_region_register(struct minidump *md,
+				   const struct qcom_minidump_region *region)
+{
+	struct minidump_ss_data *mdss_data = md->apss_data;
+	struct minidump_subsystem *mdss_toc = mdss_data->md_ss_toc;
+	unsigned int num_region;
+	int ret;
+
+	ret = qcom_md_get_region_index(mdss_data, region);
+	if (ret >= 0) {
+		dev_info(md->dev, "%s region is already registered\n", region->name);
+		return -EEXIST;
+	}
+
+	/* Check if there is a room for a new entry */
+	num_region = le32_to_cpu(mdss_toc->region_count);
+	if (num_region >= MAX_NUM_ENTRIES) {
+		dev_err(md->dev, "maximum region limit %u reached\n", num_region);
+		return -ENOSPC;
+	}
+
+	qcom_md_add_region(mdss_data, region);
+
+	return 0;
+}
+
+static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
+{
+	return region &&
+		strnlen(region->name, MAX_NAME_LENGTH) < MAX_NAME_LENGTH &&
+		region->virt_addr &&
+		region->size &&
+		IS_ALIGNED(region->size, 4);
+}
+
+/**
+ * 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);
+
+/**
+ * qcom_minidump_region_unregister() - Unregister region from APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_minidump_region_unregister(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_unregister(md, region);
+	if (ret)
+		goto unlock;
+
+	ret = qcom_md_clear_elfheader(md, region);
+unlock:
+	mutex_unlock(&md->md_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_region_unregister);
+
+static int qcom_md_add_elfheader(struct minidump *md)
+{
+	struct qcom_minidump_region elfregion;
+	struct elfhdr *ehdr;
+	struct elf_shdr *shdr;
+	struct elf_phdr *phdr;
+	unsigned int  elfh_size;
+	unsigned int strtbl_off;
+	unsigned int phdr_off;
+	unsigned int banner_len;
+	char *banner;
+
+	banner_len = strlen(linux_banner);
+	/*
+	 * Header buffer contains:
+	 * ELF header, (MAX_NUM_ENTRIES + 4) of Section and Program ELF headers,
+	 * where, 4 additional entries, one for empty header, one for string table
+	 * one for minidump table and one for linux banner.
+	 *
+	 * Linux banner is stored in minidump to aid post mortem tools to determine
+	 * the kernel version.
+	 */
+	elfh_size = sizeof(*ehdr);
+	elfh_size += MAX_STRTBL_SIZE;
+	elfh_size += banner_len + 1;
+	elfh_size += ((sizeof(*shdr) + sizeof(*phdr)) * (MAX_NUM_ENTRIES + 4));
+	elfh_size = ALIGN(elfh_size, 4);
+
+	md->elf.ehdr = devm_kzalloc(md->dev, elfh_size, GFP_KERNEL);
+	if (!md->elf.ehdr)
+		return -ENOMEM;
+
+	ehdr = md->elf.ehdr;
+	/* Assign Section/Program headers offset */
+	md->elf.shdr = shdr = (struct elf_shdr *)(ehdr + 1);
+	md->elf.phdr = phdr = (struct elf_phdr *)(shdr + MAX_NUM_ENTRIES);
+	phdr_off = sizeof(*ehdr) + (sizeof(*shdr) * MAX_NUM_ENTRIES);
+
+	memcpy(ehdr->e_ident, ELFMAG, SELFMAG);
+	ehdr->e_ident[EI_CLASS] = ELF_CLASS;
+	ehdr->e_ident[EI_DATA] = ELF_DATA;
+	ehdr->e_ident[EI_VERSION] = EV_CURRENT;
+	ehdr->e_ident[EI_OSABI] = ELF_OSABI;
+	ehdr->e_type = ET_CORE;
+	ehdr->e_machine  = ELF_ARCH;
+	ehdr->e_version = EV_CURRENT;
+	ehdr->e_ehsize = sizeof(*ehdr);
+	ehdr->e_phoff = phdr_off;
+	ehdr->e_phentsize = sizeof(*phdr);
+	ehdr->e_shoff = sizeof(*ehdr);
+	ehdr->e_shentsize = sizeof(*shdr);
+	ehdr->e_shstrndx = 1;
+
+	md->elf.elf_offset = elfh_size;
+	/*
+	 * The zeroth index of the section header is reserved and is rarely used.
+	 * Set the section header as null (SHN_UNDEF) and move to the next one.
+	 * 2nd Section is String table.
+	 */
+	md->elf.strtable_idx = 1;
+	strtbl_off = sizeof(*ehdr) + ((sizeof(*phdr) + sizeof(*shdr)) * MAX_NUM_ENTRIES);
+	shdr++;
+	shdr->sh_type = SHT_STRTAB;
+	shdr->sh_offset = (elf_addr_t)strtbl_off;
+	shdr->sh_size = MAX_STRTBL_SIZE;
+	shdr->sh_entsize = 0;
+	shdr->sh_flags = 0;
+	shdr->sh_name = append_str_to_strtable(md, "STR_TBL");
+	shdr++;
+
+	/* 3rd Section is Linux banner */
+	banner = (char *)ehdr + strtbl_off + MAX_STRTBL_SIZE;
+	memcpy(banner, linux_banner, banner_len);
+
+	shdr->sh_type = SHT_PROGBITS;
+	shdr->sh_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+	shdr->sh_size = banner_len + 1;
+	shdr->sh_addr = (elf_addr_t)linux_banner;
+	shdr->sh_entsize = 0;
+	shdr->sh_flags = SHF_WRITE;
+	shdr->sh_name = append_str_to_strtable(md, "linux_banner");
+
+	phdr->p_type = PT_LOAD;
+	phdr->p_offset = (elf_addr_t)(strtbl_off + MAX_STRTBL_SIZE);
+	phdr->p_vaddr = (elf_addr_t)linux_banner;
+	phdr->p_paddr = virt_to_phys(linux_banner);
+	phdr->p_filesz = phdr->p_memsz = banner_len + 1;
+	phdr->p_flags = PF_R | PF_W;
+
+	/*
+	 * Above are some prdefined sections/program header used
+	 * for debug, update their count here.
+	 */
+	ehdr->e_phnum = 1;
+	ehdr->e_shnum = 3;
+
+	/* Register ELF header as first region */
+	strscpy(elfregion.name, "KELF_HEADER", sizeof(elfregion.name));
+	elfregion.virt_addr = md->elf.ehdr;
+	elfregion.phys_addr = virt_to_phys(md->elf.ehdr);
+	elfregion.size = elfh_size;
+
+	return qcom_md_region_register(md, &elfregion);
+}
+
+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);
+	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;
+	}
+
+	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
+		dev_err(md->dev, "minidump table is not initialized: %d\n", ret);
+		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));
+		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));
+	md = NULL;
+
+	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");
diff --git a/drivers/soc/qcom/qcom_minidump_internal.h b/drivers/soc/qcom/qcom_minidump_internal.h
index 71709235b196..a2aebe5b690a 100644
--- a/drivers/soc/qcom/qcom_minidump_internal.h
+++ b/drivers/soc/qcom/qcom_minidump_internal.h
@@ -9,10 +9,20 @@ 
 #define MAX_NUM_OF_SS           10
 #define MAX_REGION_NAME_LENGTH  16
 #define SBL_MINIDUMP_SMEM_ID	602
+
 #define MINIDUMP_REGION_VALID	   ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_REGION_INVALID	   ('I' << 24 | 'N' << 16 | 'V' << 8 | 'A' << 0)
+#define MINIDUMP_REGION_INIT	   ('I' << 24 | 'N' << 16 | 'I' << 8 | 'T' << 0)
+#define MINIDUMP_REGION_NOINIT	   0
+
+#define MINIDUMP_SS_ENCR_REQ	   (0 << 24 | 'Y' << 16 | 'E' << 8 | 'S' << 0)
+#define MINIDUMP_SS_ENCR_NOTREQ	   (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
+#define MINIDUMP_SS_ENCR_START	   ('S' << 24 | 'T' << 16 | 'R' << 8 | 'T' << 0)
 #define MINIDUMP_SS_ENCR_DONE	   ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
 #define MINIDUMP_SS_ENABLED	   ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
 
+#define MINIDUMP_APSS_DESC	   0
+
 /**
  * struct minidump_region - Minidump region
  * @name		: Name of the region to be dumped
diff --git a/drivers/soc/qcom/smem.c b/drivers/soc/qcom/smem.c
index b0d59e815c3b..a9c8d1c85ad0 100644
--- a/drivers/soc/qcom/smem.c
+++ b/drivers/soc/qcom/smem.c
@@ -270,6 +270,7 @@  struct smem_region {
  * @partitions: list of partitions of current processor/host
  * @item_count: max accepted item number
  * @socinfo:	platform device pointer
+ * @minidump:	minidump platform device pointer
  * @num_regions: number of @regions
  * @regions:	list of the memory regions defining the shared memory
  */
@@ -280,6 +281,7 @@  struct qcom_smem {
 
 	u32 item_count;
 	struct platform_device *socinfo;
+	struct platform_device *minidump;
 	struct smem_ptable *ptable;
 	struct smem_partition global_partition;
 	struct smem_partition partitions[SMEM_HOST_COUNT];
@@ -795,6 +797,13 @@  int qcom_smem_get_soc_id(u32 *id)
 }
 EXPORT_SYMBOL_GPL(qcom_smem_get_soc_id);
 
+void *qcom_smem_minidump_ready(void)
+{
+	return (__smem && __smem->minidump) ? platform_get_drvdata(__smem->minidump) :
+		NULL;
+}
+EXPORT_SYMBOL_GPL(qcom_smem_minidump_ready);
+
 static int qcom_smem_get_sbl_version(struct qcom_smem *smem)
 {
 	struct smem_header *header;
@@ -1174,11 +1183,20 @@  static int qcom_smem_probe(struct platform_device *pdev)
 	if (IS_ERR(smem->socinfo))
 		dev_dbg(&pdev->dev, "failed to register socinfo device\n");
 
+	smem->minidump = platform_device_register_data(&pdev->dev, "qcom-minidump-smem",
+						      PLATFORM_DEVID_NONE, NULL,
+						      0);
+	if (IS_ERR(smem->minidump)) {
+		dev_dbg(&pdev->dev, "failed to register minidump device\n");
+		smem->minidump = NULL;
+	}
+
 	return 0;
 }
 
 static int qcom_smem_remove(struct platform_device *pdev)
 {
+	platform_device_unregister(__smem->minidump);
 	platform_device_unregister(__smem->socinfo);
 
 	hwspin_lock_free(__smem->hwlock);
diff --git a/include/linux/soc/qcom/smem.h b/include/linux/soc/qcom/smem.h
index 223db6a9c733..efcd15b8112b 100644
--- a/include/linux/soc/qcom/smem.h
+++ b/include/linux/soc/qcom/smem.h
@@ -13,4 +13,6 @@  phys_addr_t qcom_smem_virt_to_phys(void *p);
 
 int qcom_smem_get_soc_id(u32 *id);
 
+void *qcom_smem_minidump_ready(void);
+
 #endif
diff --git a/include/soc/qcom/qcom_minidump.h b/include/soc/qcom/qcom_minidump.h
index cd87caef919d..d993604ed2bf 100644
--- a/include/soc/qcom/qcom_minidump.h
+++ b/include/soc/qcom/qcom_minidump.h
@@ -6,6 +6,39 @@ 
 #ifndef _QCOM_MINIDUMP_H_
 #define _QCOM_MINIDUMP_H_
 
+#define MAX_NAME_LENGTH		12
+
+/**
+ * struct qcom_minidump_region - APSS Minidump region information
+ *
+ * @name:	Entry name, Minidump will dump binary with this name.
+ * @virt_addr:  Virtual address of the entry.
+ * @phys_addr:	Physical address of the entry to dump.
+ * @size:	Number of byte to dump from @address location,
+ *		and it should be 4 byte aligned.
+ */
+struct qcom_minidump_region {
+	char		name[MAX_NAME_LENGTH];
+	void		*virt_addr;
+	phys_addr_t	phys_addr;
+	size_t		size;
+};
+
+#if IS_ENABLED(CONFIG_QCOM_MINIDUMP)
+int qcom_minidump_region_register(const struct qcom_minidump_region *region);
+int qcom_minidump_region_unregister(const struct qcom_minidump_region *region);
+#else
+static inline int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+	/* Return quietly, if minidump config is not enabled */
+	return 0;
+}
+static inline int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
+{
+	return 0;
+}
+#endif /* CONFIG_QCOM_MINIDUMP */
+
 struct rproc;
 struct rproc_dump_segment;