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;
Date:   Wed, 28 Jun 2023 18:53:18 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
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,
        andy.shevchenko@gmail.com, 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
Subject: Re: [PATCH v4 00/21] Add Qualcomm Minidump kernel driver related
 support
Message-ID: <2023062812-exporter-facing-aaf9@gregkh>
References: <1687955688-20809-1-git-send-email-quic_mojha@quicinc.com>
 <2023062814-chance-flounder-f002@gregkh>
 <10dd2ead-758a-89f0-cda4-70ae927269eb@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <10dd2ead-758a-89f0-cda4-70ae927269eb@quicinc.com>
Precedence: bulk

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