Re: [PATCH v4 00/21] Add Qualcomm Minidump kernel driver related support

Greg KH
2023-06-28T16:53:25Z

On Wed, Jun 28, 2023 at 09:50:00PM +0530, Mukesh Ojha wrote:
>
>
> On 6/28/2023 9:15 PM, Greg KH wrote:
> > On Wed, Jun 28, 2023 at 06:04:27PM +0530, 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.
> > >
> > > 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 which snippets should be
> > > included in the ramdump.
> > >
> > > 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(gunyah) 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.
> >
> > If you only have one "backend" then you don't need the extra compexity
> > here at all, just remove that whole middle layer please and make this
> > much simpler and smaller and easier to review and possibly accept.
> >
> > We don't add layers when they are not needed, and never when there is no
> > actual user. If you need the extra "complexity" later, then add it
> > later when it is needed as who knows when that will ever be.
> >
> > Please redo this series based on that, thanks.
>
> I already followed without this middle layer till v3 since without
> the middle layer it will be end up with lot of code duplication if there
> is another backend.

But as this series does not have such a thing, only add it when needed
please. Don't make us review a whole bunch of stuff that is not
actually used here.

Would you want to review such a thing?

> We already have other backend implementation in the downstream, if you
> want to see them, i will try to post them in upcoming series..

Ok, so if you already have it, yes, post it as part of the series,
otherwise such a layer makes no sense.

thanks,

greg k-h