Re: [PATCH] remoteproc: qcom: Fix NULL pointer in glink_subdev_stop()

[Mukesh Ojha]

On Fri, Sep 27, 2024 at 02:59:09PM -0700, Bjorn Andersson wrote:
> On Sat, Sep 28, 2024 at 01:07:43AM +0530, Mukesh Ojha wrote:
> > On Wed, Sep 25, 2024 at 08:41:55PM -0700, Bjorn Andersson wrote:
> > > On Wed, Sep 25, 2024 at 04:03:51PM +0530, Mukesh Ojha wrote:
> > > > Multiple call to glink_subdev_stop() for the same remoteproc can happen
> > > > if rproc_stop() fails from Process-A that leaves the rproc state to
> > > > RPROC_CRASHED state later a call to recovery_store from user space in
> > > > Process B triggers rproc_trigger_recovery() of the same remoteproc to
> > > > recover it results in NULL pointer dereference issue in
> > > > qcom_glink_smem_unregister().
> > > > 
> > > > Fix it by having a NULL check in glink_subdev_stop().
> > > > 
> > > > Process-A   Process-B
> > > > 
> > > >   fatal error interrupt happens
> > > > 
> > > >   rproc_crash_handler_work()
> > > > mutex_lock_interruptible(&rproc->lock);
> > > > ...
> > > > 
> > > >rproc->state = RPROC_CRASHED;
> > > > ...
> > > > mutex_unlock(&rproc->lock);
> > > > 
> > > > rproc_trigger_recovery()
> > > >  mutex_lock_interruptible(&rproc->lock);
> > > > 
> > > >   adsp_stop()
> > > >   qcom_q6v5_pas 20c0.remoteproc: failed to shutdown: -22
> > > >   remoteproc remoteproc3: can't stop rproc: -22
> > > 
> > > I presume that at this point this remoteproc is in some undefined state
> > > and the only way to recover is for the user to reboot the machine?
> > 
> > Here, 50+ (5s) retry of scm shutdown is failing during decryption of
> > remote processor memory region, and i don't think, it is anyway to do
> > with remote processor state here, as a best effort more number of
> > retries can be tried instead of 50 or wait for some other recovery
> > command like recovery_store() to let it do the retry again from
> > beginning.
> > 
> 
> But are you saying that retrying a bit later would allow us to get out
> of this problem? If we just didn't hit the NULL pointer(s)?

I am not sure whether adding more number of retries will solve the issue
and initially thinking from perspective that, it is better to retry than
to leave the remoteproc in some unknown state however, I do get that
letting it retry could give unnecessary patching every code e.g., ssr
notifier callbacks, which is not expecting to be called twice as a
side-effect of remoteproc unknown state.
> 
> How long are we talking about here? Is the timeout too short?

5sec is very significant amount of time in wait for remote processor to
get recovered, we should not change this.
> 
> > > 
> > > 
> > > The check for glink->edge avoids one pitfall following this, but I'd
> > > prefer to see a solution that avoids issues in this scenario in the
> > > remoteproc core - rather than working around side effects of this in
> > > different places.
> > 
> > Handling in a remoteproc core means we may need another state something
> > like "RPROC_UNKNOWN" which can be kept after one attempt of recovery
> > failure and checking the same during another try return immediately with
> > some log message.
> > 
> 
> Yes, if we are failing to shut down the remoteproc and there's no way
> for us to reliably recover from that (e.g. we are not able to reclaim
> the memory), it seems reasonable that we have to mark it using a new
> state.
> 
> If that is the case, I'd call it RPROC_DEFUNCT (or something like that
> instead), because while in some "unknown" state, from a remoteproc
> framework's point of view, it's in a well known (broken) state.

Ack.

-Mukesh
> 
> Regards,
> Bjorn

Re: [PATCH] remoteproc: qcom: Fix NULL pointer in glink_subdev_stop()

[Mukesh Ojha]

On Wed, Sep 25, 2024 at 08:41:55PM -0700, Bjorn Andersson wrote:
> On Wed, Sep 25, 2024 at 04:03:51PM +0530, Mukesh Ojha wrote:
> > Multiple call to glink_subdev_stop() for the same remoteproc can happen
> > if rproc_stop() fails from Process-A that leaves the rproc state to
> > RPROC_CRASHED state later a call to recovery_store from user space in
> > Process B triggers rproc_trigger_recovery() of the same remoteproc to
> > recover it results in NULL pointer dereference issue in
> > qcom_glink_smem_unregister().
> > 
> > Fix it by having a NULL check in glink_subdev_stop().
> > 
> > Process-A   Process-B
> > 
> >   fatal error interrupt happens
> > 
> >   rproc_crash_handler_work()
> > mutex_lock_interruptible(&rproc->lock);
> > ...
> > 
> >rproc->state = RPROC_CRASHED;
> > ...
> > mutex_unlock(&rproc->lock);
> > 
> > rproc_trigger_recovery()
> >  mutex_lock_interruptible(&rproc->lock);
> > 
> >   adsp_stop()
> >   qcom_q6v5_pas 20c0.remoteproc: failed to shutdown: -22
> >   remoteproc remoteproc3: can't stop rproc: -22
> 
> I presume that at this point this remoteproc is in some undefined state
> and the only way to recover is for the user to reboot the machine?

Here, 50+ (5s) retry of scm shutdown is failing during decryption of
remote processor memory region, and i don't think, it is anyway to do
with remote processor state here, as a best effort more number of
retries can be tried instead of 50 or wait for some other recovery
command like recovery_store() to let it do the retry again from
beginning.

> 
> 
> The check for glink->edge avoids one pitfall following this, but I'd
> prefer to see a solution that avoids issues in this scenario in the
> remoteproc core - rather than working around side effects of this in
> different places.

Handling in a remoteproc core means we may need another state something
like "RPROC_UNKNOWN" which can be kept after one attempt of recovery
failure and checking the same during another try return immediately with
some log message.

-Mukesh

[PATCH] remoteproc: qcom: Fix NULL pointer in glink_subdev_stop()

[Mukesh Ojha]

Multiple call to glink_subdev_stop() for the same remoteproc can happen
if rproc_stop() fails from Process-A that leaves the rproc state to
RPROC_CRASHED state later a call to recovery_store from user space in
Process B triggers rproc_trigger_recovery() of the same remoteproc to
recover it results in NULL pointer dereference issue in
qcom_glink_smem_unregister().

Fix it by having a NULL check in glink_subdev_stop().

Process-A   Process-B

  fatal error interrupt happens

  rproc_crash_handler_work()
mutex_lock_interruptible(&rproc->lock);
...

   rproc->state = RPROC_CRASHED;
...
mutex_unlock(&rproc->lock);

rproc_trigger_recovery()
 mutex_lock_interruptible(&rproc->lock);

  adsp_stop()
  qcom_q6v5_pas 20c0.remoteproc: failed to shutdown: -22
  remoteproc remoteproc3: can't stop rproc: -22
 mutex_unlock(&rproc->lock);

echo enabled > 
/sys/class/remoteproc/remoteprocX/recovery
recovery_store()
 rproc_trigger_recovery()
  
mutex_lock_interruptible(&rproc->lock);
   rproc_stop()
glink_subdev_stop()
  
qcom_glink_smem_unregister() ==|

 |

 V
  Unable to handle kernel 
NULL pointer dereference
at virtual 
address 0358

Signed-off-by: Mukesh Ojha 
---
- We can do this NULL check in qcom_glink_smem_unregister() as it is
  exported function however, there is only one user of this. So, doing
  it with current approach should also be fine.

 drivers/remoteproc/qcom_common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 8c8688f99f0a..52d6c9b99fdb 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -209,6 +209,9 @@ static void glink_subdev_stop(struct rproc_subdev *subdev, 
bool crashed)
 {
struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
 
+   if (!glink->edge)
+   return;
+
qcom_glink_smem_unregister(glink->edge);
glink->edge = NULL;
 }
-- 
2.34.1

Re: [PATCH] rpmsg: char: fix rpmsg_eptdev structure documentation

[Mukesh Ojha]

On 5/17/2024 10:26 PM, Arnaud Pouliquen wrote:

Add missing @ tags for some rpmsg_eptdev structure parameters.

This fixes warning messages on build:
drivers/rpmsg/rpmsg_char.c:75: warning: Function parameter or struct member 
'remote_flow_restricted' not described in 'rpmsg_eptdev'
drivers/rpmsg/rpmsg_char.c:75: warning: Function parameter or struct member 
'remote_flow_updated' not described in 'rpmsg_eptdev'

Fixes: 5550201c0fe2 ("rpmsg: char: Add RPMSG GET/SET FLOWCONTROL IOCTL support")

Signed-off-by: Arnaud Pouliquen 


Reviewed-by: Mukesh Ojha 

-Mukesh

Re: [PATCH] remoteproc: mediatek: Zero out only remaining bytes of IPI buffer

[Mukesh Ojha]

On 5/20/2024 4:57 PM, AngeloGioacchino Del Regno wrote:

In scp_ipi_handler(), instead of zeroing out the entire shared
buffer, which may be as large as 600 bytes, overwrite it with the
received data, then zero out only the remaining bytes.

Signed-off-by: AngeloGioacchino Del Regno 

---
  drivers/remoteproc/mtk_scp.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index e5214d43181e..dc70cf7db44d 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -117,8 +117,8 @@ static void scp_ipi_handler(struct mtk_scp *scp)
return;
}
  
-	memset(scp->share_buf, 0, scp_sizes->ipi_share_buffer_size);

memcpy_fromio(scp->share_buf, &rcv_obj->share_buf, len);
+   memset(&scp->share_buf[len], 0, scp_sizes->ipi_share_buffer_size - len);


Although, it does not make any difference apart from a write of len 
bytes, still a good improvement to do ..


Acked-by: Mukesh Ojha 

-Mukesh


handler(scp->share_buf, len, ipi_desc[id].priv);
scp_ipi_unlock(scp, id);
  

Re: [PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module

[Mukesh Ojha]

On 4/30/2024 7:08 PM, Bjorn Andersson wrote:

On Tue, Mar 26, 2024 at 07:43:12PM +0530, Mukesh Ojha wrote:

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().



I'd prefer to see this enter the git history as one patch, extracting
this logic from the remoteproc into its own driver - rather than as
presented here give a sense that it's a new thing added. (I'll take care
of the maintainer sync...)

I also would prefer for this to include a problem description,
documenting why this is done.


I've not compared patch 1 and 3, but I'd also like a statement in the
commit message telling if there are any changes, or if the functions are
cleanly moved from one place to another.



Thanks for the review, addressed the comments here in v10.

https://lore.kernel.org/lkml/1714724287-12518-1-git-send-email-quic_mo...@quicinc.com/

-Mukesh

[PATCH v10] remoteproc: qcom: Move minidump related layout and API to soc/qcom directory

[Mukesh Ojha]

Currently, Qualcomm Minidump is being used to collect mini version of
remoteproc coredump with the help of boot firmware however, Minidump
as a feature is not limited to be used only for remote processor and
can also be used for Application processors. So, in preparation of
using it move the Minidump related data structure and its function
to its own file under drivers/soc/qcom with qcom_rproc_minidump.c
which clearly says it is only for remoteproc minidump.

Extra changes made apart from the movement is,
1. Adds new config, kernel headers and module macros to get this
   module compiled.
2. Guards the qcom_minidump() with CONFIG_QCOM_RPROC_MINIDUMP.
3. Selects this QCOM_RPROC_MINIDUMP config when QCOM_RPROC_COMMON
   enabled.
4. Added new header qcom_minidump.h .

Signed-off-by: Mukesh Ojha 
---
Changes in v10:
 - Converted all 3 changes sent in v9 to a single to properly
   reflect the movement in git history as per suggestion made by [Bjorn.A]

 - Described the reason of the change in commit text.

Changes in v9: 
https://lore.kernel.org/lkml/1711462394-21540-1-git-send-email-quic_mo...@quicinc.com/
 - Added source file driver/remoteproc/qcom_common.c copyright
   to qcom_rproc_minidump.c
 - Dissociated it from minidump series as this can go separately
   and minidump can put it dependency for the data structure files.

Nothing much changed in these three patches from previous version,
However, giving the link of their older versions.

v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6: 
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5: 
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4: 
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/

 drivers/remoteproc/Kconfig |   1 +
 drivers/remoteproc/qcom_common.c   | 160 -
 drivers/remoteproc/qcom_common.h   |   5 -
 drivers/remoteproc/qcom_q6v5_pas.c |   1 +
 drivers/soc/qcom/Kconfig   |  11 ++
 drivers/soc/qcom/Makefile  |   1 +
 drivers/soc/qcom/qcom_rproc_minidump.c | 177 +
 include/soc/qcom/qcom_minidump.h   |  23 +
 8 files changed, 214 insertions(+), 165 deletions(-)
 create mode 100644 drivers/soc/qcom/qcom_rproc_minidump.c
 create mode 100644 include/soc/qcom/qcom_minidump.h

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_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  enable

Re: [PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module

[Mukesh Ojha]

Gentle ping..

-Mukesh

On 3/26/2024 7:43 PM, Mukesh Ojha wrote:

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 
---
Changes in v9:
  - Added source file driver/remoteproc/qcom_common.c copyright
to qcom_rproc_minidump.c
  - Dissociated it from minidump series as this can go separately
and minidump can put it dependency for the data structure files.

Nothing much changed in these three patches from previous version,
However, giving the link of their older versions.

v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6: 
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5: 
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4: 
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/

  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| 115 ++
  include/soc/qcom/qcom_minidump.h  |  23 ++
  5 files changed, 213 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 5af33b0e3470..ed23e0275c22 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -277,4 +277,14 @@ config QCOM_PBS
  This module provides the APIs to the client drivers that wants to 
send the
  PBS trigger event to the PBS RAM.
  
+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 ca0bece0dfff..44664589263d 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,3 +36,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_PBS) += qcom-pbs.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 

[PATCH v9 3/3] remoteproc: qcom: Remove minidump related data from qcom_common.c

[Mukesh Ojha]

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 
---
Changes in v9:
 - Change in patch order.
 - rebased it.

v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6: 
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5: 
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4: 
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/

 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));
-  

[PATCH v9 2/3] remoteproc: qcom_q6v5_pas: Use qcom_rproc_minidump()

[Mukesh Ojha]

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 
---
Changes in v9:
 - Change in patch order from its last version.
 - Rebased it.

v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6: 
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5: 
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4: 
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/

 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 54d8005d40a3..b39f87dfd9c0 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"
@@ -141,7 +142,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

[PATCH v9 1/3] soc: qcom: Add qcom_rproc_minidump module

[Mukesh Ojha]

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 
---
Changes in v9:
 - Added source file driver/remoteproc/qcom_common.c copyright
   to qcom_rproc_minidump.c 
 - Dissociated it from minidump series as this can go separately
   and minidump can put it dependency for the data structure files.

Nothing much changed in these three patches from previous version,
However, giving the link of their older versions.

v8: https://lore.kernel.org/lkml/20240131105734.13090-1-quic_mo...@quicinc.com/
v7: https://lore.kernel.org/lkml/20240109153200.12848-1-quic_mo...@quicinc.com/
v6: 
https://lore.kernel.org/lkml/1700864395-1479-1-git-send-email-quic_mo...@quicinc.com/
v5: 
https://lore.kernel.org/lkml/1694429639-21484-1-git-send-email-quic_mo...@quicinc.com/
v4: 
https://lore.kernel.org/lkml/1687955688-20809-1-git-send-email-quic_mo...@quicinc.com/

 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| 115 ++
 include/soc/qcom/qcom_minidump.h  |  23 ++
 5 files changed, 213 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 5af33b0e3470..ed23e0275c22 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -277,4 +277,14 @@ config QCOM_PBS
  This module provides the APIs to the client drivers that wants to 
send the
  PBS trigger event to the PBS RAM.
 
+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 ca0bece0dfff..44664589263d 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -36,3 +36,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_PBS) +=  qcom-pbs.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_basep

Re: [PATCH v7 4/4] arm64: dts: qcom: sm8650: add missing qlink_logging reserved memory for mpss

[Mukesh Ojha]

On 1/23/2024 2:21 PM, Neil Armstrong wrote:

The qlink_logging memory region is also used by the modem firmware,
add it to the reserved memories and add it to the MPSS memory regions.

Signed-off-by: Neil Armstrong 


LGTM,

Reviewed-by: Mukesh Ojha 

-Mukesh


---
  arch/arm64/boot/dts/qcom/sm8650.dtsi | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi 
b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 2df77123a8c7..7a1cbc823306 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -525,6 +525,11 @@ qdss_mem: qdss@8280 {
no-map;
};
  
+		qlink_logging_mem: qlink-logging@8480 {

+   reg = <0 0x8480 0 0x20>;
+   no-map;
+   };
+
mpss_dsm_mem: mpss-dsm@86b0 {
reg = <0 0x86b0 0 0x490>;
no-map;
@@ -2627,7 +2632,8 @@ remoteproc_mpss: remoteproc@408 {
 "mss";
  
  			memory-region = <&mpss_mem>, <&q6_mpss_dtb_mem>,

-   <&mpss_dsm_mem>, <&mpss_dsm_mem_2>;
+   <&mpss_dsm_mem>, <&mpss_dsm_mem_2>,
+   <&qlink_logging_mem>;
  
  			qcom,qmp = <&aoss_qmp>;
  

Re: [PATCH v7 3/4] remoteproc: qcom: pas: Add SM8650 remoteproc support

[Mukesh Ojha]

On 1/23/2024 2:21 PM, Neil Armstrong wrote:

Add DSP Peripheral Authentication Service support for the SM8650 platform.

Reviewed-by: Dmitry Baryshkov 
Signed-off-by: Neil Armstrong 
---
  drivers/remoteproc/qcom_q6v5_pas.c | 50 ++
  1 file changed, 50 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 09e8ad9f08c4..d0b1f0f38347 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -1213,6 +1213,53 @@ static const struct adsp_data sc7280_wpss_resource = {
.ssctl_id = 0x19,
  };
  
+static const struct adsp_data sm8650_cdsp_resource = {

+   .crash_reason_smem = 601,
+   .firmware_name = "cdsp.mdt",
+   .dtb_firmware_name = "cdsp_dtb.mdt",
+   .pas_id = 18,
+   .dtb_pas_id = 0x25,
+   .minidump_id = 7,
+   .auto_boot = true,
+   .proxy_pd_names = (char*[]){
+   "cx",
+   "mxc",
+   "nsp",
+   NULL
+   },
+   .load_state = "cdsp",
+   .ssr_name = "cdsp",
+   .sysmon_name = "cdsp",
+   .ssctl_id = 0x17,
+   .region_assign_idx = 2,
+   .region_assign_count = 1,
+   .region_assign_shared = true,
+   .region_assign_vmid = QCOM_SCM_VMID_CDSP,
+};
+
+static const struct adsp_data sm8650_mpss_resource = {
+   .crash_reason_smem = 421,
+   .firmware_name = "modem.mdt",
+   .dtb_firmware_name = "modem_dtb.mdt",
+   .pas_id = 4,
+   .dtb_pas_id = 0x26,
+   .minidump_id = 3,
+   .auto_boot = false,
+   .decrypt_shutdown = true,
+   .proxy_pd_names = (char*[]){
+   "cx",
+   "mss",
+   NULL
+   },
+   .load_state = "modem",
+   .ssr_name = "mpss",
+   .sysmon_name = "modem",
+   .ssctl_id = 0x12,
+   .region_assign_idx = 2,
+   .region_assign_count = 3,


I see this has changed from 2 to 3 after qlink logging addition;


+   .region_assign_vmid = QCOM_SCM_VMID_MSS_MSA,
+};
+
  static const struct of_device_id adsp_of_match[] = {
{ .compatible = "qcom,msm8226-adsp-pil", .data = &adsp_resource_init},
{ .compatible = "qcom,msm8953-adsp-pil", .data = 
&msm8996_adsp_resource},
@@ -1268,6 +1315,9 @@ static const struct of_device_id adsp_of_match[] = {
{ .compatible = "qcom,sm8550-adsp-pas", .data = &sm8550_adsp_resource},
{ .compatible = "qcom,sm8550-cdsp-pas", .data = &sm8550_cdsp_resource},
{ .compatible = "qcom,sm8550-mpss-pas", .data = &sm8550_mpss_resource},
+   { .compatible = "qcom,sm8650-adsp-pas", .data = &sm8550_adsp_resource},


Same as sm8550;

+   { .compatible = "qcom,sm8650-cdsp-pas", .data = &sm8650_cdsp_resource},
+   { .compatible = "qcom,sm8650-mpss-pas", .data = &sm8650_mpss_resource},


LGTM,

Acked-by: Mukesh Ojha 

-Mukesh


{ },
  };
  MODULE_DEVICE_TABLE(of, adsp_of_match);

Re: [PATCH V3] remoteproc: qcom: q6v5: Get crash reason from specific SMEM partition

[Mukesh Ojha]

On 12/18/2023 11:40 AM, Vignesh Viswanathan wrote:

q6v5 fatal and watchdog IRQ handlers always retrieves the crash reason
information from SMEM global partition (QCOM_SMEM_HOST_ANY).

For some targets like IPQ9574 and IPQ5332, crash reason information is
present in target specific partition due to which the crash reason is
not printed in the current implementation.

Add support to pass crash_reason_partition along with crash_reason_item
number in qcom_q6v5_init call and use the same to get the crash
information from SMEM in fatal and watchdog IRQ handlers.

Signed-off-by: Vignesh Viswanathan 
---
Changes in V3: Updated commit message.
Changes in V2: Addressed comments in V1.

This patch depends on [1] which adds support for IPQ9574 and IPQ5332
remoteproc q5v5_mpd driver.

[1]: 
https://lore.kernel.org/all/20231110091939.3025413-1-quic_mmani...@quicinc.com/

  drivers/remoteproc/qcom_q6v5.c  | 10 ++
  drivers/remoteproc/qcom_q6v5.h  |  6 --
  drivers/remoteproc/qcom_q6v5_adsp.c |  5 +++--
  drivers/remoteproc/qcom_q6v5_mpd.c  | 14 --
  drivers/remoteproc/qcom_q6v5_mss.c  |  5 +++--
  drivers/remoteproc/qcom_q6v5_pas.c  |  3 ++-
  drivers/remoteproc/qcom_q6v5_wcss.c |  4 +++-
  7 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5.c b/drivers/remoteproc/qcom_q6v5.c
index 0e32f13c196d..e4a28bf25130 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -100,7 +100,7 @@ static irqreturn_t q6v5_wdog_interrupt(int irq, void *data)
return IRQ_HANDLED;
}
  
-	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, q6v5->crash_reason, &len);

+   msg = qcom_smem_get(q6v5->crash_reason_partition, q6v5->crash_reason_item, 
&len);
if (!IS_ERR(msg) && len > 0 && msg[0])
dev_err(q6v5->dev, "watchdog received: %s\n", msg);
else
@@ -121,7 +121,7 @@ irqreturn_t q6v5_fatal_interrupt(int irq, void *data)
if (!q6v5->running)
return IRQ_HANDLED;
  
-	msg = qcom_smem_get(QCOM_SMEM_HOST_ANY, q6v5->crash_reason, &len);

+   msg = qcom_smem_get(q6v5->crash_reason_partition, q6v5->crash_reason_item, 
&len);
if (!IS_ERR(msg) && len > 0 && msg[0])
dev_err(q6v5->dev, "fatal error received: %s\n", msg);
else
@@ -279,14 +279,16 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_panic);
   * Return: 0 on success, negative errno on failure
   */
  int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,
-  struct rproc *rproc, int crash_reason, const char 
*load_state,
+  struct rproc *rproc, int crash_reason_partition,
+  int crash_reason_item, const char *load_state,
   void (*handover)(struct qcom_q6v5 *q6v5))
  {
int ret;
  
  	q6v5->rproc = rproc;

q6v5->dev = &pdev->dev;
-   q6v5->crash_reason = crash_reason;
+   q6v5->crash_reason_partition = crash_reason_partition;
+   q6v5->crash_reason_item = crash_reason_item;
q6v5->handover = handover;
  
  	init_completion(&q6v5->start_done);

diff --git a/drivers/remoteproc/qcom_q6v5.h b/drivers/remoteproc/qcom_q6v5.h
index 4e1bb1a68284..cd02372e9856 100644
--- a/drivers/remoteproc/qcom_q6v5.h
+++ b/drivers/remoteproc/qcom_q6v5.h
@@ -40,7 +40,8 @@ struct qcom_q6v5 {
struct completion stop_done;
struct completion spawn_done;
  
-	int crash_reason;

+   int crash_reason_partition;
+   int crash_reason_item;
  
  	bool running;
  
@@ -49,7 +50,8 @@ struct qcom_q6v5 {

  };
  
  int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device *pdev,

-  struct rproc *rproc, int crash_reason, const char 
*load_state,
+  struct rproc *rproc, int crash_reason_partition,
+  int crash_reason_item, const char *load_state,
   void (*handover)(struct qcom_q6v5 *q6v5));
  void qcom_q6v5_deinit(struct qcom_q6v5 *q6v5);
  
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c

index 6c67514cc493..8feb2eb45737 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -731,8 +731,9 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto disable_pm;
  
-	ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, desc->crash_reason_smem,

-desc->load_state, qcom_adsp_pil_handover);
+   ret = qcom_q6v5_init(&adsp->q6v5, pdev, rproc, QCOM_SMEM_HOST_ANY,
+desc->crash_reason_smem, desc->load_state,


Can we also rename this ->crash_reason_smem to ->crash_reason_item to
proper reflect its meaning ?

-Mukesh


+qcom_adsp_pil_handover);
if (ret)
goto disable_pm;
  
diff --git a/drivers/remoteproc/qcom_q6v5_mpd.c b/drivers/remoteproc/qcom_q6v5_mpd.c

index b133285888c7..c893deac30e1 100644
--- a/drivers/remoteproc/qcom_q6v5_mpd.

Re: [PATCH v3 2/3] remoteproc: qcom: pas: make region assign more generic

[Mukesh Ojha]

egion_assign_size[offset],
+ &adsp->region_assign_perms[offset],
+ perm, perm_size);
+   if (ret < 0) {
+   dev_err(adsp->dev, "assign memory %d failed\n", offset);
+   return ret;
+   }
}
  
  	return 0;

