Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1687955688-20809-1-git-send-email-quic_mojha@quicinc.com> <1687955688-20809-5-git-send-email-quic_mojha@quicinc.com>
In-Reply-To: <1687955688-20809-5-git-send-email-quic_mojha@quicinc.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 28 Jun 2023 16:43:34 +0300
Message-ID: <CAHp75VfF=TVBiTBGeDuGdROPFbS=zbc4ABjPL1cCfWe0rTxhQA@mail.gmail.com>
Subject: Re: [PATCH v4 04/21] soc: qcom: Add Qualcomm APSS minidump (frontend)
 feature support
To:     Mukesh Ojha <quic_mojha@quicinc.com>
Cc:     corbet@lwn.net, agross@kernel.org, andersson@kernel.org,
        konrad.dybcio@linaro.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        keescook@chromium.org, tony.luck@intel.com, gpiccoli@igalia.com,
        mathieu.poirier@linaro.org, catalin.marinas@arm.com,
        will@kernel.org, linus.walleij@linaro.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-hardening@vger.kernel.org, linux-remoteproc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 28, 2023 at 3:35=E2=80=AFPM Mukesh Ojha <quic_mojha@quicinc.com=
> 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.
>
> 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.
>
> Minidump kernel driver implementation is divided into two parts for
> simplicity, one is minidump core which can also be called minidump
> frontend(As API gets exported from this driver for registration with
> backend) and the other part is minidump backend i.e, where the underlying
> implementation of minidump will be there. There could be different way
> how the backend is implemented like Shared memory, Memory mapped IO
> or Resource manager based where the guest region information is passed
> to hypervisor via hypercalls.
>
> Minidump Client-1     Client-2      Client-5    Client-n
>          |               |              |             |
>          |               |    ...       |   ...       |
>          |               |              |             |
>          |               |              |             |
>          |               |              |             |
>          |               |              |             |
>          |               |              |             |
>          |               |              |             |
>          |           +---+--------------+----+        |
>          +-----------+  qcom_minidump(core)  +--------+
>                      |                       |
>                      +------+-----+------+---+
>                             |     |      |
>                             |     |      |
>             +---------------+     |      +--------------------+
>             |                     |                           |
>             |                     |                           |
>             |                     |                           |
>             v                     v                           v
>  +-------------------+      +-------------------+     +------------------=
+
>  |qcom_minidump_smem |      |qcom_minidump_mmio |     | qcom_minidump_rm =
|
>  |                   |      |                   |     |                  =
|
>  +-------------------+      +-------------------+     +------------------=
+
>    Shared memory              Memory mapped IO           Resource manager
>     (backend)                   (backend)                   (backend)
>
> Here, we will be giving all analogy of backend with SMEM as it is the
> only implemented backend at present but general idea remains the same.

the general

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

the minidump

> 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 APSS
> region to be dumped as part of ram dump collection. It provides
> appropriate symbol 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.

...

> +#include <linux/device.h>
> +#include <linux/export.h>
> +#include <linux/kallsyms.h>

> +#include <linux/kernel.h>

Why?

And again a lot of missing headers, like

bug.h
dev_printk.h
errno.h
export.h
mutex.h
slab.h

> +#include <linux/module.h>
> +#include <linux/printk.h>
> +#include <linux/string.h>

...

> +/*
> + * In some of the Old Qualcomm devices, boot firmware statically allocat=
es 300
> + * as total number of supported region (including all co-processors) in

regions

> + * minidump table out of which linux was using 201. In future, this limi=
tation
> + * from boot firmware might get removed by allocating the region dynamic=
ally.
> + * So, keep it compatible with older devices, we can keep the current li=
mit for

So, to keep...

> + * Linux to 201.
> + */

...

> +static struct elf_shdr *elf_shdr_entry_addr(struct elfhdr *ehdr, int idx=
)
> +{
> +       struct elf_shdr *eshdr =3D (struct elf_shdr *)((size_t)ehdr + ehd=
r->e_shoff);

Interesting casting pointer to a size_t. Perhaps void * would work
more explicitly?
Ditto for all other cases like this.

> +       return &eshdr[idx];
> +}

...

> +       old_idx +=3D strscpy((strtab + old_idx), name, MAX_REGION_NAME_LE=
NGTH);

(Parentheses are not needed)

strscpy() might return a very big number in this case. Is it a problem?

...

> +unlock:

out_unlock: ?

Ditto for other similar cases.

> +       mutex_unlock(&md_lock);
> +       return ret;

...

> +       /*
> +        * Above are some prdefined sections/program header used

predefined

> +        * for debug, update their count here.
> +        */

...

> +#ifndef _QCOM_MINIDUMP_INTERNAL_H_
> +#define _QCOM_MINIDUMP_INTERNAL_H_

> +#include <linux/elf.h>

Not sure I see how it's used. You may provide forward declarations for
the pointers.

> +#include <soc/qcom/qcom_minidump.h>

+ kconfig.h for IS_ENABLED() ?

MIssing forward declaration:
struct device;

...

>  #ifndef _QCOM_MINIDUMP_H_
>  #define _QCOM_MINIDUMP_H_

+ types.h for phys_addr_t.

...

> + * @size:      Number of byte to dump from @address location,

bytes

> + *             and it should be 4 byte aligned.

--=20
With Best Regards,
Andy Shevchenko