Re: [GIT PULL] Qualcomm driver updates for v6.3

From: Bjorn Andersson <quic_bjorande@quicinc.com>
To: Leo Yan <leo.yan@linaro.org>
Cc: Souradeep Chowdhury <quic_schowdhu@quicinc.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Bjorn Andersson <andersson@kernel.org>, arm <arm@kernel.org>,
	<soc@kernel.org>, <linux-arm-msm@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	Andy Gross <agross@kernel.org>, "Olof Johansson" <olof@lixom.net>,
	Kevin Hilman <khilman@baylibre.com>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	Stephan Gerhold <stephan@gerhold.net>,
	AngeloGioacchino Del Regno 
	<angelogioacchino.delregno@collabora.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
	Guru Das Srinagesh <quic_gurus@quicinc.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
	Dawei Li <set_pte_at@outlook.com>,
	Loic Poulain <loic.poulain@linaro.org>,
	Melody Olvera <quic_molvera@quicinc.com>,
	"Naman Jain" <quic_namajain@quicinc.com>,
	Neil Armstrong <neil.armstrong@linaro.org>,
	Yang Li <yang.lee@linux.alibaba.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	<coresight@lists.linaro.org>,
	"Mathieu Poirier" <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Mike Leach <mike.leach@linaro.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>, Jiri Olsa <jolsa@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	<linux-perf-users@vger.kernel.org>,
	John Garry <john.g.garry@oracle.com>,
	Will Deacon <will@kernel.org>, James Clark <james.clark@arm.com>
Subject: Re: [GIT PULL] Qualcomm driver updates for v6.3
Date: Fri, 24 Mar 2023 10:14:41 -0700	[thread overview]
Message-ID: <20230324171441.GD309305@hu-bjorande-lv.qualcomm.com> (raw)
In-Reply-To: <Y/YHByya0NvoU+If@leoy-yangtze.lan>

On Wed, Feb 22, 2023 at 08:13:59PM +0800, Leo Yan wrote:
> Hi Souradeep,
> 
> On Wed, Feb 22, 2023 at 04:46:07PM +0530, Souradeep Chowdhury wrote:
> > On 2/22/2023 7:13 AM, Leo Yan wrote:
> > > On Wed, Feb 15, 2023 at 04:05:36PM +0100, Arnd Bergmann wrote:
> 
> [...]
> 
> > > > If the possible use is purely for saving some state across
> > > > a reboot, as opposed to other events, I wonder if there is
> > > > a good way to integrate it into the fs/pstore/ code, which
> > > > already has a way to multiplex various kinds of input (log
> > > > buffer, ftrace call chain, userspace strings, ...) into
> > > > various kinds of persistent buffers (sram, blockdev, mtd,
> > > > efivars, ...) with the purpose of helping analyze the
> > > > state after a reboot.
> > > 
> > > Good point!
> > > 
> > > I understand pstore/ramoops is somehow like a sink which routes the
> > > tracing data (software tracing data but not hadware tracing data) to
> > > persistent memory.  This is why we also can route these software
> > > tracing data to STM (hardware sink!).
> > > 
> > > Seems to me, Arnd suggests to connect two sinks between DCC and
> > > pstore (to persistent memory).  But I cannot give an example code in
> > > kernel for doing this way, sorry if I miss something.
> > > 
> > > Essentially, a good user case is to keep a persistent memory for the
> > > tracing data, then after rebooting cycle we can retrieve the tracing
> > > data via user space interface (like sysfs node).
> > 
> > Hi Leo/Arnd,
> > 
> > Just wanted to let you know that the justification of not using PStore was
> > already given in the version 1 of this patch series as below
> > 
> > https://lore.kernel.org/linux-arm-msm/ab30490c016f906fd9bc5d789198530b@codeaurora.org/#r
> > 
> > PStore/Ramoops only persists across warm-reboots which is present for chrome
> > devices but not for android ones.
> 
> Thanks for the info.  Just remind a subtle difference of reboots.
> 
> Besides warm reboot, kernel can reboot system after panic (see kernel
> command line option `panic`) and watchdog can reboot the system as well.
> 
> Even though Android doesn't support warm reboot, system still can reboot
> on panic or by watchdog (in particular after bus lockup), pstore/ramoops
> also can support these cases.
> 

The fact that DDR content isn't maintained across a typical reboot is
certainly causing issues for using pstore.

But afaiu, with pstore we capture various system state into the pstore
and then make that available after reboot. DCC provides a mechanism for
capturing hardware state (by reading register content) and storing that
in DCC-specific SRAM. This can be triggered either by a HW reset, or on
request from the user.

As such it doesn't seem to be a conceptual fit, we perhaps could present
the data as if it was pstore data, but I don't see how the reports
created at runtime would fit into that model.

> > Also the dcc_sram contents can
> > also be collected by going for a software trigger after loading the kernel
> > and the dcc_sram is parsed to get the register values with the
> > opensource parser as below
> > 
> > https://source.codeaurora.org/quic/la/platform/vendor/qcom-opensource/tools/tree/dcc_parser
> 
> To be clear, current driver is fine for me (TBH, I didn't spend much
> time to read it but it's very neat after quickly went through it), I
> just share some info in case it's helpful for the discussion.
> 

My take is still that /dev/dcc looks to provide a reasonable ABI, and
the control mechanism was pushed to debugfs for the purpose of enabling
users to play with the interface, and improve it as necessary.

Regards,
Bjorn