mbox series

[RFC,00/14] introduce kmemdump

Message ID 20250422113156.575971-1-eugen.hristev@linaro.org
Headers show
Series introduce kmemdump | expand

Message

Eugen Hristev April 22, 2025, 11:31 a.m. UTC
kmemdump is a mechanism which allows the kernel to mark specific memory
areas for dumping or specific backend usage.
Once regions are marked, kmemdump keeps an internal list with the regions
and registers them in the backend.
Further, depending on the backend driver, these regions can be dumped using
firmware or different hardware block.
Regions being marked beforehand, when the system is up and running, there
is no need nor dependency on a panic handler, or a working kernel that can
dump the debug information.
The kmemdump approach works when pstore, kdump, or another mechanism do not.
Pstore relies on persistent storage, a dedicated RAM area or flash, which
has the disadvantage of having the memory reserved all the time, or another
specific non volatile memory. Some devices cannot keep the RAM contents on
reboot so ramoops does not work. Some devices do not allow kexec to run
another kernel to debug the crashed one.
For such devices, that have another mechanism to help debugging, like
firmware, kmemdump is a viable solution.

kmemdump can create a core image, similar with /proc/vmcore, with only
the registered regions included. This can be loaded into crash tool/gdb and
analyzed.
To have this working, specific information from the kernel is registered,
and this is done at kmemdump init time, no need for the kmemdump user to
do anything.

The implementation is based on the initial Pstore/directly mapped zones
published as an RFC here:
https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/

The back-end implementation for qcom_smem is based on the minidump
patch series and driver written by Mukesh Ojha, thanks:
https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/

I appreciate the feedback on this series, I know it is a longshot, and there
is a lot to improve, but I hope I am on the right track.

Thanks,
Eugen

PS. Here is how crash tool reports the dump:

     KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
    DUMPFILE: /home/eugen/eee
        CPUS: 8 [OFFLINE: 7]
        DATE: Thu Jan  1 02:00:00 EET 1970
      UPTIME: 00:00:28
    NODENAME: qemuarm64
     RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
     VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
     MACHINE: aarch64  (unknown Mhz)
      MEMORY: 0
       PANIC: ""

crash> log
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
[    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
[    0.000000] KASLR enabled
[...]

Eugen Hristev (14):
  Documentation: add kmemdump
  kmemdump: introduce kmemdump
  kmemdump: introduce qcom-md backend driver
  soc: qcom: smem: add minidump device
  Documentation: kmemdump: add section for coreimage ELF
  kmemdump: add coreimage ELF layer
  printk: add kmsg_kmemdump_register
  kmemdump: coreimage: add kmsg registration
  genirq: add irq_kmemdump_register
  kmemdump: coreimage: add irq registration
  panic: add panic_kmemdump_register
  kmemdump: coreimage: add panic registration
  sched: add sched_kmemdump_register
  kmemdump: coreimage: add sched registration

 Documentation/debug/index.rst      |  17 ++
 Documentation/debug/kmemdump.rst   |  83 +++++
 drivers/Kconfig                    |   2 +
 drivers/Makefile                   |   2 +
 drivers/debug/Kconfig              |  39 +++
 drivers/debug/Makefile             |   5 +
 drivers/debug/kmemdump.c           | 197 ++++++++++++
 drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
 drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
 drivers/soc/qcom/smem.c            |  10 +
 include/linux/irqnr.h              |   1 +
 include/linux/kmemdump.h           |  77 +++++
 include/linux/kmsg_dump.h          |   6 +
 include/linux/panic.h              |   1 +
 include/linux/sched.h              |   1 +
 kernel/irq/irqdesc.c               |   7 +
 kernel/panic.c                     |   8 +
 kernel/printk/printk.c             |  13 +
 kernel/sched/core.c                |   7 +
 19 files changed, 1236 insertions(+)
 create mode 100644 Documentation/debug/index.rst
 create mode 100644 Documentation/debug/kmemdump.rst
 create mode 100644 drivers/debug/Kconfig
 create mode 100644 drivers/debug/Makefile
 create mode 100644 drivers/debug/kmemdump.c
 create mode 100644 drivers/debug/kmemdump_coreimage.c
 create mode 100644 drivers/debug/qcom_md.c
 create mode 100644 include/linux/kmemdump.h

Comments

Trilok Soni April 23, 2025, 7:04 a.m. UTC | #1
On 4/22/2025 4:31 AM, Eugen Hristev wrote:
> kmemdump is a mechanism which allows the kernel to mark specific memory
> areas for dumping or specific backend usage.
> Once regions are marked, kmemdump keeps an internal list with the regions
> and registers them in the backend.
> Further, depending on the backend driver, these regions can be dumped using
> firmware or different hardware block.
> Regions being marked beforehand, when the system is up and running, there
> is no need nor dependency on a panic handler, or a working kernel that can
> dump the debug information.
> The kmemdump approach works when pstore, kdump, or another mechanism do not.
> Pstore relies on persistent storage, a dedicated RAM area or flash, which
> has the disadvantage of having the memory reserved all the time, or another
> specific non volatile memory. Some devices cannot keep the RAM contents on
> reboot so ramoops does not work. Some devices do not allow kexec to run
> another kernel to debug the crashed one.
> For such devices, that have another mechanism to help debugging, like
> firmware, kmemdump is a viable solution.
> 
> kmemdump can create a core image, similar with /proc/vmcore, with only
> the registered regions included. This can be loaded into crash tool/gdb and
> analyzed.
> To have this working, specific information from the kernel is registered,
> and this is done at kmemdump init time, no need for the kmemdump user to
> do anything.
> 
> The implementation is based on the initial Pstore/directly mapped zones
> published as an RFC here:
> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
> 
> The back-end implementation for qcom_smem is based on the minidump
> patch series and driver written by Mukesh Ojha, thanks:
> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
> 
> I appreciate the feedback on this series, I know it is a longshot, and there
> is a lot to improve, but I hope I am on the right track.


Is there any way to demonstrate this framework on non-Qualcomm device? Like any
other ARM device from TI, NXP etc; x86/RISC-V based device is also fine.
Petr Mladek May 5, 2025, 3:25 p.m. UTC | #2
On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
> to kmemdump.
> This will allow kmemdump to be able to dump specific log buffer areas on
> demand.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>  
> +void kmsg_kmemdump_register(void)
> +{
> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));

