date:20230911





On 9/11/2023 11:31 AM, Pavan Kondeti wrote:

On Sun, Sep 10, 2023 at 01:46:12AM +0530, Mukesh Ojha wrote:

Register all the pstore frontend with minidump, so that they can
be dumped as default Linux minidump region to be collected on
SoC where minidump is enabled.

Helper functions is written in separate file and built along with
the minidump driver, since it is client of minidump and also call
it at appropriate place from minidump probe so that they always
get registered.

While at it also rename the out minidump module object name during
build as qcom_apss_minidump which basically depicts as Qualcomm
Application processor subsystem minidump.

Signed-off-by: Mukesh Ojha 
---
  drivers/soc/qcom/Kconfig |  1 +
  drivers/soc/qcom/Makefile|  3 +-
  drivers/soc/qcom/qcom_minidump.c |  4 ++
  drivers/soc/qcom/qcom_ramoops_minidump.c | 88 
  drivers/soc/qcom/qcom_ramoops_minidump.h | 10 
  5 files changed, 105 insertions(+), 1 deletion(-)
  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
  create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 4b36d46807bc..b3977f1687d8 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -305,6 +305,7 @@ config QCOM_MINIDUMP
tristate "QCOM APSS Minidump driver"
depends on ARCH_QCOM || COMPILE_TEST
depends on QCOM_SMEM
+   depends on PSTORE


Can't we make QC minidump available without PSTORE? PSTORE is another
cllient for minidump, so other clients can still use minidump right?
Where is this hard dependency coming from?


Thanks for asking this question, this was intentionally put here to
continue the discussion forward about minidump existence in kernel tree
and how community want to see it in kernel tree.

Actually, there is no hard dependency of minidump on pstore and  as i 
have said in cover-letter about minidump is going to collect what 
ramoops is offering now or going to offer in future as a default

regions.

So, main minidump driver does not depends on pstore since, i have 
integrated even the minidump client with this

qcom_ramoops_minidump_{register|unregister}()

I may guard this entire qcom_ramoops_minidump_{register|unregister}()
function under CONFIG_PSTORE_RAM, if everyone agree ?

-Mukesh


Thanks,
Pavan

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

On 9/10/2023 1:46 AM, Mukesh Ojha wrote:

Hi All,

This is to continuation from the conversation happened at v4

https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f64...@quicinc.com/

https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0...@quicinc.com/

We have put abstract on LPC on this topic as well as initiated a mail thread
with other SoC vendors but did not get much traction on it.

https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02c...@quicinc.com/

We explored most of possiblity present in kernel to address this issue[1] but
solution like kdump/fadump does not seems safe/secure/performant from our
perspective.

Hence, with this series we tried to make the minidump kernel driver, simple
and tied with pstore frontends, so that it collects the present available
frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
towards enhancing generic pstore to capture more debug data which will be
helpful for first hand of debugging that can benefit both other pstore users
as well as us as minidump users.

One of the proposal made here,
https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mo...@quicinc.com/

Looking forward for your comments.

Thanks,
Mukesh

[1]
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.

The core of SMEM based 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 SMEM to minidump table i.e also
called global table of content (G-ToC). Each subsystem (APSS, ADSP, ...)
has their own table of segments to be included in the minidump and all
get their reference from 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.

Existing upstream Qualcomm remoteproc driver[1] already supports SMEM
based minidump feature for remoteproc instances like ADSP, MODEM, ...
where predefined selective segments of subsystem region can be dumped
as part of coredump collection which generates smaller size artifacts
compared to complete coredump of subsystem on crash.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142

In addition to managing and querying the APSS minidump description,
the Linux driver maintains a ELF header in a segment. This segment
gets updated with section/program header whenever a new entry gets
registered.

Changes in v5:
- On suggestion from Pavan.k, to have single function call for minidump
collection
from remoteproc driver, separated the logic to have separate minidump file
called
qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to
qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy
deletion
during region unregister in this series, will pursue it in next series.

- To simplify the minidump driver, removed the complication for frontend and
different
backend from Greg suggestion, will pursue this once main driver gets
mainlined.

- Move the dynamic ramoops region allocation from Device tree approach to
command line
approch with the introduction command line parsing and memblock reservation
during
early boot up; Not added documentation about it yet, will add if it gets
positive
response.

- Exporting linux banner from kernel to make minidump build also as module,
however,
minidump is a debug module and should be kernel built to get most debug
information
from kernel.

- Tried to address comments given on dload patch series.

Changes in v4:
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/
- Redesigned the driver and divided the driver into front end and backend
(smem) so
that any new backend can be attached easily to avoid code duplication.
- Patch reordering as per the driver and subsystem to easier review of the
code.
- Removed minidump specific code from remoteproc to minidump smem based
driver.
- Enabled the all the driver as modules.
- Address comments made on documentation and yaml and Device tree file
[Krzysztof/Konrad]
- Address comments made qcom_pstore_minidump driver and given its Device tree
same set of properties a

Re: [PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function




On 9/10/2023 1:46 AM, Mukesh Ojha wrote:

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla 
Signed-off-by: Mukesh Ojha 
---



Validated this change on IPQ9574 and IPQ53332 and system is entering 
into the download mode.


Tested-by: Kathiravan Thirumoorthy  # IP9574 
and IPQ5332




  drivers/firmware/qcom_scm.c| 20 
  include/linux/firmware/qcom/qcom_scm.h |  2 ++
  2 files changed, 22 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..5ea8fc4fd4e8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -78,6 +78,7 @@ static const char * const qcom_scm_convention_names[] = {
  };
  
  static struct qcom_scm *__scm;

+static DEFINE_SPINLOCK(lock);
  
  static int qcom_scm_clk_enable(void)

  {
@@ -407,6 +408,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
  }
  EXPORT_SYMBOL(qcom_scm_set_remote_state);
  
+int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int val)

+{
+   unsigned int old, new;
+   int ret;
+
+   spin_lock(&lock);
+   ret = qcom_scm_io_readl(addr, &old);
+   if (ret)
+   goto unlock;
+
+   new = (old & ~mask) | (val & mask);
+
+   ret = qcom_scm_io_writel(addr, new);
+unlock:
+   spin_unlock(&lock);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
+
  static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
  {
struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h 
b/include/linux/firmware/qcom/qcom_scm.h
index 250ea4efb7cb..ca41e4eb33ad 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
  
  extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);

  extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+   unsigned int val);
  
  extern bool qcom_scm_restore_sec_cfg_available(void);

  extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

Re: [PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register




On 9/10/2023 1:46 AM, Mukesh Ojha wrote:

Crashdump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Co-developed-by: Poovendhan Selvaraj 
Signed-off-by: Mukesh Ojha 
---



This change doesn't cleanly apply on next-20230911. Please rebase it.

Validated this change on IPQ9574 and IPQ5332 and system is entering into 
the download mode.


Tested-by: Kathiravan Thirumoorthy  # IPQ9574 
and IPQ5332




  drivers/firmware/qcom_scm.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5ea8fc4fd4e8..eda92f713019 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -5,6 +5,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -30,6 +32,10 @@ module_param(download_mode, bool, 0);
  #define SCM_HAS_IFACE_CLK BIT(1)
  #define SCM_HAS_BUS_CLK   BIT(2)
  
+#define QCOM_DLOAD_MASK		GENMASK(5, 4)

+#define QCOM_DLOAD_FULLDUMP0x1
+#define QCOM_DLOAD_NODUMP  0x0
+
  struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -444,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, 
bool enable)
  
  static void qcom_scm_set_download_mode(bool enable)

  {
+   u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;
  
@@ -453,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)