@@ -629,20 +653,22 @@ static int adsp_assign_memory_region(struct qcom_adsp 
*adsp)
  static void adsp_unassign_memory_region(struct qcom_adsp *adsp)
  {
struct qcom_scm_vmperm perm;
-   int ret;
+   int offset, ret;


one variable per line..

  
-	if (!adsp->region_assign_idx)

+   if (!adsp->region_assign_idx || adsp->region_assign_shared)
return;
  
-	perm.vmid = QCOM_SCM_VMID_HLOS;

-   perm.perm = QCOM_SCM_PERM_RW;
+   for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+   perm.vmid = QCOM_SCM_VMID_HLOS;
+   perm.perm = QCOM_SCM_PERM_RW;
  
-	ret = qcom_scm_assign_mem(adsp->region_assign_phys,

- adsp->region_assign_size,
- &adsp->region_assign_perms,
- &perm, 1);
-   if (ret < 0)
-   dev_err(adsp->dev, "unassign memory failed\n");
+   ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
+ adsp->region_assign_size[offset],
+ &adsp->region_assign_perms[offset],
+ &perm, 1);
+   if (ret < 0)
+   dev_err(adsp->dev, "unassign memory failed\n");


In case you are going to send another version,
you can print offset similar to the assign call failure.,

Otherwise, LGTM.
Feel free to add.
Reviewed-by: Mukesh Ojha 

-Mukesh


+   }
  }
  
  static int adsp_probe(struct platform_device *pdev)