This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
It should be enough to register the memory with the structure.

> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
> +			  sizeof(_printk_rb_static_descs));
> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
> +			  sizeof(_printk_rb_static_infos));

Also this looks wrong. These are static buffers which are used during
early boot. They might later be replaced by dynamically allocated
buffers when a bigger buffer is requested by "log_buf_len" command
line parameter.

I think that we need to register the memory of the structure
and 3 more buffers. See how the bigger buffer is allocated in
setup_log_buf().

I would expect something like:

	unsigned int descs_count;
	unsigned long data_size;

	descs_count = 2 << prb->desc_ring.count_bits;
	data_size = 2 << prb->data_ring.size_bits;

	kmemdump_register("prb", (void *)prb, sizeof(*prb));
	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
			  descs_count * sizeof(struct prb_desc));
	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
			  descs_count * sizeof(struct printk_info));
	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);


But I wonder if this is enough. The current crash dump code also needs
to export the format of the used structures, see
log_buf_vmcoreinfo_setup().

Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

> +}
> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
> +

Best Regards,
Petr
Eugen Hristev May 5, 2025, 3:51 p.m. UTC | #3
Hello Petr,

Thank you for your review.

On 5/5/25 18:25, Petr Mladek wrote:
> On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
>> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
>> to kmemdump.
>> This will allow kmemdump to be able to dump specific log buffer areas on
>> demand.
>>
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
>>  }
>>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
>>  
>> +void kmsg_kmemdump_register(void)
>> +{
>> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
>> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
>> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 
> This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
> It should be enough to register the memory with the structure.

Yes, from my perspective this should be also enough. However, when
loading the generated core dump into crash tool , the tool first looks
for the prb pointer itself, and then stops if the pointer is not readable.
After the prb pointer is being found, the crash tool dereferences it ,
and looks at the indicated address for the actual memory.
That is why the pointer is also saved as a kmemdump region in my proof
of concept.

> 
>> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
>> +			  sizeof(_printk_rb_static_descs));
>> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
>> +			  sizeof(_printk_rb_static_infos));
> 
> Also this looks wrong. These are static buffers which are used during
> early boot. They might later be replaced by dynamically allocated
> buffers when a bigger buffer is requested by "log_buf_len" command
> line parameter.
> 

I will double check whether the crash tool looks for these symbols or
only the memory, and come back with an answer

> I think that we need to register the memory of the structure
> and 3 more buffers. See how the bigger buffer is allocated in
> setup_log_buf().
> 
> I would expect something like:
> 
> 	unsigned int descs_count;
> 	unsigned long data_size;
> 
> 	descs_count = 2 << prb->desc_ring.count_bits;
> 	data_size = 2 << prb->data_ring.size_bits;
> 
> 	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> 	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
> 			  descs_count * sizeof(struct prb_desc));
> 	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
> 			  descs_count * sizeof(struct printk_info));
> 	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);
> 
> 
Thank you. It may be that in my test case, the buffer was not
extended/reallocated with a bigger one.

> But I wonder if this is enough. The current crash dump code also needs
> to export the format of the used structures, see
> log_buf_vmcoreinfo_setup().

It appears that crash tool looks for the structures into vmlinux
symbols. It can be that this information is not available to some tools,
or vmlinux not available, in which case all the used structures format
and sizes need to be exported. But right now, the crash tool does not
work without vmlinux.

> 
> Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?

I believe CONFIG_KMEMDUMP_COREIMAGE should select CONFIG_VMCORE_INFO
indeed, which is not done in my patches. Or I have not fully understood
your question ?


