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

[Greg KH]

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