@@ -696,6 +722,9 @@ static int adsp_probe(struct platform_device *pdev)
adsp->info_name = desc->sysmon_name;
adsp->decrypt_shutdown = desc->decrypt_shutdown;
adsp->region_assign_idx = desc->region_assign_idx;
+   adsp->region_assign_count = min_t(int, MAX_ASSIGN_COUNT, 
desc->region_assign_count);
+   adsp->region_assign_vmid = desc->region_assign_vmid;
+   adsp->region_assign_shared = desc->region_assign_shared;
if (dtb_fw_name) {
adsp->dtb_firmware_name = dtb_fw_name;
adsp->dtb_pas_id = desc->dtb_pas_id;
@@ -1163,6 +1192,8 @@ static const struct adsp_data sm8550_mpss_resource = {
.sysmon_name = "modem",
.ssctl_id = 0x12,
.region_assign_idx = 2,
+   .region_assign_count = 1,
+   .region_assign_vmid = QCOM_SCM_VMID_MSS_MSA,
  };
  
  static const struct of_device_id adsp_of_match[] = {

Re: [PATCH v2 2/3] remoteproc: qcom: pas: make region assign more generic

[Mukesh Ojha]

On 11/2/2023 3:56 PM, neil.armstr...@linaro.org wrote:

On 01/11/2023 15:42, Mukesh Ojha wrote:



On 10/31/2023 10:36 PM, Neil Armstrong wrote:

Hi,

On 30/10/2023 14:10, Mukesh Ojha wrote:



On 10/30/2023 3:33 PM, Neil Armstrong wrote:

The current memory region assign only supports a single
memory region.

But new platforms introduces more regions to make the
memory requirements more flexible for various use cases.
Those new platforms also shares the memory region between the
DSP and HLOS.

To handle this, make the region assign more generic in order
to support more than a single memory region and also permit
setting the regions permissions as shared.

Signed-off-by: Neil Armstrong 
---
  drivers/remoteproc/qcom_q6v5_pas.c | 102 
-

  1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c

index 913a5d2068e8..4829fd26e17d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -33,6 +33,8 @@
  #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS    100
+#define MAX_ASSIGN_COUNT 2
+
  struct adsp_data {
  int crash_reason_smem;
  const char *firmware_name;
@@ -51,6 +53,9 @@ struct adsp_data {
  int ssctl_id;
  int region_assign_idx;
+    int region_assign_count;
+    bool region_assign_shared;
+    int region_assign_vmid;
  };
  struct qcom_adsp {
@@ -87,15 +92,18 @@ struct qcom_adsp {
  phys_addr_t dtb_mem_phys;
  phys_addr_t mem_reloc;
  phys_addr_t dtb_mem_reloc;
-    phys_addr_t region_assign_phys;
+    phys_addr_t region_assign_phys[MAX_ASSIGN_COUNT];
  void *mem_region;
  void *dtb_mem_region;
  size_t mem_size;
  size_t dtb_mem_size;
-    size_t region_assign_size;
+    size_t region_assign_size[MAX_ASSIGN_COUNT];
  int region_assign_idx;
-    u64 region_assign_perms;
+    int region_assign_count;
+    bool region_assign_shared;
+    int region_assign_vmid;
+    u64 region_assign_perms[MAX_ASSIGN_COUNT];
  struct qcom_rproc_glink glink_subdev;
  struct qcom_rproc_subdev smd_subdev;
@@ -590,37 +598,52 @@ static int adsp_alloc_memory_region(struct 
qcom_adsp *adsp)

  static int adsp_assign_memory_region(struct qcom_adsp *adsp)
  {
-    struct reserved_mem *rmem = NULL;
-    struct qcom_scm_vmperm perm;
+    struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
+    unsigned int perm_size = 1;


AFAICS, not need of initialization.


Indeed, removed




  struct device_node *node;
-    int ret;
+    int offset, ret;


Nit: one variable per line.


Done




  if (!adsp->region_assign_idx)


Not related to this patch..
Should not this be valid only for > 1 ?


I don't understand, only region_assign_idx > 1 triggers a memory_assign,
and this check discards configurations with region_assign_idx == 0 as
expected.


Ah, you can ignore the comments, I got the intention after commenting
here ..







  return 0;
-    node = of_parse_phandle(adsp->dev->of_node, "memory-region", 
adsp->region_assign_idx);

-    if (node)
-    rmem = of_reserved_mem_lookup(node);
-    of_node_put(node);
-    if (!rmem) {
-    dev_err(adsp->dev, "unable to resolve shareable 
memory-region\n");

-    return -EINVAL;
-    }
+    for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+    struct reserved_mem *rmem = NULL;
+
+    node = of_parse_phandle(adsp->dev->of_node, "memory-region",
+    adsp->region_assign_idx + offset);
+    if (node)
+    rmem = of_reserved_mem_lookup(node);
+    of_node_put(node);
+    if (!rmem) {
+    dev_err(adsp->dev, "unable to resolve shareable 
memory-region index %d\n",

+    offset);
+    return -EINVAL; > +    }




-    perm.vmid = QCOM_SCM_VMID_MSS_MSA;
-    perm.perm = QCOM_SCM_PERM_RW;
+    if (adsp->region_assign_shared)  {
+    perm[0].vmid = QCOM_SCM_VMID_HLOS;
+    perm[0].perm = QCOM_SCM_PERM_RW;
+    perm[1].vmid = adsp->region_assign_vmid;
+    perm[1].perm = QCOM_SCM_PERM_RW;
+    perm_size = 2;
+    } else {
+    perm[0].vmid = adsp->region_assign_vmid;
+    perm[0].perm = QCOM_SCM_PERM_RW;
+    perm_size = 1;
+    }
-    adsp->region_assign_phys = rmem->base;
-    adsp->region_assign_size = rmem->size;
-    adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
+    adsp->region_assign_phys[offset] = rmem->base;
+    adsp->region_assign_size[offset] = rmem->size;
+    adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);


Do we need array for this, is this changing ?


We need to keep region_assign_perms for unassign, but for the other 2 
we would

need to duplicate the code from adsp_assign_memory_region into
adsp_unassign_memory_region.


Thanks got it.

Re: [PATCH] eventfs: Fix kerneldoc of eventfs_remove_rec()

[Mukesh Ojha]

On 11/2/2023 1:30 AM, Steven Rostedt wrote:

On Mon, 30 Oct 2023 21:57:13 +0530
Mukesh Ojha  wrote:


On 10/30/2023 9:45 PM, Steven Rostedt wrote:

From: "Steven Rostedt (Google)" 

The eventfs_remove_rec() had some missing parameters in the kerneldoc
comment above it. Also, rephrase the description a bit more to have a bit
more correct grammar.

Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use 
eventfs_inode");
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202310052216.4sgqaswo-...@intel.com/
Signed-off-by: Steven Rostedt (Google) 


Reviewed-by: Mukesh Ojha 


Hi Mukesh!

First, I want to thank you for your reviews. We certainly need more
reviewers.

But I need to also state that "Reviewed-by" tags should not be sent so
lightly. The only times a Reviewed-by tag should be sent is if you
participated in the discussion of the code, you have authored some of
the code that is being modified, or are marked as a reviewer of the code in
the MAINTAINERS file.


Thanks Steven for writing a suggestion note for me.

I will try to participate and take this in a good way..but i thought
for easier change where there is no discussion is needed., it is fine
to add if you have spent time in checking the code and change is proper.



For example, you added to the discussion here:

https://lore.kernel.org/all/65dcdd9c-a75b-4fe7-bdcf-471a5602d...@linaro.org/

And adding your Reviewed-by tag is appropriate.

But when a maintainer receives a Reviewed-by from someone they don't know,
without any discussion in the patch, it may make that maintainer think that
the person sending the Reviewed-by is only out to get listed in the LWN
"Reviewed-by" count.


I understand..



I review other developers' code all the time, and unless the code touches
something I worked on or I'm marked as a reviewer in the MAINTAINERS file,
I do not send a Reviewed-by tag unless I added some input to the patch in
question.


Will keep this in mind.
To get involve in LKML discussion, Sending Reviewed-by may not be
important but the discussions/engagement/contribution is .


My advice to you is to keep up the reviewing, I appreciate it (I really
do!), but don't send out Reviewed-by tags unless you are marked as a
reviewer of the code, or participated in a discussion on that code.


Sure, thanks, will try to do my bit.

-Mukesh



Thanks,

-- Steve

Re: [PATCH v2 2/3] remoteproc: qcom: pas: make region assign more generic

[Mukesh Ojha]

On 10/31/2023 10:36 PM, Neil Armstrong wrote:

Hi,

On 30/10/2023 14:10, Mukesh Ojha wrote:



On 10/30/2023 3:33 PM, Neil Armstrong wrote:

The current memory region assign only supports a single
memory region.

But new platforms introduces more regions to make the
memory requirements more flexible for various use cases.
Those new platforms also shares the memory region between the
DSP and HLOS.

To handle this, make the region assign more generic in order
to support more than a single memory region and also permit
setting the regions permissions as shared.

Signed-off-by: Neil Armstrong 
---
  drivers/remoteproc/qcom_q6v5_pas.c | 102 
-

  1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c

index 913a5d2068e8..4829fd26e17d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -33,6 +33,8 @@
  #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS    100
+#define MAX_ASSIGN_COUNT 2
+
  struct adsp_data {
  int crash_reason_smem;
  const char *firmware_name;
@@ -51,6 +53,9 @@ struct adsp_data {
  int ssctl_id;
  int region_assign_idx;
+    int region_assign_count;
+    bool region_assign_shared;
+    int region_assign_vmid;
  };
  struct qcom_adsp {
@@ -87,15 +92,18 @@ struct qcom_adsp {
  phys_addr_t dtb_mem_phys;
  phys_addr_t mem_reloc;
  phys_addr_t dtb_mem_reloc;
-    phys_addr_t region_assign_phys;
+    phys_addr_t region_assign_phys[MAX_ASSIGN_COUNT];
  void *mem_region;
  void *dtb_mem_region;
  size_t mem_size;
  size_t dtb_mem_size;
-    size_t region_assign_size;
+    size_t region_assign_size[MAX_ASSIGN_COUNT];
  int region_assign_idx;
-    u64 region_assign_perms;
+    int region_assign_count;
+    bool region_assign_shared;
+    int region_assign_vmid;
+    u64 region_assign_perms[MAX_ASSIGN_COUNT];
  struct qcom_rproc_glink glink_subdev;
  struct qcom_rproc_subdev smd_subdev;
@@ -590,37 +598,52 @@ static int adsp_alloc_memory_region(struct 
qcom_adsp *adsp)

  static int adsp_assign_memory_region(struct qcom_adsp *adsp)
  {
-    struct reserved_mem *rmem = NULL;
-    struct qcom_scm_vmperm perm;
+    struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
+    unsigned int perm_size = 1;


AFAICS, not need of initialization.


Indeed, removed




  struct device_node *node;
-    int ret;
+    int offset, ret;


Nit: one variable per line.


Done




  if (!adsp->region_assign_idx)


Not related to this patch..
Should not this be valid only for > 1 ?


I don't understand, only region_assign_idx > 1 triggers a memory_assign,
and this check discards configurations with region_assign_idx == 0 as
expected.


Ah, you can ignore the comments, I got the intention after commenting
here ..







  return 0;
-    node = of_parse_phandle(adsp->dev->of_node, "memory-region", 
adsp->region_assign_idx);

-    if (node)
-    rmem = of_reserved_mem_lookup(node);
-    of_node_put(node);
-    if (!rmem) {
-    dev_err(adsp->dev, "unable to resolve shareable 
memory-region\n");

-    return -EINVAL;
-    }
+    for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+    struct reserved_mem *rmem = NULL;
+
+    node = of_parse_phandle(adsp->dev->of_node, "memory-region",
+    adsp->region_assign_idx + offset);
+    if (node)
+    rmem = of_reserved_mem_lookup(node);
+    of_node_put(node);
+    if (!rmem) {
+    dev_err(adsp->dev, "unable to resolve shareable 
memory-region index %d\n",

+    offset);
+    return -EINVAL; > +    }




-    perm.vmid = QCOM_SCM_VMID_MSS_MSA;
-    perm.perm = QCOM_SCM_PERM_RW;
+    if (adsp->region_assign_shared)  {
+    perm[0].vmid = QCOM_SCM_VMID_HLOS;
+    perm[0].perm = QCOM_SCM_PERM_RW;
+    perm[1].vmid = adsp->region_assign_vmid;
+    perm[1].perm = QCOM_SCM_PERM_RW;
+    perm_size = 2;
+    } else {
+    perm[0].vmid = adsp->region_assign_vmid;
+    perm[0].perm = QCOM_SCM_PERM_RW;
+    perm_size = 1;
+    }
-    adsp->region_assign_phys = rmem->base;
-    adsp->region_assign_size = rmem->size;
-    adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
+    adsp->region_assign_phys[offset] = rmem->base;
+    adsp->region_assign_size[offset] = rmem->size;
+    adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);


Do we need array for this, is this changing ?


We need to keep region_assign_perms for unassign, but for the other 2 we 
would

need to duplicate the code from adsp_assign_memory_region into
adsp_unassign_memory_region.


Thanks got it.






-    ret = qcom_scm_assign_mem(adsp->region_assign_phys,
-  adsp->region_assi

Re: [PATCH] tracing/kprobes: Fix the order of argument descriptions

[Mukesh Ojha]

On 10/31/2023 9:43 AM, Yujie Liu wrote:

The order of descriptions should be consistent with the argument list of
the function, so "kretprobe" should be the second one.

int __kprobe_event_gen_cmd_start(struct dynevent_cmd *cmd, bool kretprobe,
  const char *name, const char *loc, ...)

Fixes: 2a588dd1d5d6 ("tracing: Add kprobe event command generation functions")
Suggested-by: Mukesh Ojha 
Signed-off-by: Yujie Liu 


Thanks.

Reviewed-by: Mukesh Ojha 

-Mukesh


---
  kernel/trace/trace_kprobe.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index e834f149695b..47812aa16bb5 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1020,9 +1020,9 @@ EXPORT_SYMBOL_GPL(kprobe_event_cmd_init);
  /**
   * __kprobe_event_gen_cmd_start - Generate a kprobe event command from arg 
list
   * @cmd: A pointer to the dynevent_cmd struct representing the new event
+ * @kretprobe: Is this a return probe?
   * @name: The name of the kprobe event
   * @loc: The location of the kprobe event
- * @kretprobe: Is this a return probe?
   * @...: Variable number of arg (pairs), one pair for each field
   *
   * NOTE: Users normally won't want to call this function directly, but

Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node

[Mukesh Ojha]

On 10/30/2023 8:33 PM, Doug Anderson wrote:

Hi,

On Mon, Oct 30, 2023 at 7:43 AM Luca Weiss  wrote:


On Mon Oct 30, 2023 at 3:11 PM CET, Doug Anderson wrote:

Hi,

On Mon, Oct 30, 2023 at 2:12 AM Luca Weiss  wrote:


On Mon Oct 30, 2023 at 10:04 AM CET, Mukesh Ojha wrote:



On 10/27/2023 7:50 PM, Luca Weiss wrote:

Add the node for the ADSP found on the SC7280 SoC, using standard
Qualcomm firmware.

The memory region for sc7280-chrome-common.dtsi is taken from msm-5.4
yupik.dtsi since the other areas also seem to match that file there,
though I cannot be sure there.

Signed-off-by: Luca Weiss 
---
   arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   5 +
   arch/arm64/boot/dts/qcom/sc7280.dtsi   | 138 
+
   2 files changed, 143 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
index eb55616e0892..6e5a9d4c1fda 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
@@ -29,6 +29,11 @@ adsp_mem: memory@8670 {
 no-map;
 };

+   cdsp_mem: memory@88f0 {
+   reg = <0x0 0x88f0 0x0 0x1e0>;
+   no-map;
+   };
+


Just a question, why to do it here, if chrome does not use this ?


Other memory regions in sc7280.dtsi also get referenced but not actually
defined in that file, like mpss_mem and wpss_mem. Alternatively we can
also try and solve this differently, but then we should probably also
adjust mpss and wpss to be consistent.

Apart from either declaring cdsp_mem in sc7280.dtsi or
"/delete-property/ memory-region;" for CDSP I don't really have better
ideas though.

I also imagine these ChromeOS devices will want to enable cdsp at some
point but I don't know any plans there.


Given that "remoteproc_cdsp" has status "disabled" in the dtsi, it
feels like the dtsi shouldn't be reserving memory. I guess maybe
memory regions can't be status "disabled"?


Hi Doug,

That's how it works in really any qcom dtsi though. I think in most/all
cases normally the reserved-memory is already declared in the SoC dtsi
file and also used with the memory-region property.

I wouldn't be against adjusting sc7280.dtsi to match the way it's done
in the other dtsi files though, so to have all the required labels
already defined in the dtsi so it doesn't rely on these labels being
defined in the device dts.

In other words, currently if you include sc7280.dtsi and try to build,
you first have to define the labels mpss_mem and wpss_mem (after this
patch series also cdsp_mem and adsp_mem) for it to build.

I'm quite neutral either way, let me know :)


I haven't done a ton of thinking about this, so if I'm spouting
gibberish then feel free to ignore me. :-P It just feels weird that
when all the "dtsi" files are combined and you look at what you end up
on a sc7280 Chrome board that you'll be reserving 32MB of memory for a
device that's set (in the same device tree) to be "disabled", right?
...the 32MB is completely wasted, I think. If we wanted to enable the
CDSP we'd have to modify the device tree anyway, so it seems like that
same modification would set the CDSP to "okay" and also reserve the
memory...

In that vein, it seems like maybe you could move the "cdsp_mem" to the
SoC .dsti file with a status of "disabled". . I guess we don't do that
elsewhere, but maybe we should be? As far as I can tell without
testing it (just looking at fdt_scan_reserved_mem()) this should
work...


What do you think about moving present reserve memory block from
sc7280-chrome-common to sc7280.dtsi and delete the stuff which
chrome does not need it sc7280-chrome-common ?

-Mukesh


-Doug

Re: [PATCH] eventfs: Fix kerneldoc of eventfs_remove_rec()

[Mukesh Ojha]

On 10/30/2023 9:45 PM, Steven Rostedt wrote:

From: "Steven Rostedt (Google)" 

The eventfs_remove_rec() had some missing parameters in the kerneldoc
comment above it. Also, rephrase the description a bit more to have a bit
more correct grammar.

Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use 
eventfs_inode");
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202310052216.4sgqaswo-...@intel.com/
Signed-off-by: Steven Rostedt (Google) 


Reviewed-by: Mukesh Ojha 

-Mukesh

---
  fs/tracefs/event_inode.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 5a3cc5394294..1c28e013201f 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -977,9 +977,11 @@ static void free_rcu_ei(struct rcu_head *head)
  /**
   * eventfs_remove_rec - remove eventfs dir or file from list
   * @ei: eventfs_inode to be removed.
+ * @head: the list head to place the deleted @ei and children
+ * @level: prevent recursion from going more than 3 levels deep.
   *
- * This function recursively remove eventfs_inode which
- * contains info of file or dir.
+ * This function recursively removes eventfs_inodes which
+ * contains info of files and/or directories.
   */
  static void eventfs_remove_rec(struct eventfs_inode *ei, struct list_head 
*head, int level)
  {

Re: [PATCH v2 2/3] remoteproc: qcom: pas: make region assign more generic

[Mukesh Ojha]

On 10/30/2023 3:33 PM, Neil Armstrong wrote:

The current memory region assign only supports a single
memory region.

But new platforms introduces more regions to make the
memory requirements more flexible for various use cases.
Those new platforms also shares the memory region between the
DSP and HLOS.

To handle this, make the region assign more generic in order
to support more than a single memory region and also permit
setting the regions permissions as shared.

Signed-off-by: Neil Armstrong 
---
  drivers/remoteproc/qcom_q6v5_pas.c | 102 -
  1 file changed, 66 insertions(+), 36 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
b/drivers/remoteproc/qcom_q6v5_pas.c
index 913a5d2068e8..4829fd26e17d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -33,6 +33,8 @@
  
  #define ADSP_DECRYPT_SHUTDOWN_DELAY_MS	100
  
+#define MAX_ASSIGN_COUNT 2

+
  struct adsp_data {
int crash_reason_smem;
const char *firmware_name;
@@ -51,6 +53,9 @@ struct adsp_data {
int ssctl_id;
  
  	int region_assign_idx;

+   int region_assign_count;
+   bool region_assign_shared;
+   int region_assign_vmid;
  };
  
  struct qcom_adsp {

@@ -87,15 +92,18 @@ struct qcom_adsp {
phys_addr_t dtb_mem_phys;
phys_addr_t mem_reloc;
phys_addr_t dtb_mem_reloc;
-   phys_addr_t region_assign_phys;
+   phys_addr_t region_assign_phys[MAX_ASSIGN_COUNT];
void *mem_region;
void *dtb_mem_region;
size_t mem_size;
size_t dtb_mem_size;
-   size_t region_assign_size;
+   size_t region_assign_size[MAX_ASSIGN_COUNT];
  
  	int region_assign_idx;

-   u64 region_assign_perms;
+   int region_assign_count;
+   bool region_assign_shared;
+   int region_assign_vmid;
+   u64 region_assign_perms[MAX_ASSIGN_COUNT];
  
  	struct qcom_rproc_glink glink_subdev;

struct qcom_rproc_subdev smd_subdev;
@@ -590,37 +598,52 @@ static int adsp_alloc_memory_region(struct qcom_adsp 
*adsp)
  
  static int adsp_assign_memory_region(struct qcom_adsp *adsp)

  {
-   struct reserved_mem *rmem = NULL;
-   struct qcom_scm_vmperm perm;
+   struct qcom_scm_vmperm perm[MAX_ASSIGN_COUNT];
+   unsigned int perm_size = 1;


AFAICS, not need of initialization.


struct device_node *node;
-   int ret;
+   int offset, ret;


Nit: one variable per line.

  
  	if (!adsp->region_assign_idx)


Not related to this patch..
Should not this be valid only for > 1 ?



return 0;
  
-	node = of_parse_phandle(adsp->dev->of_node, "memory-region", adsp->region_assign_idx);

-   if (node)
-   rmem = of_reserved_mem_lookup(node);
-   of_node_put(node);
-   if (!rmem) {
-   dev_err(adsp->dev, "unable to resolve shareable 
memory-region\n");
-   return -EINVAL;
-   }
+   for (offset = 0; offset < adsp->region_assign_count; ++offset) {
+   struct reserved_mem *rmem = NULL;
+
+   node = of_parse_phandle(adsp->dev->of_node, "memory-region",
+   adsp->region_assign_idx + offset);
+   if (node)
+   rmem = of_reserved_mem_lookup(node);
+   of_node_put(node);
+   if (!rmem) {
+   dev_err(adsp->dev, "unable to resolve shareable 
memory-region index %d\n",
+   offset);
+   return -EINVAL; > +  }



  
-	perm.vmid = QCOM_SCM_VMID_MSS_MSA;

-   perm.perm = QCOM_SCM_PERM_RW;
+   if (adsp->region_assign_shared)  {
+   perm[0].vmid = QCOM_SCM_VMID_HLOS;
+   perm[0].perm = QCOM_SCM_PERM_RW;
+   perm[1].vmid = adsp->region_assign_vmid;
+   perm[1].perm = QCOM_SCM_PERM_RW;
+   perm_size = 2;
+   } else {
+   perm[0].vmid = adsp->region_assign_vmid;
+   perm[0].perm = QCOM_SCM_PERM_RW;
+   perm_size = 1;
+   }
  
-	adsp->region_assign_phys = rmem->base;

-   adsp->region_assign_size = rmem->size;
-   adsp->region_assign_perms = BIT(QCOM_SCM_VMID_HLOS);
+   adsp->region_assign_phys[offset] = rmem->base;
+   adsp->region_assign_size[offset] = rmem->size;
+   adsp->region_assign_perms[offset] = BIT(QCOM_SCM_VMID_HLOS);


Do we need array for this, is this changing ?

  
-	ret = qcom_scm_assign_mem(adsp->region_assign_phys,

- adsp->region_assign_size,
- &adsp->region_assign_perms,
- &perm, 1);
-   if (ret < 0) {
-   dev_err(adsp->dev, "assign memory failed\n");
-   return ret;
+   ret = qcom_scm_assign_mem(adsp->region_assign_phys[offset],
+

Re: [PATCH 7/9] arm64: dts: qcom: sc7280: Add CDSP node

[Mukesh Ojha]

On 10/27/2023 7:50 PM, Luca Weiss wrote:

Add the node for the ADSP found on the SC7280 SoC, using standard
Qualcomm firmware.

The memory region for sc7280-chrome-common.dtsi is taken from msm-5.4
yupik.dtsi since the other areas also seem to match that file there,
though I cannot be sure there.

Signed-off-by: Luca Weiss 
---
  arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi |   5 +
  arch/arm64/boot/dts/qcom/sc7280.dtsi   | 138 +
  2 files changed, 143 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
index eb55616e0892..6e5a9d4c1fda 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-chrome-common.dtsi
@@ -29,6 +29,11 @@ adsp_mem: memory@8670 {
no-map;
};
  
+		cdsp_mem: memory@88f0 {

+   reg = <0x0 0x88f0 0x0 0x1e0>;
+   no-map;
+   };
+


Just a question, why to do it here, if chrome does not use this ?

-Mukesh


camera_mem: memory@8ad0 {
reg = <0x0 0x8ad0 0x0 0x50>;
no-map;
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index cc153f4e6979..e15646289bf7 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3815,6 +3815,144 @@ nsp_noc: interconnect@a0c {
qcom,bcm-voters = <&apps_bcm_voter>;
};
  
+		remoteproc_cdsp: remoteproc@a30 {

+   compatible = "qcom,sc7280-cdsp-pas";
+   reg = <0 0x0a30 0 0x1>;
+
+   interrupts-extended = <&intc GIC_SPI 578 
IRQ_TYPE_LEVEL_HIGH>,
+ <&cdsp_smp2p_in 0 
IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 1 
IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 2 
IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 3 
IRQ_TYPE_EDGE_RISING>,
+ <&cdsp_smp2p_in 7 
IRQ_TYPE_EDGE_RISING>;
+   interrupt-names = "wdog", "fatal", "ready", "handover",
+ "stop-ack", "shutdown-ack";
+
+   clocks = <&rpmhcc RPMH_CXO_CLK>;
+   clock-names = "xo";
+
+   power-domains = <&rpmhpd SC7280_CX>,
+   <&rpmhpd SC7280_MX>;
+   power-domain-names = "cx", "mx";
+
+   interconnects = <&nsp_noc MASTER_CDSP_PROC 0 &mc_virt 
SLAVE_EBI1 0>;
+
+   memory-region = <&cdsp_mem>;
+
+   qcom,qmp = <&aoss_qmp>;
+
+   qcom,smem-states = <&cdsp_smp2p_out 0>;
+   qcom,smem-state-names = "stop";
+
+   status = "disabled";
+
+   glink-edge {
+   interrupts-extended = <&ipcc IPCC_CLIENT_CDSP
+
IPCC_MPROC_SIGNAL_GLINK_QMP
+
IRQ_TYPE_EDGE_RISING>;
+   mboxes = <&ipcc IPCC_CLIENT_CDSP
+   IPCC_MPROC_SIGNAL_GLINK_QMP>;
+
+   label = "cdsp";
+   qcom,remote-pid = <5>;
+
+   fastrpc {
+   compatible = "qcom,fastrpc";
+   qcom,glink-channels = 
"fastrpcglink-apps-dsp";
+   label = "cdsp";
+   qcom,non-secure-domain;
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   compute-cb@1 {
+   compatible = 
"qcom,fastrpc-compute-cb";
+   reg = <1>;
+   iommus = <&apps_smmu 0x11a1 
0x0420>,
+<&apps_smmu 0x1181 
0x0420>;
+   };
+
+   compute-cb@2 {
+   compatible = 
"qcom,fastrpc-compute-cb";
+   reg = <2>;
+   iommus = <&apps_smmu 0x11a2 
0x0420>,
+<&apps_smmu 0x1182 
0x0420>;
+   };
+
+   compute-cb@3 {
+   

Re: [PATCH] tracing/kprobes: Fix the description of variable length arguments

[Mukesh Ojha]

On 10/27/2023 9:43 AM, Yujie Liu wrote:

Fix the following kernel-doc warnings:

kernel/trace/trace_kprobe.c:1029: warning: Excess function parameter 'args' 
description in '__kprobe_event_gen_cmd_start'
kernel/trace/trace_kprobe.c:1097: warning: Excess function parameter 'args' 
description in '__kprobe_event_add_fields'

Refer to the usage of variable length arguments elsewhere in the kernel
code, "@..." is the proper way to express it in the description.

Fixes: 2a588dd1d5d6 ("tracing: Add kprobe event command generation functions")
Reported-by: kernel test robot 
Closes: 
https://lore.kernel.org/oe-kbuild-all/202310190437.pai6lyjf-...@intel.com/
Signed-off-by: Yujie Liu 


Not related to this patch, but I see order of the argument as well is 
not proper in the document of the __kprobe_event_gen_cmd_start(),

if you can fix that too.

LGTM, Thanks for this.
Reviewed-by: Mukesh Ojha 

-Mukesh


---
  kernel/trace/trace_kprobe.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
index a8fef6ab0872..95c5b0668cb7 100644
--- a/kernel/trace/trace_kprobe.c
+++ b/kernel/trace/trace_kprobe.c
@@ -1007,7 +1007,7 @@ EXPORT_SYMBOL_GPL(kprobe_event_cmd_init);
   * @name: The name of the kprobe event
   * @loc: The location of the kprobe event
   * @kretprobe: Is this a return probe?
- * @args: Variable number of arg (pairs), one pair for each field
+ * @...: Variable number of arg (pairs), one pair for each field
   *
   * NOTE: Users normally won't want to call this function directly, but
   * rather use the kprobe_event_gen_cmd_start() wrapper, which automatically
@@ -1080,7 +1080,7 @@ EXPORT_SYMBOL_GPL(__kprobe_event_gen_cmd_start);
  /**
   * __kprobe_event_add_fields - Add probe fields to a kprobe command from arg 
list
   * @cmd: A pointer to the dynevent_cmd struct representing the new event
- * @args: Variable number of arg (pairs), one pair for each field
+ * @...: Variable number of arg (pairs), one pair for each field
   *
   * NOTE: Users normally won't want to call this function directly, but
   * rather use the kprobe_event_add_fields() wrapper, which

Re: [PATCH] eventfs: Fix typo in eventfs_inode union comment

[Mukesh Ojha]

On 10/24/2023 10:40 PM, Steven Rostedt wrote:

From: "Steven Rostedt (Google)" 

It's eventfs_inode not eventfs_indoe. There's no deer involved!


:-)



Fixes: 5790b1fb3d672 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Signed-off-by: Steven Rostedt (Google) 


Reviewed-by: Mukesh Ojha 

-Mukesh

---
  fs/tracefs/internal.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tracefs/internal.h b/fs/tracefs/internal.h
index 298d3ecaf621..64fde9490f52 100644
--- a/fs/tracefs/internal.h
+++ b/fs/tracefs/internal.h
@@ -37,7 +37,7 @@ struct eventfs_inode {
/*
 * Union - used for deletion
 * @del_list:   list of eventfs_inode to delete
-* @rcu:eventfs_indoe to delete in RCU
+* @rcu:eventfs_inode to delete in RCU
 * @is_freed:   node is freed if one of the above is set
 */
union {

Re: [PATCH] tracing/histograms: Simplify last_cmd_set()

[Mukesh Ojha]

On 10/21/2023 8:12 PM, Christophe JAILLET wrote:

Turn a kzalloc()+strcpy()+strncat() into an equivalent and less verbose
kasprintf().

Signed-off-by: Christophe JAILLET 
---
  kernel/trace/trace_events_hist.c | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index d06938ae0717..1abc07fba1b9 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -774,23 +774,16 @@ static void last_cmd_set(struct trace_event_file *file, 
char *str)
  {
const char *system = NULL, *name = NULL;
struct trace_event_call *call;
-   int len;
  
  	if (!str)

return;
  
-	/* sizeof() contains the nul byte */

-   len = sizeof(HIST_PREFIX) + strlen(str);
kfree(last_cmd);
-   last_cmd = kzalloc(len, GFP_KERNEL);
+
+   last_cmd = kasprintf(GFP_KERNEL, HIST_PREFIX "%s", str);
if (!last_cmd)
return;
  
-	strcpy(last_cmd, HIST_PREFIX);

-   /* Again, sizeof() contains the nul byte */
-   len -= sizeof(HIST_PREFIX);
-   strncat(last_cmd, str, len);


LGTM, careful optimization., Thanks.
Reviewed-by: Mukesh ojha 


-Mukesh


-
if (file) {
call = file->event_call;
system = call->class->system;

Re: [PATCH] tracefs/eventfs: Modify mismatched function name

[Mukesh Ojha]

On 10/19/2023 8:43 AM, Jiapeng Chong wrote:

No functional modification involved.

fs/tracefs/event_inode.c:864: warning: expecting prototype for 
eventfs_remove(). Prototype was for eventfs_remove_dir() instead.

Reported-by: Abaci Robot 
Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=6939
Signed-off-by: Jiapeng Chong 


Reviewed-by: Mukesh Ojha 

-Mukesh

---
  fs/tracefs/event_inode.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 1ccd100bc565..ba9d1cb0d24c 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -855,7 +855,7 @@ static void unhook_dentry(struct dentry **dentry, struct 
dentry **list)
}
  }
  /**
- * eventfs_remove - remove eventfs dir or file from list
+ * eventfs_remove_dir - remove eventfs dir or file from list
   * @ei: eventfs_inode to be removed.
   *
   * This function acquire the eventfs_mutex lock and call eventfs_remove_rec()

Re: [PATCH] eventfs: Fix failure path in eventfs_create_events_dir()

[Mukesh Ojha]

On 10/20/2023 6:11 AM, Steven Rostedt wrote:

From: "Steven Rostedt (Google)" 

The failure path of allocating ei goes to a path that dereferences ei.
Add another label that skips over the ei dereferences to do the rest of
the clean up.

Link: https://lore.kernel.org/all/70e7bace-561c-95f-1117-706c2c22...@inria.fr/

Fixes: 5790b1fb3d67 ("eventfs: Remove eventfs_file and just use eventfs_inode")
Reported-by: Julia Lawall 
Signed-off-by: Steven Rostedt (Google) 


Reviewed-by: Mukesh Ojha 

-Mukesh


---
  fs/tracefs/event_inode.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 9f19b6608954..1885f1f1f339 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -735,7 +735,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char 
*name, struct dentry
  
  	ei = kzalloc(sizeof(*ei), GFP_KERNEL);

if (!ei)
-   goto fail;
+   goto fail_ei;
  
  	inode = tracefs_get_inode(dentry->d_sb);

if (unlikely(!inode))
@@ -781,6 +781,7 @@ struct eventfs_inode *eventfs_create_events_dir(const char 
*name, struct dentry
   fail:
kfree(ei->d_children);
kfree(ei);
+ fail_ei:
tracefs_failed_creating(dentry);
return ERR_PTR(-ENOMEM);
  }

Re: [PATCH v3] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

Hi Kees/All,

Could you please review again?  I have addressed your comment made on 
last patch version.


Thanks,
Mukesh

On 3/23/2021 12:12 AM, Mukesh Ojha wrote:

There could be a scenario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v3:
  - Minor code and documentation update done as per comment given by Kees.

Changes in v2:
  - if-else converted to switch case
  - updated MODULE_PARM_DESC with new memory type.
  - default setting is still intact.

  Documentation/admin-guide/ramoops.rst  |  4 +++-
  .../devicetree/bindings/reserved-memory/ramoops.txt| 10 --
  fs/pstore/ram.c|  7 ++-
  fs/pstore/ram_core.c   | 18 --
  4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
  
  Sergiu Iordache 
  
-Updated: 17 November 2011

+Updated: 10 Feb 2021
  
  Introduction

  
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
  depends on atomic operations. At least on ARM, pgprot_noncached causes the
  memory to be mapped strongly ordered, and atomic operations on strongly 
ordered
  memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
  
  The memory area is divided into ``record_size`` chunks (also rounded down to

  power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt 
b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index b7886fe..6f1cb20 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -42,8 +42,14 @@ Optional properties:
  - pmsg-size: size in bytes of log buffer reserved for userspace messages
(defaults to 0: disabled)
  
-- unbuffered: if present, use unbuffered mappings to map the reserved region

-  (defaults to buffered mappings)
+- mem-type: if present, sets the type of mapping is to be used to map the
+  reserved region. mem-type: 0 = write-combined (default), 1 = unbuffered,
+  2 = cached.
+
+- unbuffered: deprecated, use mem_type instead. if present, and mem_type is
+  not specified, it is equivalent to mem_type = 1 and uses unbuffered mappings
+  to map the reserved region (defaults to buffered mappings mem_type = 0). If
+  both are specified -- "mem_type" overrides "unbuffered".
  
  - max-reason: if present, sets maximum type of kmsg dump reasons to store

(defaults to 2: log Oopses and Panics). This can be set to INT_MAX to
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..fefe3d3 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
  static unsigned int mem_type;
  module_param(mem_type, uint, 0400);
  MODULE_PARM_DESC(mem_type,
-   "set to 1 to try to use unbuffered memory (default 0)");
+   "memory type: 0=write-combined (default), 1=unbuffered, 
2=cached");
  
  static int ramoops_max_reason = -1;

  module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -648,6 +648,10 @@ static int ramoops_parse_dt(struct platform_device *pdev,
  
  	pdata->mem_size = resource_size(res);

pdata->mem_address = res->start;
+   /*
+* Setting "unbuffered" is deprecated and will be ignored if
+* "mem_type" is also specified.
+*/
pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
/*
 * Setting "no-dump-oops" is deprecated and will be ignored if
@@ -666,6 +670,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
  
+	parse_u32("mem-type", pdata->record_size, pdata->mem_type);

parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index ff

[PATCH v3] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

There could be a scenario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v3:
 - Minor code and documentation update done as per comment given by Kees.

Changes in v2:
 - if-else converted to switch case
 - updated MODULE_PARM_DESC with new memory type.
 - default setting is still intact.

 Documentation/admin-guide/ramoops.rst  |  4 +++-
 .../devicetree/bindings/reserved-memory/ramoops.txt| 10 --
 fs/pstore/ram.c|  7 ++-
 fs/pstore/ram_core.c   | 18 --
 4 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 17 November 2011
+Updated: 10 Feb 2021
 
 Introduction
 
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
 depends on atomic operations. At least on ARM, pgprot_noncached causes the
 memory to be mapped strongly ordered, and atomic operations on strongly ordered
 memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
 
 The memory area is divided into ``record_size`` chunks (also rounded down to
 power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt 
b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
index b7886fe..6f1cb20 100644
--- a/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/ramoops.txt
@@ -42,8 +42,14 @@ Optional properties:
 - pmsg-size: size in bytes of log buffer reserved for userspace messages
   (defaults to 0: disabled)
 
-- unbuffered: if present, use unbuffered mappings to map the reserved region
-  (defaults to buffered mappings)
+- mem-type: if present, sets the type of mapping is to be used to map the
+  reserved region. mem-type: 0 = write-combined (default), 1 = unbuffered,
+  2 = cached.
+
+- unbuffered: deprecated, use mem_type instead. if present, and mem_type is
+  not specified, it is equivalent to mem_type = 1 and uses unbuffered mappings
+  to map the reserved region (defaults to buffered mappings mem_type = 0). If
+  both are specified -- "mem_type" overrides "unbuffered".
 
 - max-reason: if present, sets maximum type of kmsg dump reasons to store
   (defaults to 2: log Oopses and Panics). This can be set to INT_MAX to
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..fefe3d3 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
 static unsigned int mem_type;
 module_param(mem_type, uint, 0400);
 MODULE_PARM_DESC(mem_type,
-   "set to 1 to try to use unbuffered memory (default 0)");
+   "memory type: 0=write-combined (default), 1=unbuffered, 
2=cached");
 
 static int ramoops_max_reason = -1;
 module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -648,6 +648,10 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 
pdata->mem_size = resource_size(res);
pdata->mem_address = res->start;
+   /*
+* Setting "unbuffered" is deprecated and will be ignored if
+* "mem_type" is also specified.
+*/
pdata->mem_type = of_property_read_bool(of_node, "unbuffered");
/*
 * Setting "no-dump-oops" is deprecated and will be ignored if
@@ -666,6 +670,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
 
+   parse_u32("mem-type", pdata->record_size, pdata->mem_type);
parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index fff363b..fe53050 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz)

Re: [RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

Hi All,

can you please review this ?

Thanks,

Mukesh

On 3/2/2021 1:59 PM, Mukesh Ojha wrote:

Hi Kees,

i have updated the patch based on your last comments.
please review.

Thanks,
Mukesh

On 2/25/2021 9:30 PM, Mukesh Ojha wrote:

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
  - if-else converted to switch case
  - updated MODULE_PARM_DESC with new memory type.
  - default setting is still intact.

  Documentation/admin-guide/ramoops.rst |  4 +++-
  fs/pstore/ram.c   |  3 ++-
  fs/pstore/ram_core.c  | 18 --
  3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst

index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
    Sergiu Iordache 
  -Updated: 17 November 2011
+Updated: 10 Feb 2021
    Introduction
  
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting 
``mem_type=1`` attempts to use
  depends on atomic operations. At least on ARM, pgprot_noncached 
causes the
  memory to be mapped strongly ordered, and atomic operations on 
strongly ordered
  memory are implementation defined, and won't work on many ARMs such 
as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal 
memory,

+which enables full cache on it. This can improve the performance.
    The memory area is divided into ``record_size`` chunks (also 
rounded down to

  power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..af4ca6a4 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
  static unsigned int mem_type;
  module_param(mem_type, uint, 0400);
  MODULE_PARM_DESC(mem_type,
-    "set to 1 to try to use unbuffered memory (default 0)");
+    "set to 1 to use unbuffered memory, 2 for cached memory 
(default 0)");

    static int ramoops_max_reason = -1;
  module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct 
platform_device *pdev,

  field = value;    \
  }
  +    parse_u32("mem-type", pdata->record_size, pdata->mem_type);
  parse_u32("record-size", pdata->record_size, 0);
  parse_u32("console-size", pdata->console_size, 0);
  parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..0da012f 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct 
persistent_ram_zone *prz)

  persistent_ram_update_header_ecc(prz);
  }
  +#define MEM_TYPE_WCOMBINE    0
+#define MEM_TYPE_NONCACHED    1
+#define MEM_TYPE_NORMAL    2
+
  static void *persistent_ram_vmap(phys_addr_t start, size_t size,
  unsigned int memtype)
  {
@@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t 
start, size_t size,

  page_start = start - offset_in_page(start);
  page_count = DIV_ROUND_UP(size + offset_in_page(start), 
PAGE_SIZE);

  -    if (memtype)
+    switch (memtype) {
+    case MEM_TYPE_NORMAL:
+    prot = PAGE_KERNEL;
+    break;
+    case MEM_TYPE_NONCACHED:
  prot = pgprot_noncached(PAGE_KERNEL);
-    else
+    break;
+    case MEM_TYPE_WCOMBINE:
  prot = pgprot_writecombine(PAGE_KERNEL);
+    break;
+    default:
+    pr_err("invalid memory type\n");
+    return NULL;
+    }
    pages = kmalloc_array(page_count, sizeof(struct page *), 
GFP_KERNEL);

  if (!pages) {

Re: [RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

Hi Kees,

i have updated the patch based on your last comments.
please review.

Thanks,
Mukesh

On 2/25/2021 9:30 PM, Mukesh Ojha wrote:

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
  - if-else converted to switch case
  - updated MODULE_PARM_DESC with new memory type.
  - default setting is still intact.

  Documentation/admin-guide/ramoops.rst |  4 +++-
  fs/pstore/ram.c   |  3 ++-
  fs/pstore/ram_core.c  | 18 --
  3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
  
  Sergiu Iordache 
  
-Updated: 17 November 2011

+Updated: 10 Feb 2021
  
  Introduction

  
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
  depends on atomic operations. At least on ARM, pgprot_noncached causes the
  memory to be mapped strongly ordered, and atomic operations on strongly 
ordered
  memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
  
  The memory area is divided into ``record_size`` chunks (also rounded down to

  power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..af4ca6a4 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
  static unsigned int mem_type;
  module_param(mem_type, uint, 0400);
  MODULE_PARM_DESC(mem_type,
-   "set to 1 to try to use unbuffered memory (default 0)");
+   "set to 1 to use unbuffered memory, 2 for cached memory (default 
0)");
  
  static int ramoops_max_reason = -1;

  module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
  
+	parse_u32("mem-type", pdata->record_size, pdata->mem_type);

parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..0da012f 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz);
  }
  
+#define MEM_TYPE_WCOMBINE	0

+#define MEM_TYPE_NONCACHED 1
+#define MEM_TYPE_NORMAL2
+
  static void *persistent_ram_vmap(phys_addr_t start, size_t size,
unsigned int memtype)
  {
@@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, 
size_t size,
page_start = start - offset_in_page(start);
page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
  
-	if (memtype)

+   switch (memtype) {
+   case MEM_TYPE_NORMAL:
+   prot = PAGE_KERNEL;
+   break;
+   case MEM_TYPE_NONCACHED:
prot = pgprot_noncached(PAGE_KERNEL);
-   else
+   break;
+   case MEM_TYPE_WCOMBINE:
prot = pgprot_writecombine(PAGE_KERNEL);
+   break;
+   default:
+   pr_err("invalid memory type\n");
+   return NULL;
+   }
  
  	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);

if (!pages) {

[RESEND PATCH v2 2/2] pstore: Add buffer start check during init

[Mukesh Ojha]

From: Huang Yiwei 

In a scenario of panic, when we use DRAM to store log instead
of persistant storage and during warm reset when we copy these
data outside of ram. Missing check on prz->start(write position)
can cause crash because it can be any value and can point outside
the mapped region. So add the start check to avoid.

Signed-off-by: Huang Yiwei 
Signed-off-by: Mukesh Ojha 
---
change in v2:
 - this is on top of first patchset.

 fs/pstore/ram_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0da012f..a15748a 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -514,7 +514,7 @@ static int persistent_ram_post_init(struct 
persistent_ram_zone *prz, u32 sig,
sig ^= PERSISTENT_RAM_SIG;
 
if (prz->buffer->sig == sig) {
-   if (buffer_size(prz) == 0) {
+   if (buffer_size(prz) == 0 && buffer_start(prz) == 0) {
pr_debug("found existing empty buffer\n");
return 0;
}
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[RESEND PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
 - if-else converted to switch case
 - updated MODULE_PARM_DESC with new memory type.
 - default setting is still intact.

 Documentation/admin-guide/ramoops.rst |  4 +++-
 fs/pstore/ram.c   |  3 ++-
 fs/pstore/ram_core.c  | 18 --
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 17 November 2011
+Updated: 10 Feb 2021
 
 Introduction
 
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
 depends on atomic operations. At least on ARM, pgprot_noncached causes the
 memory to be mapped strongly ordered, and atomic operations on strongly ordered
 memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
 
 The memory area is divided into ``record_size`` chunks (also rounded down to
 power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..af4ca6a4 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
 static unsigned int mem_type;
 module_param(mem_type, uint, 0400);
 MODULE_PARM_DESC(mem_type,
-   "set to 1 to try to use unbuffered memory (default 0)");
+   "set to 1 to use unbuffered memory, 2 for cached memory 
(default 0)");
 
 static int ramoops_max_reason = -1;
 module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
 
+   parse_u32("mem-type", pdata->record_size, pdata->mem_type);
parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..0da012f 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz);
 }
 
+#define MEM_TYPE_WCOMBINE  0
+#define MEM_TYPE_NONCACHED 1
+#define MEM_TYPE_NORMAL2
+
 static void *persistent_ram_vmap(phys_addr_t start, size_t size,
unsigned int memtype)
 {
@@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, 
size_t size,
page_start = start - offset_in_page(start);
page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-   if (memtype)
+   switch (memtype) {
+   case MEM_TYPE_NORMAL:
+   prot = PAGE_KERNEL;
+   break;
+   case MEM_TYPE_NONCACHED:
prot = pgprot_noncached(PAGE_KERNEL);
-   else
+   break;
+   case MEM_TYPE_WCOMBINE:
prot = pgprot_writecombine(PAGE_KERNEL);
+   break;
+   default:
+   pr_err("invalid memory type\n");
+   return NULL;
+   }
 
pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
if (!pages) {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH v2 1/2] pstore: Add mem_type property DT parsing support

[Mukesh Ojha]

From: Mukesh Ojha 

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
 - if-else converted to switch case
 - updated MODULE_PARM_DESC with new memory type.
 - default setting is still intact.

 Documentation/admin-guide/ramoops.rst |  4 +++-
 fs/pstore/ram.c   |  3 ++-
 fs/pstore/ram_core.c  | 18 --
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 17 November 2011
+Updated: 10 Feb 2021
 
 Introduction
 
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
 depends on atomic operations. At least on ARM, pgprot_noncached causes the
 memory to be mapped strongly ordered, and atomic operations on strongly ordered
 memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
 
 The memory area is divided into ``record_size`` chunks (also rounded down to
 power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..af4ca6a4 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -56,7 +56,7 @@ MODULE_PARM_DESC(mem_size,
 static unsigned int mem_type;
 module_param(mem_type, uint, 0400);
 MODULE_PARM_DESC(mem_type,
-   "set to 1 to try to use unbuffered memory (default 0)");
+   "set to 1 to use unbuffered memory, 2 for cached memory 
(default 0)");
 
 static int ramoops_max_reason = -1;
 module_param_named(max_reason, ramoops_max_reason, int, 0400);
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
 
+   parse_u32("mem-type", pdata->record_size, pdata->mem_type);
parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..0da012f 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz);
 }
 
+#define MEM_TYPE_WCOMBINE  0
+#define MEM_TYPE_NONCACHED 1
+#define MEM_TYPE_NORMAL2
+
 static void *persistent_ram_vmap(phys_addr_t start, size_t size,
unsigned int memtype)
 {
@@ -409,10 +413,20 @@ static void *persistent_ram_vmap(phys_addr_t start, 
size_t size,
page_start = start - offset_in_page(start);
page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-   if (memtype)
+   switch (memtype) {
+   case MEM_TYPE_NORMAL:
+   prot = PAGE_KERNEL;
+   break;
+   case MEM_TYPE_NONCACHED:
prot = pgprot_noncached(PAGE_KERNEL);
-   else
+   break;
+   case MEM_TYPE_WCOMBINE:
prot = pgprot_writecombine(PAGE_KERNEL);
+   break;
+   default:
+   pr_err("invalid memory type\n");
+   return NULL;
+   }
 
pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
if (!pages) {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 2/2] pstore: Add buffer start check during init

[Mukesh Ojha]

From: Huang Yiwei 

In a scenario of panic, when we use DRAM to store log instead
of persistant storage and during warm reset when we copy these
data outside of ram. Missing check on prz->start(write position)
can cause crash because it can be any value and can point outside
the mapped region. So add the start check to avoid.

Signed-off-by: Huang Yiwei 
Signed-off-by: Mukesh Ojha 
---
change in v2:
 - this is on top of first patchset.

 fs/pstore/ram_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 0da012f..a15748a 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -514,7 +514,7 @@ static int persistent_ram_post_init(struct 
persistent_ram_zone *prz, u32 sig,
sig ^= PERSISTENT_RAM_SIG;
 
if (prz->buffer->sig == sig) {
-   if (buffer_size(prz) == 0) {
+   if (buffer_size(prz) == 0 && buffer_start(prz) == 0) {
pr_debug("found existing empty buffer\n");
return 0;
}
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH] pstore/ram : Add support for cached pages

[Mukesh Ojha]

On 2/11/2021 1:47 AM, Kees Cook wrote:

On Wed, Feb 10, 2021 at 08:22:21PM +0530, Mukesh Ojha wrote:

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

Cool; yeah. I like this idea.


Thanks.




This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

What's the safest default setting?


We could keep it default, like it already exist to have unbuffered or 
not to have it in Device tree.


and could  use mem-type to cover normal memory(DDR)  as cached one and 
it will also cover buffered and unbufferd mapping


by mentioning it in device tree as numeric value.




Signed-off-by: Huang Yiwei 
Signed-off-by: Mukesh Ojha 
---
  Documentation/admin-guide/ramoops.rst |  4 +++-
  fs/pstore/ram.c   |  1 +
  fs/pstore/ram_core.c  | 10 --
  3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
  
  Sergiu Iordache 
  
-Updated: 17 November 2011

+Updated: 10 Feb 2021
  
  Introduction

  
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
  depends on atomic operations. At least on ARM, pgprot_noncached causes the
  memory to be mapped strongly ordered, and atomic operations on strongly 
ordered
  memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
  
  The memory area is divided into ``record_size`` chunks (also rounded down to

  power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..b262c57 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
  
+	parse_u32("mem-type", pdata->record_size, pdata->mem_type);

This was handled by "unbuffered" above. Can you find a way to resolve
potential conflicts between these?
Currently there is no conflict code-wise, even after this change, as it 
overrides if mem-type is there


otherwise everything is intact.

However, if we want to use only  mem-type property then i can remove 
unbuffered property.


not having unbuffered property i.e default=> mem-type = 0
having unbuffered property => mem-type = 1
mem-type = 2 for cached normal memory




parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..83cd612 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz);
  }
  
+#define MEM_TYPE_WCOMBINE	0

+#define MEM_TYPE_NONCACHED 1
+#define MEM_TYPE_NORMAL2

It might be nice for this to have a human-readable name too, but let's
start with numeric. :)

Please update the mem_type MODULE_PARM_DESC to detail the new values too.


Ok, will do in next patch.




+
  static void *persistent_ram_vmap(phys_addr_t start, size_t size,
unsigned int memtype)
  {
@@ -409,9 +413,11 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t 
size,
page_start = start - offset_in_page(start);
page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
  
-	if (memtype)

+   if (memtype == MEM_TYPE_NORMAL)
+   prot = PAGE_KERNEL;
+   else if (memtype == MEM_TYPE_NONCACHED)
prot = pgprot_noncached(PAGE_KERNEL);
-   else
+   else if (memtype == MEM_TYPE_WCOMBINE)
prot = pgprot_writecombine(PAGE_KERNEL);

Let's make this a switch statement.


Ok, will do in next patch.


Thanks

Mukesh



  
  	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);

--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: Pstore : Query on using ramoops driver for DDR

[Mukesh Ojha]

On 2/10/2021 2:32 AM, Luck, Tony wrote:

Can we use existing backend pstore ram driver (fs/pstore/ram.c) for DDR
instead of SRAM ?

The expectation for pstore is that the system will go through a reset when it
crashes. Most systems do not preserve DDR contents across reset.

to support DDR, If we have a mechanism to copy stored data from DDR to
external device after the crash.

Was the current driver written only to support persistant RAM like SRAM
or it can accept further change

Linux is in a constant state of change :-)

See above about DDR contents.   But if you have a platform that does preserve
DDR contents until your "mechanism" can copy the pstore buffer, then post
a patch.


Thanks for the reply

I have posted the patch.

https://lore.kernel.org/patchwork/patch/1378949/

-Mukesh







-Tony

[PATCH] pstore/ram : Add support for cached pages

[Mukesh Ojha]

There could be a sceanario where we define some region
in normal memory and use them store to logs which is later
retrieved by bootloader during warm reset.

In this scenario, we wanted to treat this memory as normal
cacheable memory instead of default behaviour which
is an overhead. Making it cacheable could improve
performance.

This commit gives control to change mem_type from Device
tree, and also documents the value for normal memory.

Signed-off-by: Huang Yiwei 
Signed-off-by: Mukesh Ojha 
---
 Documentation/admin-guide/ramoops.rst |  4 +++-
 fs/pstore/ram.c   |  1 +
 fs/pstore/ram_core.c  | 10 --
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/ramoops.rst 
b/Documentation/admin-guide/ramoops.rst
index b0a1ae7..8f107d8 100644
--- a/Documentation/admin-guide/ramoops.rst
+++ b/Documentation/admin-guide/ramoops.rst
@@ -3,7 +3,7 @@ Ramoops oops/panic logger
 
 Sergiu Iordache 
 
-Updated: 17 November 2011
+Updated: 10 Feb 2021
 
 Introduction
 
@@ -30,6 +30,8 @@ mapping to pgprot_writecombine. Setting ``mem_type=1`` 
attempts to use
 depends on atomic operations. At least on ARM, pgprot_noncached causes the
 memory to be mapped strongly ordered, and atomic operations on strongly ordered
 memory are implementation defined, and won't work on many ARMs such as omaps.
+Setting ``mem_type=2`` attempts to treat the memory region as normal memory,
+which enables full cache on it. This can improve the performance.
 
 The memory area is divided into ``record_size`` chunks (also rounded down to
 power of two) and each kmesg dump writes a ``record_size`` chunk of
diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index ca6d8a8..b262c57 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -666,6 +666,7 @@ static int ramoops_parse_dt(struct platform_device *pdev,
field = value;  \
}
 
+   parse_u32("mem-type", pdata->record_size, pdata->mem_type);
parse_u32("record-size", pdata->record_size, 0);
parse_u32("console-size", pdata->console_size, 0);
parse_u32("ftrace-size", pdata->ftrace_size, 0);
diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index aa8e0b6..83cd612 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -396,6 +396,10 @@ void persistent_ram_zap(struct persistent_ram_zone *prz)
persistent_ram_update_header_ecc(prz);
 }
 
+#define MEM_TYPE_WCOMBINE  0
+#define MEM_TYPE_NONCACHED 1
+#define MEM_TYPE_NORMAL2
+
 static void *persistent_ram_vmap(phys_addr_t start, size_t size,
unsigned int memtype)
 {
@@ -409,9 +413,11 @@ static void *persistent_ram_vmap(phys_addr_t start, size_t 
size,
page_start = start - offset_in_page(start);
page_count = DIV_ROUND_UP(size + offset_in_page(start), PAGE_SIZE);
 
-   if (memtype)
+   if (memtype == MEM_TYPE_NORMAL)
+   prot = PAGE_KERNEL;
+   else if (memtype == MEM_TYPE_NONCACHED)
prot = pgprot_noncached(PAGE_KERNEL);
-   else
+   else if (memtype == MEM_TYPE_WCOMBINE)
prot = pgprot_writecombine(PAGE_KERNEL);
 
pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Pstore : Query on using ramoops driver for DDR

[Mukesh Ojha]

Hi All,

Can we use existing backend pstore ram driver (fs/pstore/ram.c) for DDR 
instead of SRAM ?
Was the current driver written only to support persistant RAM like SRAM 
or it can accept further change
to support DDR, If we have a mechanism to copy stored data from DDR to 
external device after the crash.


Thanks,
Mukesh

[PATCH] thermal: Fix NULL pointer dereference issue

[Mukesh Ojha]

Cooling stats variable inside thermal_cooling_device_stats_update()
can get NULL. We should add a NULL check on stat inside for sanity.

Signed-off-by: Mukesh Ojha 
---
 drivers/thermal/thermal_sysfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a6f371f..f52708f 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -754,6 +754,9 @@ void thermal_cooling_device_stats_update(struct 
thermal_cooling_device *cdev,
 {
struct cooling_dev_stats *stats = cdev->stats;
 
+   if (!stats)
+   return;
+
spin_lock(&stats->lock);
 
if (stats->state == new_state)
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 3/3] cpuidle: Trivial fixes

[Mukesh Ojha]

iterrupts => interrupts
stratight => straight

Minor comment correction.

Signed-off-by: Mukesh Ojha 
---
 kernel/sched/idle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 8dad5aa..2df8ae1 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -158,8 +158,8 @@ static void cpuidle_idle_call(void)
/*
 * Suspend-to-idle ("s2idle") is a system state in which all user space
 * has been frozen, all I/O devices have been suspended and the only
-* activity happens here and in iterrupts (if any).  In that case bypass
-* the cpuidle governor and go stratight for the deepest idle state
+* activity happens here is in interrupts (if any).  In that case bypass
+* the cpuidle governor and go straight for the deepest idle state
 * available.  Possibly also suspend the local tick and the entire
 * timekeeping to prevent timer interrupts from kicking us out of idle
 * until a proper wakeup interrupt happens.
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 2/3] time: Fix spelling mistake in comment

[Mukesh Ojha]

witin => within

Signed-off-by: Mukesh Ojha 
---
 kernel/time/time.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index 5c54ca6..d31661c4 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -179,7 +179,7 @@ int do_sys_settimeofday64(const struct timespec64 *tv, 
const struct timezone *tz
return error;
 
if (tz) {
-   /* Verify we're witin the +-15 hrs range */
+   /* Verify we're within the +-15 hrs range */
if (tz->tz_minuteswest > 15*60 || tz->tz_minuteswest < -15*60)
return -EINVAL;
 
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 1/3] time/jiffies: Fixes some typo

[Mukesh Ojha]

accuratly => accurately

while at it change `clock source` to clocksource to make
it align with its usage at other places.

Signed-off-by: Mukesh Ojha 
---
 kernel/time/jiffies.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
index d23b434..e7f08f2 100644
--- a/kernel/time/jiffies.c
+++ b/kernel/time/jiffies.c
@@ -39,12 +39,12 @@ static u64 jiffies_read(struct clocksource *cs)
 
 /*
  * The Jiffies based clocksource is the lowest common
- * denominator clock source which should function on
+ * denominator clocksource which should function on
  * all systems. It has the same coarse resolution as
  * the timer interrupt frequency HZ and it suffers
  * inaccuracies caused by missed or lost timer
  * interrupts and the inability for the timer
- * interrupt hardware to accuratly tick at the
+ * interrupt hardware to accurately tick at the
  * requested HZ value. It is also not recommended
  * for "tick-less" systems.
  */
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH] seqlock: Minor comment correction

[Mukesh Ojha]

write_seqcountbeqin => write_seqcount_begin

Signed-off-by: Mukesh Ojha 
---
 include/linux/seqlock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index bcf4cf2..370ef8f 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -42,7 +42,7 @@
 /*
  * Version using sequence counter only.
  * This can be used when code has its own mutex protecting the
- * updating starting before the write_seqcountbeqin() and ending
+ * updating starting before the write_seqcount_begin() and ending
  * after the write_seqcount_end().
  */
 typedef struct seqcount {
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH V5 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

On 8/12/2019 4:12 PM, Jiri Olsa wrote:

On Fri, Aug 02, 2019 at 12:16:53AM +0530, Mukesh Ojha wrote:

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V5:
=
- Rebased it.

note that we might need to change how we store cpu topology,
now that it can change during the sampling.. like below it's
the comparison of header data with and without cpu 1

I think some of the report code checks on topology or caches
and it might get confused

perhaps we could watch cpu topology in record and update the
data as we see it changing.. future TODO list ;-)


Hi Jiri,

Can we do something like below  to address issue  related to header 
update while perf record with offline cpus.


--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1432,7 +1432,7 @@ static int __cmd_record(struct record *rec, int 
argc, const char **argv)

    opts->no_bpf_event = true;
    }

-   err = record__synthesize(rec, false);
+   err = record__synthesize(rec, true);
    if (err < 0)
    goto out_child;

@@ -1652,7 +1652,7 @@ static int __cmd_record(struct record *rec, int 
argc, const char **argv)

    } else
    status = err;

-   record__synthesize(rec, true);
+   record__synthesize(rec, false);
    /* this will be recalculated during process_buildids() */
    rec->samples = 0;

Thanks.
Mukesh




perf stat is probably fine

jirka


---
-# nrcpus online : 39
+# nrcpus online : 40
  # nrcpus avail : 40
  # cpudesc : Intel(R) Xeon(R) Silver 4114 CPU @ 2.20GHz
  # cpuid : GenuineIntel,6,85,4
...
  # sibling sockets : 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# sibling sockets : 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# sibling sockets : 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # sibling dies: 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# sibling dies: 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# sibling dies: 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # sibling threads : 0,20
+# sibling threads : 1,21
  # sibling threads : 2,22
  # sibling threads : 3,23
  # sibling threads : 4,24
@@ -38,9 +39,8 @@
  # sibling threads : 17,37
  # sibling threads : 18,38
  # sibling threads : 19,39
-# sibling threads : 21
  # CPU 0: Core ID 0, Die ID 0, Socket ID 0
-# CPU 1: Core ID -1, Die ID -1, Socket ID -1
+# CPU 1: Core ID 0, Die ID 0, Socket ID 1
  # CPU 2: Core ID 4, Die ID 0, Socket ID 0
  # CPU 3: Core ID 4, Die ID 0, Socket ID 1
  # CPU 4: Core ID 1, Die ID 0, Socket ID 0
@@ -79,14 +79,16 @@
  # CPU 37: Core ID 9, Die ID 0, Socket ID 1
  # CPU 38: Core ID 10, Die ID 0, Socket ID 0
  # CPU 39: Core ID 10, Die ID 0, Socket ID 1
-# node0 meminfo  : total = 47391616 kB, free = 46536844 kB
+# node0 meminfo  : total = 47391616 kB, free = 46548348 kB
  # node0 cpu list : 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# node1 meminfo  : total = 49539612 kB, free = 48908820 kB
-# node1 cpu list : 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# node1 meminfo  : total = 49539612 kB, free = 48897176 kB
+# node1 cpu list : 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # pmu mappings: intel_pt = 8, uncore_cha_1 = 25, uncore_irp_3 = 49, software 
= 1, uncore_imc_5 = 18, uncore_m3upi_0 = 21, uncore_iio_free_running_5 = 45, 
uncore_irp_1 = 47, uncore_m2m_1 = 12, uncore_imc_3 = 16, uncore_cha_8 = 32, 
uncore_iio_free_running_3 = 43, uncore_imc_1 = 14, uncore_upi_1 = 20, power = 
10, uncore_cha_6 = 30, uncore_iio_free_running_1 = 41, uncore_iio_4 = 38, 
uprobe = 7, cpu = 4, uncore_cha_4 = 28, uncore_iio_2 = 36, cstate_core = 53, 
breakpoint = 5, uncore_cha_2 = 26, uncore_irp_4 = 50, uncore_m3upi_1 = 22, 
uncore_iio_0 = 34, tracepoint = 2, uncore_cha_0 = 24, uncore_irp_2 = 48, 
cstate_pkg = 54, uncore_imc_4 = 17, uncore_cha_9 = 33, 
uncore_iio_free_running_4 = 44, uncore_ubox = 23, uncore_irp_0 = 46, 
uncore_m2m_0 =

Re: [PATCH] perf test: fix spelling mistake "allos" -> "allocate"

[Mukesh Ojha]

On 9/11/2019 8:51 PM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake in a TEST_ASSERT_VAL message. Fix it.

Signed-off-by: Colin Ian King 


Reviewed-by: Mukesh Ojha 

Thanks,
Mukesh


---
  tools/perf/tests/event_update.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
index cac4290e233a..7f0868a31a7f 100644
--- a/tools/perf/tests/event_update.c
+++ b/tools/perf/tests/event_update.c
@@ -92,7 +92,7 @@ int test__event_update(struct test *test __maybe_unused, int 
subtest __maybe_unu
  
  	evsel = perf_evlist__first(evlist);
  
-	TEST_ASSERT_VAL("failed to allos ids",

+   TEST_ASSERT_VAL("failed to allocate ids",
!perf_evsel__alloc_id(evsel, 1, 1));
  
  	perf_evlist__id_add(evlist, evsel, 0, 0, 123);

Re: [PATCH V5 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

On 8/12/2019 4:12 PM, Jiri Olsa wrote:

On Fri, Aug 02, 2019 at 12:16:53AM +0530, Mukesh Ojha wrote:

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V5:
=
- Rebased it.

note that we might need to change how we store cpu topology,
now that it can change during the sampling.. like below it's
the comparison of header data with and without cpu 1

I think some of the report code checks on topology or caches
and it might get confused

perhaps we could watch cpu topology in record and update the
data as we see it changing.. future TODO list ;-)



Thanks Jiri for pointing it out.
I will take a look at perf record header update code.

It would be good, if we get more reviews on this patch as it is core 
event framework change.


-Mukesh



perf stat is probably fine

jirka


---
-# nrcpus online : 39
+# nrcpus online : 40
  # nrcpus avail : 40
  # cpudesc : Intel(R) Xeon(R) Silver 4114 CPU @ 2.20GHz
  # cpuid : GenuineIntel,6,85,4
...
  # sibling sockets : 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# sibling sockets : 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# sibling sockets : 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # sibling dies: 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# sibling dies: 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# sibling dies: 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # sibling threads : 0,20
+# sibling threads : 1,21
  # sibling threads : 2,22
  # sibling threads : 3,23
  # sibling threads : 4,24
@@ -38,9 +39,8 @@
  # sibling threads : 17,37
  # sibling threads : 18,38
  # sibling threads : 19,39
-# sibling threads : 21
  # CPU 0: Core ID 0, Die ID 0, Socket ID 0
-# CPU 1: Core ID -1, Die ID -1, Socket ID -1
+# CPU 1: Core ID 0, Die ID 0, Socket ID 1
  # CPU 2: Core ID 4, Die ID 0, Socket ID 0
  # CPU 3: Core ID 4, Die ID 0, Socket ID 1
  # CPU 4: Core ID 1, Die ID 0, Socket ID 0
@@ -79,14 +79,16 @@
  # CPU 37: Core ID 9, Die ID 0, Socket ID 1
  # CPU 38: Core ID 10, Die ID 0, Socket ID 0
  # CPU 39: Core ID 10, Die ID 0, Socket ID 1
-# node0 meminfo  : total = 47391616 kB, free = 46536844 kB
+# node0 meminfo  : total = 47391616 kB, free = 46548348 kB
  # node0 cpu list : 0,2,4,6,8,10,12,14,16,18,20,22,24,26,28,30,32,34,36,38
-# node1 meminfo  : total = 49539612 kB, free = 48908820 kB
-# node1 cpu list : 3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
+# node1 meminfo  : total = 49539612 kB, free = 48897176 kB
+# node1 cpu list : 1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31,33,35,37,39
  # pmu mappings: intel_pt = 8, uncore_cha_1 = 25, uncore_irp_3 = 49, software 
= 1, uncore_imc_5 = 18, uncore_m3upi_0 = 21, uncore_iio_free_running_5 = 45, 
uncore_irp_1 = 47, uncore_m2m_1 = 12, uncore_imc_3 = 16, uncore_cha_8 = 32, 
uncore_iio_free_running_3 = 43, uncore_imc_1 = 14, uncore_upi_1 = 20, power = 
10, uncore_cha_6 = 30, uncore_iio_free_running_1 = 41, uncore_iio_4 = 38, 
uprobe = 7, cpu = 4, uncore_cha_4 = 28, uncore_iio_2 = 36, cstate_core = 53, 
breakpoint = 5, uncore_cha_2 = 26, uncore_irp_4 = 50, uncore_m3upi_1 = 22, 
uncore_iio_0 = 34, tracepoint = 2, uncore_cha_0 = 24, uncore_irp_2 = 48, 
cstate_pkg = 54, uncore_imc_4 = 17, uncore_cha_9 = 33, 
uncore_iio_free_running_4 = 44, uncore_ubox = 23, uncore_irp_0 = 46, 
uncore_m2m_0 = 11, uncore_imc_2 = 15, kprobe = 6, uncore_cha_7 = 31, 
uncore_iio_free_running_2 = 42, uncore_iio_5 = 39, uncore_imc_0 = 13, 
uncore_upi_0 = 19, uncore_cha_5 = 29, uncore_iio_free_running_0 = 40, 
uncore_pcu = 52, msr = 9, uncore_iio_3 = 37, uncore_cha_3
   = 27, uncore_irp_5 = 51, uncore_iio_1 = 35
  # CPU cache info:
  #  L1 Data 32K [0,20]
  #  L1 Instruction  32K [0,20]
+#  L1 Data 32K [1,21]
+#  L1 Instruction  32K [1,21]
  #  L1 Data 32K [2,22]
  #  L1 Instruction  32K [2,22]
  #  L1 Data 32K [3,23]
@@ -123,9 +125,8 @@
  #  L1 Instruction

Re: [PATCH V1]Perf: Return error code for perf_session__new function on failure

[Mukesh Ojha]

On 8/20/2019 4:51 PM, Mamatha Inamdar wrote:

This Patch is to return error code of perf_new_session function
on failure instead of NULL
--
Test Results:

Before Fix:

$ perf c2c report -input
failed to open nput: No such file or directory

$ echo $?
0
--
After Fix:

$ ./perf c2c report -input
failed to open nput: No such file or directory

$ echo $?
254

Signed-off-by: Mamatha Inamdar 
Acked-by: Ravi Bangoria 
Reported-by: Nageswara R Sastry 
Tested-by: Nageswara R Sastry 


Looks good to me.

Reviewed-by: Mukesh Ojha 

Thanks,
Mukesh


---
  tools/perf/builtin-annotate.c  |5 +++--
  tools/perf/builtin-buildid-cache.c |5 +++--
  tools/perf/builtin-buildid-list.c  |5 +++--
  tools/perf/builtin-c2c.c   |6 --
  tools/perf/builtin-diff.c  |9 +
  tools/perf/builtin-evlist.c|5 +++--
  tools/perf/builtin-inject.c|5 +++--
  tools/perf/builtin-kmem.c  |5 +++--
  tools/perf/builtin-kvm.c   |9 +
  tools/perf/builtin-lock.c  |5 +++--
  tools/perf/builtin-mem.c   |5 +++--
  tools/perf/builtin-record.c|5 +++--
  tools/perf/builtin-report.c|4 ++--
  tools/perf/builtin-sched.c |   11 ++-
  tools/perf/builtin-script.c|9 +
  tools/perf/builtin-stat.c  |   11 ++-
  tools/perf/builtin-timechart.c |5 +++--
  tools/perf/builtin-top.c   |5 +++--
  tools/perf/builtin-trace.c |4 ++--
  tools/perf/util/data-convert-bt.c  |5 -
  tools/perf/util/session.c  |   13 +
  21 files changed, 81 insertions(+), 55 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 9bb6371..b3b9631 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -33,6 +33,7 @@
  #include "util/data.h"
  #include "arch/common.h"
  #include "util/block-range.h"
+#include 
  
  #include 

  #include 
@@ -581,8 +582,8 @@ int cmd_annotate(int argc, const char **argv)
data.path = input_name;
  
  	annotate.session = perf_session__new(&data, false, &annotate.tool);

-   if (annotate.session == NULL)
-   return -1;
+   if (IS_ERR(annotate.session))
+   return PTR_ERR(annotate.session);
  
  	annotate.has_br_stack = perf_header__has_feat(&annotate.session->header,

  HEADER_BRANCH_STACK);
diff --git a/tools/perf/builtin-buildid-cache.c 
b/tools/perf/builtin-buildid-cache.c
index 10457b1..7bab695 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -26,6 +26,7 @@
  #include "util/symbol.h"
  #include "util/time-utils.h"
  #include "util/probe-file.h"
+#include 
  
  static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid)

  {
@@ -420,8 +421,8 @@ int cmd_buildid_cache(int argc, const char **argv)
data.force = force;
  
  		session = perf_session__new(&data, false, NULL);

-   if (session == NULL)
-   return -1;
+   if (IS_ERR(session))
+   return PTR_ERR(session);
}
  
  	if (symbol__init(session ? &session->header.env : NULL) < 0)

diff --git a/tools/perf/builtin-buildid-list.c 
b/tools/perf/builtin-buildid-list.c
index f403e19..95036ee 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -18,6 +18,7 @@
  #include "util/symbol.h"
  #include "util/data.h"
  #include 
+#include 
  
  static int sysfs__fprintf_build_id(FILE *fp)

  {
@@ -65,8 +66,8 @@ static int perf_session__list_build_ids(bool force, bool 
with_hits)
goto out;
  
  	session = perf_session__new(&data, false, &build_id__mark_dso_hit_ops);

-   if (session == NULL)
-   return -1;
+   if (IS_ERR(session))
+   return PTR_ERR(session);
  
  	/*

 * We take all buildids when the file contains AUX area tracing data
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index f0aae6e..a26a33c 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -34,6 +34,7 @@
  #include "thread.h"
  #include "mem2node.h"
  #include "symbol.h"
+#include 
  
  struct c2c_hists {

struct histshists;
@@ -2774,8 +2775,9 @@ static int perf_c2c__report(int argc, const char **argv)
}
  
  	session = perf_session__new(&data, 0, &c2c.tool);

-   if (session == NULL) {
-   pr_debug("No memory for session\n");
+   if (IS_ERR(session)) {
+   err = PTR_ERR(session);
+   pr_debug("Error creating perf session\n");
go

[tip:locking/core] locking/mutex: Use mutex flags macro instead of hard code

[tip-bot for Mukesh Ojha]

Commit-ID:  a037d269221c0ae15f47046757afcbd1a7177bbf
Gitweb: https://git.kernel.org/tip/a037d269221c0ae15f47046757afcbd1a7177bbf
Author: Mukesh Ojha 
AuthorDate: Wed, 31 Jul 2019 20:35:04 +0530
Committer:  Peter Zijlstra 
CommitDate: Tue, 6 Aug 2019 12:49:16 +0200

locking/mutex: Use mutex flags macro instead of hard code

Use the mutex flag macro instead of hard code value inside
__mutex_owner().

Signed-off-by: Mukesh Ojha 
Signed-off-by: Peter Zijlstra (Intel) 
Cc: mi...@redhat.com
Cc: w...@kernel.org
Link: 
https://lkml.kernel.org/r/1564585504-3543-2-git-send-email-mo...@codeaurora.org
---
 kernel/locking/mutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index ac4929f1e085..b4bcb0236d7a 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -85,7 +85,7 @@ EXPORT_SYMBOL(__mutex_init);
  */
 static inline struct task_struct *__mutex_owner(struct mutex *lock)
 {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
 }
 
 static inline struct task_struct *__owner_task(unsigned long owner)

[tip:locking/core] locking/mutex: Make __mutex_owner static to mutex.c

[tip-bot for Mukesh Ojha]

Commit-ID:  5f35d5a66b3ec62cb5ec4ec2ad9aebe2ac325673
Gitweb: https://git.kernel.org/tip/5f35d5a66b3ec62cb5ec4ec2ad9aebe2ac325673
Author: Mukesh Ojha 
AuthorDate: Wed, 31 Jul 2019 20:35:03 +0530
Committer:  Peter Zijlstra 
CommitDate: Tue, 6 Aug 2019 12:49:16 +0200

locking/mutex: Make __mutex_owner static to mutex.c

__mutex_owner() should only be used by the mutex api's.
So, to put this restiction let's move the __mutex_owner()
function definition from linux/mutex.h to mutex.c file.

There exist functions that uses __mutex_owner() like
mutex_is_locked() and mutex_trylock_recursive(), So
to keep legacy thing intact move them as well and
export them.

Move mutex_waiter structure also to keep it private to the
file.

Signed-off-by: Mukesh Ojha 
Signed-off-by: Peter Zijlstra (Intel) 
Cc: mi...@redhat.com
Cc: w...@kernel.org
Link: 
https://lkml.kernel.org/r/1564585504-3543-1-git-send-email-mo...@codeaurora.org
---
 include/linux/mutex.h  | 38 +++---
 kernel/locking/mutex.c | 39 +++
 kernel/locking/mutex.h |  2 ++
 3 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index dcd03fee6e01..eb8c62aba263 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -65,29 +65,6 @@ struct mutex {
 #endif
 };
 
-/*
- * Internal helper function; C doesn't allow us to hide it :/
- *
- * DO NOT USE (outside of mutex code).
- */
-static inline struct task_struct *__mutex_owner(struct mutex *lock)
-{
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
-}
-
-/*
- * This is the control structure for tasks blocked on mutex,
- * which resides on the blocked task's kernel stack:
- */
-struct mutex_waiter {
-   struct list_headlist;
-   struct task_struct  *task;
-   struct ww_acquire_ctx   *ww_ctx;
-#ifdef CONFIG_DEBUG_MUTEXES
-   void*magic;
-#endif
-};
-
 #ifdef CONFIG_DEBUG_MUTEXES
 
 #define __DEBUG_MUTEX_INITIALIZER(lockname)\
@@ -144,10 +121,7 @@ extern void __mutex_init(struct mutex *lock, const char 
*name,
  *
  * Returns true if the mutex is locked, false if unlocked.
  */
-static inline bool mutex_is_locked(struct mutex *lock)
-{
-   return __mutex_owner(lock) != NULL;
-}
+extern bool mutex_is_locked(struct mutex *lock);
 
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
@@ -220,13 +194,7 @@ enum mutex_trylock_recursive_enum {
  *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
  *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
  */
-static inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
-mutex_trylock_recursive(struct mutex *lock)
-{
-   if (unlikely(__mutex_owner(lock) == current))
-   return MUTEX_TRYLOCK_RECURSIVE;
-
-   return mutex_trylock(lock);
-}
+extern /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock);
 
 #endif /* __LINUX_MUTEX_H */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5e069734363c..ac4929f1e085 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -36,6 +36,19 @@
 # include "mutex.h"
 #endif
 
+/*
+ * This is the control structure for tasks blocked on mutex,
+ * which resides on the blocked task's kernel stack:
+ */
+struct mutex_waiter {
+   struct list_headlist;
+   struct task_struct  *task;
+   struct ww_acquire_ctx   *ww_ctx;
+#ifdef CONFIG_DEBUG_MUTEXES
+   void*magic;
+#endif
+};
+
 void
 __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
 {
@@ -65,11 +78,37 @@ EXPORT_SYMBOL(__mutex_init);
 
 #define MUTEX_FLAGS0x07
 
+/*
+ * Internal helper function; C doesn't allow us to hide it :/
+ *
+ * DO NOT USE (outside of mutex code).
+ */
+static inline struct task_struct *__mutex_owner(struct mutex *lock)
+{
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+}
+
 static inline struct task_struct *__owner_task(unsigned long owner)
 {
return (struct task_struct *)(owner & ~MUTEX_FLAGS);
 }
 
+bool mutex_is_locked(struct mutex *lock)
+{
+   return __mutex_owner(lock) != NULL;
+}
+EXPORT_SYMBOL(mutex_is_locked);
+
+__must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+   if (unlikely(__mutex_owner(lock) == current))
+   return MUTEX_TRYLOCK_RECURSIVE;
+
+   return mutex_trylock(lock);
+}
+EXPORT_SYMBOL(mutex_trylock_recursive);
+
 static inline unsigned long __owner_flags(unsigned long owner)
 {
return owner & MUTEX_FLAGS;
diff --git a/kernel/locking/mutex.h b/kernel/locking/mutex.h
index 1c2287d3fa71..7cde5c6d414e 100644
--- a/kernel/locking/mutex.h
+++ b/kernel/locking/mutex.h
@@ -19,6 +19,8 @@
 #define de

Re: [PATCH] perf tools: Fix a typo in Makefile

[Mukesh Ojha]

On 8/1/2019 8:58 AM, Masanari Iida wrote:

This patch fix a spelling typo in Makefile.

Signed-off-by: Masanari Iida 


Reviewed-by: Mukesh Ojha 

-Mukesh


---
  tools/perf/Documentation/Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/Documentation/Makefile 
b/tools/perf/Documentation/Makefile
index 6d148a40551c..adc5a7e44b98 100644
--- a/tools/perf/Documentation/Makefile
+++ b/tools/perf/Documentation/Makefile
@@ -242,7 +242,7 @@ $(OUTPUT)doc.dep : $(wildcard *.txt) build-docdep.perl
$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
mv $@+ $@
  
--include $(OUPTUT)doc.dep

+-include $(OUTPUT)doc.dep
  
  _cmds_txt = cmds-ancillaryinterrogators.txt \

cmds-ancillarymanipulators.txt \

[PATCH V5 0/1] perf: Add CPU hotplug support for events

[Mukesh Ojha]

The embedded world, specifically Android mobile SoCs, rely on CPU
hotplugs to manage power and thermal constraints. These hotplugs
can happen at a very rapid pace. Adjacently, they also relies on
many perf event counters for its management. Therefore, there is
a need to preserve these events across hotplugs.

In such a scenario, a perf client (kernel or user-space) can create
events even when the CPU is offline. If the CPU comes online during
the lifetime of the event, the registered event can start counting
spontaneously. As an extension to this, the events' count can also
be preserved across CPU hotplugs. This takes the burden off of the
clients to monitor the state of the CPU.

The tests were conducted on arm64 device.
/* CPU-1 is offline: Event created when CPU1 is offline */

/ # cat /sys/devices/system/cpu/cpu1/online
1
/ # echo 0 > /sys/devices/system/cpu/cpu1/online

Test used for testing
#!/bin/sh

chmod +x *

# Count the cycles events on cpu-1 for every 200 ms
./perf stat -e cycles -I 200 -C 1 &

# Make the CPU-1 offline and online continuously
while true; do
sleep 2
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 2
echo 1 > /sys/devices/system/cpu/cpu1/online
done

Results:
/ # ./test.sh
#   time counts unit events
 0.200145885cycles
 0.410115208cycles
 0.619922551cycles
 0.829904635cycles
 1.039751614cycles
 1.249547603cycles
 1.459228280cycles
 1.665606561cycles
 1.874981926cycles
 2.084297811cycles
 2.293471249cycles
 2.503231561cycles
 2.712993332cycles
 2.922744478cycles
 3.132502186cycles
 3.342255050cycles
 3.552010102cycles
 3.761760363cycles

/* CPU-1 made online: Event started counting */

 3.9714592691925429  cycles
 4.181325206   19391145  cycles
 4.391074164 113894  cycles
 4.5991305193150152  cycles
 4.805564737 487122  cycles
 5.015164581 247533  cycles
 5.224764529 103622  cycles
#   time counts unit events
 5.434360831 238179  cycles
 5.645293799 238895  cycles
 5.854909320 367543  cycles
 6.0644879662383428  cycles

 /* CPU-1 made offline: counting stopped

 6.274289476cycles
 6.483493903cycles
 6.693202705cycles
 6.902956195cycles
 7.112714268cycles
 7.322465570cycles
 7.53340cycles
 7.741975830cycles
 7.951686246cycles

/* CPU-1 made online: Event started counting

 8.161469892   22040750  cycles
 8.371219528 114977  cycles
 8.580979111 259952  cycles
 8.790757132 444661  cycles
 9.000559215 248512  cycles
 9.210385256 246590  cycles
 9.420187704 243819  cycles
 9.6300522877102438  cycles
 9.839848225 337454  cycles
10.049645048 644072  cycles
10.2594762461855410  cycles


Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 

Mukesh Ojha (1):
  perf: event preserve and create across cpu hotplug

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 122 +
 2 files changed, 79 insertions(+), 44 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH V5 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V5:
=
- Rebased it.

Change in V4:
=
- Released, __get_cpu_context would not be correct way to get the
  cpu context of the cpu which is offline, instead use
  container_of to get the cpuctx from ctx.

- Changed the goto label name inside event_function_call from
  'remove_event_from_context' to 'out'.

Change in V3:
=
- Jiri has tried perf stat -a and removed one of the cpu from the other
  terminal. This resulted in a crash. Crash was because in
  event_function_call(), we were passing NULL as cpuctx in 
  func(event, NULL, ctx, data).Fixed it in this patch.

Change in V2:
=
As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
   Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
   delete events from context list while cpu is offline.

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 123 -
 2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3dc01cf..52b14b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -511,6 +511,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 118ad1a..82b5106 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -248,6 +248,8 @@ static int event_function(void *info)
 static void event_function_call(struct perf_event *event, event_f func, void 
*data)
 {
struct perf_event_context *ctx = event->ctx;
+   struct perf_cpu_context *cpuctx =
+   container_of(ctx, struct perf_cpu_context, ctx);
struct task_struct *task = READ_ONCE(ctx->task); /* verified in 
event_function */
struct event_function_struct efs = {
.event = event,
@@ -264,17 +266,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
 
-   if (!task) {
-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
 
 again:
-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
 
raw_spin_lock_irq(&ctx->lock);
/*
@@ -286,11 +289,17 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto out;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lock);
goto again;
}
-   func(event, NULL, ctx, data);
+
+out:
+   func(event, cpuctx, ctx, data);
raw_spin_unlock_irq(&

[PATCH 2/2 v3] locking/mutex: Use mutex flags macro instead of hard code

[Mukesh Ojha]

Use the mutex flag macro instead of hard code value inside
__mutex_owner().

Signed-off-by: Mukesh Ojha 
---
Changes in v3:
 - no change.

Changes in v2:
 - Framed the commit according the changes done in 1/2 of
   the patchset.

 kernel/locking/mutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index ac4929f..b4bcb02 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -85,7 +85,7 @@ struct mutex_waiter {
  */
 static inline struct task_struct *__mutex_owner(struct mutex *lock)
 {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
 }
 
 static inline struct task_struct *__owner_task(unsigned long owner)
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 1/2 v3] locking/mutex: Make __mutex_owner static to mutex.c

[Mukesh Ojha]

__mutex_owner() should only be used by the mutex api's.
So, to put this restiction let's move the __mutex_owner()
function definition from linux/mutex.h to mutex.c file.

There exist functions that uses __mutex_owner() like
mutex_is_locked() and mutex_trylock_recursive(), So
to keep legacy thing intact move them as well and
export them.

Move mutex_waiter structure also to keep it private to the
file.

Signed-off-by: Mukesh Ojha 
---
Changes in v3:
- Moved mutex_waiter and removed inline from mutex_trylock_recursive()
  mutex_is_owner().
  
Changes in v2:
- On Peterz suggestion, moved __mutex_owner() to mutex.c to
  make it static to mutex.c.

- Exported mutex_is_owner() and mutex_trylock_recursive()
  to keep the existing thing intact as there are loadable
  modules which uses these functions.

 include/linux/mutex.h  | 38 +++---
 kernel/locking/mutex.c | 39 +++
 2 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index dcd03fe..eb8c62a 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -65,29 +65,6 @@ struct mutex {
 #endif
 };
 
-/*
- * Internal helper function; C doesn't allow us to hide it :/
- *
- * DO NOT USE (outside of mutex code).
- */
-static inline struct task_struct *__mutex_owner(struct mutex *lock)
-{
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
-}
-
-/*
- * This is the control structure for tasks blocked on mutex,
- * which resides on the blocked task's kernel stack:
- */
-struct mutex_waiter {
-   struct list_headlist;
-   struct task_struct  *task;
-   struct ww_acquire_ctx   *ww_ctx;
-#ifdef CONFIG_DEBUG_MUTEXES
-   void*magic;
-#endif
-};
-
 #ifdef CONFIG_DEBUG_MUTEXES
 
 #define __DEBUG_MUTEX_INITIALIZER(lockname)\
@@ -144,10 +121,7 @@ extern void __mutex_init(struct mutex *lock, const char 
*name,
  *
  * Returns true if the mutex is locked, false if unlocked.
  */
-static inline bool mutex_is_locked(struct mutex *lock)
-{
-   return __mutex_owner(lock) != NULL;
-}
+extern bool mutex_is_locked(struct mutex *lock);
 
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
@@ -220,13 +194,7 @@ enum mutex_trylock_recursive_enum {
  *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
  *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
  */
-static inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
-mutex_trylock_recursive(struct mutex *lock)
-{
-   if (unlikely(__mutex_owner(lock) == current))
-   return MUTEX_TRYLOCK_RECURSIVE;
-
-   return mutex_trylock(lock);
-}
+extern /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock);
 
 #endif /* __LINUX_MUTEX_H */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5e06973..ac4929f 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -36,6 +36,19 @@
 # include "mutex.h"
 #endif
 
+/*
+ * This is the control structure for tasks blocked on mutex,
+ * which resides on the blocked task's kernel stack:
+ */
+struct mutex_waiter {
+   struct list_headlist;
+   struct task_struct  *task;
+   struct ww_acquire_ctx   *ww_ctx;
+#ifdef CONFIG_DEBUG_MUTEXES
+   void*magic;
+#endif
+};
+
 void
 __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
 {
@@ -65,11 +78,37 @@
 
 #define MUTEX_FLAGS0x07
 
+/*
+ * Internal helper function; C doesn't allow us to hide it :/
+ *
+ * DO NOT USE (outside of mutex code).
+ */
+static inline struct task_struct *__mutex_owner(struct mutex *lock)
+{
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+}
+
 static inline struct task_struct *__owner_task(unsigned long owner)
 {
return (struct task_struct *)(owner & ~MUTEX_FLAGS);
 }
 
+bool mutex_is_locked(struct mutex *lock)
+{
+   return __mutex_owner(lock) != NULL;
+}
+EXPORT_SYMBOL(mutex_is_locked);
+
+__must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+   if (unlikely(__mutex_owner(lock) == current))
+   return MUTEX_TRYLOCK_RECURSIVE;
+
+   return mutex_trylock(lock);
+}
+EXPORT_SYMBOL(mutex_trylock_recursive);
+
 static inline unsigned long __owner_flags(unsigned long owner)
 {
return owner & MUTEX_FLAGS;
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH 2/2] locking/mutex: Use mutex flags macro instead of hard code value

[Mukesh Ojha]

On 7/30/2019 9:32 PM, Peter Zijlstra wrote:

On Tue, Jul 30, 2019 at 06:10:49PM +0530, Mukesh Ojha wrote:


To make it static , i have to export mutex_is_locked() after moving it
inside mutex.c, so that other module can use it.

Yep, see below -- completely untested.


Also are we thinking of removing
static inline /* __deprecated */ __must_check enum
mutex_trylock_recursive_enum
mutex_trylock_recursive(struct mutex *lock)

inside linux/mutex.h in future ?

As i see it is used at one or two places and there is a check inside
checkpatch guarding its further use .

That was the idea; recursive locking is evil, but we have these two
legacy sites.

---

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index dcd03fee6e01..eb8c62aba263 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -65,29 +65,6 @@ struct mutex {
  #endif
  };
  
-/*

- * Internal helper function; C doesn't allow us to hide it :/
- *
- * DO NOT USE (outside of mutex code).
- */
-static inline struct task_struct *__mutex_owner(struct mutex *lock)
-{
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
-}
-
-/*
- * This is the control structure for tasks blocked on mutex,
- * which resides on the blocked task's kernel stack:
- */
-struct mutex_waiter {
-   struct list_headlist;
-   struct task_struct  *task;
-   struct ww_acquire_ctx   *ww_ctx;
-#ifdef CONFIG_DEBUG_MUTEXES
-   void*magic;
-#endif
-};
-
  #ifdef CONFIG_DEBUG_MUTEXES
  
  #define __DEBUG_MUTEX_INITIALIZER(lockname)\

@@ -144,10 +121,7 @@ extern void __mutex_init(struct mutex *lock, const char 
*name,
   *
   * Returns true if the mutex is locked, false if unlocked.
   */
-static inline bool mutex_is_locked(struct mutex *lock)
-{
-   return __mutex_owner(lock) != NULL;
-}
+extern bool mutex_is_locked(struct mutex *lock);
  
  /*

   * See kernel/locking/mutex.c for detailed documentation of these APIs.
@@ -220,13 +194,7 @@ enum mutex_trylock_recursive_enum {
   *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
   *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
   */
-static inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
-mutex_trylock_recursive(struct mutex *lock)
-{
-   if (unlikely(__mutex_owner(lock) == current))
-   return MUTEX_TRYLOCK_RECURSIVE;
-
-   return mutex_trylock(lock);
-}
+extern /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock);
  
  #endif /* __LINUX_MUTEX_H */

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5e069734363c..2f73935a6053 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -36,6 +36,19 @@
  # include "mutex.h"
  #endif
  
+/*

+ * This is the control structure for tasks blocked on mutex,
+ * which resides on the blocked task's kernel stack:
+ */
+struct mutex_waiter {
+   struct list_headlist;
+   struct task_struct  *task;
+   struct ww_acquire_ctx   *ww_ctx;
+#ifdef CONFIG_DEBUG_MUTEXES
+   void*magic;
+#endif
+};


i already did in v2  except this above waiter struct, will do it in v3.

Cheers,

Mukesh


+
  void
  __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key)
  {
@@ -65,6 +78,16 @@ EXPORT_SYMBOL(__mutex_init);
  
  #define MUTEX_FLAGS		0x07
  
+/*

+ * Internal helper function; C doesn't allow us to hide it :/
+ *
+ * DO NOT USE (outside of mutex code).
+ */
+static inline struct task_struct *__mutex_owner(struct mutex *lock)
+{
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
+}
+
  static inline struct task_struct *__owner_task(unsigned long owner)
  {
return (struct task_struct *)(owner & ~MUTEX_FLAGS);
@@ -75,6 +98,22 @@ static inline unsigned long __owner_flags(unsigned long 
owner)
return owner & MUTEX_FLAGS;
  }
  
+bool mutex_is_locked(struct mutex *lock)

+{
+   return __mutex_owner(lock) != NULL;
+}
+EXPORT_SYMBOL(mutex_is_locked);
+
+__must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+   if (unlikely(__mutex_owner(lock) == current))
+   return MUTEX_TRYLOCK_RECURSIVE;
+
+   return mutex_trylock(lock);
+}
+EXPORT_SYMBOL(mutex_trylock_recursive);
+
  /*
   * Trylock variant that retuns the owning task on failure.
   */

[PATCH 2/2 v2] locking/mutex: Use mutex flags macro instead of hard code value

[Mukesh Ojha]

Use the mutex flag macro instead of hard code value inside
__mutex_owner().

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
 - Framed the commit according the changes done in 1/2 of
   the patchset.

 kernel/locking/mutex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index f73250a..1c8f86a 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -72,7 +72,7 @@
  */
 static inline struct task_struct *__mutex_owner(struct mutex *lock)
 {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
 }
 
 /**
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 1/2 v2] locking/mutex: Make __mutex_owner static to mutex.c

[Mukesh Ojha]

__mutex_owner() should only be used by the mutex api's.
So, put this restiction let's move the __mutex_owner()
function definition from linux/mutex.h to mutex.c file.

There exist functions that uses __mutex_owner() like
mutex_is_locked() and mutex_trylock_recursive(), So
to keep the thing intact move them as well and
export them.

Signed-off-by: Mukesh Ojha 
---
Changes in v2:
- On Peterz suggestion, moved __mutex_owner() to mutex.c to
  make it static to mutex.c.

- Exported mutex_is_owner() and mutex_trylock_recursive()
  to keep the existing thing intact as there are loadable
  modules which uses these functions.

 include/linux/mutex.h  | 44 +++-
 kernel/locking/mutex.c | 44 
 2 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index dcd03fe..841b47d 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -66,16 +66,6 @@ struct mutex {
 };
 
 /*
- * Internal helper function; C doesn't allow us to hide it :/
- *
- * DO NOT USE (outside of mutex code).
- */
-static inline struct task_struct *__mutex_owner(struct mutex *lock)
-{
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
-}
-
-/*
  * This is the control structure for tasks blocked on mutex,
  * which resides on the blocked task's kernel stack:
  */
@@ -138,17 +128,10 @@ static inline void mutex_destroy(struct mutex *lock) {}
 extern void __mutex_init(struct mutex *lock, const char *name,
 struct lock_class_key *key);
 
-/**
- * mutex_is_locked - is the mutex locked
- * @lock: the mutex to be queried
- *
- * Returns true if the mutex is locked, false if unlocked.
- */
-static inline bool mutex_is_locked(struct mutex *lock)
-{
-   return __mutex_owner(lock) != NULL;
-}
+extern inline bool mutex_is_locked(struct mutex *lock);
 
+extern inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock);
 /*
  * See kernel/locking/mutex.c for detailed documentation of these APIs.
  * Also see Documentation/locking/mutex-design.rst.
@@ -208,25 +191,4 @@ enum mutex_trylock_recursive_enum {
MUTEX_TRYLOCK_RECURSIVE,
 };
 
-/**
- * mutex_trylock_recursive - trylock variant that allows recursive locking
- * @lock: mutex to be locked
- *
- * This function should not be used, _ever_. It is purely for hysterical GEM
- * raisins, and once those are gone this will be removed.
- *
- * Returns:
- *  - MUTEX_TRYLOCK_FAILED- trylock failed,
- *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
- *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
- */
-static inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
-mutex_trylock_recursive(struct mutex *lock)
-{
-   if (unlikely(__mutex_owner(lock) == current))
-   return MUTEX_TRYLOCK_RECURSIVE;
-
-   return mutex_trylock(lock);
-}
-
 #endif /* __LINUX_MUTEX_H */
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5e06973..f73250a 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -65,6 +65,50 @@
 
 #define MUTEX_FLAGS0x07
 
+/*
+ * Internal helper function; C doesn't allow us to hide it :/
+ *
+ * DO NOT USE (outside of mutex code).
+ */
+static inline struct task_struct *__mutex_owner(struct mutex *lock)
+{
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+}
+
+/**
+ * mutex_is_locked - is the mutex locked
+ * @lock: the mutex to be queried
+ *
+ * Returns true if the mutex is locked, false if unlocked.
+ */
+inline bool mutex_is_locked(struct mutex *lock)
+{
+   return __mutex_owner(lock) != NULL;
+}
+EXPORT_SYMBOL(mutex_is_locked);
+
+/**
+ * mutex_trylock_recursive - trylock variant that allows recursive locking
+ * @lock: mutex to be locked
+ *
+ * This function should not be used, _ever_. It is purely for hysterical GEM
+ * raisins, and once those are gone this will be removed.
+ *
+ * Returns:
+ *  - MUTEX_TRYLOCK_FAILED- trylock failed,
+ *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
+ *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
+ */
+inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
+mutex_trylock_recursive(struct mutex *lock)
+{
+   if (unlikely(__mutex_owner(lock) == current))
+   return MUTEX_TRYLOCK_RECURSIVE;
+
+   return mutex_trylock(lock);
+}
+EXPORT_SYMBOL(mutex_trylock_recursive);
+
 static inline struct task_struct *__owner_task(unsigned long owner)
 {
return (struct task_struct *)(owner & ~MUTEX_FLAGS);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH 2/2] locking/mutex: Use mutex flags macro instead of hard code value

[Mukesh Ojha]

On 7/30/2019 1:33 PM, Peter Zijlstra wrote:

On Tue, Jul 30, 2019 at 01:23:13PM +0530, Mukesh Ojha wrote:

On 7/29/2019 4:37 PM, Peter Zijlstra wrote:

On Mon, Jul 29, 2019 at 04:22:58PM +0530, Mukesh Ojha wrote:

Let's use the mutex flag macro(which got moved from mutex.c
to linux/mutex.h in the last patch) instead of hard code
value which was used in __mutex_owner().

Signed-off-by: Mukesh Ojha 
---
   include/linux/mutex.h | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 79b28be..c3833ba 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -87,7 +87,7 @@ struct mutex {
*/
   static inline struct task_struct *__mutex_owner(struct mutex *lock)
   {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
   }

I would _much_ rather move __mutex_owner() out of line, you're exposing
far too much stuff.

if i understand you correctly, you want me to move __mutex_owner() to
mutex.c
__mutex_owner() is used in mutex_is_locked() and mutex_trylock_recursive
inside linux/mutex.h.

Shall i move them as well ?

Yes, then you can make __mutex_owner() static.


To make it static , i have to export mutex_is_locked() after moving it 
inside mutex.c, so that other module can use it.


Also are we thinking of removing
static inline /* __deprecated */ __must_check enum 
mutex_trylock_recursive_enum

mutex_trylock_recursive(struct mutex *lock)

inside linux/mutex.h in future ?

As i see it is used at one or two places and there is a check inside 
checkpatch guarding its further use .


Thanks,
Mukesh




Thanks!

Re: [PATCH 2/2] locking/mutex: Use mutex flags macro instead of hard code value

[Mukesh Ojha]

On 7/29/2019 4:37 PM, Peter Zijlstra wrote:

On Mon, Jul 29, 2019 at 04:22:58PM +0530, Mukesh Ojha wrote:

Let's use the mutex flag macro(which got moved from mutex.c
to linux/mutex.h in the last patch) instead of hard code
value which was used in __mutex_owner().

Signed-off-by: Mukesh Ojha 
---
  include/linux/mutex.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 79b28be..c3833ba 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -87,7 +87,7 @@ struct mutex {
   */
  static inline struct task_struct *__mutex_owner(struct mutex *lock)
  {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
  }

I would _much_ rather move __mutex_owner() out of line, you're exposing
far too much stuff.


if i understand you correctly, you want me to move __mutex_owner() to 
mutex.c
__mutex_owner() is used in mutex_is_locked() and mutex_trylock_recursive 
inside linux/mutex.h.


Shall i move them as well ?

Thanks,
Mukesh

[PATCH 2/2] locking/mutex: Use mutex flags macro instead of hard code value

[Mukesh Ojha]

Let's use the mutex flag macro(which got moved from mutex.c
to linux/mutex.h in the last patch) instead of hard code
value which was used in __mutex_owner().

Signed-off-by: Mukesh Ojha 
---
 include/linux/mutex.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 79b28be..c3833ba 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -87,7 +87,7 @@ struct mutex {
  */
 static inline struct task_struct *__mutex_owner(struct mutex *lock)
 {
-   return (struct task_struct *)(atomic_long_read(&lock->owner) & ~0x07);
+   return (struct task_struct *)(atomic_long_read(&lock->owner) & 
~MUTEX_FLAGS);
 }
 
 /*
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH 1/2] locking/mutex: Move mutex flag macros to linux/mutex.h

[Mukesh Ojha]

There exist a place where we use hard code value for mutex
flag(e.g in mutex.h __mutex_owner()). Let's move the mutex
flag macros to header linux/mutex.h, so that it could be
reused at other places as well.

Signed-off-by: Mukesh Ojha 
---
 include/linux/mutex.h  | 15 +++
 kernel/locking/mutex.c | 15 ---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index dcd03fe..79b28be 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -20,6 +20,21 @@
 #include 
 #include 
 
+/*
+ * @owner: contains: 'struct task_struct *' to the current lock owner,
+ * NULL means not owned. Since task_struct pointers are aligned at
+ * at least L1_CACHE_BYTES, we have low bits to store extra state.
+ *
+ * Bit0 indicates a non-empty waiter list; unlock must issue a wakeup.
+ * Bit1 indicates unlock needs to hand the lock to the top-waiter
+ * Bit2 indicates handoff has been done and we're waiting for pickup.
+ */
+#define MUTEX_FLAG_WAITERS 0x01
+#define MUTEX_FLAG_HANDOFF 0x02
+#define MUTEX_FLAG_PICKUP  0x04
+
+#define MUTEX_FLAGS0x07
+
 struct ww_acquire_ctx;
 
 /*
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 5e06973..b74c87d 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -50,21 +50,6 @@
 }
 EXPORT_SYMBOL(__mutex_init);
 
-/*
- * @owner: contains: 'struct task_struct *' to the current lock owner,
- * NULL means not owned. Since task_struct pointers are aligned at
- * at least L1_CACHE_BYTES, we have low bits to store extra state.
- *
- * Bit0 indicates a non-empty waiter list; unlock must issue a wakeup.
- * Bit1 indicates unlock needs to hand the lock to the top-waiter
- * Bit2 indicates handoff has been done and we're waiting for pickup.
- */
-#define MUTEX_FLAG_WAITERS 0x01
-#define MUTEX_FLAG_HANDOFF 0x02
-#define MUTEX_FLAG_PICKUP  0x04
-
-#define MUTEX_FLAGS0x07
-
 static inline struct task_struct *__owner_task(unsigned long owner)
 {
return (struct task_struct *)(owner & ~MUTEX_FLAGS);
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

[PATCH] rcu: Fix spelling mistake "greate"->"great"

[Mukesh Ojha]

There is a spelling mistake in file tree_exp.h,
fix this.

Signed-off-by: Mukesh Ojha 
---
 kernel/rcu/tree_exp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
index af7e7b9..609fc87 100644
--- a/kernel/rcu/tree_exp.h
+++ b/kernel/rcu/tree_exp.h
@@ -781,7 +781,7 @@ static int rcu_print_task_exp_stall(struct rcu_node *rnp)
  * other hand, if the CPU is not in an RCU read-side critical section,
  * the IPI handler reports the quiescent state immediately.
  *
- * Although this is a greate improvement over previous expedited
+ * Although this is a great improvement over previous expedited
  * implementations, it is still unfriendly to real-time workloads, so is
  * thus not recommended for any sort of common-case code.  In fact, if
  * you are using synchronize_rcu_expedited() in a loop, please restructure
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH RESEND V4 0/1] perf: Add CPU hotplug support for events

[Mukesh Ojha]

Hi All,

Can you please review this ?

Thanks,
Mukesh

On 6/24/2019 2:31 PM, Mukesh Ojha wrote:

Friendly ping.

On 6/18/2019 7:16 PM, Mukesh Ojha wrote:

The embedded world, specifically Android mobile SoCs, rely on CPU
hotplugs to manage power and thermal constraints. These hotplugs
can happen at a very rapid pace. Adjacently, they also relies on
many perf event counters for its management. Therefore, there is
a need to preserve these events across hotplugs.

In such a scenario, a perf client (kernel or user-space) can create
events even when the CPU is offline. If the CPU comes online during
the lifetime of the event, the registered event can start counting
spontaneously. As an extension to this, the events' count can also
be preserved across CPU hotplugs. This takes the burden off of the
clients to monitor the state of the CPU.

The tests were conducted on arm64 device.
/* CPU-1 is offline: Event created when CPU1 is offline */

/ # cat /sys/devices/system/cpu/cpu1/online
1
/ # echo 0 > /sys/devices/system/cpu/cpu1/online

Test used for testing
#!/bin/sh

chmod +x *

# Count the cycles events on cpu-1 for every 200 ms
./perf stat -e cycles -I 200 -C 1 &

# Make the CPU-1 offline and online continuously
while true; do
 sleep 2
 echo 0 > /sys/devices/system/cpu/cpu1/online
 sleep 2
 echo 1 > /sys/devices/system/cpu/cpu1/online
done

Results:
/ # ./test.sh
#   time counts unit events
  0.200145885    cycles
  0.410115208    cycles
  0.619922551    cycles
  0.829904635    cycles
  1.039751614    cycles
  1.249547603    cycles
  1.459228280    cycles
  1.665606561    cycles
  1.874981926    cycles
  2.084297811    cycles
  2.293471249    cycles
  2.503231561    cycles
  2.712993332    cycles
  2.922744478    cycles
  3.132502186    cycles
  3.342255050    cycles
  3.552010102    cycles
  3.761760363    cycles

 /* CPU-1 made online: Event started counting */

  3.971459269    1925429  cycles
  4.181325206   19391145  cycles
  4.391074164 113894  cycles
  4.599130519    3150152  cycles
  4.805564737 487122  cycles
  5.015164581 247533  cycles
  5.224764529 103622  cycles
#   time counts unit events
  5.434360831 238179  cycles
  5.645293799 238895  cycles
  5.854909320 367543  cycles
  6.064487966    2383428  cycles

  /* CPU-1 made offline: counting stopped

  6.274289476    cycles
  6.483493903    cycles
  6.693202705    cycles
  6.902956195    cycles
  7.112714268    cycles
  7.322465570    cycles
  7.53340    cycles
  7.741975830    cycles
  7.951686246    cycles

 /* CPU-1 made online: Event started counting

  8.161469892   22040750  cycles
  8.371219528 114977  cycles
  8.580979111 259952  cycles
  8.790757132 444661  cycles
  9.000559215 248512  cycles
  9.210385256 246590  cycles
  9.420187704 243819  cycles
  9.630052287    7102438  cycles
  9.839848225 337454  cycles
 10.049645048 644072  cycles
 10.259476246    1855410      cycles

Mukesh Ojha (1):
   perf: event preserve and create across cpu hotplug

  include/linux/perf_event.h |   1 +
  kernel/events/core.c   | 122 
+

  2 files changed, 79 insertions(+), 44 deletions(-)

Re: [PATCH v5] driver core: Fix use-after-free and double free on glue directory

[Mukesh Ojha]

On 7/24/2019 7:11 PM, Mukesh Ojha wrote:


On 7/18/2019 4:49 PM, Muchun Song wrote:
There is a race condition between removing glue directory and adding 
a new

device under the glue directory. It can be reproduced in following test:

path 1: Add the child device under glue dir
device_add()
 get_device_parent()
 mutex_lock(&gdp_mutex);
 
 /*find parent from glue_dirs.list*/
 list_for_each_entry(k, &dev->class->p->glue_dirs.list, entry)
 if (k->parent == parent_kobj) {
 kobj = kobject_get(k);
 break;
 }
 
 mutex_unlock(&gdp_mutex);
 
 
 kobject_add()
 kobject_add_internal()
 create_dir()
 sysfs_create_dir_ns()
 if (kobj->parent)
 parent = kobj->parent->sd;
 
 kernfs_create_dir_ns(parent)
 kernfs_new_node()
 kernfs_get(parent)
 
 /* link in */
 rc = kernfs_add_one(kn);
 if (!rc)
 return kn;

 kernfs_put(kn)
 
 repeat:
 kmem_cache_free(kn)
 kn = parent;

 if (kn) {
 if (atomic_dec_and_test(&kn->count))
 goto repeat;
 }
 

path2: Remove last child device under glue dir
device_del()
 cleanup_glue_dir()
 mutex_lock(&gdp_mutex);
 if (!kobject_has_children(glue_dir))
 kobject_del(glue_dir);
 kobject_put(glue_dir);
 mutex_unlock(&gdp_mutex);

Before path2 remove last child device under glue dir, If path1 add a new
device under glue dir, the glue_dir kobject reference count will be
increase to 2 via kobject_get(k) in get_device_parent(). And path1 has
been called kernfs_new_node(), but not call kernfs_get(parent).
Meanwhile, path2 call kobject_del(glue_dir) beacause 0 is returned by
kobject_has_children(). This result in glue_dir->sd is freed and it's
reference count will be 0. Then path1 call kernfs_get(parent) will 
trigger
a warning in kernfs_get()(WARN_ON(!atomic_read(&kn->count))) and 
increase
it's reference count to 1. Because glue_dir->sd is freed by path2, 
the next

call kernfs_add_one() by path1 will fail(This is also use-after-free)
and call atomic_dec_and_test() to decrease reference count. Because the
reference count is decremented to 0, it will also call kmem_cache_free()
to free glue_dir->sd again. This will result in double free.

In order to avoid this happening, we also should make sure that 
kernfs_node
for glue_dir is released in path2 only when refcount for glue_dir 
kobj is

1 to fix this race.

The following calltrace is captured in kernel 4.14 with the following 
patch

applied:

commit 726e41097920 ("drivers: core: Remove glue dirs from sysfs 
earlier")


-- 


[    3.633703] WARNING: CPU: 4 PID: 513 at .../fs/kernfs/dir.c:494
 Here is WARN_ON(!atomic_read(&kn->count) in 
kernfs_get().


[    3.633986] Call trace:
[    3.633991]  kernfs_create_dir_ns+0xa8/0xb0
[    3.633994]  sysfs_create_dir_ns+0x54/0xe8
[    3.634001]  kobject_add_internal+0x22c/0x3f0
[    3.634005]  kobject_add+0xe4/0x118
[    3.634011]  device_add+0x200/0x870
[    3.634017]  _request_firmware+0x958/0xc38
[    3.634020]  request_firmware_into_buf+0x4c/0x70

[    3.634064] kernel BUG at .../mm/slub.c:294!
 Here is BUG_ON(object == fp) in set_freepointer().

[    3.634346] Call trace:
[    3.634351]  kmem_cache_free+0x504/0x6b8
[    3.634355]  kernfs_put+0x14c/0x1d8
[    3.634359]  kernfs_create_dir_ns+0x88/0xb0
[    3.634362]  sysfs_create_dir_ns+0x54/0xe8
[    3.634366]  kobject_add_internal+0x22c/0x3f0
[    3.634370]  kobject_add+0xe4/0x118
[    3.634374]  device_add+0x200/0x870
[    3.634378]  _request_firmware+0x958/0xc38
[    3.634381]  request_firmware_into_buf+0x4c/0x70
-- 



Fixes: 726e41097920 ("drivers: core: Remove glue dirs from sysfs 
earlier")


Signed-off-by: Muchun Song 
---

Change in v5:
    1. Revert to the v1 fix.
    2. Add some comment to explain why we need do this in
   cleanup_glue_dir().
Change in v4:
    1. Add some kerneldoc comment.
    2. Remove unlock_if_glue_dir().
    3. Rename get_device_parent_locked_if_glue_dir() to
   get_device_parent_locked.
    4. Update commit message.
Change in 

Re: [PATCH v5] driver core: Fix use-after-free and double free on glue directory

[Mukesh Ojha]

 also.

  drivers/base/core.c | 54 -
  1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4aeaa0c92bda..1a67ee325584 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1825,7 +1825,59 @@ static void cleanup_glue_dir(struct device *dev, struct 
kobject *glue_dir)
return;
  
  	mutex_lock(&gdp_mutex);

-   if (!kobject_has_children(glue_dir))
+   /**
+* There is a race condition between removing glue directory
+* and adding a new device under the glue directory.
+*
+* path 1: Add the child device under glue dir
+* device_add()
+*  get_device_parent()
+*  mutex_lock(&gdp_mutex);
+*  
+*  list_for_each_entry(k, &dev->class->p->glue_dirs.list,
+*  entry)
+*  if (k->parent == parent_kobj) {
+*  kobj = kobject_get(k);
+*  break;
+*  }
+*  
+*  mutex_unlock(&gdp_mutex);
+*  
+*  
+*  kobject_add()
+*  kobject_add_internal()
+*  create_dir()
+*  
+*  if (kobj->parent)
+*  parent = kobj->parent->sd;
+*  
+*  kernfs_create_dir_ns(parent)
+*  
+*
+* path2: Remove last child device under glue dir
+* device_del()
+*  cleanup_glue_dir()
+*  
+*  mutex_lock(&gdp_mutex);
+*  if (!kobject_has_children(glue_dir))
+*  kobject_del(glue_dir);
+*  
+*  mutex_unlock(&gdp_mutex);
+*
+* Before path2 remove last child device under glue dir, if path1 add
+* a new device under glue dir, the glue_dir kobject reference count
+* will be increase to 2 via kobject_get(k) in get_device_parent().
+* And path1 has been called kernfs_create_dir_ns(). Meanwhile,
+* path2 call kobject_del(glue_dir) beacause 0 is returned by
+* kobject_has_children(). This result in glue_dir->sd is freed.
+* Then the path1 will see a stale "empty" but still potentially used
+* glue dir around.
+*
+* In order to avoid this happening, we also should make sure that
+* kernfs_node for glue_dir is released in path2 only when refcount
+* for glue_dir kobj is 1.
+*/
+   if (!kobject_has_children(glue_dir) && kref_read(&glue_dir->kref) == 1)
Instead of hardcoding "1 " , can't we check against zero Like Prateek 
sood did in his patch.https://lkml.org/lkml/2019/5/1/3


+ref = kref_read(&glue_dir->kref);
+if  (!kobject_has_children(glue_dir) && --ref)


We have seen many pattern of this issue which is coming from different 
driver e.g uinput as well in 4.14 
kernel.(https://lkml.org/lkml/2019/4/10/146)

The reason why it  started coming after

726e41097920 ("drivers: core: Remove glue dirs from sysfs earlier")

Looks good to me

Reviewed-by: Mukesh Ojha  


-Mukesh



kobject_del(glue_dir);
kobject_put(glue_dir);
mutex_unlock(&gdp_mutex);

Re: [PATCH RESEND V4 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Friendly Ping.

More explanation of the usecase added in the coverletter
[PATCH RESEND V4 0/1] perf: Add CPU hotplug support for events.

-Mukesh

On 6/18/2019 7:16 PM, Mukesh Ojha wrote:

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V4:
=
- Released, __get_cpu_context would not be correct way to get the
   cpu context of the cpu which is offline, instead use
   container_of to get the cpuctx from ctx.

- Changed the goto label name inside event_function_call from
   'remove_event_from_context' to 'out'.

Change in V3:
=
- Jiri has tried perf stat -a and removed one of the cpu from the other
   terminal. This resulted in a crash. Crash was because in
   event_function_call(), we were passing NULL as cpuctx in
   func(event, NULL, ctx, data).Fixed it in this patch.

Change in V2:
=
As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
delete events from context list while cpu is offline.

  include/linux/perf_event.h |   1 +
  kernel/events/core.c   | 123 -
  2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3dc01cf..52b14b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -511,6 +511,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
  };
  
  struct file;

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 118ad1a..82b5106 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -248,6 +248,8 @@ static int event_function(void *info)
  static void event_function_call(struct perf_event *event, event_f func, void 
*data)
  {
struct perf_event_context *ctx = event->ctx;
+   struct perf_cpu_context *cpuctx =
+   container_of(ctx, struct perf_cpu_context, ctx);
struct task_struct *task = READ_ONCE(ctx->task); /* verified in 
event_function */
struct event_function_struct efs = {
.event = event,
@@ -264,17 +266,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
  
-	if (!task) {

-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
  
  again:

-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
  
  	raw_spin_lock_irq(&ctx->lock);

/*
@@ -286,11 +289,17 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto out;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lo

Re: Perf framework : Cluster based counter support

[Mukesh Ojha]

On 6/28/2019 10:29 PM, Mark Rutland wrote:

On Fri, Jun 28, 2019 at 10:23:10PM +0530, Mukesh Ojha wrote:

Hi All,

Hi Mukesh,


Is it looks considerable to add cluster based event support to add in
current perf event framework and later in userspace perf to support
such events ?

Could you elaborate on what you mean by "cluster based event"?

I assume you mean something like events for a cluster-affine shared
resource like some level of cache?

If so, there's a standard pattern for supporting such system/uncore
PMUs, see drivers/perf/qcom_l2_pmu.c and friends for examples.



Thanks Mark for pointing it out.
Also What is stopping us in adding cluster based event e.g L2 cache 
hit/miss or some other type raw events  in core framework ?



Thanks.
Mukesh








Thanks,
Mark.

Perf framework : Cluster based counter support

[Mukesh Ojha]

Hi All,

Is it looks considerable to add cluster based event support to add in 
current perf event framework and later

in userspace perf to support such events ?

Thanks,
Mukesh

Re: [PATCH RESEND V4 0/1] perf: Add CPU hotplug support for events

[Mukesh Ojha]

Friendly ping.

On 6/18/2019 7:16 PM, Mukesh Ojha wrote:

The embedded world, specifically Android mobile SoCs, rely on CPU
hotplugs to manage power and thermal constraints. These hotplugs
can happen at a very rapid pace. Adjacently, they also relies on
many perf event counters for its management. Therefore, there is
a need to preserve these events across hotplugs.

In such a scenario, a perf client (kernel or user-space) can create
events even when the CPU is offline. If the CPU comes online during
the lifetime of the event, the registered event can start counting
spontaneously. As an extension to this, the events' count can also
be preserved across CPU hotplugs. This takes the burden off of the
clients to monitor the state of the CPU.

The tests were conducted on arm64 device.
/* CPU-1 is offline: Event created when CPU1 is offline */

/ # cat /sys/devices/system/cpu/cpu1/online
1
/ # echo 0 > /sys/devices/system/cpu/cpu1/online

Test used for testing
#!/bin/sh

chmod +x *

# Count the cycles events on cpu-1 for every 200 ms
./perf stat -e cycles -I 200 -C 1 &

# Make the CPU-1 offline and online continuously
while true; do
 sleep 2
 echo 0 > /sys/devices/system/cpu/cpu1/online
 sleep 2
 echo 1 > /sys/devices/system/cpu/cpu1/online
done

Results:
/ # ./test.sh
#   time counts unit events
  0.200145885cycles
  0.410115208cycles
  0.619922551cycles
  0.829904635cycles
  1.039751614cycles
  1.249547603cycles
  1.459228280cycles
  1.665606561cycles
  1.874981926cycles
  2.084297811cycles
  2.293471249cycles
  2.503231561cycles
  2.712993332cycles
  2.922744478cycles
  3.132502186cycles
  3.342255050cycles
  3.552010102cycles
  3.761760363cycles

 /* CPU-1 made online: Event started counting */

  3.9714592691925429  cycles
  4.181325206   19391145  cycles
  4.391074164 113894  cycles
  4.5991305193150152  cycles
  4.805564737 487122  cycles
  5.015164581 247533  cycles
  5.224764529 103622  cycles
#   time counts unit events
  5.434360831 238179  cycles
  5.645293799 238895  cycles
  5.854909320 367543  cycles
  6.0644879662383428  cycles

  /* CPU-1 made offline: counting stopped

  6.274289476cycles
  6.483493903cycles
  6.693202705cycles
  6.902956195cycles
  7.112714268cycles
  7.322465570cycles
  7.53340cycles
  7.741975830cycles
  7.951686246cycles

 /* CPU-1 made online: Event started counting

  8.161469892   22040750  cycles
  8.371219528 114977  cycles
  8.580979111 259952  cycles
  8.790757132 444661  cycles
  9.000559215 248512  cycles
  9.210385256 246590  cycles
  9.420187704 243819  cycles
  9.6300522877102438  cycles
  9.839848225 337454  cycles
 10.049645048 644072  cycles
 10.2594762461855410      cycles

Mukesh Ojha (1):
   perf: event preserve and create across cpu hotplug

  include/linux/perf_event.h |   1 +
  kernel/events/core.c   | 122 +
  2 files changed, 79 insertions(+), 44 deletions(-)

[PATCH RESEND V4 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V4:
=
- Released, __get_cpu_context would not be correct way to get the
  cpu context of the cpu which is offline, instead use
  container_of to get the cpuctx from ctx.

- Changed the goto label name inside event_function_call from
  'remove_event_from_context' to 'out'.

Change in V3:
=
- Jiri has tried perf stat -a and removed one of the cpu from the other
  terminal. This resulted in a crash. Crash was because in
  event_function_call(), we were passing NULL as cpuctx in 
  func(event, NULL, ctx, data).Fixed it in this patch.

Change in V2:
=
As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
   Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
   delete events from context list while cpu is offline.

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 123 -
 2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3dc01cf..52b14b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -511,6 +511,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 118ad1a..82b5106 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -248,6 +248,8 @@ static int event_function(void *info)
 static void event_function_call(struct perf_event *event, event_f func, void 
*data)
 {
struct perf_event_context *ctx = event->ctx;
+   struct perf_cpu_context *cpuctx =
+   container_of(ctx, struct perf_cpu_context, ctx);
struct task_struct *task = READ_ONCE(ctx->task); /* verified in 
event_function */
struct event_function_struct efs = {
.event = event,
@@ -264,17 +266,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
 
-   if (!task) {
-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
 
 again:
-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
 
raw_spin_lock_irq(&ctx->lock);
/*
@@ -286,11 +289,17 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto out;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lock);
goto again;
}
-   func(event, NULL, ctx, data);
+
+out:
+   func(event, cpuctx, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
 }
 
@@ -2310,7 +2319,7 @@ sta

[PATCH RESEND V4 0/1] perf: Add CPU hotplug support for events

[Mukesh Ojha]

The embedded world, specifically Android mobile SoCs, rely on CPU
hotplugs to manage power and thermal constraints. These hotplugs
can happen at a very rapid pace. Adjacently, they also relies on
many perf event counters for its management. Therefore, there is
a need to preserve these events across hotplugs.

In such a scenario, a perf client (kernel or user-space) can create
events even when the CPU is offline. If the CPU comes online during
the lifetime of the event, the registered event can start counting
spontaneously. As an extension to this, the events' count can also
be preserved across CPU hotplugs. This takes the burden off of the
clients to monitor the state of the CPU.

The tests were conducted on arm64 device.
/* CPU-1 is offline: Event created when CPU1 is offline */

/ # cat /sys/devices/system/cpu/cpu1/online
1
/ # echo 0 > /sys/devices/system/cpu/cpu1/online

Test used for testing
#!/bin/sh

chmod +x *

# Count the cycles events on cpu-1 for every 200 ms
./perf stat -e cycles -I 200 -C 1 &

# Make the CPU-1 offline and online continuously
while true; do
sleep 2
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 2
echo 1 > /sys/devices/system/cpu/cpu1/online
done

Results:
/ # ./test.sh
#   time counts unit events
 0.200145885cycles
 0.410115208cycles
 0.619922551cycles
 0.829904635cycles
 1.039751614cycles
 1.249547603cycles
 1.459228280cycles
 1.665606561cycles
 1.874981926cycles
 2.084297811cycles
 2.293471249cycles
 2.503231561cycles
 2.712993332cycles
 2.922744478cycles
 3.132502186cycles
 3.342255050cycles
 3.552010102cycles
 3.761760363cycles

/* CPU-1 made online: Event started counting */

 3.9714592691925429  cycles
 4.181325206   19391145  cycles
 4.391074164 113894  cycles
 4.5991305193150152  cycles
 4.805564737 487122  cycles
 5.015164581 247533  cycles
 5.224764529 103622  cycles
#   time counts unit events
 5.434360831 238179  cycles
 5.645293799 238895  cycles
 5.854909320 367543  cycles
 6.0644879662383428  cycles

 /* CPU-1 made offline: counting stopped

 6.274289476cycles
 6.483493903cycles
 6.693202705cycles
 6.902956195cycles
 7.112714268cycles
 7.322465570cycles
 7.53340cycles
 7.741975830cycles
 7.951686246cycles

/* CPU-1 made online: Event started counting

 8.161469892   22040750  cycles
 8.371219528 114977  cycles
 8.580979111 259952  cycles
 8.790757132 444661  cycles
 9.000559215 248512  cycles
 9.210385256 246590  cycles
 9.420187704 243819  cycles
 9.6300522877102438  cycles
 9.839848225 337454  cycles
10.049645048 644072  cycles
10.2594762461855410      cycles

Mukesh Ojha (1):
  perf: event preserve and create across cpu hotplug

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 122 +
 2 files changed, 79 insertions(+), 44 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: [PATCH V4] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

On 6/18/2019 5:53 PM, Peter Zijlstra wrote:

On Tue, Jun 18, 2019 at 02:24:51PM +0530, Mukesh Ojha wrote:

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore,

That's not a therefore. There's a distinct lack of rationale here. Why
do you want this?


Thanks Peter for coming back on this.

Missed to send the coverletter,
https://lkml.org/lkml/2019/5/31/438
Will resend this with coverletter.

Btw,  This patch

is based on suggestion given by you on this
https://lkml.org/lkml/2018/2/16/763


Thanks.
Mukesh


Thanks.
Mukesh

[PATCH V4] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V4:
=
- Released, __get_cpu_context would not be correct way to get the
  cpu context of the cpu which is offline, instead use
  container_of to get the cpuctx from ctx.

- Changed the goto label name inside event_function_call from
  'remove_event_from_context' to 'out'.

Change in V3:
=
- Jiri has tried perf stat -a and removed one of the cpu from the other
  terminal. This resulted in a crash. Crash was because in
  event_function_call(), we were passing NULL as cpuctx in 
  func(event, NULL, ctx, data).Fixed it in this patch.

Change in V2:
=
As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
   Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
   delete events from context list while cpu is offline.

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 123 -
 2 files changed, 79 insertions(+), 45 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3dc01cf..52b14b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -511,6 +511,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 118ad1a..82b5106 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -248,6 +248,8 @@ static int event_function(void *info)
 static void event_function_call(struct perf_event *event, event_f func, void 
*data)
 {
struct perf_event_context *ctx = event->ctx;
+   struct perf_cpu_context *cpuctx =
+   container_of(ctx, struct perf_cpu_context, ctx);
struct task_struct *task = READ_ONCE(ctx->task); /* verified in 
event_function */
struct event_function_struct efs = {
.event = event,
@@ -264,17 +266,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
 
-   if (!task) {
-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
 
 again:
-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
 
raw_spin_lock_irq(&ctx->lock);
/*
@@ -286,11 +289,17 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto out;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lock);
goto again;
}
-   func(event, NULL, ctx, data);
+
+out:
+   func(event, cpuctx, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
 }
 
@@ -2310,7 +2319,7 @@ sta

[PATCH V3] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V3:
- Jiri has tried perf stat -a and removed one of the cpu from the other
  terminal. This resulted in a crash. Crash was because in
  event_function_call(), we were passing NULL as cpuctx in 
  func(event, NULL, ctx, data).Fixed it in this patch.

Change in V2:

As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
   Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
   delete events from context list while cpu is offline.

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 122 -
 2 files changed, 78 insertions(+), 45 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 3dc01cf..52b14b2 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -511,6 +511,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 118ad1a..7f0bd11 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -248,6 +248,7 @@ static int event_function(void *info)
 static void event_function_call(struct perf_event *event, event_f func, void 
*data)
 {
struct perf_event_context *ctx = event->ctx;
+   struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
struct task_struct *task = READ_ONCE(ctx->task); /* verified in 
event_function */
struct event_function_struct efs = {
.event = event,
@@ -264,17 +265,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
 
-   if (!task) {
-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
 
 again:
-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
 
raw_spin_lock_irq(&ctx->lock);
/*
@@ -286,11 +288,17 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto remove_event_from_ctx;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lock);
goto again;
}
-   func(event, NULL, ctx, data);
+
+remove_event_from_ctx:
+   func(event, cpuctx, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
 }
 
@@ -2310,7 +2318,7 @@ static void perf_set_shadow_time(struct perf_event *event,
struct perf_event *event, *partial_group = NULL;
struct pmu *pmu = ctx->pmu;
 
-   if (group_event->state == PERF_EVENT_STATE_OFF)
+   if (group_event->state <= PERF_EVENT_STATE_OFF)
return 0;
 
pmu->start_txn(pmu, PERF_PMU_TXN_ADD);
@@ -2389

Re: [PATCH V2 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

On 6/3/2019 11:23 PM, Jiri Olsa wrote:

On Fri, May 31, 2019 at 07:09:16PM +0530, Mukesh Ojha wrote:

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V2:

As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to
delete events from context list while cpu is offline.


  include/linux/perf_event.h |   1 +
  kernel/events/core.c   | 119 -
  2 files changed, 76 insertions(+), 44 deletions(-)


hi,
I got crash on running "perf stat -a" and switching off
one cpu in another terminal via:
   "echo 0 > /sys/devices/system/cpu/cpu2/online"


Thanks Jiri for the testing it.



jirka


---
ibm-x3650m4-01 login: [  554.951780] smpboot: CPU 2 is now offline
[  554.958301] BUG: kernel NULL pointer dereference, address: 0168
[  554.966070] #PF: supervisor read access in kernel mode
[  554.971802] #PF: error_code(0x) - not-present page
[  554.977532] PGD 0 P4D 0
[  554.980356] Oops:  [#1] SMP PTI
[  554.984256] CPU: 9 PID: 4782 Comm: bash Tainted: GW 
5.2.0-rc1+ #3
[  554.992605] Hardware name: IBM System x3650 M4 : -[7915E2G]-/00Y7683, BIOS 
-[VVE124AUS-1.30]- 11/21/2012
[  555.003190] RIP: 0010:__perf_remove_from_context+0xcd/0x130
[  555.009407] Code: 8b 3d 97 51 5f 60 e8 b2 4d ee ff 48 8b 95 b8 00 00 00 48 01 c2 
48 2b 95 c0 00 00 00 48 89 85 c0 00 00 00 48 89 95 b8 00 00 00 <49> 8b 9c 24 68 
01 00 00 48 85 db 0f 84 43 ff ff ff 65 8b 3d 5b 51
[  555.030361] RSP: 0018:aea78542fcb8 EFLAGS: 00010002
[  555.036190] RAX: 008136178cad RBX: a0c0f78b0008 RCX: 001f
[  555.044151] RDX: 0080bf70f331 RSI: 4219 RDI: fffc405d97b9b8a9
[  555.052112] RBP: a0c0f78b R08: 0002 R09: 00029500
[  555.060074] R10: 00078047268106e0 R11: 0001 R12: 
[  555.068037] R13: a0bf86a5c000 R14: 0001 R15: 0001
[  555.076000] FS:  7f0ace1c9740() GS:a0c2f78c() 
knlGS:
[  555.085029] CS:  0010 DS:  ES:  CR0: 80050033
[  555.091438] CR2: 0168 CR3: 00046c908001 CR4: 000606e0
[  555.099401] Call Trace:
[  555.102133]  ? perf_event_read_event+0x110/0x110
[  555.107286]  ? list_del_event+0x140/0x140
[  555.111757]  event_function_call+0x113/0x130
[  555.116521]  ? list_del_event+0x140/0x140
[  555.120994]  ? perf_event_read_event+0x110/0x110
[  555.126144]  perf_remove_from_context+0x20/0x70
[  555.131200]  perf_event_release_kernel+0x79/0x330
[  555.136451]  hardlockup_detector_perf_cleanup+0x33/0x8d



Looks like the support for arm64 is missing for 
hardlockup_detector_perf_cleanup() 
(HAVE_HARDLOCKUP_DETECTOR_PERF/HAVE_PERF_EVENTS_NMI),

i.e why it has passed for me.

Wondering what could have caused this,


lockup_detector_offline_cpu
 watchdog_disable
   watchdog_nmi_disable
 perf_event_disable(event);

Above path might have disabled the event, perf_remove_from_context this 
might try to remove event from the context list.


My change only effects when event is destroyed while the offline cpu . 
will look more into it .


Thanks,
Mukesh


[  555.142282]  lockup_detector_cleanup+0x16/0x30
[  555.147241]  _cpu_down+0xf3/0x1c0
[  555.150941]  do_cpu_down+0x2c/0x50
[  555.154737]  device_

[PATCH V2 1/1] perf: event preserve and create across cpu hotplug

[Mukesh Ojha]

Perf framework doesn't allow preserving CPU events across
CPU hotplugs. The events are scheduled out as and when the
CPU walks offline. Moreover, the framework also doesn't
allow the clients to create events on an offline CPU. As
a result, the clients have to keep on monitoring the CPU
state until it comes back online.

Therefore, introducing the perf framework to support creation
and preserving of (CPU) events for offline CPUs. Through
this, the CPU's online state would be transparent to the
client and it not have to worry about monitoring the CPU's
state. Success would be returned to the client even while
creating the event on an offline CPU. If during the lifetime
of the event the CPU walks offline, the event would be
preserved and would continue to count as soon as (and if) the
CPU comes back online.

Co-authored-by: Peter Zijlstra 
Signed-off-by: Raghavendra Rao Ananta 
Signed-off-by: Mukesh Ojha 
Cc: Peter Zijlstra 
Cc: Ingo Molnar 
Cc: Arnaldo Carvalho de Melo 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Alexei Starovoitov 
---
Change in V2:

As per long back discussion happened at
https://lkml.org/lkml/2018/2/15/1324

Peter.Z. has suggested to do thing in different way and shared
patch as well. This patch fixes the issue seen while trying
to achieve the purpose.

Fixed issue on top of Peter's patch:
===
1. Added a NULL check on task to avoid crash in __perf_install_in_context.

2. while trying to add event to context when cpu is offline.
   Inside add_event_to_ctx() to make consistent state machine while hotplug.

-event->state += PERF_EVENT_STATE_HOTPLUG_OFFSET;
+event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;

3. In event_function_call(), added a label 'remove_event_from_ctx' to 
   delete events from context list while cpu is offline.


 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 119 -
 2 files changed, 76 insertions(+), 44 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index e47ef76..31a3a5d 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -510,6 +510,7 @@ enum perf_event_state {
PERF_EVENT_STATE_OFF= -1,
PERF_EVENT_STATE_INACTIVE   =  0,
PERF_EVENT_STATE_ACTIVE =  1,
+   PERF_EVENT_STATE_HOTPLUG_OFFSET = -32,
 };
 
 struct file;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 72d06e30..fbfffca 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -264,17 +264,18 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
lockdep_assert_held(&ctx->mutex);
}
 
-   if (!task) {
-   cpu_function_call(event->cpu, event_function, &efs);
-   return;
-   }
-
if (task == TASK_TOMBSTONE)
return;
 
 again:
-   if (!task_function_call(task, event_function, &efs))
-   return;
+   if (task) {
+   if (!task_function_call(task, event_function, &efs))
+   return;
+   } else {
+   if (!cpu_function_call(event->cpu, event_function, &efs))
+   return;
+   }
+
 
raw_spin_lock_irq(&ctx->lock);
/*
@@ -286,10 +287,16 @@ static void event_function_call(struct perf_event *event, 
event_f func, void *da
raw_spin_unlock_irq(&ctx->lock);
return;
}
+
+   if (!task)
+   goto remove_event_from_ctx;
+
if (ctx->is_active) {
raw_spin_unlock_irq(&ctx->lock);
goto again;
}
+
+remove_event_from_ctx:
func(event, NULL, ctx, data);
raw_spin_unlock_irq(&ctx->lock);
 }
@@ -2309,7 +2316,7 @@ static void perf_set_shadow_time(struct perf_event *event,
struct perf_event *event, *partial_group = NULL;
struct pmu *pmu = ctx->pmu;
 
-   if (group_event->state == PERF_EVENT_STATE_OFF)
+   if (group_event->state <= PERF_EVENT_STATE_OFF)
return 0;
 
pmu->start_txn(pmu, PERF_PMU_TXN_ADD);
@@ -2388,6 +2395,14 @@ static int group_can_go_on(struct perf_event *event,
 static void add_event_to_ctx(struct perf_event *event,
   struct perf_event_context *ctx)
 {
+   if (!ctx->task) {
+   struct perf_cpu_context *cpuctx =
+   container_of(ctx, struct perf_cpu_context, ctx);
+
+   if (!cpuctx->online)
+   event->state = PERF_EVENT_STATE_HOTPLUG_OFFSET;
+   }
+
list_add_event(event, ctx);
perf_group_attach(event);
 }
@@ -2565,11 +2580,6 @@ static int  __perf_install_in_context(void *info)
 */
smp_store_release(&event->ctx, ctx);
 
-   if (!task) {
-   cpu_function_call(cpu,

[PATCH V2 0/1] perf: Add CPU hotplug support for events

[Mukesh Ojha]

The embedded world, specifically Android mobile SoCs, rely on CPU
hotplugs to manage power and thermal constraints. These hotplugs
can happen at a very rapid pace. Adjacently, they also relies on
many perf event counters for its management. Therefore, there is
a need to preserve these events across hotplugs.

In such a scenario, a perf client (kernel or user-space) can create
events even when the CPU is offline. If the CPU comes online during
the lifetime of the event, the registered event can start counting
spontaneously. As an extension to this, the events' count can also
be preserved across CPU hotplugs. This takes the burden off of the
clients to monitor the state of the CPU.

The tests were conducted on arm64 device.
/* CPU-1 is offline: Event created when CPU1 is offline */

/ # cat /sys/devices/system/cpu/cpu1/online
1
/ # echo 0 > /sys/devices/system/cpu/cpu1/online

Test used for testing
#!/bin/sh

chmod +x *

# Count the cycles events on cpu-1 for every 200 ms
./perf stat -e cycles -I 200 -C 1 &

# Make the CPU-1 offline and online continuously
while true; do
sleep 2
echo 0 > /sys/devices/system/cpu/cpu1/online
sleep 2
echo 1 > /sys/devices/system/cpu/cpu1/online
done

Results:
/ # ./test.sh
#   time counts unit events
 0.200145885cycles
 0.410115208cycles
 0.619922551cycles
 0.829904635cycles
 1.039751614cycles
 1.249547603cycles
 1.459228280cycles
 1.665606561cycles
 1.874981926cycles
 2.084297811cycles
 2.293471249cycles
 2.503231561cycles
 2.712993332cycles
 2.922744478cycles
 3.132502186cycles
 3.342255050cycles
 3.552010102cycles
 3.761760363cycles

/* CPU-1 made online: Event started counting */

 3.9714592691925429  cycles
 4.181325206   19391145  cycles
 4.391074164 113894  cycles
 4.5991305193150152  cycles
 4.805564737 487122  cycles
 5.015164581 247533  cycles
 5.224764529 103622  cycles
#   time counts unit events
 5.434360831 238179  cycles
 5.645293799 238895  cycles
 5.854909320 367543  cycles
 6.0644879662383428  cycles

 /* CPU-1 made offline: counting stopped

 6.274289476cycles
 6.483493903cycles
 6.693202705cycles
 6.902956195cycles
 7.112714268cycles
 7.322465570cycles
 7.53340cycles
 7.741975830cycles
 7.951686246cycles

/* CPU-1 made online: Event started counting

 8.161469892   22040750  cycles
 8.371219528 114977  cycles
 8.580979111 259952  cycles
 8.790757132 444661  cycles
 9.000559215 248512  cycles
 9.210385256 246590  cycles
 9.420187704 243819  cycles
 9.6300522877102438  cycles
 9.839848225 337454  cycles
10.049645048 644072  cycles
10.2594762461855410      cycles

Mukesh Ojha (1):
  perf: event preserve and create across cpu hotplug

 include/linux/perf_event.h |   1 +
 kernel/events/core.c   | 122 +
 2 files changed, 79 insertions(+), 44 deletions(-)

-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative 
Project

Re: Perf: Preserving the event across CPU hotunplug/plug and Creation of an event on offine CPU

[Mukesh Ojha]

Friendly Ping.


On 5/23/2019 6:39 PM, Mukesh Ojha wrote:

Hi Peter/All,

This is regarding the discussion happen in the past about 
https://lkml.org/lkml/2018/2/15/1324


Where the exact ask is to allow preserving and creation of events on a 
offline CPU, so that when the CPU

comes online it will start counting.

I had a look at your patch too and resolve crash during while trying 
to create an event on an offline cpu.


In your patch,  you seem to disable event when cpu goes offline which 
is exactly deleting the event

from the pmu and add when it comes online, it seems to  work.

But, For the purpose of allowing the creation of event while CPU is 
offline is not able to count event while

CPU coming online, for that i did some change, that did work.

Also, I have query about the events which gets destroyed while CPU is 
offline and we need to remove them
once cpu comes online right ? As Raghavendra also queried the same in 
the above thread.


Don't we need  a list where we maintain the events which gets 
destroyed while CPU is dead ?

and clean it  up when CPU comes online ?

Thanks.
Mukesh

Perf: Preserving the event across CPU hotunplug/plug and Creation of an event on offine CPU

[Mukesh Ojha]

Hi Peter/All,

This is regarding the discussion happen in the past about 
https://lkml.org/lkml/2018/2/15/1324


Where the exact ask is to allow preserving and creation of events on a 
offline CPU, so that when the CPU

comes online it will start counting.

I had a look at your patch too and resolve crash during while trying to 
create an event on an offline cpu.


In your patch,  you seem to disable event when cpu goes offline which is 
exactly deleting the event

from the pmu and add when it comes online, it seems to  work.

But, For the purpose of allowing the creation of event while CPU is 
offline is not able to count event while

CPU coming online, for that i did some change, that did work.

Also, I have query about the events which gets destroyed while CPU is 
offline and we need to remove them
once cpu comes online right ? As Raghavendra also queried the same in 
the above thread.


Don't we need  a list where we maintain the events which gets destroyed 
while CPU is dead ?

and clean it  up when CPU comes online ?

Thanks.
Mukesh

Re: [PATCH] spi: bitbang: Fix NULL pointer dereference in spi_unregister_master

[Mukesh Ojha]

On 5/16/2019 1:26 PM, YueHaibing wrote:

If spi_register_master fails in spi_bitbang_start
because device_add failure, We should return the
error code other than 0, otherwise calling
spi_bitbang_stop may trigger NULL pointer dereference
like this:

BUG: KASAN: null-ptr-deref in __list_del_entry_valid+0x45/0xd0
Read of size 8 at addr  by task syz-executor.0/3661

CPU: 0 PID: 3661 Comm: syz-executor.0 Not tainted 5.1.0+ #28
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 
04/01/2014
Call Trace:
  dump_stack+0xa9/0x10e
  ? __list_del_entry_valid+0x45/0xd0
  ? __list_del_entry_valid+0x45/0xd0
  __kasan_report+0x171/0x18d
  ? __list_del_entry_valid+0x45/0xd0
  kasan_report+0xe/0x20
  __list_del_entry_valid+0x45/0xd0
  spi_unregister_controller+0x99/0x1b0
  spi_lm70llp_attach+0x3ae/0x4b0 [spi_lm70llp]
  ? 0xc1128000
  ? klist_next+0x131/0x1e0
  ? driver_detach+0x40/0x40 [parport]
  port_check+0x3b/0x50 [parport]
  bus_for_each_dev+0x115/0x180
  ? subsys_dev_iter_exit+0x20/0x20
  __parport_register_driver+0x1f0/0x210 [parport]
  ? 0xc115
  do_one_initcall+0xb9/0x3b5
  ? perf_trace_initcall_level+0x270/0x270
  ? kasan_unpoison_shadow+0x30/0x40
  ? kasan_unpoison_shadow+0x30/0x40
  do_init_module+0xe0/0x330
  load_module+0x38eb/0x4270
  ? module_frob_arch_sections+0x20/0x20
  ? kernel_read_file+0x188/0x3f0
  ? find_held_lock+0x6d/0xd0
  ? fput_many+0x1a/0xe0
  ? __do_sys_finit_module+0x162/0x190
  __do_sys_finit_module+0x162/0x190
  ? __ia32_sys_init_module+0x40/0x40
  ? __mutex_unlock_slowpath+0xb4/0x3f0
  ? wait_for_completion+0x240/0x240
  ? vfs_write+0x160/0x2a0
  ? lockdep_hardirqs_off+0xb5/0x100
  ? mark_held_locks+0x1a/0x90
  ? do_syscall_64+0x14/0x2a0
  do_syscall_64+0x72/0x2a0
  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Reported-by: Hulk Robot 
Fixes: 702a4879ec33 ("spi: bitbang: Let spi_bitbang_start() take a reference to 
master")
Signed-off-by: YueHaibing 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  drivers/spi/spi-bitbang.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index dd9a8c54..be95be4 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -403,7 +403,7 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
if (ret)
spi_master_put(master);
  
-	return 0;

+   return ret;
  }
  EXPORT_SYMBOL_GPL(spi_bitbang_start);
  

Re: [PATCH v3 1/1] bsr: do not use assignment in if condition

[Mukesh Ojha]

On 5/15/2019 9:47 PM, parna.naveenku...@gmail.com wrote:

From: Naveen Kumar Parna 

checkpatch.pl does not like assignment in if condition

Signed-off-by: Naveen Kumar Parna 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
Changes in v3:
The first patch has an extra space in if statement, so fixed it in v2 but
forgot add what changed from the previous version. In v3 added the
complete change history.

  drivers/char/bsr.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/bsr.c b/drivers/char/bsr.c
index a6cef548e01e..70de334554a8 100644
--- a/drivers/char/bsr.c
+++ b/drivers/char/bsr.c
@@ -322,7 +322,8 @@ static int __init bsr_init(void)
goto out_err_2;
}
  
-	if ((ret = bsr_create_devs(np)) < 0) {

+   ret = bsr_create_devs(np);
+   if (ret < 0) {
np = NULL;
goto out_err_3;
}

Re: [PATCH] objtool: Allow AR to be overridden with HOSTAR

[Mukesh Ojha]

On 5/15/2019 4:10 AM, Nathan Chancellor wrote:

Currently, this Makefile hardcodes GNU ar, meaning that if it is not
available, there is no way to supply a different one and the build will
fail.

$ make AR=llvm-ar CC=clang LD=ld.lld HOSTAR=llvm-ar HOSTCC=clang \
HOSTLD=ld.lld HOSTLDFLAGS=-fuse-ld=lld defconfig modules_prepare
...
   AR   /out/tools/objtool/libsubcmd.a
/bin/sh: 1: ar: not found
...

Follow the logic of HOST{CC,LD} and allow the user to specify a
different ar tool via HOSTAR (which is used elsewhere in other
tools/ Makefiles).

Link: https://github.com/ClangBuiltLinux/linux/issues/481
Signed-off-by: Nathan Chancellor 



Nice catch.
Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  tools/objtool/Makefile | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 53f8be0f4a1f..88158239622b 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -7,11 +7,12 @@ ARCH := x86
  endif
  
  # always use the host compiler

+HOSTAR ?= ar
  HOSTCC?= gcc
  HOSTLD?= ld
+AR  = $(HOSTAR)
  CC = $(HOSTCC)
  LD = $(HOSTLD)
-AR  = ar
  
  ifeq ($(srctree),)

  srctree := $(patsubst %/,%,$(dir $(CURDIR)))

Re: [PATCH] drm/nouveau/bios/init: fix spelling mistake "CONDITON" -> "CONDITION"

[Mukesh Ojha]

On 5/15/2019 2:27 AM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake in a warning message. Fix it.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c 
b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c
index ec0e9f7224b5..3f4f27d191ae 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c
@@ -834,7 +834,7 @@ init_generic_condition(struct nvbios_init *init)
init_exec_set(init, false);
break;
default:
-   warn("INIT_GENERIC_CONDITON: unknown 0x%02x\n", cond);
+   warn("INIT_GENERIC_CONDITION: unknown 0x%02x\n", cond);
init->offset += size;
break;
}

Re: [PATCH] driver core: Fix use-after-free and double free on glue directory

[Mukesh Ojha]

++

On 5/4/2019 8:17 PM, Muchun Song wrote:

Benjamin Herrenschmidt  于2019年5月2日周四 下午2:25写道：


The basic idea yes, the whole bool *locked is horrid though.
Wouldn't it
work to have a get_device_parent_locked that always returns with
the mutex held,
or just move the mutex to the caller or something simpler like this
?


Greg and Rafael, do you have any suggestions for this? Or you also
agree with Ben?

Ping guys ? This is worth fixing...

I also agree with you. But Greg and Rafael seem to be high latency right now.

 From your suggestions, I think introduce get_device_parent_locked() may easy
to fix. So, do you agree with the fix of the following code snippet
(You can also
view attachments)?

I introduce a new function named get_device_parent_locked_if_glue_dir() which
always returns with the mutex held only when we live in glue dir. We should call
unlock_if_glue_dir() to release the mutex. The
get_device_parent_locked_if_glue_dir()
and unlock_if_glue_dir() should be called in pairs.

---
drivers/base/core.c | 44 
1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4aeaa0c92bda..5112755c43fa 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -1739,8 +1739,9 @@ class_dir_create_and_add(struct class *class,
struct kobject *parent_kobj)
static DEFINE_MUTEX(gdp_mutex);
-static struct kobject *get_device_parent(struct device *dev,
-struct device *parent)
+static struct kobject *__get_device_parent(struct device *dev,
+struct device *parent,
+bool lock)
{
if (dev->class) {
struct kobject *kobj = NULL;
@@ -1779,14 +1780,16 @@ static struct kobject
*get_device_parent(struct device *dev,
}
spin_unlock(&dev->class->p->glue_dirs.list_lock);
if (kobj) {
-   mutex_unlock(&gdp_mutex);
+   if (!lock)
+   mutex_unlock(&gdp_mutex);
return kobj;
}
/* or create a new class-directory at the parent device */
k = class_dir_create_and_add(dev->class, parent_kobj);
/* do not emit an uevent for this simple "glue" directory */
-   mutex_unlock(&gdp_mutex);
+   if (!lock)
+   mutex_unlock(&gdp_mutex);
return k;
}
@@ -1799,6 +1802,19 @@ static struct kobject *get_device_parent(struct
device *dev,
return NULL;
}
+static inline struct kobject *get_device_parent(struct device *dev,
+   struct device *parent)
+{
+   return __get_device_parent(dev, parent, false);
+}
+
+static inline struct kobject *
+get_device_parent_locked_if_glue_dir(struct device *dev,
+struct device *parent)
+{
+   return __get_device_parent(dev, parent, true);
+}
+
static inline bool live_in_glue_dir(struct kobject *kobj,
 struct device *dev)
{
@@ -1831,6 +1847,16 @@ static void cleanup_glue_dir(struct device
*dev, struct kobject *glue_dir)
mutex_unlock(&gdp_mutex);
}
+static inline void unlock_if_glue_dir(struct device *dev,
+struct kobject *glue_dir)
+{
+   /* see if we live in a "glue" directory */
+   if (!live_in_glue_dir(glue_dir, dev))
+   return;
+
+   mutex_unlock(&gdp_mutex);
+}
+
static int device_add_class_symlinks(struct device *dev)
{
struct device_node *of_node = dev_of_node(dev);
@@ -2040,7 +2066,7 @@ int device_add(struct device *dev)
pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
parent = get_device(dev->parent);
-   kobj = get_device_parent(dev, parent);
+   kobj = get_device_parent_locked_if_glue_dir(dev, parent);
if (IS_ERR(kobj)) {
error = PTR_ERR(kobj);
goto parent_error;
@@ -2055,10 +2081,12 @@ int device_add(struct device *dev)
/* first, register with generic layer. */
/* we require the name to be set before, and pass NULL */
error = kobject_add(&dev->kobj, dev->kobj.parent, NULL);
-   if (error) {
-   glue_dir = get_glue_dir(dev);
+
+   glue_dir = get_glue_dir(dev);
+   unlock_if_glue_dir(dev, glue_dir);
+
+   if (error)
goto Error;
-   }
/* notify platform of device entry */
error = device_platform_notify(dev, KOBJ_ADD);
--

Re: [PATCH][next] ASoC: SOF: Intel: fix spelling mistake "incompatble" -> "incompatible"

[Mukesh Ojha]

On 5/1/2019 3:53 PM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake in a hda_dsp_rom_msg message, fix it.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  sound/soc/sof/intel/hda.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index b8fc19790f3b..84baf275b467 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -54,7 +54,7 @@ static const struct hda_dsp_msg_code hda_dsp_rom_msg[] = {
{HDA_DSP_ROM_L2_CACHE_ERROR, "error: L2 cache error"},
{HDA_DSP_ROM_LOAD_OFFSET_TO_SMALL, "error: load offset too small"},
{HDA_DSP_ROM_API_PTR_INVALID, "error: API ptr invalid"},
-   {HDA_DSP_ROM_BASEFW_INCOMPAT, "error: base fw incompatble"},
+   {HDA_DSP_ROM_BASEFW_INCOMPAT, "error: base fw incompatible"},
{HDA_DSP_ROM_UNHANDLED_INTERRUPT, "error: unhandled interrupt"},
{HDA_DSP_ROM_MEMORY_HOLE_ECC, "error: ECC memory hole"},
{HDA_DSP_ROM_KERNEL_EXCEPTION, "error: kernel exception"},

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Mukesh Ojha]

Sorry to come late on this

On 4/25/2019 4:26 AM, Al Viro wrote:

On Wed, Apr 24, 2019 at 07:39:03PM +0530, Mukesh Ojha wrote:


This was my simple program no multithreading just to understand f_counting

int main()
{
     int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
     ioctl(fd, UI_SET_EVBIT, EV_KEY);
     close(fd);
     return 0;
}

            uinput-532   [002]     45.312044: SYSC_ioctl: 2   <= f_count

Er...  So how does it manage to hit ioctl(2) before open(2)?  Confused...


I was confused too about this earlier, but after printing fd got to know 
this is not for the same fd
opening for /dev/uinput, may it is for something while running the 
executable.





     
   uinput-532   [002]     45.312055: SYSC_ioctl: 2
>>>                         /* All the above calls happened for the
open() in userspace*/

   uinput-532   [004]     45.313783: SYSC_ioctl: 1 /* This print
is for the trace, i put after fdget */
   uinput-532   [004]     45.313788: uinput_ioctl_handler:
uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl
driver */

   uinput-532   [004]     45.313835: SYSC_ioctl: 1 /* This print
is for the trace, i put after fdput*/
   uinput-532   [004]     45.313843: uinput_release: uinput:  0
/* And this is from the close()  */


Should fdget not suppose to increment the f_count here, as it is coming 1 ?
This f_count to one is done at the open, but i have no idea how this  below
f_count 2 came before open() for
this simple program.

If descriptor table is not shared, fdget() will simply return you the reference
from there, without bothering to bump the refcount.  _And_ having it marked
"don't drop refcount" in struct fd.

Rationale: since it's not shared, nobody other than our process can modify
it.  So unless we remove (and drop) the reference from it ourselves (which
we certainly have no business doing in ->ioctl() and do not do anywhere
in drivers/input), it will remain there until we return from syscall.

Nobody is allowed to modify descriptor table other than their own.
And if it's not shared, no other owners can appear while the only
existing one is in the middle of syscall other than clone() (with
CLONE_FILES in flags, at that).

For shared descriptor table fdget() bumps file refcount, since there
the reference in descriptor table itself could be removed and dropped
by another thread.

And fdget() marks whether fput() is needed in fd.flags, so that
fdput() does the right thing.



Thanks Al, it is quite clear that issue can't happen while a ioctl is in 
progress.

Actually the issue seems to be a race while glue_dir input is removed.

  114.339374] input: syz1 as /devices/virtual/input/input278
[  114.345619] input: syz1 as /devices/virtual/input/input279
[  114.353502] input: syz1 as /devices/virtual/input/input280
[  114.361907] input: syz1 as /devices/virtual/input/input281
[  114.367276] input: syz1 as /devices/virtual/input/input282
[  114.382292] input: syz1 as /devices/virtual/input/input283

in our case it is input which is getting removed while a inputxx is 
trying make node inside input.


Similar issue https://lkml.org/lkml/2019/5/1/3

Thanks,
Mukesh

Re: [PATCH] firmware_loader: Fix a typo ("syfs" -> "sysfs")

[Mukesh Ojha]

On 4/30/2019 8:26 PM, Jonathan Neuschäfer wrote:

"sysfs" was misspelled in a comment and a log message.

Signed-off-by: Jonathan Neuschäfer 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  drivers/base/firmware_loader/fallback.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b5c865fe263b..f962488546b6 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -674,8 +674,8 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
   *
   * This function is called if direct lookup for the firmware failed, it 
enables
   * a fallback mechanism through userspace by exposing a sysfs loading
- * interface. Userspace is in charge of loading the firmware through the syfs
- * loading interface. This syfs fallback mechanism may be disabled completely
+ * interface. Userspace is in charge of loading the firmware through the sysfs
+ * loading interface. This sysfs fallback mechanism may be disabled completely
   * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
   * If this false we check if the internal API caller set the 
@FW_OPT_NOFALLBACK
   * flag, if so it would also disable the fallback mechanism. A system may want
@@ -693,7 +693,7 @@ int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
return ret;

if (!(opt_flags & FW_OPT_NO_WARN))
-   dev_warn(device, "Falling back to syfs fallback for: %s\n",
+   dev_warn(device, "Falling back to sysfs fallback for: %s\n",
 name);
else
dev_dbg(device, "Falling back to sysfs fallback for: %s\n",
--
2.20.1

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Mukesh Ojha]

On 4/24/2019 6:37 PM, Al Viro wrote:

On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote:

Al,

i tried to put traceprintk inside ioctl after fdget and fdput on a simple
call of open  => ioctl => close

in a loop, and multithreaded, presumably?


on /dev/uinput.

   uinput-532   [002]     45.312044: SYSC_ioctl: 2     <= f_count

     
   uinput-532   [002]     45.312055: SYSC_ioctl: 2

Look at ksys_ioctl():
int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
 int error;
 struct fd f = fdget(fd);
an error or refcount bumped
 if (!f.file)
 return -EBADF;
not an error, then.  We know that ->release() won't be called
until we drop the reference we've just acquired.
 error = security_file_ioctl(f.file, cmd, arg);
 if (!error)
 error = do_vfs_ioctl(f.file, fd, cmd, arg);
... and we are done with calling ->ioctl(), so
 fdput(f);
... we drop the reference we'd acquired.

Seeing refcount 1 inside ->ioctl() is possible, all right:

CPU1: ioctl(2) resolves fd to struct file *, refcount 2
CPU2: close(2) rips struct file * from descriptor table and does fput() to drop 
it;
refcount reaches 1 and fput() is done; no call of ->release() yet.
CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1
CPU1: done with ->ioctl(), drop our reference.  *NOW* refcount gets to 0, and
->release() is called.


Thanks for the detail reply, Al

This was my simple program no multithreading just to understand f_counting

int main()
{
    int fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
    ioctl(fd, UI_SET_EVBIT, EV_KEY);
    close(fd);
    return 0;
}

           uinput-532   [002]     45.312044: SYSC_ioctl: 2   <= 
f_count >      uinput-532   [002]     45.312055: SYSC_ioctl: 
2      uinput-532   [004]     45.313766: uinput_open: uinput: 
1   /* This is from the uinput driver uinput_open()*/


  =>>>>                         /* All the above calls happened for the 
open() in userspace*/


  uinput-532   [004]     45.313783: SYSC_ioctl: 1 /* This 
print is for the trace, i put after fdget */
  uinput-532   [004]     45.313788: uinput_ioctl_handler: 
uinput: uinput_ioctl_handler, 1 /* This print is from the uinput_ioctl 
driver */


  uinput-532   [004]     45.313835: SYSC_ioctl: 1 /* This 
print is for the trace, i put after fdput*/
  uinput-532   [004]     45.313843: uinput_release: 
uinput:  0 /* And this is from the close()  */



Should fdget not suppose to increment the f_count here, as it is coming 1 ?
This f_count to one is done at the open, but i have no idea how this  
below f_count 2 came before open() for

this simple program.

         uinput-532   [002]     45.312044: SYSC_ioctl: 2 <= f_count 
>      uinput-532   [002]     45.312055: SYSC_ioctl: 
2    

-Mukesh


IOW, in your trace fput() has already been run by close(2); having somebody else
do that again while we are in ->ioctl() would be a bug (to start with, where
did they get that struct file * and why wasn't that reference contributing to
struct file refcount?)

In all cases we only call ->release() once all references gone - both
the one(s) in descriptor tables and any transient ones acquired by
fdget(), etc.

I would really like to see a reproducer for the original use-after-free 
report...

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Mukesh Ojha]

On 4/23/2019 4:36 PM, Al Viro wrote:

On Tue, Apr 23, 2019 at 08:49:44AM +, dmitry.torok...@gmail.com wrote:

On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:

I have taken care this case from ioctl and release point of view.

Even if the release gets called first it will make the
file->private_data=NULL.
and further call to ioctl will not be a problem as the check is already
there.

Al, do we have any protections in VFS layer from userspace hanging onto
a file descriptor and calling ioctl() on it even as another thread
calls close() on the same fd?

Should the issue be solved by individual drivers, or more careful
accounting for currently running operations is needed at VFS layer?

Neither.  An overlap of ->release() and ->ioctl() is possible only
if you've got memory corruption somewhere.

close() overlapping ioctl() is certainly possible, and won't trigger
that at all - sys_ioctl() holds onto reference to struct file, so
its refcount won't reach zero until we are done with it.


Al,

i tried to put traceprintk inside ioctl after fdget and fdput on a 
simple call of open  => ioctl => close

on /dev/uinput.

  uinput-532   [002]     45.312044: SYSC_ioctl: 2     <= 
f_count >      uinput-532   [002]     45.312055: SYSC_ioctl: 
2    
  uinput-532   [004]     45.313766: uinput_open: uinput: 1
  uinput-532   [004]     45.313783: SYSC_ioctl: 1
  uinput-532   [004]     45.313788: uinput_ioctl_handler: 
uinput: uinput_ioctl_handler, 1

  uinput-532   [004]     45.313835: SYSC_ioctl: 1
  uinput-532   [004]     45.313843: uinput_release: uinput:  0


So while a ioctl is running the f_count is 1, so a fput could be run and 
do atomic_long_dec_and_test

this could call release right ?

-Mukesh

Re: [PATCH -next] staging: kpc2000: remove duplicated include from kp2000_module.c

[Mukesh Ojha]

On 4/24/2019 8:20 AM, YueHaibing wrote:

Remove duplicated include.

Signed-off-by: YueHaibing 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  drivers/staging/kpc2000/kpc2000/kp2000_module.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/staging/kpc2000/kpc2000/kp2000_module.c 
b/drivers/staging/kpc2000/kpc2000/kp2000_module.c
index 661b0b74ed66..fa3bd266ba54 100644
--- a/drivers/staging/kpc2000/kpc2000/kp2000_module.c
+++ b/drivers/staging/kpc2000/kpc2000/kp2000_module.c
@@ -5,7 +5,6 @@
  #include 
  #include 
  #include 
-#include 
  #include 
  #include 
  #include 

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[Mukesh Ojha]

On 4/19/2019 12:41 PM, dmitry.torok...@gmail.com wrote:

Hi Mukesh,

On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:

For some reason my last mail did not get delivered,  sending it again.


On 4/18/2019 11:55 AM, Mukesh Ojha wrote:


On 4/18/2019 7:13 AM, dmitry.torok...@gmail.com wrote:

Hi Mukesh,

On Mon, Apr 15, 2019 at 03:35:51PM +0530, Mukesh Ojha wrote:

Hi Dmitry,

Can you please have a look at this patch ? as this seems to reproducing
quite frequently

Thanks,
Mukesh

On 4/10/2019 1:29 PM, Mukesh Ojha wrote:

uinput_destroy_device() gets called from two places. In one place,
uinput_ioctl_handler() where it is protected under a lock
udev->mutex but there is no protection on udev device from freeing
inside uinput_release().

uinput_release() should be called when last file handle to the uinput
instance is being dropped, so there should be no other users and thus we
can't be racing with anyone.

Lets say an example where i am creating input device quite frequently

[   97.836603] input: syz0 as /devices/virtual/input/input262
[   97.845589] input: syz0 as /devices/virtual/input/input261
[   97.849415] input: syz0 as /devices/virtual/input/input263
[   97.856479] input: syz0 as /devices/virtual/input/input264
[   97.936128] input: syz0 as /devices/virtual/input/input265

e.g input265

while input265 gets created [1] and handlers are getting registered on
that device*fput* gets called on
that device as user space got to know that input265 is created and its
reference is still 1(rare but possible).

Are you saying that there are 2 threads sharing the same file
descriptor, one issuing the registration ioctl while the other closing
the same fd?


Dmitry,

I don't have a the exact look inside the app here, but this looks like 
the same as it is able to do

fput on the uinput device.

FYI
Syskaller app is running in userspace (which is for syscall fuzzing) on 
kernel which is enabled
with various config fault injection, FAULT_INJECTION,FAIL_SLAB, 
FAIL_PAGEALLOC, KASAN etc.


Thanks,
Mukesh



Thanks.

Re: [PATCH] ARM: kprobes: fix spelling mistake "undefeined" -> "undefined"

[Mukesh Ojha]

On 4/18/2019 11:08 PM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake on the TEST_UNSUPPORTED macro. Fix this.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  arch/arm/probes/kprobes/test-thumb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/probes/kprobes/test-thumb.c 
b/arch/arm/probes/kprobes/test-thumb.c
index b683b4517458..6841909757cd 100644
--- a/arch/arm/probes/kprobes/test-thumb.c
+++ b/arch/arm/probes/kprobes/test-thumb.c
@@ -787,7 +787,7 @@ CONDITION_INSTRUCTIONS(22,
  
  	TEST_UNSUPPORTED(__inst_thumb32(0xf7f08000) " @ smc #0")
  
-	TEST_UNSUPPORTED(__inst_thumb32(0xf7f0a000) " @ undefeined")

+   TEST_UNSUPPORTED(__inst_thumb32(0xf7f0a000) " @ undefined")
  
  	TEST_BF(  "b.w	2f")

TEST_BB(  "b.w 2b")

Re: [PATCH] staging: rtl8723bs: hal: fix spelling mistake "singal" -> "signal"

[Mukesh Ojha]

On 4/18/2019 5:50 PM, Colin King wrote:

From: Colin Ian King 

There are multiple spelling mistakes in variable names, fix these.

Signed-off-by: Colin Ian King 



Well, this one a bit sensitive to touch.
Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  drivers/staging/rtl8723bs/hal/hal_com.c  | 18 +-
  drivers/staging/rtl8723bs/include/rtw_recv.h |  4 ++--
  2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/hal_com.c 
b/drivers/staging/rtl8723bs/hal/hal_com.c
index f1c91483ca07..e5f1153527b9 100644
--- a/drivers/staging/rtl8723bs/hal/hal_com.c
+++ b/drivers/staging/rtl8723bs/hal/hal_com.c
@@ -1618,14 +1618,14 @@ void rtw_get_raw_rssi_info(void *sel, struct adapter 
*padapter)
isCCKrate = psample_pkt_rssi->data_rate <= DESC_RATE11M;
  
  	if (isCCKrate)

-   psample_pkt_rssi->mimo_singal_strength[0] = 
psample_pkt_rssi->pwdball;
+   psample_pkt_rssi->mimo_signal_strength[0] = 
psample_pkt_rssi->pwdball;
  
  	for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {

DBG_871X_SEL_NL(
sel,
-   "RF_PATH_%d =>singal_strength:%d(%%), 
singal_quality:%d(%%)\n",
-   rf_path, 
psample_pkt_rssi->mimo_singal_strength[rf_path],
-   psample_pkt_rssi->mimo_singal_quality[rf_path]
+   "RF_PATH_%d =>signal_strength:%d(%%), 
signal_quality:%d(%%)\n",
+   rf_path, 
psample_pkt_rssi->mimo_signal_strength[rf_path],
+   psample_pkt_rssi->mimo_signal_quality[rf_path]
);
  
  		if (!isCCKrate) {

@@ -1651,11 +1651,11 @@ void rtw_dump_raw_rssi_info(struct adapter *padapter)
isCCKrate = psample_pkt_rssi->data_rate <= DESC_RATE11M;
  
  	if (isCCKrate)

-   psample_pkt_rssi->mimo_singal_strength[0] = 
psample_pkt_rssi->pwdball;
+   psample_pkt_rssi->mimo_signal_strength[0] = 
psample_pkt_rssi->pwdball;
  
  	for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {

-   DBG_871X("RF_PATH_%d =>singal_strength:%d(%%), 
singal_quality:%d(%%)"
-   , rf_path, 
psample_pkt_rssi->mimo_singal_strength[rf_path], 
psample_pkt_rssi->mimo_singal_quality[rf_path]);
+   DBG_871X("RF_PATH_%d =>signal_strength:%d(%%), 
signal_quality:%d(%%)"
+   , rf_path, 
psample_pkt_rssi->mimo_signal_strength[rf_path], 
psample_pkt_rssi->mimo_signal_quality[rf_path]);
  
  		if (!isCCKrate) {

printk(", rx_ofdm_pwr:%d(dBm), rx_ofdm_snr:%d(dB)\n",
@@ -1682,8 +1682,8 @@ void rtw_store_phy_info(struct adapter *padapter, union 
recv_frame *prframe)
psample_pkt_rssi->pwr_all = pPhyInfo->recv_signal_power;
  
  	for (rf_path = 0; rf_path < pHalData->NumTotalRFPath; rf_path++) {

-   psample_pkt_rssi->mimo_singal_strength[rf_path] = 
pPhyInfo->rx_mimo_signal_strength[rf_path];
-   psample_pkt_rssi->mimo_singal_quality[rf_path] = 
pPhyInfo->rx_mimo_signal_quality[rf_path];
+   psample_pkt_rssi->mimo_signal_strength[rf_path] = 
pPhyInfo->rx_mimo_signal_strength[rf_path];
+   psample_pkt_rssi->mimo_signal_quality[rf_path] = 
pPhyInfo->rx_mimo_signal_quality[rf_path];
if (!isCCKrate) {
psample_pkt_rssi->ofdm_pwr[rf_path] = 
pPhyInfo->RxPwr[rf_path];
psample_pkt_rssi->ofdm_snr[rf_path] = 
pPhyInfo->RxSNR[rf_path];
diff --git a/drivers/staging/rtl8723bs/include/rtw_recv.h 
b/drivers/staging/rtl8723bs/include/rtw_recv.h
index 7d54cf211315..5de946e66302 100644
--- a/drivers/staging/rtl8723bs/include/rtw_recv.h
+++ b/drivers/staging/rtl8723bs/include/rtw_recv.h
@@ -120,8 +120,8 @@ struct rx_raw_rssi
u8 pwdball;
s8 pwr_all;
  
-	u8 mimo_singal_strength[4];/*  in 0~100 index */

-   u8 mimo_singal_quality[4];
+   u8 mimo_signal_strength[4];/*  in 0~100 index */
+   u8 mimo_signal_quality[4];
  
  	s8 ofdm_pwr[4];

u8 ofdm_snr[4];

Re: [PATCH] char/ipmi: fix spelling mistake "receieved_messages" -> "received_messages"

[Mukesh Ojha]

On 4/18/2019 3:45 PM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake in the documentation. Fix it.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  Documentation/ABI/testing/sysfs-devices-platform-ipmi | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-devices-platform-ipmi 
b/Documentation/ABI/testing/sysfs-devices-platform-ipmi
index 2a781e7513b7..afb5db856e1c 100644
--- a/Documentation/ABI/testing/sysfs-devices-platform-ipmi
+++ b/Documentation/ABI/testing/sysfs-devices-platform-ipmi
@@ -212,7 +212,7 @@ Description:
Messages may be broken into parts if
they are long.
  
-		receieved_messages:	(RO) Number of message responses

+   received_messages:  (RO) Number of message responses
received.
  
  		received_message_parts: (RO) Number of message fragments

Re: stm class: Fix possible double free

[Mukesh Ojha]

On 4/18/2019 12:52 PM, Pan Bian wrote:

The function stm_register_device() calls put_device(&stm->dev) to
release allocated memory (in stm_device_release()) on error paths.
However, after that, the freed memory stm is released again, resulting
in a double free bug. There is a similar issue in the function
stm_source_register_device. This patch fixes these issues.

Signed-off-by: Pan Bian 



Looks good to me.
Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  drivers/hwtracing/stm/core.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
index c7ba8ac..cfb5c4d 100644
--- a/drivers/hwtracing/stm/core.c
+++ b/drivers/hwtracing/stm/core.c
@@ -886,8 +886,10 @@ int stm_register_device(struct device *parent, struct 
stm_data *stm_data,
return -ENOMEM;
  
  	stm->major = register_chrdev(0, stm_data->name, &stm_fops);

-   if (stm->major < 0)
-   goto err_free;
+   if (stm->major < 0) {
+   vfree(stm);
+   return err;
+   }
  
  	device_initialize(&stm->dev);

stm->dev.devt = MKDEV(stm->major, 0);
@@ -933,8 +935,6 @@ int stm_register_device(struct device *parent, struct 
stm_data *stm_data,
  
  	/* matches device_initialize() above */

put_device(&stm->dev);
-err_free:
-   vfree(stm);
  
  	return err;

  }
@@ -1277,7 +1277,6 @@ int stm_source_register_device(struct device *parent,
  
  err:

put_device(&src->dev);
-   kfree(src);
  
  	return err;

  }

Re: [PATCH] staging: rtl8723bs: fix spelling mistake: "nonprintabl" -> "non-printable"

[Mukesh Ojha]

On 4/17/2019 5:30 PM, Colin King wrote:

From: Colin Ian King 

There is a spelling mistake in an RT_TRACE message, fix it.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c 
b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 0c8a050b2a81..bd75bca1ac6e 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -44,7 +44,7 @@ u8 rtw_validate_ssid(struct ndis_802_11_ssid *ssid)
for (i = 0; i < ssid->SsidLength; i++) {
/* wifi, printable ascii code must be supported */
if (!((ssid->Ssid[i] >= 0x20) && (ssid->Ssid[i] <= 0x7e))) {
-   RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_, ("ssid has 
nonprintabl ascii\n"));
+   RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_err_, ("ssid has 
non-printable ascii\n"));
ret = false;
break;
}

Re: [PATCH] staging: rtlwifi: fix spelling mistake "notity" -> "notify"

[Mukesh Ojha]

On 4/17/2019 5:38 PM, Colin King wrote:

From: Colin Ian King 

There are two spelling mistake in RT_TRACE messages. Fix them.

Signed-off-by: Colin Ian King 

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh

---
  drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c 
b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
index ade271cb4aab..32c797430512 100644
--- a/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
+++ b/drivers/staging/rtlwifi/btcoexist/halbtc8822b1ant.c
@@ -4730,7 +4730,7 @@ void ex_btc8822b1ant_media_status_notify(struct 
btc_coexist *btcoexist, u8 type)
if (wifi_under_b_mode) {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
-   "[BTCoex], ** (media status notity under b 
mode) **\n");
+   "[BTCoex], ** (media status notify under b 
mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
   0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,
@@ -4738,7 +4738,7 @@ void ex_btc8822b1ant_media_status_notify(struct 
btc_coexist *btcoexist, u8 type)
} else {
RT_TRACE(
rtlpriv, COMP_BT_COEXIST, DBG_LOUD,
-   "[BTCoex], ** (media status notity not under 
b mode) **\n");
+   "[BTCoex], ** (media status notify not under 
b mode) **\n");
btcoexist->btc_write_1byte(btcoexist, 0x6cd,
   0x00); /* CCK Tx */
btcoexist->btc_write_1byte(btcoexist, 0x6cf,

Re: [PATCH] perf test: fix spelling mistake "leadking" -> "leaking"

[Mukesh Ojha]

On 4/17/2019 4:25 PM, Colin King wrote:

From: Colin Ian King 

There are a couple of spelling mistakes in test assert messages. Fix them.

Signed-off-by: Colin Ian King 


Well, how are you shooting these mistakes one after the other?

Reviewed-by: Mukesh Ojha 

Cheers,
-Mukesh


---
  tools/perf/tests/dso-data.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/dso-data.c b/tools/perf/tests/dso-data.c
index 7f6c52021e41..946ab4b63acd 100644
--- a/tools/perf/tests/dso-data.c
+++ b/tools/perf/tests/dso-data.c
@@ -304,7 +304,7 @@ int test__dso_data_cache(struct test *test __maybe_unused, 
int subtest __maybe_u
/* Make sure we did not leak any file descriptor. */
nr_end = open_files_cnt();
pr_debug("nr start %ld, nr stop %ld\n", nr, nr_end);
-   TEST_ASSERT_VAL("failed leadking files", nr == nr_end);
+   TEST_ASSERT_VAL("failed leaking files", nr == nr_end);
return 0;
  }
  
@@ -380,6 +380,6 @@ int test__dso_data_reopen(struct test *test __maybe_unused, int subtest __maybe_

/* Make sure we did not leak any file descriptor. */
nr_end = open_files_cnt();
pr_debug("nr start %ld, nr stop %ld\n", nr, nr_end);
-   TEST_ASSERT_VAL("failed leadking files", nr == nr_end);
+   TEST_ASSERT_VAL("failed leaking files", nr == nr_end);
return 0;
  }