Eugen
> 
>> +}
>> +EXPORT_SYMBOL_GPL(kmsg_kmemdump_register);
>> +
> 
> Best Regards,
> Petr
Petr Mladek May 6, 2025, 7:24 a.m. UTC | #4
On Mon 2025-05-05 18:51:19, Eugen Hristev wrote:
> Hello Petr,
> 
> Thank you for your review.
> 
> On 5/5/25 18:25, Petr Mladek wrote:
> > On Tue 2025-04-22 14:31:49, Eugen Hristev wrote:
> >> Add kmsg_kmemdump_register, which registers prb, log_buf and infos/descs
> >> to kmemdump.
> >> This will allow kmemdump to be able to dump specific log buffer areas on
> >> demand.
> >>
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -4650,6 +4651,18 @@ int kmsg_dump_register(struct kmsg_dumper *dumper)
> >>  }
> >>  EXPORT_SYMBOL_GPL(kmsg_dump_register);
> >>  
> >> +void kmsg_kmemdump_register(void)
> >> +{
> >> +	kmemdump_register("log_buf", (void *)log_buf_addr_get(), log_buf_len_get());
> >> +	kmemdump_register("prb", (void *)&prb, sizeof(prb));
> >> +	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> > 
> > This looks strange. "prb" is a pointer to "struct printk_ringbuffer".
> > It should be enough to register the memory with the structure.
>
> Yes, from my perspective this should be also enough. However, when
> loading the generated core dump into crash tool , the tool first looks
> for the prb pointer itself, and then stops if the pointer is not readable.
> After the prb pointer is being found, the crash tool dereferences it ,
> and looks at the indicated address for the actual memory.
> That is why the pointer is also saved as a kmemdump region in my proof
> of concept.

I see. It makes perfect sense to store the pointer as well after all.

> >> +	kmemdump_register("prb_descs", (void *)_printk_rb_static_descs,
> >> +			  sizeof(_printk_rb_static_descs));
> >> +	kmemdump_register("prb_infos", (void *)_printk_rb_static_infos,
> >> +			  sizeof(_printk_rb_static_infos));
> > 
> > Also this looks wrong. These are static buffers which are used during
> > early boot. They might later be replaced by dynamically allocated
> > buffers when a bigger buffer is requested by "log_buf_len" command
> > line parameter.
> > 
> 
> I will double check whether the crash tool looks for these symbols or
> only the memory, and come back with an answer
> 
> > I think that we need to register the memory of the structure
> > and 3 more buffers. See how the bigger buffer is allocated in
> > setup_log_buf().
> > 
> > I would expect something like:
> > 
> > 	unsigned int descs_count;
> > 	unsigned long data_size;
> > 
> > 	descs_count = 2 << prb->desc_ring.count_bits;
> > 	data_size = 2 << prb->data_ring.size_bits;
> > 
> > 	kmemdump_register("prb", (void *)prb, sizeof(*prb));
> > 	kmemdump_register("prb_descs", (void *)prb->desc_ring->descs,
> > 			  descs_count * sizeof(struct prb_desc));
> > 	kmemdump_register("prb_infos", (void *)prb->desc_ring->infos,
> > 			  descs_count * sizeof(struct printk_info));
> > 	kmemdump_register("prb_data", (void *)prb->data_ring->data, data_size);
> > 
> > 
> Thank you. It may be that in my test case, the buffer was not
> extended/reallocated with a bigger one.

I guess so. A bigger buffer is allocated either when explicitly
requested by "log_buf_len=" command line option. Or when the kernel
is running on a huge system with many CPUs and log_buf_add_cpu()
decides that the default buffer is not big enough for backtraces from
all CPUs.

> > But I wonder if this is enough. The current crash dump code also needs
> > to export the format of the used structures, see
> > log_buf_vmcoreinfo_setup().
> 
> It appears that crash tool looks for the structures into vmlinux
> symbols. It can be that this information is not available to some tools,
> or vmlinux not available, in which case all the used structures format
> and sizes need to be exported. But right now, the crash tool does not
> work without vmlinux.
> > 
> > Is the CONFIG_VMCORE_INFO code shared with the kmemdump, please?
> 
> I believe CONFIG_KMEMDUMP_COREIMAGE should select CONFIG_VMCORE_INFO
> indeed, which is not done in my patches. Or I have not fully understood
> your question ?

I do not see CONFIG_VMCORE_INFO selected in drivers/debug/Kconfig.
But maybe the dependency is defined another way.

Honestly, I did not study all these details. I focused primary on
the printk-related interface and commented what came to my mind.

Also I was not sure how the dumped memory can be analyzed. I expected
that it should be readable by the "crash" tool. But I did not see it explained
in Documentation/debug/kmemdump.rst.

Best Regards,
Petr
Thomas Gleixner May 7, 2025, 10:18 a.m. UTC | #5
$Subject: ... See

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes

On Tue, Apr 22 2025 at 14:31, Eugen Hristev wrote:
> Add function to register irq info into kmemdump.

What is irq info? Please explain explicitly which information is exposed
and why.

>  
> +void irq_kmemdump_register(void)
> +{
> +	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
> +}
> +EXPORT_SYMBOL_GPL(irq_kmemdump_register);

Are you going to slap a gazillion of those register a single variable
functions all over the place?

That's a really horrible idea.

The obvious way to deal with that is to annotate the variable:

static unsigned int nr_irqs = NR_IRQS;
KMEMDUMP_VAR(nr_irqs);

Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
kmemdump specific section and then kmemdump can just walk that section
and dump stuff. No magic register functions and no extra storage
management for static/global variables.

No?

Thanks,

        tglx
Eugen Hristev May 7, 2025, 10:27 a.m. UTC | #6
On 5/7/25 13:18, Thomas Gleixner wrote:
> 
> $Subject: ... See
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-submission-notes
> 
> On Tue, Apr 22 2025 at 14:31, Eugen Hristev wrote:
>> Add function to register irq info into kmemdump.
> 
> What is irq info? Please explain explicitly which information is exposed
> and why.
> 
>>  
>> +void irq_kmemdump_register(void)
>> +{
>> +	kmemdump_register("irq", (void *)&nr_irqs, sizeof(nr_irqs));
>> +}
>> +EXPORT_SYMBOL_GPL(irq_kmemdump_register);
> 
> Are you going to slap a gazillion of those register a single variable
> functions all over the place?
> 
> That's a really horrible idea.
> 
> The obvious way to deal with that is to annotate the variable:
> 
> static unsigned int nr_irqs = NR_IRQS;
> KMEMDUMP_VAR(nr_irqs);
> 
> Let KMEMDUMP_VAR() store the size and the address of 'nr_irqs' in a
> kmemdump specific section and then kmemdump can just walk that section
> and dump stuff. No magic register functions and no extra storage
> management for static/global variables.
> 
> No?

Thank you very much for your review ! I will try it out.

Eugen
> 
> Thanks,
> 
>         tglx
Bjorn Andersson May 7, 2025, 4:54 p.m. UTC | #7
On Tue, Apr 22, 2025 at 02:31:42PM +0300, Eugen Hristev wrote:
> kmemdump is a mechanism which allows the kernel to mark specific memory
> areas for dumping or specific backend usage.
> Once regions are marked, kmemdump keeps an internal list with the regions
> and registers them in the backend.
> Further, depending on the backend driver, these regions can be dumped using
> firmware or different hardware block.
> Regions being marked beforehand, when the system is up and running, there
> is no need nor dependency on a panic handler, or a working kernel that can
> dump the debug information.
> The kmemdump approach works when pstore, kdump, or another mechanism do not.
> Pstore relies on persistent storage, a dedicated RAM area or flash, which
> has the disadvantage of having the memory reserved all the time, or another
> specific non volatile memory. Some devices cannot keep the RAM contents on
> reboot so ramoops does not work. Some devices do not allow kexec to run
> another kernel to debug the crashed one.
> For such devices, that have another mechanism to help debugging, like
> firmware, kmemdump is a viable solution.
> 
> kmemdump can create a core image, similar with /proc/vmcore, with only
> the registered regions included. This can be loaded into crash tool/gdb and
> analyzed.
> To have this working, specific information from the kernel is registered,
> and this is done at kmemdump init time, no need for the kmemdump user to
> do anything.
> 
> The implementation is based on the initial Pstore/directly mapped zones
> published as an RFC here:
> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
> 
> The back-end implementation for qcom_smem is based on the minidump
> patch series and driver written by Mukesh Ojha, thanks:
> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
> 
> I appreciate the feedback on this series, I know it is a longshot, and there
> is a lot to improve, but I hope I am on the right track.
> 
> Thanks,
> Eugen
> 
> PS. Here is how crash tool reports the dump:
> 
>      KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
>     DUMPFILE: /home/eugen/eee

Can you please describe the steps taken to get acquire/generate this
file and how to invoke crash?

Regards,
Bjorn

>         CPUS: 8 [OFFLINE: 7]
>         DATE: Thu Jan  1 02:00:00 EET 1970
>       UPTIME: 00:00:28
>     NODENAME: qemuarm64
>      RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
>      VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>      MACHINE: aarch64  (unknown Mhz)
>       MEMORY: 0
>        PANIC: ""
> 
> crash> log
> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
> [    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
> [    0.000000] KASLR enabled
> [...]
> 
> Eugen Hristev (14):
>   Documentation: add kmemdump
>   kmemdump: introduce kmemdump
>   kmemdump: introduce qcom-md backend driver
>   soc: qcom: smem: add minidump device
>   Documentation: kmemdump: add section for coreimage ELF
>   kmemdump: add coreimage ELF layer
>   printk: add kmsg_kmemdump_register
>   kmemdump: coreimage: add kmsg registration
>   genirq: add irq_kmemdump_register
>   kmemdump: coreimage: add irq registration
>   panic: add panic_kmemdump_register
>   kmemdump: coreimage: add panic registration
>   sched: add sched_kmemdump_register
>   kmemdump: coreimage: add sched registration
> 
>  Documentation/debug/index.rst      |  17 ++
>  Documentation/debug/kmemdump.rst   |  83 +++++
>  drivers/Kconfig                    |   2 +
>  drivers/Makefile                   |   2 +
>  drivers/debug/Kconfig              |  39 +++
>  drivers/debug/Makefile             |   5 +
>  drivers/debug/kmemdump.c           | 197 ++++++++++++
>  drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
>  drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
>  drivers/soc/qcom/smem.c            |  10 +
>  include/linux/irqnr.h              |   1 +
>  include/linux/kmemdump.h           |  77 +++++
>  include/linux/kmsg_dump.h          |   6 +
>  include/linux/panic.h              |   1 +
>  include/linux/sched.h              |   1 +
>  kernel/irq/irqdesc.c               |   7 +
>  kernel/panic.c                     |   8 +
>  kernel/printk/printk.c             |  13 +
>  kernel/sched/core.c                |   7 +
>  19 files changed, 1236 insertions(+)
>  create mode 100644 Documentation/debug/index.rst
>  create mode 100644 Documentation/debug/kmemdump.rst
>  create mode 100644 drivers/debug/Kconfig
>  create mode 100644 drivers/debug/Makefile
>  create mode 100644 drivers/debug/kmemdump.c
>  create mode 100644 drivers/debug/kmemdump_coreimage.c
>  create mode 100644 drivers/debug/qcom_md.c
>  create mode 100644 include/linux/kmemdump.h
> 
> -- 
> 2.43.0
>
Eugen Hristev May 9, 2025, 3:19 p.m. UTC | #8
Hello Bjorn,

On 5/7/25 19:54, Bjorn Andersson wrote:
> On Tue, Apr 22, 2025 at 02:31:42PM +0300, Eugen Hristev wrote:
>> kmemdump is a mechanism which allows the kernel to mark specific memory
>> areas for dumping or specific backend usage.
>> Once regions are marked, kmemdump keeps an internal list with the regions
>> and registers them in the backend.
>> Further, depending on the backend driver, these regions can be dumped using
>> firmware or different hardware block.
>> Regions being marked beforehand, when the system is up and running, there
>> is no need nor dependency on a panic handler, or a working kernel that can
>> dump the debug information.
>> The kmemdump approach works when pstore, kdump, or another mechanism do not.
>> Pstore relies on persistent storage, a dedicated RAM area or flash, which
>> has the disadvantage of having the memory reserved all the time, or another
>> specific non volatile memory. Some devices cannot keep the RAM contents on
>> reboot so ramoops does not work. Some devices do not allow kexec to run
>> another kernel to debug the crashed one.
>> For such devices, that have another mechanism to help debugging, like
>> firmware, kmemdump is a viable solution.
>>
>> kmemdump can create a core image, similar with /proc/vmcore, with only
>> the registered regions included. This can be loaded into crash tool/gdb and
>> analyzed.
>> To have this working, specific information from the kernel is registered,
>> and this is done at kmemdump init time, no need for the kmemdump user to
>> do anything.
>>
>> The implementation is based on the initial Pstore/directly mapped zones
>> published as an RFC here:
>> https://lore.kernel.org/all/20250217101706.2104498-1-eugen.hristev@linaro.org/
>>
>> The back-end implementation for qcom_smem is based on the minidump
>> patch series and driver written by Mukesh Ojha, thanks:
>> https://lore.kernel.org/lkml/20240131110837.14218-1-quic_mojha@quicinc.com/
>>
>> I appreciate the feedback on this series, I know it is a longshot, and there
>> is a lot to improve, but I hope I am on the right track.
>>
>> Thanks,
>> Eugen
>>
>> PS. Here is how crash tool reports the dump:
>>
>>      KERNEL: /home/eugen/linux-minidump/vmlinux  [TAINTED]
>>     DUMPFILE: /home/eugen/eee
> 
> Can you please describe the steps taken to get acquire/generate this
> file and how to invoke crash?
> 

Thank you for looking into this.

Next week, on 16th of May, on Friday, there will be a talk related to
this patch series at Linaro Connect in Lisbon. In that talk I will also
show a demo in which all the process of acquiring the core dump and
crash will be covered.
I will be traveling the following days, if I get the time I will submit
the steps as a reply to this email, if not, then for sure I will submit
them after the talk in Lisbon.

Eugen

> Regards,
> Bjorn
> 
>>         CPUS: 8 [OFFLINE: 7]
>>         DATE: Thu Jan  1 02:00:00 EET 1970
>>       UPTIME: 00:00:28
>>     NODENAME: qemuarm64
>>      RELEASE: 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty
>>      VERSION: #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>>      MACHINE: aarch64  (unknown Mhz)
>>       MEMORY: 0
>>        PANIC: ""
>>
>> crash> log
>> [    0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd4b2]
>> [    0.000000] Linux version 6.14.0-rc5-next-20250303-00014-g011eb2aaf7b6-dirty (eugen@eugen-station) (aarch64-none-linux-gnu-gcc (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 13.3.1 20240614, GNU ld (Arm GNU Toolchain 13.3.Rel1 (Build arm-13.24)) 2.42.0.20240614) #169 SMP PREEMPT Thu Apr 17 14:12:21 EEST 2025
>> [    0.000000] KASLR enabled
>> [...]
>>
>> Eugen Hristev (14):
>>   Documentation: add kmemdump
>>   kmemdump: introduce kmemdump
>>   kmemdump: introduce qcom-md backend driver
>>   soc: qcom: smem: add minidump device
>>   Documentation: kmemdump: add section for coreimage ELF
>>   kmemdump: add coreimage ELF layer
>>   printk: add kmsg_kmemdump_register
>>   kmemdump: coreimage: add kmsg registration
>>   genirq: add irq_kmemdump_register
>>   kmemdump: coreimage: add irq registration
>>   panic: add panic_kmemdump_register
>>   kmemdump: coreimage: add panic registration
>>   sched: add sched_kmemdump_register
>>   kmemdump: coreimage: add sched registration
>>
>>  Documentation/debug/index.rst      |  17 ++
>>  Documentation/debug/kmemdump.rst   |  83 +++++
>>  drivers/Kconfig                    |   2 +
>>  drivers/Makefile                   |   2 +
>>  drivers/debug/Kconfig              |  39 +++
>>  drivers/debug/Makefile             |   5 +
>>  drivers/debug/kmemdump.c           | 197 ++++++++++++
>>  drivers/debug/kmemdump_coreimage.c | 293 ++++++++++++++++++
>>  drivers/debug/qcom_md.c            | 467 +++++++++++++++++++++++++++++
>>  drivers/soc/qcom/smem.c            |  10 +
>>  include/linux/irqnr.h              |   1 +
>>  include/linux/kmemdump.h           |  77 +++++
>>  include/linux/kmsg_dump.h          |   6 +
>>  include/linux/panic.h              |   1 +
>>  include/linux/sched.h              |   1 +
>>  kernel/irq/irqdesc.c               |   7 +
>>  kernel/panic.c                     |   8 +
>>  kernel/printk/printk.c             |  13 +
>>  kernel/sched/core.c                |   7 +
>>  19 files changed, 1236 insertions(+)
>>  create mode 100644 Documentation/debug/index.rst
>>  create mode 100644 Documentation/debug/kmemdump.rst
>>  create mode 100644 drivers/debug/Kconfig
>>  create mode 100644 drivers/debug/Makefile
>>  create mode 100644 drivers/debug/kmemdump.c
>>  create mode 100644 drivers/debug/kmemdump_coreimage.c
>>  create mode 100644 drivers/debug/qcom_md.c
>>  create mode 100644 include/linux/kmemdump.h
>>
>> -- 
>> 2.43.0
>>
Trilok Soni May 9, 2025, 5:31 p.m. UTC | #9
On 4/22/2025 4:31 AM, Eugen Hristev wrote:
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> ---
>  Documentation/debug/index.rst    | 17 +++++++
>  Documentation/debug/kmemdump.rst | 77 ++++++++++++++++++++++++++++++++
>  2 files changed, 94 insertions(+)
>  create mode 100644 Documentation/debug/index.rst
>  create mode 100644 Documentation/debug/kmemdump.rst
> 
> diff --git a/Documentation/debug/index.rst b/Documentation/debug/index.rst
> new file mode 100644
> index 000000000000..9a9365c62f02
> --- /dev/null
> +++ b/Documentation/debug/index.rst
> @@ -0,0 +1,17 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===
> +kmemdump
> +===
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   kmemdump
> +
> +.. only::  subproject and html
> +
> +   Indices
> +   =======
> +
> +   * :ref:`genindex`
> diff --git a/Documentation/debug/kmemdump.rst b/Documentation/debug/kmemdump.rst
> new file mode 100644
> index 000000000000..dfee755a1be1
> --- /dev/null
> +++ b/Documentation/debug/kmemdump.rst
> @@ -0,0 +1,77 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +==========================
> +kmemdump
> +==========================
> +
> +This document provides information about the kmemdump feature.
> +
> +Overview
> +========
> +
> +kmemdump is a mechanism that allows any driver or producer to register a
> +chunk of memory into kmemdump, to be used at a later time for a specific
> +purpose like debugging or memory dumping.
> +
> +kmemdump allows a backend to be connected, this backend interfaces a
> +specific hardware that can debug or dump the memory registered into
> +kmemdump.
> +
> +kmemdump Internals
> +=============

I feel that we are missing to explain "what you need from your firmware"
to support kmemdump on your platform. We should add that section and 
given an example on how Qualcomm does it in their firmware etc;
Bjorn Andersson May 9, 2025, 11:21 p.m. UTC | #10
On Tue, Apr 22, 2025 at 02:31:45PM +0300, Eugen Hristev wrote:
> Qualcomm Minidump is a backend driver for kmemdump.
> Regions are being registered into the shared memory on Qualcomm platforms
> and into the table of contents.
> Further, the firmware can read the table of contents and dump the memory
> accordingly.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
> Co-developed-by: Mukesh Ojha <quic_mojha@quicinc.com>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
>  drivers/debug/Kconfig   |  13 ++
>  drivers/debug/Makefile  |   1 +
>  drivers/debug/qcom_md.c | 467 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 481 insertions(+)
>  create mode 100644 drivers/debug/qcom_md.c
> 
> diff --git a/drivers/debug/Kconfig b/drivers/debug/Kconfig
> index 22348608d187..72a906487e02 100644
> --- a/drivers/debug/Kconfig
> +++ b/drivers/debug/Kconfig
> @@ -13,4 +13,17 @@ config DRIVER_KMEMDUMP
>  
>  	  Note that modules using this feature must be rebuilt if option
>  	  changes.
> +
> +config QCOM_MD_KMEMDUMP_BACKEND

s/md/minidump/ throughout the patch.

> +	tristate "Qualcomm Minidump kmemdump backend driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on DRIVER_KMEMDUMP
> +	help
> +	  Say y here to enable the Qualcomm Minidump kmemdump backend
> +	  driver.
> +	  With this backend, the registered regions are being linked
> +	  into the minidump table of contents. Further on, the firmware
> +	  will be able to read the table of contents and extract the
> +	  memory regions on case-by-case basis.
> +
>  endmenu
> diff --git a/drivers/debug/Makefile b/drivers/debug/Makefile
> index cc14dea250e3..d8a9db29cd15 100644
> --- a/drivers/debug/Makefile
> +++ b/drivers/debug/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_DRIVER_KMEMDUMP) += kmemdump.o
> +obj-$(CONFIG_QCOM_MD_KMEMDUMP_BACKEND) += qcom_md.o
> diff --git a/drivers/debug/qcom_md.c b/drivers/debug/qcom_md.c
> new file mode 100644
> index 000000000000..1aff28e18230
> --- /dev/null
> +++ b/drivers/debug/qcom_md.c
> @@ -0,0 +1,467 @@
> +// SPDX-License-Identifier: GPL-2.0-only

I think you should inherit some copyrights from
drivers/remoteproc/qcom_common.c here...

> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/slab.h>
> +#include <linux/soc/qcom/smem.h>
> +#include <linux/soc/qcom/socinfo.h>

What do you need socinfo.h for? What about of*.h?

> +#include <linux/kmemdump.h>
> +
> +/*
> + * 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

MAX_NUM_REGIONS

> +
> +#define MAX_NUM_OF_SS           10

Drop the "OF" and expand "subsystem".

> +#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_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_SS_ENCR_NOTREQ	   (0 << 24 | 0 << 16 | 'N' << 8 | 'R' << 0)
> +
> +#define MINIDUMP_APSS_DESC	   0

MINIDUMP_SUBSYSTEM_APSS

> +
> +/**
> + * struct minidump - Minidump driver data information
> + * @apss_data: APSS driver data
> + * @md_lock: Lock to protect access to APSS minidump table
> + */
> +struct minidump {

The ordering of structures and functions in this file is haphazard.
You're mixing structures that relates to memory layout with driver
context and related functions are intermixed with unrelated functions.

Effectively the flow through this driver is:
 register
 find_region
 unregister
 register,
 valid_region,
 register
 unregister, 
 init,
 register,
 unregister,
 probe,

Or I guess "flow" is the wrong word here...

> +	struct device		*dev;
> +	struct minidump_ss_data	*apss_data;
> +	struct mutex		md_lock;
> +};
> +
> +/**
> + * struct minidump_region - Minidump region
> + * @name		: Name of the region to be dumped
> + * @seq_num:		: Use to differentiate regions with same name.
> + * @valid		: This entry to be dumped (if set to 1)
> + * @address		: Physical address of region to be dumped
> + * @size		: Size of the region
> + */
> +struct minidump_region {
> +	char	name[MAX_REGION_NAME_LENGTH];
> +	__le32	seq_num;
> +	__le32	valid;
> +	__le64	address;
> +	__le64	size;
> +};
> +
> +/**
> + * struct minidump_subsystem - Subsystem's SMEM Table of content
> + * @status : Subsystem toc init status
> + * @enabled : if set to 1, this region would be copied during coredump
> + * @encryption_status: Encryption status for this subsystem
> + * @encryption_required : Decides to encrypt the subsystem regions or not
> + * @region_count : Number of regions added in this subsystem toc
> + * @regions_baseptr : regions base pointer of the subsystem
> + */
> +struct minidump_subsystem {
> +	__le32	status;
> +	__le32	enabled;
> +	__le32	encryption_status;
> +	__le32	encryption_required;
> +	__le32	region_count;
> +	__le64	regions_baseptr;
> +};
> +
> +/**
> + * struct minidump_global_toc - Global Table of Content
> + * @status : Global Minidump init status
> + * @md_revision : Minidump revision
> + * @enabled : Minidump enable status
> + * @subsystems : Array of subsystems toc
> + */
> +struct minidump_global_toc {
> +	__le32				status;
> +	__le32				md_revision;

Why is this member prefixed with md_ when the others aren't?

> +	__le32				enabled;
> +	struct minidump_subsystem	subsystems[MAX_NUM_OF_SS];
> +};
> +
> +/**
> + * 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;

Please clean these member names up.

> +};
> +
> +#define MINIDUMP_MAX_NAME_LENGTH	12

Why is this 12 here, 16 above, and 8 in kmemdump.c?

> +/**
> + * struct qcom_minidump_region - Minidump region information
> + *
> + * @name:	Minidump region name
> + * @virt_addr:  Virtual address of the entry.
> + * @phys_addr:	Physical address of the entry to dump.
> + * @size:	Number of bytes to dump from @address location,
> + *		and it should be 4 byte aligned.
> + */
> +struct qcom_minidump_region {
> +	char		name[MINIDUMP_MAX_NAME_LENGTH];
> +	void		*virt_addr;
> +	phys_addr_t	phys_addr;
> +	size_t		size;
> +	unsigned int	id;
> +};
> +
> +static LIST_HEAD(apss_md_rlist);

Took me a while to understand that this is the "application subsystem
minidump regions list". Perhaps this could be named
"minidump_regions_list" or something like that? 

> +
> +/**
> + * struct md_region_list - Minidump region list struct
> + *
> + * @md_region:	associated minidump region
> + * @list:  list head entry
> + */
> +struct md_region_list {
> +	struct qcom_minidump_region md_region;
> +	struct list_head list;
> +};
> +
> +static struct minidump *md;
> +
> +/**
> + * qcom_md_add_region() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: None
> + */
> +static void qcom_md_add_region(const struct qcom_minidump_region *region)
> +{
> +	struct minidump_subsystem *mdss_toc = md->apss_data->md_ss_toc;
> +	struct minidump_region *mdr;
> +	unsigned int region_cnt;
> +
> +	region_cnt = le32_to_cpu(mdss_toc->region_count);
> +	mdr = &md->apss_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);
> +}
> +
> +/**
> + * qcom_md_get_region_index() - Lookup minidump region by name
> + * @mdss_data: minidump subsystem data
> + * @region: minidump region.
> + *
> + * Return: On success, it returns the region index, on failure, returns
> + *	negative error value
> + */
> +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;
> +}
> +
> +/**
> + * qcom_md_region_unregister() - Unregister region from APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_md_region_unregister(const struct qcom_minidump_region *region)

Could you please make this API pass the backend pointer, so that backend
drivers doesn't all need to rely on a global pointer to their context.

> +{
> +	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(*mdr));
> +	memset(&mdr[region_cnt - 1], 0, sizeof(*mdr));
> +	region_cnt--;
> +	mdss_toc->region_count = cpu_to_le32(region_cnt);
> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_md_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_md_region_register(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);

struct minidump_region->seq_num tells us that you can have multiple
entries with the same name; so why are you prohibiting this here?

If I understand the code two stack frames up correctly, you have a
cyclic id already - can't you use that as the index?

> +		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(region);

Wouldn't it be better to inline qcom_md_add_region() here, to avoid
checking region_count in one place and then "blindly" appending items to
the array in another place.

> +
> +	return 0;
> +}
> +
> +/**
> + * qcom_minidump_valid_region() - Checks if region is valid
> + * @region: minidump region.
> + *
> + * Return: true if region is valid, false otherwise.
> + */
> +static bool qcom_minidump_valid_region(const struct qcom_minidump_region *region)
> +{
> +	return region &&
> +		strnlen(region->name, MINIDUMP_MAX_NAME_LENGTH) < MINIDUMP_MAX_NAME_LENGTH &&
> +			region->virt_addr &&

This indentation makes it look like region->virt_addr is an argument to
the function call on the line above.

> +			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.
> + */
> +static int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> +	int ret;
> +
> +	if (!qcom_minidump_valid_region(region))

How could that be, you created "region" from scratch in the calling
stack frame?

> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_register(region);
> +
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +
> +/**
> + * 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.
> + */
> +static int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
> +{
> +	int ret;
> +
> +	if (!qcom_minidump_valid_region(region))
> +		return -EINVAL;
> +
> +	mutex_lock(&md->md_lock);
> +	ret = qcom_md_region_unregister(region);
> +
> +	mutex_unlock(&md->md_lock);
> +	return ret;
> +}
> +
> +/**
> + * qcom_apss_md_table_init() - Initialize the minidump table
> + * @mdss_toc: minidump subsystem table of contents
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int qcom_apss_md_table_init(struct minidump_subsystem *mdss_toc)
> +{
> +	struct minidump_ss_data *mdss_data;
> +
> +	mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);

Why is minidump_ss_data a separate object from struct minidump?

> +	if (!mdss_data)
> +		return -ENOMEM;
> +
> +	mdss_data->md_ss_toc = mdss_toc;
> +	mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
> +					     sizeof(*mdss_data->md_regions),
> +					     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;
> +}
> +
> +/**
> + * register_md_region() - Register a new minidump region
> + * @id: unique id to identify the region
> + * @name: name of the region
> + * @vaddr: virtual memory address of the region start
> + * @size: size of the region
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int register_md_region(unsigned int id, char *name, void *vaddr,
> +			      size_t size)
> +{
> +	struct qcom_minidump_region *md_region;
> +	int ret;
> +
> +	struct md_region_list *mdr_list =
> +		kzalloc(sizeof(*mdr_list), GFP_KERNEL);

Weird line wrapping. Use up to 100 characters if you need to... Here
you'll hit 81 though...

> +	if (!mdr_list)
> +		return -ENOMEM;
> +	md_region = &mdr_list->md_region;
> +
> +	scnprintf(md_region->name, sizeof(md_region->name), "K%d%.8s", id, name);
> +	md_region->virt_addr = vaddr;
> +	md_region->phys_addr = virt_to_phys(vaddr);
> +	md_region->size = ALIGN(size, 4);
> +	md_region->id = id;
> +
> +	ret = qcom_minidump_region_register(md_region);
> +	if (ret < 0) {
> +		pr_err("failed to register region in minidump %d %s: err: %d\n",
> +		       id, name, ret);

You already printed when you get back here with an error.

And you're leaking the mdr_list allocation.

> +		return ret;
> +	}
> +
> +	list_add(&mdr_list->list, &apss_md_rlist);
> +	return 0;
> +}
> +
> +/**
> + * unregister_md_region() - Unregister a previously registered minidump region
> + * @id: unique id to identify the region
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +static int unregister_md_region(unsigned int id)
> +{
> +	int ret = -ENOENT;
> +	struct md_region_list *mdr_list;
> +	struct md_region_list *tmp;
> +
> +	list_for_each_entry_safe(mdr_list, tmp, &apss_md_rlist, list) {
> +		struct qcom_minidump_region *region;
> +
> +		region = &mdr_list->md_region;
> +		if (region->id == id) {
> +			ret = qcom_minidump_region_unregister(region);

What does it mean that "ret" is non-zero here? Does it make sense to
return a value from this function? Who's going to do what with that
information?

> +			list_del(&mdr_list->list);
> +			return ret;
> +		}
> +	}
> +
> +	pr_err("failed to unregister region from minidump %d\n", ret);

You can be more specific here...
> +
> +	return ret;

That would be -ENOENT...

> +}
> +
> +static struct kmemdump_backend qcom_md_backend = {
> +	.name = "qcom_md",
> +	.register_region = register_md_region,
> +	.unregister_region = unregister_md_region,
> +};
> +
> +static int qcom_md_probe(struct platform_device *pdev)
> +{
> +	struct minidump_global_toc *mdgtoc;
> +	size_t size;
> +	int ret;
> +
> +	md = devm_kzalloc(&pdev->dev, sizeof(*md), GFP_KERNEL);

Don't devres allocate resources and store the result in a global
pointer. Also, you forgot to check if it failed.

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

> +	}
> +
> +	if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> +		dev_err(md->dev, "minidump table is not initialized %d\n", ret);
> +		return -ENAVAIL;
> +	}
> +
> +	mutex_init(&md->md_lock);
> +
> +	ret = qcom_apss_md_table_init(&mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
> +	if (ret) {
> +		dev_err(md->dev, "apss minidump initialization failed %d\n", ret);

The only way qcom_apss_md_table_init() can fails is malloc(), in which
case you already got a more specific error message. So you can drop this
one.

> +		return ret;

When you return here "md" is a dangling pointer.

Regards,
Bjorn

> +	}
> +
> +	return kmemdump_register_backend(&qcom_md_backend);
> +}
> +
> +static void qcom_md_remove(struct platform_device *pdev)
> +{
> +	kmemdump_unregister_backend(&qcom_md_backend);
> +}
> +
> +static struct platform_driver qcom_md_driver = {
> +	.probe = qcom_md_probe,
> +	.remove = qcom_md_remove,
> +	.driver  = {
> +		.name = "qcom-md",
> +	},
> +};
> +
> +module_platform_driver(qcom_md_driver);
> +
> +MODULE_AUTHOR("Eugen Hristev <eugen.hristev@linaro.org>");
> +MODULE_AUTHOR("Mukesh Ojha <quic_mojha@quicinc.com>");
> +MODULE_DESCRIPTION("Qualcomm kmemdump minidump backend driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.43.0
>