if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
-   ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-   enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+   ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+  QCOM_DLOAD_MASK,
+  FIELD_PREP(QCOM_DLOAD_MASK, 
val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");

Re: [PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function




On 9/11/2023 1:38 PM, Kathiravan Thirumoorthy wrote:


On 9/10/2023 1:46 AM, Mukesh Ojha wrote:

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla 
Signed-off-by: Mukesh Ojha 
---



Validated this change on IPQ9574 and IPQ53332 and system is entering 
into the download mode.


Tested-by: Kathiravan Thirumoorthy  # 
IP9574 and IPQ5332



Couple of typos. Apologize...

Validated this change on IPQ9574 and IPQ5332 and system is entering into 
the download mode.


Tested-by: Kathiravan Thirumoorthy  # IPQ9574 
and IPQ5332







  drivers/firmware/qcom_scm.c    | 20 
  include/linux/firmware/qcom/qcom_scm.h |  2 ++
  2 files changed, 22 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index fde33acd46b7..5ea8fc4fd4e8 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -78,6 +78,7 @@ static const char * const 
qcom_scm_convention_names[] = {

  };
    static struct qcom_scm *__scm;
+static DEFINE_SPINLOCK(lock);
    static int qcom_scm_clk_enable(void)
  {
@@ -407,6 +408,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
  }
  EXPORT_SYMBOL(qcom_scm_set_remote_state);
  +int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, 
unsigned int val)

+{
+    unsigned int old, new;
+    int ret;
+
+    spin_lock(&lock);
+    ret = qcom_scm_io_readl(addr, &old);
+    if (ret)
+    goto unlock;
+
+    new = (old & ~mask) | (val & mask);
+
+    ret = qcom_scm_io_writel(addr, new);
+unlock:
+    spin_unlock(&lock);
+    return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
+
  static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
  {
  struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h 
b/include/linux/firmware/qcom/qcom_scm.h

index 250ea4efb7cb..ca41e4eb33ad 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
    extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
  extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int 
mask,

+    unsigned int val);
    extern bool qcom_scm_restore_sec_cfg_available(void);
  extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);

Re: [PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register




On 9/10/2023 1:46 AM, Mukesh Ojha wrote:

Crashdump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Co-developed-by: Poovendhan Selvaraj 



Poovendhan's signed-off is also required. Please refer [1]

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/process/submitting-patches.rst#n489



Signed-off-by: Mukesh Ojha 
---
  drivers/firmware/qcom_scm.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5ea8fc4fd4e8..eda92f713019 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -5,6 +5,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -30,6 +32,10 @@ module_param(download_mode, bool, 0);
  #define SCM_HAS_IFACE_CLK BIT(1)
  #define SCM_HAS_BUS_CLK   BIT(2)
  
+#define QCOM_DLOAD_MASK		GENMASK(5, 4)

+#define QCOM_DLOAD_FULLDUMP0x1
+#define QCOM_DLOAD_NODUMP  0x0
+
  struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -444,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, 
bool enable)
  
  static void qcom_scm_set_download_mode(bool enable)

  {
+   u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;
  
@@ -453,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)

if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
-   ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-   enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+   ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+  QCOM_DLOAD_MASK,
+  FIELD_PREP(QCOM_DLOAD_MASK, 
val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

2023-09-11 Thread Bagas Sanjaya

On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:
> Hi All,
> 
> This is to continuation from the conversation happened at v4
> 
> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f64...@quicinc.com/
> 
> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0...@quicinc.com/
> 
> We have put abstract on LPC on this topic as well as initiated a mail thread
> with other SoC vendors but did not get much traction on it.
> 
> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02c...@quicinc.com/
> 
> We explored most of possiblity present in kernel to address this issue[1] but
> solution like kdump/fadump does not seems safe/secure/performant from our
> perspective.
> 
> Hence, with this series we tried to make the minidump kernel driver, simple
> and tied with pstore frontends, so that it collects the present available
> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
> towards enhancing generic pstore to capture more debug data which will be
> helpful for first hand of debugging that can benefit both other pstore users
> as well as us as minidump users.
> 
> One of the proposal made here,
> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mo...@quicinc.com/
> 
> Looking forward for your comments.
> 
> Thanks,
> Mukesh
> 
> [1]
> 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.
> 
> The core of SMEM based 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 SMEM to minidump table i.e also
> called global table of content (G-ToC). Each subsystem (APSS, ADSP, ...)
> has their own table of segments to be included in the minidump and all
> get their reference from 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.
> 
> Existing upstream Qualcomm remoteproc driver[1] already supports SMEM
> based minidump feature for remoteproc instances like ADSP, MODEM, ...
> where predefined selective segments of subsystem region can be dumped
> as part of coredump collection which generates smaller size artifacts
> compared to complete coredump of subsystem on crash.
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142
> 
> In addition to managing and querying the APSS minidump description,
> the Linux driver maintains a ELF header in a segment. This segment
> gets updated with section/program header whenever a new entry gets
> registered.
> 
> Changes in v5:
>  - On suggestion from Pavan.k, to have single function call for minidump 
> collection
>from remoteproc driver, separated the logic to have separate minidump file 
> called
>qcom_rproc_minidump.c and also renamed the function from qcom_minidump() 
> to 
>qcom_rproc_minidump(); however, dropped his suggestion about rework on 
> lazy deletion
>during region unregister in this series, will pursue it in next series.
> 
>  - To simplify the minidump driver, removed the complication for frontend and 
> different
>backend from Greg suggestion, will pursue this once main driver gets 
> mainlined.
> 
>  - Move the dynamic ramoops region allocation from Device tree approach to 
> command line
>approch with the introduction command line parsing and memblock 
> reservation during
>early boot up; Not added documentation about it yet, will add if it gets 
> positive
>response.
> 
>  - Exporting linux banner from kernel to make minidump build also as module, 
> however,
>minidump is a debug module and should be kernel built to get most debug 
> information
>from kernel.
> 
>  - Tried to address comments given on dload patch series. 
> 
> Changes in v4: 
> https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/
>  - Redesigned the driver and divided the driver into front end and backend 
> (smem) so
>that any new backend can be attached easily to avoid code duplication.
>  - Patch reordering as per the driver and subsystem to easier review of the 
> code.
>  - Removed minidump specific code from remoteproc to minidump smem based 
> driver.
>  - Enabled the all the

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

On 9/11/2023 2:22 PM, Bagas Sanjaya wrote:

On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:

Hi All,

This is to continuation from the conversation happened at v4

https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f64...@quicinc.com/

https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0...@quicinc.com/

We have put abstract on LPC on this topic as well as initiated a mail thread
with other SoC vendors but did not get much traction on it.

https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02c...@quicinc.com/

We explored most of possiblity present in kernel to address this issue[1] but
solution like kdump/fadump does not seems safe/secure/performant from our
perspective.

One of the proposal made here,
https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mo...@quicinc.com/

Looking forward for your comments.

Thanks,
Mukesh

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142

- To simplify the minidump driver, removed the complication for frontend and
different
backend from Greg suggestion, will pursue this once main driver gets
mainlined.

- Exporting linux banner from kernel to make minidump build also as module,
however,
minidump is a debug module and should be kernel built to get most debug
information
from kernel.

- Tried to address comments given on dload patch series.

Re: [PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

On 09/09/2023 22:16, Mukesh Ojha wrote:
>  static int qcom_apss_minidump_probe(struct platform_device *pdev)
>  {
>   struct minidump_global_toc *mdgtoc;
> @@ -571,7 +688,10 @@ static int qcom_apss_minidump_probe(struct 
> platform_device *pdev)
>   return ret;
>   }
>  
> + mutex_lock(&md_plist.plock);
>   platform_set_drvdata(pdev, md);

Why this is locked?

> + qcom_apss_register_pending_regions(md);

Why this one is locked? It seems ordering of your operations is not
correct if you need to lock the providers probe().

> + mutex_unlock(&md_plist.plock);


Best regards,
Krzysztof

Re: [REBASE PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register




On 9/11/2023 4:23 PM, Mukesh Ojha wrote:

Crashdump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Co-developed-by: Poovendhan Selvaraj 
Signed-off-by: Poovendhan Selvaraj 
Signed-off-by: Mukesh Ojha 
Tested-by: Kathiravan Thirumoorthy  # IPQ9574 and 
IPQ5332
---
  drivers/firmware/qcom_scm.c | 16 ++--
  1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 321133f0950d..5cacae63ee2a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -5,6 +5,8 @@
  #include 
  #include 
  #include 
+#include 
+#include 
  #include 
  #include 
  #include 
@@ -26,6 +28,14 @@
  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
  module_param(download_mode, bool, 0);
  
+#define SCM_HAS_CORE_CLK	BIT(0)

+#define SCM_HAS_IFACE_CLK  BIT(1)
+#define SCM_HAS_BUS_CLKBIT(2)



Is this intentional to add these macros back again?



+
+#define QCOM_DLOAD_MASKGENMASK(5, 4)
+#define QCOM_DLOAD_FULLDUMP0x1
+#define QCOM_DLOAD_NODUMP  0x0
+
  struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -440,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, 
bool enable)
  
  static void qcom_scm_set_download_mode(bool enable)

  {
+   u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;
  
@@ -449,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)

if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
-   ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-   enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+   ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+  QCOM_DLOAD_MASK,
+  FIELD_PREP(QCOM_DLOAD_MASK, 
val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");

Re: [REBASE PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

On 11/09/2023 12:53, Mukesh Ojha wrote:

...

> +module_platform_driver(qcom_minidump_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-minidump-smem");

Nothing improved here and in other places. To avoid review being spread
all over, I will just NAK this one.

NAK

Best regards,
Krzysztof

[REBASE PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

Pending regions are those apss minidump regions which came for
registration before minidump was initialized or ready to do
register the region.

We can add regions to pending region list and register all of
them from apss minidump driver probe in one go.

Signed-off-by: Mukesh Ojha 
---
 drivers/soc/qcom/qcom_minidump.c | 140 ---
 1 file changed, 130 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 86f4d09a7b4e..4ce36f154e89 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -63,6 +64,33 @@ struct minidump {
struct mutexmd_lock;
 };
 
+/**
+ * struct minidump_pregion - Minidump pending region
+ * @list   : Pending region list pointer
+ * @region : APSS minidump client region
+ */
+struct minidump_pregion {
+   struct list_head list;
+   struct qcom_minidump_region  region;
+};
+
+/**
+ * struct minidump_plist - Minidump pending region list
+ * @plist  : List of pending region to be registered
+ * @pregion_cnt: Count of the pending region to be registered
+ */
+struct minidump_plist {
+   struct list_head  plist;
+   int   pregion_cnt;
+   struct mutex  plock;
+};
+
+static struct minidump_plist md_plist = {
+   .plist = LIST_HEAD_INIT(md_plist.plist),
+   .pregion_cnt = 0,
+   .plock = __MUTEX_INITIALIZER(md_plist.plock),
+};
+
 /*
  * In some of the Old Qualcomm devices, boot firmware statically allocates 300
  * as total number of supported region (including all co-processors) in
@@ -336,6 +364,26 @@ static bool qcom_minidump_valid_region(const struct 
qcom_minidump_region *region
IS_ALIGNED(region->size, 4);
 }
 
+static struct minidump_pregion *
+check_region_in_plist(const struct qcom_minidump_region *region)
+{
+   struct minidump_pregion *md_pregion;
+   struct minidump_pregion *tmp;
+   bool found = false;
+
+   list_for_each_entry_safe(md_pregion, tmp, &md_plist.plist, list) {
+   struct qcom_minidump_region *md_region;
+
+   md_region = &md_pregion->region;
+   if (!strcmp(md_region->name, region->name)) {
+   found = true;
+   break;
+   }
+   }
+
+   return found ? md_pregion : NULL;
+}
+
 /**
  * qcom_minidump_region_register() - Register region in APSS Minidump table.
  * @region: minidump region.
@@ -344,16 +392,44 @@ static bool qcom_minidump_valid_region(const struct 
qcom_minidump_region *region
  */
 int qcom_minidump_region_register(const struct qcom_minidump_region *region)
 {
+   struct minidump_pregion *md_pregion;
struct minidump *md;
-   int ret;
-
-   md = qcom_smem_minidump_ready();
-   if (!md)
-   return -EPROBE_DEFER;
+   int ret = 0;
 
if (!qcom_minidump_valid_region(region))
return -EINVAL;
 
+   mutex_lock(&md_plist.plock);
+   md = qcom_smem_minidump_ready();
+   if (!md) {
+   if (md_plist.pregion_cnt >= MAX_NUM_ENTRIES - 1) {
+   pr_err("maximum region limit %u reached\n", 
md_plist.pregion_cnt);
+   ret = -ENOSPC;
+   goto unlock_plock;
+   }
+
+   md_pregion = check_region_in_plist(region);
+   if (md_pregion) {
+   pr_info("%s region is already exist\n", region->name);
+   ret = -EEXIST;
+   goto unlock_plock;
+   }
+   /*
+* Maintain a list of client regions which came before
+* minidump driver was ready and once it is ready,
+* register them in one go from minidump probe function.
+*/
+   md_pregion = kzalloc(sizeof(*md_pregion), GFP_KERNEL);
+   if (!md_pregion) {
+   ret = -ENOMEM;
+   goto unlock_plock;
+   }
+   md_pregion->region = *region;
+   list_add_tail(&md_pregion->list, &md_plist.plist);
+   md_plist.pregion_cnt++;
+   goto unlock_plock;
+   }
+
mutex_lock(&md->md_lock);
ret = qcom_md_region_register(md, region);
if (ret)
@@ -362,6 +438,10 @@ int qcom_minidump_region_register(const struct 
qcom_minidump_region *region)
qcom_md_update_elfheader(md, region);
 unlock:
mutex_unlock(&md->md_lock);
+   return 0;
+
+unlock_plock:
+   mutex_unlock(&md_plist.plock);
return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
@@ -374,16 +454,28 @@ EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
  */
 int qcom_minidump_region_unregister(const struct qcom_minidump_region *region)
 {
+

[REBASE PATCH v5 08/17] arm64: mm: Add dynamic ramoops region support through command line

The reserved memory region for ramoops is assumed to be at a fixed
and known location when read from the devicetree. This may not be
required for something like Qualcomm's minidump which is interested
in knowing addresses of ramoops region but it does not put hard
requirement of address being fixed as most of it's SoC does not
support warm reset and does not use pstorefs at all instead it has
firmware way of collecting ramoops region if it gets to know the
address and register it with apss minidump table which is sitting
in shared memory region in DDR and firmware will have access to
these table during reset and collects it on crash of SoC.

So, add the support of reserving ramoops region to be dynamically
allocated early during boot if it is request through command line
via 'dyn_ramoops_size=' and fill up reserved resource structure and
export the structure, so that it can be read by ramoops driver.

Signed-off-by: Mukesh Ojha 
---
 arch/arm64/mm/init.c   | 94 ++
 include/linux/pstore_ram.h |  2 +
 2 files changed, 96 insertions(+)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 8a0f8604348b..680c1ce18fbe 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -100,6 +101,93 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
 #define ARM64_MEMSTART_ALIGN   (1UL << ARM64_MEMSTART_SHIFT)
 #endif
 
+#define RAMOOPS_ADDR_HIGH_MAX  (PHYS_MASK + 1)
+
+/* Location of the reserved area for the dynamic ramoops */
+struct resource dyn_ramoops_res = {
+   .name  = "ramoops",
+   .start = 0,
+   .end   = 0,
+   .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
+   .desc  = IORES_DESC_NONE,
+};
+EXPORT_SYMBOL(dyn_ramoops_res);
+
+static int __init parse_dynamic_ramoops(char *cmdline, unsigned long long 
*size)
+{
+   const char *name = "dyn_ramoops_size=";
+   char *p = NULL;
+   char *q = NULL;
+   char *tmp;
+
+   if (!cmdline)
+   return -ENOENT;
+
+   /* Check for "dyn_ramoops_size" and use the later if there are more */
+   p = strstr(cmdline, name);
+   while (p) {
+   q = p;
+   p = strchr(p, ' ');
+   if (!p)
+   break;
+
+   p = strstr(p + 1, name);
+   }
+
+   if (!q) {
+   pr_err("ramoops: No entry found for %s\n", name);
+   return -ENOENT;
+   }
+
+   p = q + strlen(name);
+   *size = memparse(p, &tmp);
+   if (p == tmp) {
+   pr_err("ramoops: memory value expected\n");
+   return -EINVAL;
+   }
+
+   return 0;
+}
+
+static int __init parse_dyn_ramoops_size_dummy(char *arg)
+{
+   return 0;
+}
+early_param("dyn_ramoops_size", parse_dyn_ramoops_size_dummy);
+
+/*
+ * reserve_dynamic_ramoops() - reserves memory for dynamic ramoops
+ *
+ * This enable dynamic reserve memory support for ramoops through
+ * command line.
+ */
+static void __init reserve_dynamic_ramoops(void)
+{
+   char *cmdline = boot_command_line;
+   unsigned long long ramoops_base;
+   unsigned long long ramoops_size;
+
+   if (!IS_ENABLED(CONFIG_PSTORE_RAM))
+   return;
+
+   if (parse_dynamic_ramoops(cmdline, &ramoops_size))
+   return;
+
+   ramoops_base = memblock_phys_alloc_range(ramoops_size, SMP_CACHE_BYTES,
+0, RAMOOPS_ADDR_HIGH_MAX);
+   if (!ramoops_base) {
+   pr_err("cannot allocate ramoops dynamic memory 
(size:0x%llx).\n",
+   ramoops_size);
+   return;
+   }
+
+   kmemleak_ignore_phys(ramoops_base);
+
+   dyn_ramoops_res.start = ramoops_base;
+   dyn_ramoops_res.end = ramoops_base + ramoops_size - 1;
+   insert_resource(&iomem_resource, &dyn_ramoops_res);
+}
+
 static int __init reserve_crashkernel_low(unsigned long long low_size)
 {
unsigned long long low_base;
@@ -481,6 +569,12 @@ void __init bootmem_init(void)
 */
reserve_crashkernel();
 
+   /*
+* Reserving ramoops region resource dynamically in case it is
+* requested from command line.
+*/
+   reserve_dynamic_ramoops();
+
memblock_dump_all();
 }
 
diff --git a/include/linux/pstore_ram.h b/include/linux/pstore_ram.h
index 9d65ff94e216..07d700b7649d 100644
--- a/include/linux/pstore_ram.h
+++ b/include/linux/pstore_ram.h
@@ -10,6 +10,8 @@
 
 #include 
 
+extern struct resource dyn_ramoops_res;
+
 struct persistent_ram_ecc_info {
int block_size;
int ecc_size;
-- 
2.7.4

[REBASE PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

As dynamic ramoops command line parsing is now added, so
lets add the support in ramoops driver to get the resource
structure and add it during platform device registration.

Signed-off-by: Mukesh Ojha 
---
 fs/pstore/ram.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index d36702c7ab3c..ab551caa1d2a 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -914,13 +914,17 @@ static void __init ramoops_register_dummy(void)
 
/*
 * Prepare a dummy platform data structure to carry the module
-* parameters. If mem_size isn't set, then there are no module
-* parameters, and we can skip this.
+* parameters. If mem_size isn't set, check for dynamic ramoops
+* size and extract the information if it is set.
 */
-   if (!mem_size)
+   if (!mem_size && !dyn_ramoops_res.end)
return;
 
pr_info("using module parameters\n");
+   if (dyn_ramoops_res.end) {
+   mem_size = resource_size(&dyn_ramoops_res);
+   mem_address = dyn_ramoops_res.start;
+   }
 
memset(&pdata, 0, sizeof(pdata));
pdata.mem_size = mem_size;
-- 
2.7.4

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

On 09/09/2023 22:16, Mukesh Ojha wrote:
> +/**
> + * qcom_minidump_region_register() - Register region in APSS Minidump table.
> + * @region: minidump region.
> + *
> + * Return: On success, it returns 0 and negative error value on failure.
> + */
> +int qcom_minidump_region_register(const struct qcom_minidump_region *region)
> +{
> + struct minidump *md;
> + int ret;
> +
> + md = qcom_smem_minidump_ready();
> + if (!md)
> + return -EPROBE_DEFER;
> +
> + if (!qcom_minidump_valid_region(region))
> + return -EINVAL;
> +
> + mutex_lock(&md->md_lock);
> + ret = qcom_md_region_register(md, region);
> + if (ret)
> + goto unlock;
> +
> + qcom_md_update_elfheader(md, region);
> +unlock:
> + mutex_unlock(&md->md_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);

NAK, there is no user for this.

Drop all exports from minidump drivers. Your upstream driver *must not*
expose stuff to your vendor drivers.

Best regards,
Krzysztof

[REBASE PATCH v5 04/17] remoteproc: qcom: Remove minidump related data from qcom_common.c

As minidump specific data structure and functions move under
config QCOM_RPROC_MINIDUMP, so remove minidump specific data
from driver/remoteproc/qcom_common.c .

Signed-off-by: Mukesh Ojha 
---
 drivers/remoteproc/qcom_common.c | 160 ---
 1 file changed, 160 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 03e5f5d533eb..085fd73fa23a 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -17,7 +17,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "remoteproc_internal.h"
 #include "qcom_common.h"
@@ -26,61 +25,6 @@
 #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
 #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
 
-#define MAX_NUM_OF_SS   10
-#define MAX_REGION_NAME_LENGTH  16
-#define SBL_MINIDUMP_SMEM_ID   602
-#define MINIDUMP_REGION_VALID  ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' 
<< 0)
-#define MINIDUMP_SS_ENCR_DONE  ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' 
<< 0)
-#define MINIDUMP_SS_ENABLED('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' 
<< 0)
-
-/**
- * struct minidump_region - Minidump region
- * @name   : Name of the region to be dumped
- * @seq_num:   : Use to differentiate regions with same name.
- * @valid  : This entry to be dumped (if set to 1)
- * @address: Physical address of region to be dumped
- * @size   : Size of the region
- */
-struct minidump_region {
-   charname[MAX_REGION_NAME_LENGTH];
-   __le32  seq_num;
-   __le32  valid;
-   __le64  address;
-   __le64  size;
-};
-
-/**
- * struct minidump_subsystem - Subsystem's SMEM Table of content
- * @status : Subsystem toc init status
- * @enabled : if set to 1, this region would be copied during coredump
- * @encryption_status: Encryption status for this subsystem
- * @encryption_required : Decides to encrypt the subsystem regions or not
- * @region_count : Number of regions added in this subsystem toc
- * @regions_baseptr : regions base pointer of the subsystem
- */
-struct minidump_subsystem {
-   __le32  status;
-   __le32  enabled;
-   __le32  encryption_status;
-   __le32  encryption_required;
-   __le32  region_count;
-   __le64  regions_baseptr;
-};
-
-/**
- * struct minidump_global_toc - Global Table of Content
- * @status : Global Minidump init status
- * @md_revision : Minidump revision
- * @enabled : Minidump enable status
- * @subsystems : Array of subsystems toc
- */
-struct minidump_global_toc {
-   __le32  status;
-   __le32  md_revision;
-   __le32  enabled;
-   struct minidump_subsystem   subsystems[MAX_NUM_OF_SS];
-};
-
 struct qcom_ssr_subsystem {
const char *name;
struct srcu_notifier_head notifier_list;
@@ -90,110 +34,6 @@ struct qcom_ssr_subsystem {
 static LIST_HEAD(qcom_ssr_subsystem_list);
 static DEFINE_MUTEX(qcom_ssr_subsys_lock);
 
-static void qcom_minidump_cleanup(struct rproc *rproc)
-{
-   struct rproc_dump_segment *entry, *tmp;
-
-   list_for_each_entry_safe(entry, tmp, &rproc->dump_segments, node) {
-   list_del(&entry->node);
-   kfree(entry->priv);
-   kfree(entry);
-   }
-}
-
-static int qcom_add_minidump_segments(struct rproc *rproc, struct 
minidump_subsystem *subsystem,
-   void (*rproc_dumpfn_t)(struct rproc *rproc, struct 
rproc_dump_segment *segment,
-   void *dest, size_t offset, size_t size))
-{
-   struct minidump_region __iomem *ptr;
-   struct minidump_region region;
-   int seg_cnt, i;
-   dma_addr_t da;
-   size_t size;
-   char *name;
-
-   if (WARN_ON(!list_empty(&rproc->dump_segments))) {
-   dev_err(&rproc->dev, "dump segment list already populated\n");
-   return -EUCLEAN;
-   }
-
-   seg_cnt = le32_to_cpu(subsystem->region_count);
-   ptr = ioremap((unsigned long)le64_to_cpu(subsystem->regions_baseptr),
- seg_cnt * sizeof(struct minidump_region));
-   if (!ptr)
-   return -EFAULT;
-
-   for (i = 0; i < seg_cnt; i++) {
-   memcpy_fromio(®ion, ptr + i, sizeof(region));
-   if (le32_to_cpu(region.valid) == MINIDUMP_REGION_VALID) {
-   name = kstrndup(region.name, MAX_REGION_NAME_LENGTH - 
1, GFP_KERNEL);
-   if (!name) {
-   iounmap(ptr);
-   return -ENOMEM;
-   }
-   da = le64_to_cpu(region.address);
-   size = le64_to_cpu(region.size);
-   rproc_coredump_add_custom_segment(rproc, da, size, 
rproc_dumpfn_t, name);
-   }
-   }
-
-   iounmap(ptr);
-   return

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

2023-09-11 Thread Jeff Johnson


On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:

On 09/09/2023 22:16, Mukesh Ojha wrote:

+/**
+ * qcom_minidump_region_register() - Register region in APSS Minidump table.
+ * @region: minidump region.
+ *
+ * Return: On success, it returns 0 and negative error value on failure.
+ */
+int qcom_minidump_region_register(const struct qcom_minidump_region *region)
+{
+   struct minidump *md;
+   int ret;
+
+   md = qcom_smem_minidump_ready();
+   if (!md)
+   return -EPROBE_DEFER;
+
+   if (!qcom_minidump_valid_region(region))
+   return -EINVAL;
+
+   mutex_lock(&md->md_lock);
+   ret = qcom_md_region_register(md, region);
+   if (ret)
+   goto unlock;
+
+   qcom_md_update_elfheader(md, region);
+unlock:
+   mutex_unlock(&md->md_lock);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_minidump_region_register);


NAK, there is no user for this.

Drop all exports from minidump drivers. Your upstream driver *must not*
expose stuff to your vendor drivers.


Do we not expect that upstream drivers would want to register?
Mind you, in the downstream code the following was a bad limitation:
#define MAX_NUM_OF_SS   10

[REBASE PATCH v5 12/17] MAINTAINERS: Add entry for minidump related files

Add the entries into maintainer file for all the minidump related
modules.

Signed-off-by: Mukesh Ojha 
---
 MAINTAINERS | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2833e2da63e0..2ec5fafe7060 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17787,6 +17787,16 @@ S: Maintained
 F: Documentation/devicetree/bindings/regulator/vqmmc-ipq4019-regulator.yaml
 F: drivers/regulator/vqmmc-ipq4019-regulator.c
 
+QUALCOMM MINIDUMP DRIVER
+M: Mukesh Ojha 
+L: linux-arm-...@vger.kernel.org
+S: Maintained
+F: Documentation/admin-guide/qcom_minidump.rst
+F: drivers/soc/qcom/qcom_rproc_minidump.c
+F: drivers/soc/qcom/qcom_minidump.c
+F: drivers/soc/qcom/qcom_ramoops_minidump.c
+
+
 QUALCOMM NAND CONTROLLER DRIVER
 M: Manivannan Sadhasivam 
 L: linux-...@lists.infradead.org
-- 
2.7.4

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

2023-09-11 Thread Konrad Dybcio

On 11.09.2023 21:09, Jeff Johnson wrote:
> On 9/11/2023 4:07 AM, Krzysztof Kozlowski wrote:
>> On 09/09/2023 22:16, Mukesh Ojha wrote:
>>> +/**
>>> + * qcom_minidump_region_register() - Register region in APSS Minidump 
>>> table.
>>> + * @region: minidump region.
>>> + *
>>> + * Return: On success, it returns 0 and negative error value on failure.
>>> + */
>>> +int qcom_minidump_region_register(const struct qcom_minidump_region 
>>> *region)
>>> +{
>>> +    struct minidump *md;
>>> +    int ret;
>>> +
>>> +    md = qcom_smem_minidump_ready();
>>> +    if (!md)
>>> +    return -EPROBE_DEFER;
>>> +
>>> +    if (!qcom_minidump_valid_region(region))
>>> +    return -EINVAL;
>>> +
>>> +    mutex_lock(&md->md_lock);
>>> +    ret = qcom_md_region_register(md, region);
>>> +    if (ret)
>>> +    goto unlock;
>>> +
>>> +    qcom_md_update_elfheader(md, region);
>>> +unlock:
>>> +    mutex_unlock(&md->md_lock);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(qcom_minidump_region_register);
>>
>> NAK, there is no user for this.
>>
>> Drop all exports from minidump drivers. Your upstream driver *must not*
>> expose stuff to your vendor drivers.
> 
> Do we not expect that upstream drivers would want to register?
> Mind you, in the downstream code the following was a bad limitation:
> #define MAX_NUM_OF_SS   10
No, Krzysztof meant that you are not allowed to export symbols
without immediately providing a user for them - meaning if the
functions are not going to be used upstream, this change will
not be accepted.

Konrad

[REBASE PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

Register all the pstore frontend with minidump, so that they can
be dumped as default Linux minidump region to be collected on
SoC where minidump is enabled.

Helper functions is written in separate file and built along with
the minidump driver, since it is client of minidump and also call
it at appropriate place from minidump probe so that they always
get registered.

While at it also rename the out minidump module object name during
build as qcom_apss_minidump which basically depicts as Qualcomm
Application processor subsystem minidump.

Signed-off-by: Mukesh Ojha 
---
 drivers/soc/qcom/Kconfig |  1 +
 drivers/soc/qcom/Makefile|  3 +-
 drivers/soc/qcom/qcom_minidump.c |  4 ++
 drivers/soc/qcom/qcom_ramoops_minidump.c | 88 
 drivers/soc/qcom/qcom_ramoops_minidump.h | 10 
 5 files changed, 105 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.c
 create mode 100644 drivers/soc/qcom/qcom_ramoops_minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 0ac7afc2c67d..9f1a1e128fef 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -306,6 +306,7 @@ config QCOM_MINIDUMP
tristate "QCOM APSS Minidump driver"
depends on ARCH_QCOM || COMPILE_TEST
depends on QCOM_SMEM
+   depends on PSTORE
help
  This config enables linux core infrastructure for Application
  processor subsystem (APSS) minidump collection i.e, it enables
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 4b5f72f78d3c..69df41aba7a9 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -33,4 +33,5 @@ obj-$(CONFIG_QCOM_ICC_BWMON)  += icc-bwmon.o
 qcom_ice-objs  += ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)  += qcom_rproc_minidump.o
-obj-$(CONFIG_QCOM_MINIDUMP)+= qcom_minidump.o
+obj-$(CONFIG_QCOM_MINIDUMP)+= qcom_apss_minidump.o
+qcom_apss_minidump-objs+= qcom_minidump.o 
qcom_ramoops_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
index 4ce36f154e89..7930a80b9100 100644
--- a/drivers/soc/qcom/qcom_minidump.c
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -23,6 +23,7 @@
 #include 
 
 #include "qcom_minidump_internal.h"
+#include "qcom_ramoops_minidump.h"
 
 /**
  * struct minidump_ss_data - Minidump subsystem private data
@@ -688,6 +689,8 @@ static int qcom_apss_minidump_probe(struct platform_device 
*pdev)
return ret;
}
 
+   qcom_ramoops_minidump_register(md->dev);
+
mutex_lock(&md_plist.plock);
platform_set_drvdata(pdev, md);
qcom_apss_register_pending_regions(md);
@@ -701,6 +704,7 @@ static int qcom_apss_minidump_remove(struct platform_device 
*pdev)
struct minidump *md = platform_get_drvdata(pdev);
struct minidump_ss_data *mdss_data;
 
+   qcom_ramoops_minidump_unregister();
mdss_data = md->apss_data;
memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct 
minidump_subsystem));
md = NULL;
diff --git a/drivers/soc/qcom/qcom_ramoops_minidump.c 
b/drivers/soc/qcom/qcom_ramoops_minidump.c
new file mode 100644
index ..eb97310e3858
--- /dev/null
+++ b/drivers/soc/qcom/qcom_ramoops_minidump.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "qcom_ramoops_minidump.h"
+
+static LIST_HEAD(ramoops_region_list);
+
+struct md_region_list {
+   struct qcom_minidump_region md_region;
+   struct list_head list;
+};
+
+static int qcom_ramoops_region_register(struct device *dev, int type)
+{
+   struct qcom_minidump_region *md_region;
+   struct md_region_list *mdr_list;
+   struct pstore_record record;
+   unsigned int max_dump_cnt;
+   phys_addr_t phys;
+   const char *name;
+   void *virt;
+   size_t size;
+   int ret;
+
+   record.type = type;
+   record.id = 0;
+   max_dump_cnt = 0;
+   name = pstore_type_to_name(record.type);
+   do {
+   ret = pstore_region_defined(&record, &virt, &phys, &size, 
&max_dump_cnt);
+   if (ret < 0)
+   break;
+
+   mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), 
GFP_KERNEL);
+   if (!mdr_list)
+   return -ENOMEM;
+
+   md_region = &mdr_list->md_region;
+   scnprintf(md_region->name, sizeof(md_region->name) - 1, 
"K%s%llu", name, record.id);
+   md_region->virt_addr = virt;
+   md_region->phys_addr = phys;
+   md_region->size = size;
+   ret = qcom_minidump

Re: [PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

On 09/09/2023 22:16, 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.

...

> +static int qcom_apss_md_table_init(struct minidump *md,
> +struct minidump_subsystem *mdss_toc)
> +{
> + struct minidump_ss_data *mdss_data;
> +
> + mdss_data = devm_kzalloc(md->dev, sizeof(*mdss_data), GFP_KERNEL);
> + if (!mdss_data)
> + return -ENOMEM;
> +
> + mdss_data->md_ss_toc = mdss_toc;
> + mdss_data->md_regions = devm_kcalloc(md->dev, MAX_NUM_ENTRIES,
> +  sizeof(struct minidump_region),
> +  GFP_KERNEL);
> + if (!mdss_data->md_regions)
> + return -ENOMEM;
> +
> + mdss_toc = mdss_data->md_ss_toc;
> + mdss_toc->regions_baseptr = 
> cpu_to_le64(virt_to_phys(mdss_data->md_regions));
> + mdss_toc->enabled = cpu_to_le32(MINIDUMP_SS_ENABLED);
> + mdss_toc->status = cpu_to_le32(1);
> + mdss_toc->region_count = cpu_to_le32(0);
> +
> + /* Tell bootloader not to encrypt the regions of this subsystem */
> + mdss_toc->encryption_status = cpu_to_le32(MINIDUMP_SS_ENCR_DONE);
> + mdss_toc->encryption_required = cpu_to_le32(MINIDUMP_SS_ENCR_NOTREQ);
> +
> + md->apss_data = mdss_data;
> +
> + return 0;
> +}
> +
> +static int qcom_apss_minidump_probe(struct platform_device *pdev)
> +{
> + struct minidump_global_toc *mdgtoc;
> + struct minidump *md;
> + size_t size;
> + int ret;
> +
> + md = devm_kzalloc(&pdev->dev, sizeof(struct minidump), GFP_KERNEL);

sizeof(*)

Didn't you get such comments already?


> + if (!md)
> + return -ENOMEM;
> +
> + md->dev = &pdev->dev;
> + mdgtoc = qcom_smem_get(QCOM_SMEM_HOST_ANY, SBL_MINIDUMP_SMEM_ID, &size);
> + if (IS_ERR(mdgtoc)) {
> + ret = PTR_ERR(mdgtoc);
> + dev_err(md->dev, "Couldn't find minidump smem item: %d\n", ret);
> + return ret;

The syntax is:
return dev_err_probe

> + }
> +
> + if (size < sizeof(*mdgtoc) || !mdgtoc->status) {
> + dev_err(md->dev, "minidump table is not initialized: %d\n", 
> ret);

ret is uninitialized here. Please use automated tools for checking your
code:
coccinelle, smatch and sparse

> + return -EINVAL;
> + }
> +
> + mutex_init(&md->md_lock);
> + ret = qcom_apss_md_table_init(md, 
> &mdgtoc->subsystems[MINIDUMP_APSS_DESC]);
> + if (ret) {
> + dev_err(md->dev, "apss minidump initialization failed: %d\n", 
> ret);
> + return ret;
> + }
> +
> + /* First entry would be ELF header */
> + ret = qcom_md_add_elfheader(md);
> + if (ret) {
> + dev_err(md->dev, "Failed to add elf header: %d\n", ret);
> + memset(md->apss_data->md_ss_toc, 0, sizeof(struct 
> minidump_subsystem));

Why do you need it?

> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, md);
> +
> + return ret;
> +}
> +
> +static int qcom_apss_minidump_remove(struct platform_device *pdev)
> +{
> + struct minidump *md = platform_get_drvdata(pdev);
> + struct minidump_ss_data *mdss_data;
> +
> + mdss_data = md->apss_data;
> + memset(mdss_data->md_ss_toc, cpu_to_le32(0), sizeof(struct 
> minidump_subsystem));

Why do you need it?

> + md = NULL;

That's useless assignment.

> +
> + return 0;
> +}
> +
> +static struct platform_driver qcom_minidump_driver = {
> + .probe = qcom_apss_minidump_probe,
> + .remove = qcom_apss_minidump_remove,
> + .driver  = {
> + .name = "qcom-minidump-smem",
> + },
> +};
> +
> +module_platform_driver(qcom_minidump_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm APSS minidump driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:qcom-minidump-smem");

Add a proper ID table instead of re-inventing it with module aliases.

Best regards,
Krzysztof

Re: [PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

2023-09-11 Thread Bagas Sanjaya

On 11/09/2023 17:39, Mukesh Ojha wrote:
> 
> 
> On 9/11/2023 2:22 PM, Bagas Sanjaya wrote:
>> On Sun, Sep 10, 2023 at 01:46:01AM +0530, Mukesh Ojha wrote:
>>> Hi All,
>>>
>>> This is to continuation from the conversation happened at v4
>>>
>>> https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f64...@quicinc.com/
>>>
>>> https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0...@quicinc.com/
>>>
>>> We have put abstract on LPC on this topic as well as initiated a mail thread
>>> with other SoC vendors but did not get much traction on it.
>>>
>>> https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02c...@quicinc.com/
>>>
>>> We explored most of possiblity present in kernel to address this issue[1] 
>>> but
>>> solution like kdump/fadump does not seems safe/secure/performant from our
>>> perspective.
>>>
>>> Hence, with this series we tried to make the minidump kernel driver, simple
>>> and tied with pstore frontends, so that it collects the present available
>>> frontends data like dmesg, ftrace, pmsg, ftrace., Also, we will be working
>>> towards enhancing generic pstore to capture more debug data which will be
>>> helpful for first hand of debugging that can benefit both other pstore users
>>> as well as us as minidump users.
>>>
>>> One of the proposal made here,
>>> https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mo...@quicinc.com/
>>>
>>> Looking forward for your comments.
>>>
>>> Thanks,
>>> Mukesh
>>>
>>> [1]
>>> 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.
>>>
>>> The core of SMEM based 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 SMEM to minidump table i.e also
>>> called global table of content (G-ToC). Each subsystem (APSS, ADSP, ...)
>>> has their own table of segments to be included in the minidump and all
>>> get their reference from 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.
>>>
>>> Existing upstream Qualcomm remoteproc driver[1] already supports SMEM
>>> based minidump feature for remoteproc instances like ADSP, MODEM, ...
>>> where predefined selective segments of subsystem region can be dumped
>>> as part of coredump collection which generates smaller size artifacts
>>> compared to complete coredump of subsystem on crash.
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142
>>>
>>> In addition to managing and querying the APSS minidump description,
>>> the Linux driver maintains a ELF header in a segment. This segment
>>> gets updated with section/program header whenever a new entry gets
>>> registered.
>>>
>>> Changes in v5:
>>>   - On suggestion from Pavan.k, to have single function call for minidump 
>>> collection
>>>     from remoteproc driver, separated the logic to have separate minidump 
>>> file called
>>>     qcom_rproc_minidump.c and also renamed the function from 
>>> qcom_minidump() to
>>>     qcom_rproc_minidump(); however, dropped his suggestion about rework on 
>>> lazy deletion
>>>     during region unregister in this series, will pursue it in next series.
>>>
>>>   - To simplify the minidump driver, removed the complication for frontend 
>>> and different
>>>     backend from Greg suggestion, will pursue this once main driver gets 
>>> mainlined.
>>>
>>>   - Move the dynamic ramoops region allocation from Device tree approach to 
>>> command line
>>>     approch with the introduction command line parsing and memblock 
>>> reservation during
>>>     early boot up; Not added documentation about it yet, will add if it 
>>> gets positive
>>>     response.
>>>
>>>   - Exporting linux banner from kernel to make minidump build also as 
>>> module, however,
>>>     minidump is a debug module and should be kernel built to get most debug 
>>> information
>>>     from kernel.
>>>
>>>   - Tried to address comments given on dload patch series.
>>>
>>> Changes in v4: 
>>> https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/
>>>   - Redesigned the driver and divided the driver into front end

[REBASE PATCH v5 06/17] soc: qcom: Add Qualcomm APSS minidump kernel driver

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.

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

Signed-off-by: Mukesh Ojha 
---
 drivers/soc/qcom/Kconfig  |  13 +
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/qcom_minidump.c  | 603 ++
 drivers/soc/qcom/qcom_minidump_internal.h |  10 +
 drivers/soc/qcom/smem.c   |  18 +
 include/linux/soc/qcom/smem.h |   2 +
 include/soc/qcom/qcom_minidump.h  |  33 ++
 7 files changed, 680 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_minidump.c

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index ceca58eaa445..0ac7afc2c67d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -301,4 +301,17 @@ config QCOM_RPROC_MINIDUMP
  query predefined minidump segments associated with the remote 
processor
  and check its validity and end up collecting the dump on remote 
processor
  crash during its recovery.
+
+config QCOM_MINIDUMP
+   tristate "QCOM APSS Minidump driver"
+   depends on ARCH_QCOM || COMPILE_TEST
+   depends on QCOM_SMEM
+   help
+ This config enables linux core infrastructure for Application
+ processor subsystem (APSS) minidump collection i.e, it enables
+ Linux clients drivers to register their internal data structures
+ and debug messages as part of the apss minidump table and when
+ the SoC is crashed, these selective regions will be dumped
+ instead of the entire DDR dump. This saves significant amount
+ of time and/or storage space.
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 838528e7e30a..4b5f72f78d3c 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_QCOM_ICC_BWMON)  += icc-bwmon.o
 qcom_ice-objs  += ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)+= qcom_ice.o
 obj-$(CONFIG_QCOM_RPROC_MINIDUMP)  += qcom_rproc_minidump.o
+obj-$(CONFIG_QCOM_MINIDUMP)+= qcom_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump.c b/drivers/soc/qcom/qcom_minidump.c
new file mode 100644
index ..86f4d09a7b4e
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump.c
@@ -0,0 +1,603 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "qcom_minidump_internal.h"
+
+/**
+ * struct minidump_ss_data - Minidump subsystem private data
+ * @md_ss_toc  : Application Subsystem TOC pointer
+ * @md_regions : Application Subsystem region base pointer
+ */
+struct minidump_ss_data {
+   struct minidump_subsystem   *md_ss_toc;
+   struct minidump_region  *md_regions;
+};
+
+/**
+ * struct minidump_elfhdr - Minidump table elf header
+ * @ehdr: elf main header
+ * @shdr: Section header
+ * @phdr: Program header
+ * @elf_offset: Section offset in elf
+ * @strtable_idx: String table current index position
+ */
+struct minidump_elfhdr {
+   struct elfhdr   *ehdr;
+   struct elf_shdr *shdr;

[REBASE PATCH v5 02/17] soc: qcom: Add qcom_rproc_minidump module

Add qcom_rproc_minidump module in a preparation to remove
minidump specific code from driver/remoteproc/qcom_common.c
and provide needed exported API, this as well helps to
abstract minidump specific data layout from qualcomm's
remoteproc driver.

It is just a copying of qcom_minidump() functionality from
driver/remoteproc/qcom_common.c into a separate file under
qcom_rproc_minidump().

Signed-off-by: Mukesh Ojha 
---
 drivers/soc/qcom/Kconfig  |  10 +++
 drivers/soc/qcom/Makefile |   1 +
 drivers/soc/qcom/qcom_minidump_internal.h |  64 +
 drivers/soc/qcom/qcom_rproc_minidump.c| 111 ++
 include/soc/qcom/qcom_minidump.h  |  23 +++
 5 files changed, 209 insertions(+)
 create mode 100644 drivers/soc/qcom/qcom_minidump_internal.h
 create mode 100644 drivers/soc/qcom/qcom_rproc_minidump.c
 create mode 100644 include/soc/qcom/qcom_minidump.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 715348869d04..ceca58eaa445 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -291,4 +291,14 @@ config QCOM_INLINE_CRYPTO_ENGINE
tristate
select QCOM_SCM
 
+config QCOM_RPROC_MINIDUMP
+   tristate "QCOM Remoteproc Minidump Support"
+   depends on ARCH_QCOM || COMPILE_TEST
+   depends on QCOM_SMEM
+   help
+ Enablement of core minidump feature is controlled from boot firmware
+ side, so if it is enabled from firmware, this config allow linux to
+ query predefined minidump segments associated with the remote 
processor
+ and check its validity and end up collecting the dump on remote 
processor
+ crash during its recovery.
 endmenu
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index bbca2e1e55bb..838528e7e30a 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -32,3 +32,4 @@ obj-$(CONFIG_QCOM_KRYO_L2_ACCESSORS) +=   
kryo-l2-accessors.o
 obj-$(CONFIG_QCOM_ICC_BWMON)   += icc-bwmon.o
 qcom_ice-objs  += ice.o
 obj-$(CONFIG_QCOM_INLINE_CRYPTO_ENGINE)+= qcom_ice.o
+obj-$(CONFIG_QCOM_RPROC_MINIDUMP)  += qcom_rproc_minidump.o
diff --git a/drivers/soc/qcom/qcom_minidump_internal.h 
b/drivers/soc/qcom/qcom_minidump_internal.h
new file mode 100644
index ..71709235b196
--- /dev/null
+++ b/drivers/soc/qcom/qcom_minidump_internal.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _QCOM_MINIDUMP_INTERNAL_H_
+#define _QCOM_MINIDUMP_INTERNAL_H_
+
+#define MAX_NUM_OF_SS   10
+#define MAX_REGION_NAME_LENGTH  16
+#define SBL_MINIDUMP_SMEM_ID   602
+#define MINIDUMP_REGION_VALID ('V' << 24 | 'A' << 16 | 'L' << 8 | 'I' << 0)
+#define MINIDUMP_SS_ENCR_DONE ('D' << 24 | 'O' << 16 | 'N' << 8 | 'E' << 0)
+#define MINIDUMP_SS_ENABLED   ('E' << 24 | 'N' << 16 | 'B' << 8 | 'L' << 0)
+
+/**
+ * struct minidump_region - Minidump region
+ * @name   : Name of the region to be dumped
+ * @seq_num:   : Use to differentiate regions with same name.
+ * @valid  : This entry to be dumped (if set to 1)
+ * @address: Physical address of region to be dumped
+ * @size   : Size of the region
+ */
+struct minidump_region {
+   charname[MAX_REGION_NAME_LENGTH];
+   __le32  seq_num;
+   __le32  valid;
+   __le64  address;
+   __le64  size;
+};
+
+/**
+ * struct minidump_subsystem - Subsystem's SMEM Table of content
+ * @status : Subsystem toc init status
+ * @enabled : if set to 1, this region would be copied during coredump
+ * @encryption_status: Encryption status for this subsystem
+ * @encryption_required : Decides to encrypt the subsystem regions or not
+ * @region_count : Number of regions added in this subsystem toc
+ * @regions_baseptr : regions base pointer of the subsystem
+ */
+struct minidump_subsystem {
+   __le32  status;
+   __le32  enabled;
+   __le32  encryption_status;
+   __le32  encryption_required;
+   __le32  region_count;
+   __le64  regions_baseptr;
+};
+
+/**
+ * struct minidump_global_toc - Global Table of Content
+ * @status : Global Minidump init status
+ * @md_revision : Minidump revision
+ * @enabled : Minidump enable status
+ * @subsystems : Array of subsystems toc
+ */
+struct minidump_global_toc {
+   __le32  status;
+   __le32  md_revision;
+   __le32  enabled;
+   struct minidump_subsystem   subsystems[MAX_NUM_OF_SS];
+};
+
+#endif /* _QCOM_MINIDUMP_INTERNAL_H_ */
diff --git a/drivers/soc/qcom/qcom_rproc_minidump.c 
b/drivers/soc/qcom/qcom_rproc_minidump.c
new file mode 100644
index ..9bc84cc2536f
--- /dev/null
+++ b/drivers/soc/qcom/qcom_rproc_minidump.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*

[REBASE PATCH v5 05/17] init: export linux_banner data variable

Some debug loadable module like minidump is interested in knowing
the kernel version against which it is being build. Let's export
linux_banner.

Signed-off-by: Mukesh Ojha 
---
 include/linux/init.h | 3 +++
 init/version-timestamp.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/init.h b/include/linux/init.h
index 29e75bbe7984..7c24d3ec0ff2 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -149,6 +149,9 @@ extern char *saved_command_line;
 extern unsigned int saved_command_line_len;
 extern unsigned int reset_devices;
 
+/* Defined in init/version-timestamp.c */
+extern const char linux_banner[];
+
 /* used by init/main.c */
 void setup_arch(char **);
 void prepare_namespace(void);
diff --git a/init/version-timestamp.c b/init/version-timestamp.c
index 043cbf80a766..a48f2c19e5d7 100644
--- a/init/version-timestamp.c
+++ b/init/version-timestamp.c
@@ -6,6 +6,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct uts_namespace init_uts_ns = {
.ns.count = REFCOUNT_INIT(2),
@@ -28,3 +29,5 @@ struct uts_namespace init_uts_ns = {
 const char linux_banner[] =
"Linux version " UTS_RELEASE " (" LINUX_COMPILE_BY "@"
LINUX_COMPILE_HOST ") (" LINUX_COMPILER ") " UTS_VERSION "\n";
+
+EXPORT_SYMBOL_GPL(linux_banner);
-- 
2.7.4

[REBASE PATCH v5 03/17] remoteproc: qcom_q6v5_pas: Use qcom_rproc_minidump()

Now, as all the minidump specific data structure is moved to
minidump specific files and implementation wise qcom_rproc_minidump()
and qcom_minidump() exactly same and the name qcom_rproc_minidump
make more sense as it happen to collect the minidump for the
remoteproc processors. So, let's use qcom_rproc_minidump() and
we will be removing qcom_minidump() and minidump related stuff
from driver/remoteproc/qcom_common.c .

Signed-off-by: Mukesh Ojha 
---
 drivers/remoteproc/Kconfig | 1 +
 drivers/remoteproc/qcom_q6v5_pas.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 48845dc8fa85..cea960749e2c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -166,6 +166,7 @@ config QCOM_PIL_INFO
 
 config QCOM_RPROC_COMMON
tristate
+   select QCOM_RPROC_MINIDUMP
 
 config QCOM_Q6V5_COMMON
tristate
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index b5447dd2dd35..b0c766870524 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -25,6 +25,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "qcom_common.h"
 #include "qcom_pil_info.h"
@@ -131,7 +132,7 @@ static void adsp_minidump(struct rproc *rproc)
if (rproc->dump_conf == RPROC_COREDUMP_DISABLED)
return;
 
-   qcom_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
+   qcom_rproc_minidump(rproc, adsp->minidump_id, adsp_segment_dump);
 }
 
 static int adsp_pds_enable(struct qcom_adsp *adsp, struct device **pds,
-- 
2.7.4

[REBASE PATCH v5 14/17] pinctrl: qcom: Use qcom_scm_io_update_field()

Use qcom_scm_io_update_field() exported function in
pinctrl-msm driver.

Signed-off-by: Mukesh Ojha 
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c 
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 115b83e2d8e6..2b9182375702 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1078,22 +1078,20 @@ static int msm_gpio_irq_set_type(struct irq_data *d, 
unsigned int type)
if (g->intr_target_width)
intr_target_mask = GENMASK(g->intr_target_width - 1, 0);
 
+   intr_target_mask <<= g->intr_target_bit;
if (pctrl->intr_target_use_scm) {
u32 addr = pctrl->phys_base[0] + g->intr_target_reg;
int ret;
 
-   qcom_scm_io_readl(addr, &val);
-   val &= ~(intr_target_mask << g->intr_target_bit);
-   val |= g->intr_target_kpss_val << g->intr_target_bit;
-
-   ret = qcom_scm_io_writel(addr, val);
+   val = g->intr_target_kpss_val << g->intr_target_bit;
+   ret = qcom_scm_io_update_field(addr, intr_target_mask, val);
if (ret)
dev_err(pctrl->dev,
"Failed routing %lu interrupt to Apps proc",
d->hwirq);
} else {
val = msm_readl_intr_target(pctrl, g);
-   val &= ~(intr_target_mask << g->intr_target_bit);
+   val &= ~intr_target_mask;
val |= g->intr_target_kpss_val << g->intr_target_bit;
msm_writel_intr_target(val, pctrl, g);
}
-- 
2.7.4

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource





On 9/11/2023 11:03 AM, Pavan Kondeti wrote:

On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:

As dynamic ramoops command line parsing is now added, so
lets add the support in ramoops driver to get the resource
structure and add it during platform device registration.

Signed-off-by: Mukesh Ojha 
---
  fs/pstore/ram.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)



Documentation/admin-guide/ramoops.rst might need an update as well.


I have said in the cover-letter under changes in v5, it is open for
comment and not yet documented it yet.




diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index 2f625e1fa8d8..e73fbbc1b5b5 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -913,13 +913,17 @@ static void __init ramoops_register_dummy(void)
  
  	/*

 * Prepare a dummy platform data structure to carry the module
-* parameters. If mem_size isn't set, then there are no module
-* parameters, and we can skip this.
+* parameters. If mem_size isn't set, check for dynamic ramoops
+* size and extract the information if it is set.
 */
-   if (!mem_size)
+   if (!mem_size && !dyn_ramoops_res.end)
return;
  
  	pr_info("using module parameters\n");

+   if (dyn_ramoops_res.end) {
+   mem_size = resource_size(&dyn_ramoops_res);
+   mem_address = dyn_ramoops_res.start;
+   }
  
  	memset(&pdata, 0, sizeof(pdata));

pdata.mem_size = mem_size;


You might want to add "arch_" prefix to dyn_ramoops resource so that it
would be clear that it is coming from arch code. This code needs to
guard against arch not supplying this.


Sure, thanks for pointing this.
Agree, if we finally decide to keep it in arch/arm64 .

-Mukesh


Thanks,
Pavan

Re: [PATCH v5 07/17] soc: qcom: minidump: Add pending region registration

2023-09-11 Thread Jeff Johnson


On 9/9/2023 1:16 PM, Mukesh Ojha wrote:

+/**
+ * struct minidump_pregion - Minidump pending region
+ * @list   : Pending region list pointer
+ * @region : APSS minidump client region


does kernel-doc parse this correctly? should not be whitespace between 
@ID and ":"


refer to

Re: [PATCH v5 11/17] qcom_minidump: Register ramoops region with minidump

On 09/09/2023 22:16, Mukesh Ojha wrote:
> Register all the pstore frontend with minidump, so that they can
> be dumped as default Linux minidump region to be collected on
> SoC where minidump is enabled.
> 

...

> +
> + record.type = type;
> + record.id = 0;
> + max_dump_cnt = 0;
> + name = pstore_type_to_name(record.type);
> + do {
> + ret = pstore_region_defined(&record, &virt, &phys, &size, 
> &max_dump_cnt);
> + if (ret < 0)
> + break;
> +
> + mdr_list = devm_kzalloc(dev, sizeof(struct md_region_list), 
> GFP_KERNEL);

sizeof(*)

Please fix it everywhere in your code.

> + if (!mdr_list)
> + return -ENOMEM;
> +
> + md_region = &mdr_list->md_region;
> + scnprintf(md_region->name, sizeof(md_region->name) - 1, 
> "K%s%llu", name, record.id);
> + md_region->virt_addr = virt;
> + md_region->phys_addr = phys;
> + md_region->size = size;
> + ret = qcom_minidump_region_register(md_region);
> + if (ret) {
> + pr_err("failed to register minidump region\n");
> + break;
> + }
> +
> + list_add(&mdr_list->list, &ramoops_region_list);
> + } while (record.id < max_dump_cnt && ++record.id);
> +
> + return ret;
> +}


Best regards,
Krzysztof

[REBASE PATCH v5 16/17] firmware: qcom_scm: Refactor code to support multiple download mode

Currently on Qualcomm SoC, download_mode is enabled if
CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT is selected.

Refactor the code such that it supports multiple download
modes and drop CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT config
instead, give interface to set the download mode from
module parameter.

Signed-off-by: Mukesh Ojha 
---
 drivers/firmware/Kconfig| 11 -
 drivers/firmware/qcom_scm.c | 56 +++--
 2 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index b59e3041fd62..ff7e9f330559 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -215,17 +215,6 @@ config MTK_ADSP_IPC
 config QCOM_SCM
tristate
 
-config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
-   bool "Qualcomm download mode enabled by default"
-   depends on QCOM_SCM
-   help
- A device with "download mode" enabled will upon an unexpected
- warm-restart enter a special debug mode that allows the user to
- "download" memory content over USB for offline postmortem analysis.
- The feature can be enabled/disabled on the kernel command line.
-
- Say Y here to enable "download mode" by default.
-
 config SYSFB
bool
select BOOT_VESA_SUPPORT
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 5cacae63ee2a..4ecdb0d025a4 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -20,13 +20,13 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
 #include "qcom_scm.h"
 
-static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
-module_param(download_mode, bool, 0);
+static u32 download_mode;
 
 #define SCM_HAS_CORE_CLK   BIT(0)
 #define SCM_HAS_IFACE_CLK  BIT(1)
@@ -83,6 +83,11 @@ static const char * const qcom_scm_convention_names[] = {
[SMC_CONVENTION_LEGACY] = "smc legacy",
 };
 
+static const char * const download_mode_name[] = {
+   [QCOM_DLOAD_NODUMP] = "off",
+   [QCOM_DLOAD_FULLDUMP]   = "full",
+};
+
 static struct qcom_scm *__scm;
 static DEFINE_SPINLOCK(lock);
 
@@ -448,9 +453,10 @@ static int __qcom_scm_set_dload_mode(struct device *dev, 
bool enable)
return qcom_scm_call_atomic(__scm->dev, &desc, NULL);
 }
 
-static void qcom_scm_set_download_mode(bool enable)
+static void qcom_scm_set_download_mode(u32 download_mode)
 {
-   u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
+   bool enable = !!download_mode;
+   u32 val = download_mode;
bool avail;
int ret = 0;
 
@@ -1430,6 +1436,42 @@ static irqreturn_t qcom_scm_irq_handler(int irq, void 
*data)
return IRQ_HANDLED;
 }
 
+static int get_download_mode(char *buffer, const struct kernel_param *kp)
+{
+   if (download_mode >= ARRAY_SIZE(download_mode_name))
+   return sysfs_emit(buffer, "unknown mode\n");
+
+   return sysfs_emit(buffer, "%s\n", download_mode_name[download_mode]);
+}
+
+static int set_download_mode(const char *val, const struct kernel_param *kp)
+{
+   u32 old = download_mode;
+   int ret;
+
+   ret = sysfs_match_string(download_mode_name, val);
+   if (ret < 0) {
+   download_mode = old;
+   pr_err("qcom_scm: unknown download mode: %s\n", val);
+   return -EINVAL;
+   }
+
+   download_mode = ret;
+   if (__scm)
+   qcom_scm_set_download_mode(download_mode);
+
+   return 0;
+}
+
+static const struct kernel_param_ops download_mode_param_ops = {
+   .get = get_download_mode,
+   .set = set_download_mode,
+};
+
+module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
+MODULE_PARM_DESC(download_mode,
+   "download mode: off/full are acceptable values");
+
 static int qcom_scm_probe(struct platform_device *pdev)
 {
struct qcom_scm *scm;
@@ -1493,12 +1535,12 @@ static int qcom_scm_probe(struct platform_device *pdev)
__get_convention();
 
/*
-* If requested enable "download mode", from this point on warmboot
+* If "download mode" is requested, from this point on warmboot
 * will cause the boot stages to enter download mode, unless
 * disabled below by a clean shutdown/reboot.
 */
if (download_mode)
-   qcom_scm_set_download_mode(true);
+   qcom_scm_set_download_mode(download_mode);
 
return 0;
 }
@@ -1506,7 +1548,7 @@ static int qcom_scm_probe(struct platform_device *pdev)
 static void qcom_scm_shutdown(struct platform_device *pdev)
 {
/* Clean shutdown, disable download mode to allow normal restart */
-   qcom_scm_set_download_mode(false);
+   qcom_scm_set_download_mode(QCOM_DLOAD_NODUMP);
 }
 
 static const struct of_device_id qcom_scm_dt_match[] = {
-- 
2.7.4

Re: [REBASE PATCH v5 01/17] docs: qcom: Add qualcomm minidump guide

On 11/09/2023 12:53, Mukesh Ojha wrote:
> Add the qualcomm minidump guide for the users which tries to cover
> the dependency, API use and the way to test and collect minidump
> on Qualcomm supported SoCs.
> 
> Signed-off-by: Mukesh Ojha 
> ---

Please let us review your previous patch before sending new versions or
resends or rebases.

Best regards,
Krzysztof

[REBASE PATCH v5 10/17] pstore: Add pstore_region_defined() helper and export it

There are users like Qualcomm minidump which is interested in
knowing the pstore frontend addresses and sizes from the backend
(ram) to be able to register it with firmware to finally collect
them during crash for debugging.

Signed-off-by: Mukesh Ojha 
---
 fs/pstore/platform.c   | 15 +++
 fs/pstore/ram.c| 42 ++
 include/linux/pstore.h |  6 ++
 3 files changed, 63 insertions(+)

diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
index e5bca9a004cc..fdac951059c1 100644
--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -139,6 +139,21 @@ enum pstore_type_id pstore_name_to_type(const char *name)
 }
 EXPORT_SYMBOL_GPL(pstore_name_to_type);
 
+int pstore_region_defined(struct pstore_record *record,
+ void **virt, phys_addr_t *phys,
+ size_t *size, unsigned int *max_dump_cnt)
+{
+   if (!psinfo)
+   return -EINVAL;
+
+   record->psi = psinfo;
+
+   return psinfo->region_info ?
+  psinfo->region_info(record, virt, phys, size, max_dump_cnt) :
+  -EINVAL;
+}
+EXPORT_SYMBOL_GPL(pstore_region_defined);
+
 static void pstore_timer_kick(void)
 {
if (pstore_update_ms < 0)
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ab551caa1d2a..62202f3ddf63 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -437,6 +437,47 @@ static int ramoops_pstore_erase(struct pstore_record 
*record)
return 0;
 }
 
+static int ramoops_region_info(struct pstore_record *record,
+  void **virt, phys_addr_t *phys,
+  size_t *size, unsigned int *max_dump_cnt)
+{
+   struct ramoops_context *cxt = record->psi->data;
+   struct persistent_ram_zone *prz;
+
+   switch (record->type) {
+   case PSTORE_TYPE_DMESG:
+   if (record->id >= cxt->max_dump_cnt)
+   return -EINVAL;
+   prz = cxt->dprzs[record->id];
+   *max_dump_cnt = cxt->max_dump_cnt;
+   break;
+   case PSTORE_TYPE_CONSOLE:
+   if (!cxt->console_size)
+   return -EINVAL;
+   prz = cxt->cprz;
+   break;
+   case PSTORE_TYPE_FTRACE:
+   if (record->id >= cxt->max_ftrace_cnt)
+   return -EINVAL;
+   prz = cxt->fprzs[record->id];
+   *max_dump_cnt = cxt->max_ftrace_cnt;
+   break;
+   case PSTORE_TYPE_PMSG:
+   if (!cxt->pmsg_size)
+   return -EINVAL;
+   prz = cxt->mprz;
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   *virt = prz->vaddr;
+   *phys = prz->paddr;
+   *size = prz->size;
+
+   return 0;
+}
+
 static struct ramoops_context oops_cxt = {
.pstore = {
.owner  = THIS_MODULE,
@@ -446,6 +487,7 @@ static struct ramoops_context oops_cxt = {
.write  = ramoops_pstore_write,
.write_user = ramoops_pstore_write_user,
.erase  = ramoops_pstore_erase,
+   .region_info = ramoops_region_info,
},
 };
 
diff --git a/include/linux/pstore.h b/include/linux/pstore.h
index 638507a3c8ff..a64d866e8711 100644
--- a/include/linux/pstore.h
+++ b/include/linux/pstore.h
@@ -199,6 +199,9 @@ struct pstore_info {
int (*write_user)(struct pstore_record *record,
  const char __user *buf);
int (*erase)(struct pstore_record *record);
+   int (*region_info)(struct pstore_record *record,
+  void **virt, phys_addr_t *phys,
+  size_t *size, unsigned int 
*max_dump_cnt);
 };
 
 /* Supported frontends */
@@ -230,6 +233,9 @@ struct pstore_ftrace_record {
 #define TS_CPU_SHIFT 8
 #define TS_CPU_MASK (BIT(TS_CPU_SHIFT) - 1)
 
+int pstore_region_defined(struct pstore_record *record,
+ void **virt, phys_addr_t *phys,
+ size_t *size, unsigned int *max_dump_cnt);
 /*
  * If CPU number can be stored in IP, store it there, otherwise store it in
  * the time stamp. This means more timestamp resolution is available when
-- 
2.7.4

[REBASE PATCH v5 17/17] firmware: qcom_scm: Add multiple download mode support

Currently, scm driver only supports full dump when download
mode is selected. Add support to enable minidump as well as
enable it along with fulldump.

Signed-off-by: Mukesh Ojha 
---
 drivers/firmware/qcom_scm.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4ecdb0d025a4..c70ab9afd3cb 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -34,6 +34,8 @@ static u32 download_mode;
 
 #define QCOM_DLOAD_MASKGENMASK(5, 4)
 #define QCOM_DLOAD_FULLDUMP0x1
+#define QCOM_DLOAD_MINIDUMP0x2
+#define QCOM_DLOAD_BOTHDUMP(QCOM_DLOAD_FULLDUMP | QCOM_DLOAD_MINIDUMP)
 #define QCOM_DLOAD_NODUMP  0x0
 
 struct qcom_scm {
@@ -86,6 +88,8 @@ static const char * const qcom_scm_convention_names[] = {
 static const char * const download_mode_name[] = {
[QCOM_DLOAD_NODUMP] = "off",
[QCOM_DLOAD_FULLDUMP]   = "full",
+   [QCOM_DLOAD_MINIDUMP]   = "mini",
+   [QCOM_DLOAD_BOTHDUMP]   = "full,mini",
 };
 
 static struct qcom_scm *__scm;
@@ -1470,7 +1474,7 @@ static const struct kernel_param_ops 
download_mode_param_ops = {
 
 module_param_cb(download_mode, &download_mode_param_ops, NULL, 0644);
 MODULE_PARM_DESC(download_mode,
-   "download mode: off/full are acceptable values");
+   "download mode: off/full/mini/full,mini are acceptable values");
 
 static int qcom_scm_probe(struct platform_device *pdev)
 {
-- 
2.7.4

[REBASE PATCH v5 00/17] Add Qualcomm Minidump kernel driver related support

Hi All,

I apologise that the last v5 was on sent on older tag and it was reported
that it does not apply to linux-next tag cleanly, thanks to Kathiravan and
Bagas.S for giving me early notice.

I would like continue the conversation happened at v4

https://lore.kernel.org/lkml/632c5b97-4a91-c3e8-1e6c-33d6c4f64...@quicinc.com/

https://lore.kernel.org/lkml/695133e6-105f-de2a-5559-555cea0a0...@quicinc.com/

We have put abstract on LPC on this topic as well as initiated a mail thread
with other SoC vendors but did not get much traction on it.

https://lore.kernel.org/lkml/0199db00-1b1d-0c63-58ff-03efae02c...@quicinc.com/

We explored most of possiblity present in kernel to address this issue[1] but
solution like kdump/fadump does not seems safe/secure/performant from our
perspective.

One of the proposal made here,
https://lore.kernel.org/lkml/1683561060-2197-1-git-send-email-quic_mo...@quicinc.com/

Looking forward for your comments.

Thanks,
Mukesh

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/remoteproc/qcom_common.c#n142

Change in rebase v5:
- Rebased it on latest tag available on linux-next
- Added missed Poovendhan sign-off on 15/17 and tested-by tag from
Kathiravan. Thanks to him for testing and reminding me of missing sign-off.

Changes in v5:
https://lore.kernel.org/lkml/1694290578-17733-1-git-send-email-quic_mo...@quicinc.com/
- On suggestion from Pavan.k, to have single function call for minidump
collection
from remoteproc driver, separated the logic to have separate minidump file
called
qcom_rproc_minidump.c and also renamed the function from qcom_minidump() to
qcom_rproc_minidump(); however, dropped his suggestion about rework on lazy
deletion
during region unregister in this series, will pursue it in next series.

- To simplify the minidump driver, removed the complication for frontend and
different
backend from Greg suggestion, will pursue this once main driver gets
mainlined.

- Exporting linux banner from kernel to make minidump build also as module,
however,
minidump is a debug module and should be kernel built to get most debug
information
from kernel.

- Tried to address comments given on dload patch series.

[REBASE PATCH v5 15/17] firmware: scm: Modify only the download bits in TCSR register

Crashdump collection is based on the DLOAD bit of TCSR register.
To retain other bits, we read the register and modify only the
DLOAD bit as the other bits have their own significance.

Co-developed-by: Poovendhan Selvaraj 
Signed-off-by: Poovendhan Selvaraj 
Signed-off-by: Mukesh Ojha 
Tested-by: Kathiravan Thirumoorthy  # IPQ9574 and 
IPQ5332
---
 drivers/firmware/qcom_scm.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 321133f0950d..5cacae63ee2a 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -5,6 +5,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -26,6 +28,14 @@
 static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
 module_param(download_mode, bool, 0);
 
+#define SCM_HAS_CORE_CLK   BIT(0)
+#define SCM_HAS_IFACE_CLK  BIT(1)
+#define SCM_HAS_BUS_CLKBIT(2)
+
+#define QCOM_DLOAD_MASKGENMASK(5, 4)
+#define QCOM_DLOAD_FULLDUMP0x1
+#define QCOM_DLOAD_NODUMP  0x0
+
 struct qcom_scm {
struct device *dev;
struct clk *core_clk;
@@ -440,6 +450,7 @@ static int __qcom_scm_set_dload_mode(struct device *dev, 
bool enable)
 
 static void qcom_scm_set_download_mode(bool enable)
 {
+   u32 val = enable ? QCOM_DLOAD_FULLDUMP : QCOM_DLOAD_NODUMP;
bool avail;
int ret = 0;
 
@@ -449,8 +460,9 @@ static void qcom_scm_set_download_mode(bool enable)
if (avail) {
ret = __qcom_scm_set_dload_mode(__scm->dev, enable);
} else if (__scm->dload_mode_addr) {
-   ret = qcom_scm_io_writel(__scm->dload_mode_addr,
-   enable ? QCOM_SCM_BOOT_SET_DLOAD_MODE : 0);
+   ret = qcom_scm_io_update_field(__scm->dload_mode_addr,
+  QCOM_DLOAD_MASK,
+  FIELD_PREP(QCOM_DLOAD_MASK, 
val));
} else {
dev_err(__scm->dev,
"No available mechanism for setting download mode\n");
-- 
2.7.4

[REBASE PATCH v5 13/17] firmware: qcom_scm: provide a read-modify-write function

It was realized by Srinivas K. that there is a need of
read-modify-write scm exported function so that it can
be used by multiple clients.

Let's introduce qcom_scm_io_update_field() which masks
out the bits and write the passed value to that
bit-offset. Subsequent patch will use this function.

Suggested-by: Srinivas Kandagatla 
Signed-off-by: Mukesh Ojha 
Tested-by: Kathiravan Thirumoorthy  # IPQ9574 and 
IPQ5332
---
 drivers/firmware/qcom_scm.c| 20 
 include/linux/firmware/qcom/qcom_scm.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 06fe8aca870d..321133f0950d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -74,6 +74,7 @@ static const char * const qcom_scm_convention_names[] = {
 };
 
 static struct qcom_scm *__scm;
+static DEFINE_SPINLOCK(lock);
 
 static int qcom_scm_clk_enable(void)
 {
@@ -403,6 +404,25 @@ int qcom_scm_set_remote_state(u32 state, u32 id)
 }
 EXPORT_SYMBOL_GPL(qcom_scm_set_remote_state);
 
+int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask, unsigned int 
val)
+{
+   unsigned int old, new;
+   int ret;
+
+   spin_lock(&lock);
+   ret = qcom_scm_io_readl(addr, &old);
+   if (ret)
+   goto unlock;
+
+   new = (old & ~mask) | (val & mask);
+
+   ret = qcom_scm_io_writel(addr, new);
+unlock:
+   spin_unlock(&lock);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(qcom_scm_io_update_field);
+
 static int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
struct qcom_scm_desc desc = {
diff --git a/include/linux/firmware/qcom/qcom_scm.h 
b/include/linux/firmware/qcom/qcom_scm.h
index 0c091a3f6d49..eb750286abb6 100644
--- a/include/linux/firmware/qcom/qcom_scm.h
+++ b/include/linux/firmware/qcom/qcom_scm.h
@@ -84,6 +84,8 @@ extern bool qcom_scm_pas_supported(u32 peripheral);
 
 extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
+extern int qcom_scm_io_update_field(phys_addr_t addr, unsigned int mask,
+   unsigned int val);
 
 extern bool qcom_scm_restore_sec_cfg_available(void);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
-- 
2.7.4

[REBASE PATCH v5 01/17] docs: qcom: Add qualcomm minidump guide

Add the qualcomm minidump guide for the users which tries to cover
the dependency, API use and the way to test and collect minidump
on Qualcomm supported SoCs.

Signed-off-by: Mukesh Ojha 
---
 Documentation/admin-guide/index.rst |   1 +
 Documentation/admin-guide/qcom_minidump.rst | 272 
 2 files changed, 273 insertions(+)
 create mode 100644 Documentation/admin-guide/qcom_minidump.rst

diff --git a/Documentation/admin-guide/index.rst 
b/Documentation/admin-guide/index.rst
index 43ea35613dfc..251d070486c2 100644
--- a/Documentation/admin-guide/index.rst
+++ b/Documentation/admin-guide/index.rst
@@ -120,6 +120,7 @@ configure specific aspects of kernel behavior to your 
liking.
perf-security
pm/index
pnp
+   qcom_minidump
rapidio
ras
rtc
diff --git a/Documentation/admin-guide/qcom_minidump.rst 
b/Documentation/admin-guide/qcom_minidump.rst
new file mode 100644
index ..20202da8ca40
--- /dev/null
+++ b/Documentation/admin-guide/qcom_minidump.rst
@@ -0,0 +1,272 @@
+Qualcomm minidump feature
+=
+
+Introduction
+
+
+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 RAM dumps 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 selected region should be included in
+the ramdump.
+
+
+::
+
+   +---+
+   |   DDR   +-+   |
+   | |  SS0-ToC|   |
+   | ++ ++ |   |
+   | |Shared memory   | | SS1-ToC| |   |
+   | |(SMEM)  | || |   |
+   | || +-->|+   | |   |
+   | |G-ToC   | |   | SS-ToC  \  | |   |
+   | |+-+ | |   | +---+  | |   |
+   | ||-| | |   | |---|  | |   |
+   | || SS0-ToC | | | +-|<|SS1 region1|  | |   |
+   | ||-| | | | | |---|  | |   |
+   | || SS1-ToC |-|>+ | | |SS1 region2|  | |   |
+   | ||-| |   | | |---|  | |   |
+   | || SS2-ToC | |   | | |  ...  |  | |   |
+   | ||-| |   | | |---|  | |   |
+   | ||  ...| |   |-|<|SS1 regionN|  | |   |
+   | ||-| |   | | |---|  | |   |
+   | || SSn-ToC | |   | | +---+  | |   |
+   | |+-+ |   | || |   |
+   | ||   | || |   |
+   | ||   +>|  regionN   | |   |
+   | ||   | || |   |
+   | ++   | || |   |
+   |  | || |   |
+   |  +>|  region1   | |   |
+   ||| |   |
+   ||| |   |
+   |||-+   |
+   ||  region5   | |
+   ||| |
+   ||| |
+   |  Region information++ |
+   | +---+ |
+   | |region name| |
+   | |---| |
+   | |region address | |
+   | |---| |
+   | |region size| |
+   | +---+ |
+   +---+
+   G-ToC: Global table of contents
+   SS-ToC: Subsystem table of contents
+   SS0-SSn: Subsystem numbered from 0 to n
+
+It depends on SoC where the underlying firmware is keeping the
+minidump global table taking care of subsystem ToC part for
+minidump like for above diagram, it is for shared memory sitting
+in DDR and it is shared among various master however it is possible
+that this could be implemented via memory mapped regions but the
+general idea should remain same. Here, various subsystem could be
+DSP's like ADSP/CDSP/MODEM etc, along with Application processor
+(APSS) where Linux runs. DSP minidump gets collected when DSP's goes
+for recovery followed by a crash. The minidump part of code for
+that resides in ``qcom_rproc_minidump.c``.
+
+
+SMEM as backend
+
+
+In this document, SMEM will be used as the backend implementation
+o

Re: [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line

2023-09-11 Thread Waiman Long


On 9/10/23 04:29, guo...@kernel.org wrote:

From: Guo Ren 

Allow cmdline to force the kernel to use queued_spinlock when
CONFIG_RISCV_COMBO_SPINLOCKS=y.

Signed-off-by: Guo Ren 
Signed-off-by: Guo Ren 
---
  Documentation/admin-guide/kernel-parameters.txt |  2 ++
  arch/riscv/kernel/setup.c   | 16 +++-
  2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 7dfb540c4f6c..61cacb8dfd0e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4693,6 +4693,8 @@
[KNL] Number of legacy pty's. Overwrites compiled-in
default number.
  
+	qspinlock	[RISCV] Force to use qspinlock or auto-detect spinlock.

+
qspinlock.numa_spinlock_threshold_ns=   [NUMA, PV_OPS]
Set the time threshold in nanoseconds for the
number of intra-node lock hand-offs before the
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index a447cf360a18..0f084f037651 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -270,6 +270,15 @@ static void __init parse_dtb(void)
  }
  
  #ifdef CONFIG_RISCV_COMBO_SPINLOCKS

+bool enable_qspinlock_key = false;


You can use __ro_after_init qualifier for enable_qspinlock_key. BTW, 
this is not a static key, just a simple flag. So what is the point of 
the _key suffix?


Cheers,
Longman

Re: [PATCH] sched: Change the name of preempt mode from full to low_latency

2023-09-11 Thread Peter Zijlstra

On Mon, Sep 11, 2023 at 07:00:46PM +0800, Yajun Deng wrote:
> There is a similar name fully in Kconfig.preempt, but it corresponds to
> PREEMPT_RT. In order to distinguish them, change the name of preempt mode
> from full to low_latency.
> 
> Also, define a global array and variable that used to save preempt mode
> name and size.

Yeah, let's not rename this for giggles. Regular preemption is a fully
preemptible kernel model. No need to invent new names for it.

Re: [PATCH V11 01/17] asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock

2023-09-11 Thread Leonardo Brás

On Sun, 2023-09-10 at 04:28 -0400, guo...@kernel.org wrote:
> From: Guo Ren 
> 
> The arch_spinlock_t of qspinlock has contained the atomic_t val, which
> satisfies the ticket-lock requirement. Thus, unify the arch_spinlock_t
> into qspinlock_types.h. This is the preparation for the next combo
> spinlock.
> 
> Signed-off-by: Guo Ren 
> Signed-off-by: Guo Ren 
> ---
>  include/asm-generic/spinlock.h   | 14 +++---
>  include/asm-generic/spinlock_types.h | 12 ++--
>  2 files changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
> index 90803a826ba0..4773334ee638 100644
> --- a/include/asm-generic/spinlock.h
> +++ b/include/asm-generic/spinlock.h
> @@ -32,7 +32,7 @@
>  
>  static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
>  {
> - u32 val = atomic_fetch_add(1<<16, lock);
> + u32 val = atomic_fetch_add(1<<16, &lock->val);
>   u16 ticket = val >> 16;
>  
>   if (ticket == (u16)val)
> @@ -46,31 +46,31 @@ static __always_inline void 
> arch_spin_lock(arch_spinlock_t *lock)
>* have no outstanding writes due to the atomic_fetch_add() the extra
>* orderings are free.
>*/
> - atomic_cond_read_acquire(lock, ticket == (u16)VAL);
> + atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
>   smp_mb();
>  }
>  
>  static __always_inline bool arch_spin_trylock(arch_spinlock_t *lock)
>  {
> - u32 old = atomic_read(lock);
> + u32 old = atomic_read(&lock->val);
>  
>   if ((old >> 16) != (old & 0x))
>   return false;
>  
> - return atomic_try_cmpxchg(lock, &old, old + (1<<16)); /* SC, for RCsc */
> + return atomic_try_cmpxchg(&lock->val, &old, old + (1<<16)); /* SC, for 
> RCsc */
>  }
>  
>  static __always_inline void arch_spin_unlock(arch_spinlock_t *lock)
>  {
>   u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> - u32 val = atomic_read(lock);
> + u32 val = atomic_read(&lock->val);
>  
>   smp_store_release(ptr, (u16)val + 1);
>  }
>  
>  static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>  {
> - u32 val = lock.counter;
> + u32 val = lock.val.counter;
>  
>   return ((val >> 16) == (val & 0x));
>  }

This one seems to be different in torvalds/master, but I suppose it's because of
the requirement patches I have not merged.

> @@ -84,7 +84,7 @@ static __always_inline int 
> arch_spin_is_locked(arch_spinlock_t *lock)
>  
>  static __always_inline int arch_spin_is_contended(arch_spinlock_t *lock)
>  {
> - u32 val = atomic_read(lock);
> + u32 val = atomic_read(&lock->val);
>  
>   return (s16)((val >> 16) - (val & 0x)) > 1;
>  }
> diff --git a/include/asm-generic/spinlock_types.h 
> b/include/asm-generic/spinlock_types.h
> index 8962bb730945..f534aa5de394 100644
> --- a/include/asm-generic/spinlock_types.h
> +++ b/include/asm-generic/spinlock_types.h
> @@ -3,15 +3,7 @@
>  #ifndef __ASM_GENERIC_SPINLOCK_TYPES_H
>  #define __ASM_GENERIC_SPINLOCK_TYPES_H
>  
> -#include 
> -typedef atomic_t arch_spinlock_t;
> -
> -/*
> - * qrwlock_types depends on arch_spinlock_t, so we must typedef that before 
> the
> - * include.
> - */
> -#include 
> -
> -#define __ARCH_SPIN_LOCK_UNLOCKEDATOMIC_INIT(0)
> +#include 
> +#include 
>  
>  #endif /* __ASM_GENERIC_SPINLOCK_TYPES_H */

FWIW, LGTM:

Reviewed-by: Leonardo Bras 


Just a suggestion: In this patch I could see a lot of usage changes to
arch_spinlock_t, and only at the end I could see the actual change in the .h
file.

In cases like this, it looks nicer to see the .h file first.

I recently found out about this git diff.orderFile option, which helps to
achieve exactly this.

I use the following git.orderfile, adapted from qemu:


#
# order file for git, to produce patches which are easier to review
# by diffing the important stuff like interface changes first.
#
# one-off usage:
#   git diff -O scripts/git.orderfile ...
#
# add to git config:
#   git config diff.orderFile scripts/git.orderfile
#

MAINTAINERS

# Documentation
Documentation/*
*.rst
*.rst.inc

# build system
Kbuild
Makefile*
*.mak

# semantic patches
*.cocci

# headers
*.h
*.h.inc

# code
*.c
*.c.inc

Re: [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line

2023-09-11 Thread Waiman Long


On 9/10/23 04:29, guo...@kernel.org wrote:

From: Guo Ren 

Allow cmdline to force the kernel to use queued_spinlock when
CONFIG_RISCV_COMBO_SPINLOCKS=y.

Signed-off-by: Guo Ren 
Signed-off-by: Guo Ren 
---
  Documentation/admin-guide/kernel-parameters.txt |  2 ++
  arch/riscv/kernel/setup.c   | 16 +++-
  2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 7dfb540c4f6c..61cacb8dfd0e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4693,6 +4693,8 @@
[KNL] Number of legacy pty's. Overwrites compiled-in
default number.
  
+	qspinlock	[RISCV] Force to use qspinlock or auto-detect spinlock.

+
qspinlock.numa_spinlock_threshold_ns=   [NUMA, PV_OPS]
Set the time threshold in nanoseconds for the
number of intra-node lock hand-offs before the


Your patch series is still based on top of numa-aware qspinlock patchset 
which isn't upstream yet. Please rebase it without that as that will 
cause merge conflict during upstream merge.


Cheers,
Longman

Re: [PATCH V11 00/17] riscv: Add Native/Paravirt qspinlock support

2023-09-11 Thread Conor Dooley

On Mon, Sep 11, 2023 at 11:36:27AM +0800, Guo Ren wrote:
> On Mon, Sep 11, 2023 at 3:45 AM Conor Dooley  wrote:
> >
> > On Sun, Sep 10, 2023 at 05:49:13PM +0800, Guo Ren wrote:
> > > On Sun, Sep 10, 2023 at 5:32 PM Conor Dooley  wrote:
> > > >
> > > > On Sun, Sep 10, 2023 at 05:16:46PM +0800, Guo Ren wrote:
> > > > > On Sun, Sep 10, 2023 at 4:58 PM Conor Dooley  wrote:
> > > > > >
> > > > > > On Sun, Sep 10, 2023 at 04:28:54AM -0400, guo...@kernel.org wrote:
> > > > > >
> > > > > > > Changlog:
> > > > > > > V11:
> > > > > > >  - Based on Leonardo Bras's cmpxchg_small patches v5.
> > > > > > >  - Based on Guo Ren's Optimize arch_spin_value_unlocked patch v3.
> > > > > > >  - Remove abusing alternative framework and use jump_label 
> > > > > > > instead.
> > > > > >
> > > > > > btw, I didn't say that using alternatives was the problem, it was
> > > > > > abusing the errata framework to perform feature detection that I had
> > > > > > a problem with. That's not changed in v11.
> > > > > I've removed errata feature detection. The only related patches are:
> > > > >  - riscv: qspinlock: errata: Add ERRATA_THEAD_WRITE_ONCE fixup
> > > > >  - riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors
> > > > >
> > > > > Which one is your concern? Could you reply on the exact patch thread? 
> > > > > Thx.
> > > >
> > > > riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors
> > > >
> > > > Please go back and re-read the comments I left on v11 about using the
> > > > errata code for feature detection.
> > > >
> > > > > > A stronger forward progress guarantee is not an erratum, AFAICT.
> > > >
> > > > > Sorry, there is no erratum of "stronger forward progress guarantee" 
> > > > > in the V11.
> > > >
> > > > "riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors" still
> > > > uses the errata framework to detect the presence of the stronger forward
> > > > progress guarantee in v11.
> > > Oh, thx for pointing it out. I could replace it with this:
> > >
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 88690751f2ee..4be92766d3e3 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -310,7 +310,8 @@ static void __init riscv_spinlock_init(void)
> > >  {
> > >  #ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > > if (!enable_qspinlock_key &&
> > > -   (sbi_get_firmware_id() != SBI_EXT_BASE_IMPL_ID_KVM)) {
> > > +   (sbi_get_firmware_id() != SBI_EXT_BASE_IMPL_ID_KVM) &&
> > > +   (sbi_get_mvendorid() != THEAD_VENDOR_ID)) {
> > > static_branch_disable(&combo_qspinlock_key);
> > > pr_info("Ticket spinlock: enabled\n");
> > > } else {
> >
> > As I said on v11, I am opposed to feature probing using mvendorid & Co,
> > partially due to the exact sort of check here to see if the kernel is
> > running as a KVM guest. IMO, whether a platform has this stronger

> KVM can't use any fairness lock, so forcing it using a Test-Set lock
> or paravirt qspinlock is the right way. KVM is not a vendor platform.

My point is that KVM should be telling the guest what additional features
it is capable of using, rather than the kernel making some assumptions
based on$vendorid etc that are invalid when the kernel is running as a
KVM guest.
If the mvendorid etc related assumptions were dropped, the kernel would
then default away from your qspinlock & there'd not be a need to
special-case KVM AFAICT.

> > guarantee needs to be communicated by firmware, using ACPI or DT.
> > I made some comments on v11, referring similar discussion about the
> > thead vector stuff. Please go take a look at that.
> I prefer forcing T-HEAD processors using qspinlock, but if all people
> thought it must be in the ACPI or DT, I would compromise. Then, I
> would delete the qspinlock cmdline param patch and move it into DT.
> 
> By the way, what's the kind of DT format? How about:

I added the new "riscv,isa-extensions" property in part to make
communicating vendor extensions like this easier. Please try to use
that. "qspinlock" is software configuration though, the vendor extension
should focus on the guarantee of strong forward progress, since that is
the non-standard aspect of your IP.

A commandline property may still be desirable, to control the locking
method used, since the DT should be a description of the hardware, not
for configuring software policy in your operating system.

Thanks,
Conor.

> cpus {
> #address-cells = <1>;
> #size-cells = <0>;
> +  qspinlock;
> cpu0: cpu@0 {
> compatible = "sifive,bullet0", "riscv";
> device_type = "cpu";
> i-cache-block-size = <64>;
> i-cache-sets = <128>;
> 
> --
> Best Regards
>  Guo Ren


signature.asc
Description: PGP signature

Re: [PATCH V11 04/17] locking/qspinlock: Improve xchg_tail for number of cpus >= 16k

2023-09-11 Thread Waiman Long


On 9/10/23 23:09, Guo Ren wrote:

On Mon, Sep 11, 2023 at 10:35 AM Waiman Long  wrote:


On 9/10/23 04:28, guo...@kernel.org wrote:

From: Guo Ren 

The target of xchg_tail is to write the tail to the lock value, so
adding prefetchw could help the next cmpxchg step, which may
decrease the cmpxchg retry loops of xchg_tail. Some processors may
utilize this feature to give a forward guarantee, e.g., RISC-V
XuanTie processors would block the snoop channel & irq for several
cycles when prefetch.w instruction (from Zicbop extension) retired,
which guarantees the next cmpxchg succeeds.

Signed-off-by: Guo Ren 
Signed-off-by: Guo Ren 
---
   kernel/locking/qspinlock.c | 5 -
   1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index d3f99060b60f..96b54e2ade86 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -223,7 +223,10 @@ static __always_inline void 
clear_pending_set_locked(struct qspinlock *lock)
*/
   static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
   {
- u32 old, new, val = atomic_read(&lock->val);
+ u32 old, new, val;
+
+ prefetchw(&lock->val);
+ val = atomic_read(&lock->val);

   for (;;) {
   new = (val & _Q_LOCKED_PENDING_MASK) | tail;

That looks a bit weird. You pre-fetch and then immediately read it. How
much performance gain you get by this change alone?

Maybe you can define an arch specific primitive that default back to
atomic_read() if not defined.

Thx for the reply. This is a generic optimization point I would like
to talk about with you.

First, prefetchw() makes cacheline an exclusive state and serves for
the next cmpxchg loop semantic, which writes the idx_tail part of
arch_spin_lock. The atomic_read only makes cacheline in the shared
state, which couldn't give any guarantee for the next cmpxchg loop
semantic. Micro-architecture could utilize prefetchw() to provide a
strong forward progress guarantee for the xchg_tail, e.g., the T-HEAD
XuanTie processor would hold the exclusive cacheline state until the
next cmpxchg write success.

In the end, Let's go back to the principle: the xchg_tail is an atomic
swap operation that contains write eventually, so giving a prefetchw()
at the beginning is acceptable for all architectures..



I did realize afterward that prefetchw gets the cacheline in exclusive 
state. I will suggest you mention that in your commit log as well as 
adding a comment about its purpose in the code.


Thanks,
Longman


Cheers,
Longman

[PATCH] sched: Change the name of preempt mode from full to low_latency

2023-09-11 Thread Yajun Deng

There is a similar name fully in Kconfig.preempt, but it corresponds to
PREEMPT_RT. In order to distinguish them, change the name of preempt mode
from full to low_latency.

Also, define a global array and variable that used to save preempt mode
name and size.

Signed-off-by: Yajun Deng 
---
 .../admin-guide/kernel-parameters.txt |  2 +-
 include/linux/sched.h |  8 ++--
 kernel/sched/core.c   | 43 ++-
 kernel/sched/debug.c  |  5 +--
 kernel/sched/sched.h  |  2 +
 kernel/trace/trace.c  |  4 +-
 6 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 0a1731a0f0ef..9284fd7999d7 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4581,7 +4581,7 @@
Select preemption mode if you have 
CONFIG_PREEMPT_DYNAMIC
none - Limited to cond_resched() calls
voluntary - Limited to cond_resched() and might_sleep() 
calls
-   full - Any section that isn't explicitly preempt 
disabled
+   low_latency - Any section that isn't explicitly preempt 
disabled
   can be preempted anytime.
 
print-fatal-signals=
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6d1341b1673f..ea607a0ce6f6 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2181,7 +2181,7 @@ static inline void cond_resched_rcu(void)
 
 extern bool preempt_model_none(void);
 extern bool preempt_model_voluntary(void);
-extern bool preempt_model_full(void);
+extern bool preempt_model_low_latency(void);
 
 #else
 
@@ -2193,14 +2193,14 @@ static inline bool preempt_model_voluntary(void)
 {
return IS_ENABLED(CONFIG_PREEMPT_VOLUNTARY);
 }
-static inline bool preempt_model_full(void)
+static inline bool preempt_model_low_latency(void)
 {
return IS_ENABLED(CONFIG_PREEMPT);
 }
 
 #endif
 
-static inline bool preempt_model_rt(void)
+static inline bool preempt_model_fully(void)
 {
return IS_ENABLED(CONFIG_PREEMPT_RT);
 }
@@ -2215,7 +2215,7 @@ static inline bool preempt_model_rt(void)
  */
 static inline bool preempt_model_preemptible(void)
 {
-   return preempt_model_full() || preempt_model_rt();
+   return preempt_model_low_latency() || preempt_model_fully();
 }
 
 /*
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2299a5cfbfb9..2abbc0baaae7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8705,7 +8705,7 @@ EXPORT_SYMBOL(__cond_resched_rwlock_write);
  *   preempt_schedule_notrace   <- NOP
  *   irqentry_exit_cond_resched <- NOP
  *
- * FULL:
+ * LOW_LATENCY:
  *   cond_resched   <- RET0
  *   might_resched  <- RET0
  *   preempt_schedule   <- preempt_schedule
@@ -8717,21 +8717,25 @@ enum {
preempt_dynamic_undefined = -1,
preempt_dynamic_none,
preempt_dynamic_voluntary,
-   preempt_dynamic_full,
+   preempt_dynamic_low_latency,
 };
-
 int preempt_dynamic_mode = preempt_dynamic_undefined;
 
+const char *preempt_modes[] = {
+   [preempt_dynamic_none]= "none",
+   [preempt_dynamic_voluntary]   = "voluntary",
+   [preempt_dynamic_low_latency] = "low_latency",
+};
+int preempt_modes_size = ARRAY_SIZE(preempt_modes);
+
 int sched_dynamic_mode(const char *str)
 {
-   if (!strcmp(str, "none"))
-   return preempt_dynamic_none;
-
-   if (!strcmp(str, "voluntary"))
-   return preempt_dynamic_voluntary;
+   int i;
 
-   if (!strcmp(str, "full"))
-   return preempt_dynamic_full;
+   for (i = 0; i < preempt_modes_size; i++) {
+   if (!strcmp(str, preempt_modes[i]))
+   return i;
+   }
 
return -EINVAL;
 }
@@ -8752,7 +8756,7 @@ static bool klp_override;
 static void __sched_dynamic_update(int mode)
 {
/*
-* Avoid {NONE,VOLUNTARY} -> FULL transitions from ever ending up in
+* Avoid {NONE,VOLUNTARY} -> LOW_LATENCY transitions from ever ending 
up in
 * the ZERO state, which is invalid.
 */
if (!klp_override)
@@ -8770,8 +8774,6 @@ static void __sched_dynamic_update(int mode)
preempt_dynamic_disable(preempt_schedule);
preempt_dynamic_disable(preempt_schedule_notrace);
preempt_dynamic_disable(irqentry_exit_cond_resched);
-   if (mode != preempt_dynamic_mode)
-   pr_info("Dynamic Preempt: none\n");
break;
 
case preempt_dynamic_voluntary:
@@ -8781,22 +8783,21 @@ static void __sched_dynamic_update(int mode)
preempt_dynamic_disable(preempt_schedule);
preempt_dynamic_disable(preempt_schedule_no

[PATCH v2] Documentation: stable: clarify patch series prerequisites

2023-09-11 Thread Hugo Villeneuve

From: Hugo Villeneuve 

Add some clarifications for patches that have dependencies within the
patch series.

Signed-off-by: Hugo Villeneuve 
---
Changes since v1: rebase on latest torvalds/master

 Documentation/process/stable-kernel-rules.rst | 13 +
 1 file changed, 13 insertions(+)

diff --git a/Documentation/process/stable-kernel-rules.rst 
b/Documentation/process/stable-kernel-rules.rst
index 41f1e07abfdf..1704f1c686d0 100644
--- a/Documentation/process/stable-kernel-rules.rst
+++ b/Documentation/process/stable-kernel-rules.rst
@@ -101,6 +101,19 @@ comment:
  git cherry-pick fd21073
  git cherry-pick 
 
+   Note that for a patch series, you do not have to list as prerequisites the
+   patches present in the series itself. For example, if you have the following
+   patch series:
+
+   .. code-block:: none
+
+ patch1
+ patch2
+
+   where patch2 depends on patch1, you do not have to list patch1 as
+   prerequisite of patch2 if you have already marked patch1 for stable
+   inclusion.
+
  * For patches that may have kernel version prerequisites specify them using
the following format in the sign-off area:
 

base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
-- 
2.30.2

Re: [PATCH v5 09/17] pstore/ram: Use dynamic ramoops reserve resource

2023-09-11 Thread Pavan Kondeti

On Mon, Sep 11, 2023 at 04:21:44PM +0530, Mukesh Ojha wrote:
> 
> 
> On 9/11/2023 11:03 AM, Pavan Kondeti wrote:
> > On Sun, Sep 10, 2023 at 01:46:10AM +0530, Mukesh Ojha wrote:
> > > As dynamic ramoops command line parsing is now added, so
> > > lets add the support in ramoops driver to get the resource
> > > structure and add it during platform device registration.
> > > 
> > > Signed-off-by: Mukesh Ojha 
> > > ---
> > >   fs/pstore/ram.c | 10 +++---
> > >   1 file changed, 7 insertions(+), 3 deletions(-)
> > > 
> > 
> > Documentation/admin-guide/ramoops.rst might need an update as well.
> 
> I have said in the cover-letter under changes in v5, it is open for
> comment and not yet documented it yet.
> 
Sure.

To easy on the reviewers, the under cut portion of a specific patch could be
used to add footer notes like TODO/Testing etc. In this case, I was lazy to 
read the loong cover letter posted in this series ;-)

Thanks,
Pavan

Re: [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line

On Mon, Sep 11, 2023 at 11:34 PM Waiman Long  wrote:
>
> On 9/10/23 04:29, guo...@kernel.org wrote:
> > From: Guo Ren 
> >
> > Allow cmdline to force the kernel to use queued_spinlock when
> > CONFIG_RISCV_COMBO_SPINLOCKS=y.
> >
> > Signed-off-by: Guo Ren 
> > Signed-off-by: Guo Ren 
> > ---
> >   Documentation/admin-guide/kernel-parameters.txt |  2 ++
> >   arch/riscv/kernel/setup.c   | 16 +++-
> >   2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 7dfb540c4f6c..61cacb8dfd0e 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4693,6 +4693,8 @@
> >   [KNL] Number of legacy pty's. Overwrites compiled-in
> >   default number.
> >
> > + qspinlock   [RISCV] Force to use qspinlock or auto-detect 
> > spinlock.
> > +
> >   qspinlock.numa_spinlock_threshold_ns=   [NUMA, PV_OPS]
> >   Set the time threshold in nanoseconds for the
> >   number of intra-node lock hand-offs before the
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index a447cf360a18..0f084f037651 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -270,6 +270,15 @@ static void __init parse_dtb(void)
> >   }
> >
> >   #ifdef CONFIG_RISCV_COMBO_SPINLOCKS
> > +bool enable_qspinlock_key = false;
>
> You can use __ro_after_init qualifier for enable_qspinlock_key. BTW,
> this is not a static key, just a simple flag. So what is the point of
> the _key suffix?
Okay, I would change it to:
bool enable_qspinlock_flag __ro_after_init = false;

>
> Cheers,
> Longman
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V11 04/17] locking/qspinlock: Improve xchg_tail for number of cpus >= 16k

On Mon, Sep 11, 2023 at 9:03 PM Waiman Long  wrote:
>
> On 9/10/23 23:09, Guo Ren wrote:
> > On Mon, Sep 11, 2023 at 10:35 AM Waiman Long  wrote:
> >>
> >> On 9/10/23 04:28, guo...@kernel.org wrote:
> >>> From: Guo Ren 
> >>>
> >>> The target of xchg_tail is to write the tail to the lock value, so
> >>> adding prefetchw could help the next cmpxchg step, which may
> >>> decrease the cmpxchg retry loops of xchg_tail. Some processors may
> >>> utilize this feature to give a forward guarantee, e.g., RISC-V
> >>> XuanTie processors would block the snoop channel & irq for several
> >>> cycles when prefetch.w instruction (from Zicbop extension) retired,
> >>> which guarantees the next cmpxchg succeeds.
> >>>
> >>> Signed-off-by: Guo Ren 
> >>> Signed-off-by: Guo Ren 
> >>> ---
> >>>kernel/locking/qspinlock.c | 5 -
> >>>1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> >>> index d3f99060b60f..96b54e2ade86 100644
> >>> --- a/kernel/locking/qspinlock.c
> >>> +++ b/kernel/locking/qspinlock.c
> >>> @@ -223,7 +223,10 @@ static __always_inline void 
> >>> clear_pending_set_locked(struct qspinlock *lock)
> >>> */
> >>>static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
> >>>{
> >>> - u32 old, new, val = atomic_read(&lock->val);
> >>> + u32 old, new, val;
> >>> +
> >>> + prefetchw(&lock->val);
> >>> + val = atomic_read(&lock->val);
> >>>
> >>>for (;;) {
> >>>new = (val & _Q_LOCKED_PENDING_MASK) | tail;
> >> That looks a bit weird. You pre-fetch and then immediately read it. How
> >> much performance gain you get by this change alone?
> >>
> >> Maybe you can define an arch specific primitive that default back to
> >> atomic_read() if not defined.
> > Thx for the reply. This is a generic optimization point I would like
> > to talk about with you.
> >
> > First, prefetchw() makes cacheline an exclusive state and serves for
> > the next cmpxchg loop semantic, which writes the idx_tail part of
> > arch_spin_lock. The atomic_read only makes cacheline in the shared
> > state, which couldn't give any guarantee for the next cmpxchg loop
> > semantic. Micro-architecture could utilize prefetchw() to provide a
> > strong forward progress guarantee for the xchg_tail, e.g., the T-HEAD
> > XuanTie processor would hold the exclusive cacheline state until the
> > next cmpxchg write success.
> >
> > In the end, Let's go back to the principle: the xchg_tail is an atomic
> > swap operation that contains write eventually, so giving a prefetchw()
> > at the beginning is acceptable for all architectures..
> > 
>
> I did realize afterward that prefetchw gets the cacheline in exclusive
> state. I will suggest you mention that in your commit log as well as
> adding a comment about its purpose in the code.
Okay, I would do that in v12, thx.

>
> Thanks,
> Longman
>
> >> Cheers,
> >> Longman
> >>
> >
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V11 07/17] riscv: qspinlock: Introduce qspinlock param for command line

On Mon, Sep 11, 2023 at 11:22 PM Waiman Long  wrote:
>
> On 9/10/23 04:29, guo...@kernel.org wrote:
> > From: Guo Ren 
> >
> > Allow cmdline to force the kernel to use queued_spinlock when
> > CONFIG_RISCV_COMBO_SPINLOCKS=y.
> >
> > Signed-off-by: Guo Ren 
> > Signed-off-by: Guo Ren 
> > ---
> >   Documentation/admin-guide/kernel-parameters.txt |  2 ++
> >   arch/riscv/kernel/setup.c   | 16 +++-
> >   2 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt 
> > b/Documentation/admin-guide/kernel-parameters.txt
> > index 7dfb540c4f6c..61cacb8dfd0e 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -4693,6 +4693,8 @@
> >   [KNL] Number of legacy pty's. Overwrites compiled-in
> >   default number.
> >
> > + qspinlock   [RISCV] Force to use qspinlock or auto-detect 
> > spinlock.
> > +
> >   qspinlock.numa_spinlock_threshold_ns=   [NUMA, PV_OPS]
> >   Set the time threshold in nanoseconds for the
> >   number of intra-node lock hand-offs before the
>
> Your patch series is still based on top of numa-aware qspinlock patchset
> which isn't upstream yet. Please rebase it without that as that will
> cause merge conflict during upstream merge.
Okay, thx for pointing it out.

>
> Cheers,
> Longman
>


-- 
Best Regards
 Guo Ren

Re: [PATCH V11 00/17] riscv: Add Native/Paravirt qspinlock support