Re: [PATCH] remoteproc: Create a separate workqueue for recovery tasks

[rishabhb]

On 2020-12-21 16:35, Bjorn Andersson wrote:

On Thu 17 Dec 12:49 CST 2020, Alex Elder wrote:


On 12/17/20 12:21 PM, risha...@codeaurora.org wrote:
> On 2020-12-17 08:12, Alex Elder wrote:
> > On 12/15/20 4:55 PM, Bjorn Andersson wrote:
> > > On Sat 12 Dec 14:48 CST 2020, Rishabh Bhatnagar wrote:
> > >
> > > > Create an unbound high priority workqueue for recovery tasks.
> >
> > I have been looking at a different issue that is caused by
> > crash notification.
> >
> > What happened was that the modem crashed while the AP was
> > in system suspend (or possibly even resuming) state.  And
> > there is no guarantee that the system will have called a
> > driver's ->resume callback when the crash notification is
> > delivered.
> >
> > In my case (in the IPA driver), handling a modem crash
> > cannot be done while the driver is suspended; i.e. the
> > activities in its ->resume callback must be completed
> > before we can recover from the crash.
> >
> > For this reason I might like to change the way the
> > crash notification is handled, but what I'd rather see
> > is to have the work queue not run until user space
> > is unfrozen, which would guarantee that all drivers
> > that have registered for a crash notification will
> > be resumed when the notification arrives.
> >
> > I'm not sure how that interacts with what you are
> > looking for here.  I think the workqueue could still
> > be unbound, but its work would be delayed longer before
> > any notification (and recovery) started.
> >
> >     -Alex
> >
> >
> In that case, maybe adding a "WQ_FREEZABLE" flag might help?

Yes, exactly.  But how does that affect whatever you were
trying to do with your patch?



I don't see any impact on Rishabh's change in particular, syntactically
it would just be a matter of adding another flag and the impact would 
be

separate from his patch.

In other words, creating a separate work queue to get the long running
work off the system_wq and making sure that these doesn't run during
suspend & resume seems very reasonable to me.

The one piece that I'm still contemplating is the HIPRIO, I would like
to better understand the actual impact - or perhaps is this a result of
everyone downstream moving all their work to HIPRIO work queues,
starving the recovery?


Hi Bjorn,
You are right, this is a result of downstream having HIPRIO workqueues
therefore starving recovery. I don't have actual data to support the 
flag

as of now. If needed for now we can skip this flag and add it later with
sufficient data?

Regards,
Bjorn

Re: [PATCH] remoteproc: Create a separate workqueue for recovery tasks

[rishabhb]

On 2020-12-17 08:12, Alex Elder wrote:

On 12/15/20 4:55 PM, Bjorn Andersson wrote:

On Sat 12 Dec 14:48 CST 2020, Rishabh Bhatnagar wrote:


Create an unbound high priority workqueue for recovery tasks.


I have been looking at a different issue that is caused by
crash notification.

What happened was that the modem crashed while the AP was
in system suspend (or possibly even resuming) state.  And
there is no guarantee that the system will have called a
driver's ->resume callback when the crash notification is
delivered.

In my case (in the IPA driver), handling a modem crash
cannot be done while the driver is suspended; i.e. the
activities in its ->resume callback must be completed
before we can recover from the crash.

For this reason I might like to change the way the
crash notification is handled, but what I'd rather see
is to have the work queue not run until user space
is unfrozen, which would guarantee that all drivers
that have registered for a crash notification will
be resumed when the notification arrives.

I'm not sure how that interacts with what you are
looking for here.  I think the workqueue could still
be unbound, but its work would be delayed longer before
any notification (and recovery) started.

-Alex



In that case, maybe adding a "WQ_FREEZABLE" flag might help?



This simply repeats $subject


Recovery time is an important parameter for a subsystem and there
might be situations where multiple subsystems crash around the same
time.  Scheduling into an unbound workqueue increases parallelization
and avoids time impact.


You should be able to write this more succinctly. The important part 
is

that you want an unbound work queue to allow recovery to happen in
parallel - which naturally implies that you care about recovery 
latency.



Also creating a high priority workqueue
will utilize separate worker threads with higher nice values than
normal ones.



This doesn't describe why you need the higher priority.


I believe, and certainly with the in-line coredump, that we're running
our recovery work for way too long to be queued on the system_wq. As
such the content of the patch looks good!

Regards,
Bjorn


Signed-off-by: Rishabh Bhatnagar 
---
  drivers/remoteproc/remoteproc_core.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

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

index 46c2937..8fd8166 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -48,6 +48,8 @@ static DEFINE_MUTEX(rproc_list_mutex);
  static LIST_HEAD(rproc_list);
  static struct notifier_block rproc_panic_nb;
  +static struct workqueue_struct *rproc_wq;
+
  typedef int (*rproc_handle_resource_t)(struct rproc *rproc,
 void *, int offset, int avail);
  @@ -2475,7 +2477,7 @@ void rproc_report_crash(struct rproc *rproc, 
enum rproc_crash_type type)

rproc->name, rproc_crash_to_string(type));
/* create a new task to handle the error */
-   schedule_work(>crash_handler);
+   queue_work(rproc_wq, >crash_handler);
  }
  EXPORT_SYMBOL(rproc_report_crash);
  @@ -2520,6 +2522,10 @@ static void __exit rproc_exit_panic(void)
static int __init remoteproc_init(void)
  {
+   rproc_wq = alloc_workqueue("rproc_wq", WQ_UNBOUND | WQ_HIGHPRI, 0);
+   if (!rproc_wq)
+   return -ENOMEM;
+
rproc_init_sysfs();
rproc_init_debugfs();
rproc_init_cdev();
@@ -2536,6 +2542,7 @@ static void __exit remoteproc_exit(void)
rproc_exit_panic();
rproc_exit_debugfs();
rproc_exit_sysfs();
+   destroy_workqueue(rproc_wq);
  }
  module_exit(remoteproc_exit);
  -- The Qualcomm Innovation Center, Inc. is a member of the Code 
Aurora Forum,

a Linux Foundation Collaborative Project

Re: [PATCH] remoteproc: Add a rproc_set_firmware() API

[rishabhb]

On 2020-11-20 19:20, Suman Anna wrote:
A new API, rproc_set_firmware() is added to allow the remoteproc 
platform

drivers and remoteproc client drivers to be able to configure a custom
firmware name that is different from the default name used during
remoteproc registration. This function is being introduced to provide
a kernel-level equivalent of the current sysfs interface to remoteproc
client drivers, and can only change firmwares when the remoteproc is
offline. This allows some remoteproc drivers to choose different 
firmwares
at runtime based on the functionality the remote processor is 
providing.

The TI PRU Ethernet driver will be an example of such usage as it
requires to use different firmwares for different supported protocols.

Also, update the firmware_store() function used by the sysfs interface
to reuse this function to avoid code duplication.

Signed-off-by: Suman Anna 
---
 drivers/remoteproc/remoteproc_core.c  | 63 +++
 drivers/remoteproc/remoteproc_sysfs.c | 33 +-
 include/linux/remoteproc.h|  1 +
 3 files changed, 66 insertions(+), 31 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index dab2c0f5caf0..46c2937ebea9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1934,6 +1934,69 @@ struct rproc *rproc_get_by_phandle(phandle 
phandle)

 #endif
 EXPORT_SYMBOL(rproc_get_by_phandle);

+/**
+ * rproc_set_firmware() - assign a new firmware
+ * @rproc: rproc handle to which the new firmware is being assigned
+ * @fw_name: new firmware name to be assigned
+ *
+ * This function allows remoteproc drivers or clients to configure a 
custom
+ * firmware name that is different from the default name used during 
remoteproc
+ * registration. The function does not trigger a remote processor 
boot,
+ * only sets the firmware name used for a subsequent boot. This 
function

+ * should also be called only when the remote processor is offline.
+ *
+ * This allows either the userspace to configure a different name 
through
+ * sysfs or a kernel-level remoteproc or a remoteproc client driver to 
set
+ * a specific firmware when it is controlling the boot and shutdown of 
the

+ * remote processor.
+ *
+ * Return: 0 on success or a negative value upon failure
+ */
+int rproc_set_firmware(struct rproc *rproc, const char *fw_name)
+{
+   struct device *dev;
+   int ret, len;
+   char *p;
+
+   if (!rproc || !fw_name)
+   return -EINVAL;
+
+   dev = rproc->dev.parent;
+
+   ret = mutex_lock_interruptible(>lock);
+   if (ret) {
+   dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
+   return -EINVAL;
+   }
+
+   if (rproc->state != RPROC_OFFLINE) {
+   dev_err(dev, "can't change firmware while running\n");
+   ret = -EBUSY;
+   goto out;
+   }
+
+   len = strcspn(fw_name, "\n");
+   if (!len) {
+   dev_err(dev, "can't provide empty string for firmware name\n");
+   ret = -EINVAL;
+   goto out;
+   }
+
+   p = kstrndup(fw_name, len, GFP_KERNEL);
+   if (!p) {
+   ret = -ENOMEM;
+   goto out;
+   }
+
+   kfree(rproc->firmware);
+   rproc->firmware = p;
+
+out:
+   mutex_unlock(>lock);
+   return ret;
+}
+EXPORT_SYMBOL(rproc_set_firmware);
+
 static int rproc_validate(struct rproc *rproc)
 {
switch (rproc->state) {
diff --git a/drivers/remoteproc/remoteproc_sysfs.c
b/drivers/remoteproc/remoteproc_sysfs.c
index 3fd18a71c188..cf846caf2e1a 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -159,42 +159,13 @@ static ssize_t firmware_store(struct device *dev,
  const char *buf, size_t count)
 {
struct rproc *rproc = to_rproc(dev);
-   char *p;
-   int err, len = count;
+   int err;

/* restrict sysfs operations if not allowed by remoteproc drivers */
if (rproc->deny_sysfs_ops)
return -EPERM;

-   err = mutex_lock_interruptible(>lock);
-   if (err) {
-   dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
-   return -EINVAL;
-   }
-
-   if (rproc->state != RPROC_OFFLINE) {
-   dev_err(dev, "can't change firmware while running\n");
-   err = -EBUSY;
-   goto out;
-   }
-
-   len = strcspn(buf, "\n");
-   if (!len) {
-   dev_err(dev, "can't provide a NULL firmware\n");
-   err = -EINVAL;
-   goto out;
-   }
-
-   p = kstrndup(buf, len, GFP_KERNEL);
-   if (!p) {
-   err = -ENOMEM;
-   goto out;
-   }
-
-   kfree(rproc->firmware);
-   rproc->firmware = p;
-out:
-   mutex_unlock(>lock);
+   err = rproc_set_firmware(rproc, buf);

Re: [PATCH v3 1/4] remoteproc: sysmon: Ensure remote notification ordering

[rishabhb]

On 2020-11-21 21:41, Bjorn Andersson wrote:

The reliance on the remoteproc's state for determining when to send
sysmon notifications to a remote processor is racy with regard to
concurrent remoteproc operations.

Further more the advertisement of the state of other remote processor 
to

a newly started remote processor might not only send the wrong state,
but might result in a stream of state changes that are out of order.

Address this by introducing state tracking within the sysmon instances
themselves and extend the locking to ensure that the notifications are
consistent with this state.

Fixes: 1f36ab3f6e3b ("remoteproc: sysmon: Inform current rproc about
all active rprocs")
Fixes: 1877f54f75ad ("remoteproc: sysmon: Add notifications for 
events")

Fixes: 1fb82ee806d1 ("remoteproc: qcom: Introduce sysmon")
Cc: sta...@vger.kernel.org
Signed-off-by: Bjorn Andersson 
---

Changes since v2:
- Hold sysmon_lock during traversal of sysmons in sysmon_start()

 drivers/remoteproc/qcom_sysmon.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

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

index 9eb2f6bccea6..b37b111b15b3 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -22,6 +22,9 @@ struct qcom_sysmon {
struct rproc_subdev subdev;
struct rproc *rproc;

+   int state;
+   struct mutex state_lock;
+
struct list_head node;

const char *name;
@@ -448,7 +451,10 @@ static int sysmon_prepare(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_BEFORE_POWERUP
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_BEFORE_POWERUP;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

return 0;
 }
@@ -472,20 +478,25 @@ static int sysmon_start(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_AFTER_POWERUP;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

mutex_lock(_lock);
list_for_each_entry(target, _list, node) {
-   if (target == sysmon ||
-   target->rproc->state != RPROC_RUNNING)
+   if (target == sysmon)
continue;

+   mutex_lock(>state_lock);
event.subsys_name = target->name;
+   event.ssr_event = target->state;

if (sysmon->ssctl_version == 2)
ssctl_send_event(sysmon, );
else if (sysmon->ept)
sysmon_send_event(sysmon, );
+   mutex_unlock(>state_lock);
}
mutex_unlock(_lock);

@@ -500,7 +511,10 @@ static void sysmon_stop(struct rproc_subdev
*subdev, bool crashed)
.ssr_event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

/* Don't request graceful shutdown if we've crashed */
if (crashed)
@@ -521,7 +535,10 @@ static void sysmon_unprepare(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_AFTER_SHUTDOWN
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_AFTER_SHUTDOWN;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);
 }

 /**
@@ -534,11 +551,10 @@ static int sysmon_notify(struct notifier_block
*nb, unsigned long event,
 void *data)
 {
 	struct qcom_sysmon *sysmon = container_of(nb, struct qcom_sysmon, 
nb);

-   struct rproc *rproc = sysmon->rproc;
struct sysmon_event *sysmon_event = data;

/* Skip non-running rprocs and the originating instance */
-   if (rproc->state != RPROC_RUNNING ||
+   if (sysmon->state != SSCTL_SSR_EVENT_AFTER_POWERUP ||
!strcmp(sysmon_event->subsys_name, sysmon->name)) {
dev_dbg(sysmon->dev, "not notifying %s\n", sysmon->name);
return NOTIFY_DONE;
@@ -591,6 +607,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct
rproc *rproc,
init_completion(>ind_comp);
init_completion(>shutdown_comp);
mutex_init(>lock);
+   mutex_init(>state_lock);

sysmon->shutdown_irq = of_irq_get_byname(sysmon->dev->of_node,
 "shutdown-ack");


Reviewed-by: Rishabh Bhatnagar 

Re: [PATCH v2 4/4] remoteproc: sysmon: Improve error messages

[rishabhb]

On 2020-11-04 20:50, Bjorn Andersson wrote:

Improve the style of a few of the error messages printed by the sysmon
implementation and fix the copy-pasted shutdown error in the send-event
function.

Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- New patch

 drivers/remoteproc/qcom_sysmon.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

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

index 1c42f00010d3..47683932512a 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -352,9 +352,9 @@ static bool ssctl_request_shutdown(struct
qcom_sysmon *sysmon)

ret = qmi_txn_wait(, 5 * HZ);
if (ret < 0) {
-   dev_err(sysmon->dev, "failed receiving QMI response\n");
+   dev_err(sysmon->dev, "timeout waiting for shutdown response\n");
} else if (resp.resp.result) {
-   dev_err(sysmon->dev, "shutdown request failed\n");
+   dev_err(sysmon->dev, "shutdown request rejected\n");
} else {
dev_dbg(sysmon->dev, "shutdown request completed\n");
acked = true;
@@ -397,18 +397,18 @@ static void ssctl_send_event(struct qcom_sysmon 
*sysmon,

   SSCTL_SUBSYS_EVENT_REQ, 40,
   ssctl_subsys_event_req_ei, );
if (ret < 0) {
-   dev_err(sysmon->dev, "failed to send shutdown request\n");
+   dev_err(sysmon->dev, "failed to send subsystem event\n");
qmi_txn_cancel();
return;
}

ret = qmi_txn_wait(, 5 * HZ);
if (ret < 0)
-   dev_err(sysmon->dev, "failed receiving QMI response\n");
+		dev_err(sysmon->dev, "timeout waiting for subsystem event 
response\n");

else if (resp.resp.result)
-   dev_err(sysmon->dev, "ssr event send failed\n");
+   dev_err(sysmon->dev, "subsystem event rejected\n");
else
-   dev_dbg(sysmon->dev, "ssr event send completed\n");
+   dev_dbg(sysmon->dev, "subsystem event accepted\n");
 }

 /**

Reviewed-by: Rishabh Bhatnagar 

Re: [PATCH v2 3/4] remoteproc: qcom: q6v5: Query sysmon before graceful shutdown

[rishabhb]

On 2020-11-04 20:50, Bjorn Andersson wrote:

Requesting a graceful shutdown through the shared memory state signals
will not be acked in the event that sysmon has already successfully 
shut

down the remote firmware. So extend the stop request API to optinally

optionally

take the remoteproc's sysmon instance and query if there's already been
a successful shutdown attempt, before doing the signal dance.

Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- New patch

 drivers/remoteproc/qcom_q6v5.c  | 8 +++-
 drivers/remoteproc/qcom_q6v5.h  | 3 ++-
 drivers/remoteproc/qcom_q6v5_adsp.c | 2 +-
 drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
 drivers/remoteproc/qcom_q6v5_pas.c  | 2 +-
 drivers/remoteproc/qcom_q6v5_wcss.c | 2 +-
 6 files changed, 13 insertions(+), 6 deletions(-)

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

index fd6fd36268d9..9627a950928e 100644
--- a/drivers/remoteproc/qcom_q6v5.c
+++ b/drivers/remoteproc/qcom_q6v5.c
@@ -13,6 +13,7 @@
 #include 
 #include 
 #include 
+#include "qcom_common.h"
 #include "qcom_q6v5.h"

 #define Q6V5_PANIC_DELAY_MS200
@@ -146,15 +147,20 @@ static irqreturn_t q6v5_stop_interrupt(int irq,
void *data)
 /**
  * qcom_q6v5_request_stop() - request the remote processor to stop
  * @q6v5:  reference to qcom_q6v5 context
+ * @sysmon:reference to the remote's sysmon instance, or NULL
  *
  * Return: 0 on success, negative errno on failure
  */
-int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5)
+int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5, struct qcom_sysmon 
*sysmon)

 {
int ret;

q6v5->running = false;

+	/* Don't perform SMP2P dance if sysmon already shut down the remote 
*/

+   if (qcom_sysmon_shutdown_acked(sysmon))
+   return 0;
+
qcom_smem_state_update_bits(q6v5->state,
BIT(q6v5->stop_bit), BIT(q6v5->stop_bit));

diff --git a/drivers/remoteproc/qcom_q6v5.h 
b/drivers/remoteproc/qcom_q6v5.h

index c4ed887c1499..1c212f670cbc 100644
--- a/drivers/remoteproc/qcom_q6v5.h
+++ b/drivers/remoteproc/qcom_q6v5.h
@@ -8,6 +8,7 @@

 struct rproc;
 struct qcom_smem_state;
+struct qcom_sysmon;

 struct qcom_q6v5 {
struct device *dev;
@@ -40,7 +41,7 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct
platform_device *pdev,

 int qcom_q6v5_prepare(struct qcom_q6v5 *q6v5);
 int qcom_q6v5_unprepare(struct qcom_q6v5 *q6v5);
-int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5);
+int qcom_q6v5_request_stop(struct qcom_q6v5 *q6v5, struct qcom_sysmon 
*sysmon);

 int qcom_q6v5_wait_for_start(struct qcom_q6v5 *q6v5, int timeout);
 unsigned long qcom_q6v5_panic(struct qcom_q6v5 *q6v5);

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
b/drivers/remoteproc/qcom_q6v5_adsp.c
index efb2c1aa80a3..9db0380236e7 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -264,7 +264,7 @@ static int adsp_stop(struct rproc *rproc)
int handover;
int ret;

-   ret = qcom_q6v5_request_stop(>q6v5);
+   ret = qcom_q6v5_request_stop(>q6v5, adsp->sysmon);
if (ret == -ETIMEDOUT)
dev_err(adsp->dev, "timed out on wait\n");

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
b/drivers/remoteproc/qcom_q6v5_mss.c
index 9a473cfef758..501764934014 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1370,7 +1370,7 @@ static int q6v5_stop(struct rproc *rproc)
struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
int ret;

-   ret = qcom_q6v5_request_stop(>q6v5);
+   ret = qcom_q6v5_request_stop(>q6v5, qproc->sysmon);
if (ret == -ETIMEDOUT)
dev_err(qproc->dev, "timed out on wait\n");

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3837f23995e0..ed1772bfa55d 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -214,7 +214,7 @@ static int adsp_stop(struct rproc *rproc)
int handover;
int ret;

-   ret = qcom_q6v5_request_stop(>q6v5);
+   ret = qcom_q6v5_request_stop(>q6v5, adsp->sysmon);
if (ret == -ETIMEDOUT)
dev_err(adsp->dev, "timed out on wait\n");

diff --git a/drivers/remoteproc/qcom_q6v5_wcss.c
b/drivers/remoteproc/qcom_q6v5_wcss.c
index 8846ef0b0f1a..d6639856069b 100644
--- a/drivers/remoteproc/qcom_q6v5_wcss.c
+++ b/drivers/remoteproc/qcom_q6v5_wcss.c
@@ -390,7 +390,7 @@ static int q6v5_wcss_stop(struct rproc *rproc)
int ret;

/* WCSS powerdown */
-   ret = qcom_q6v5_request_stop(>q6v5);
+   ret = qcom_q6v5_request_stop(>q6v5, wcss->sysmon);
if (ret == -ETIMEDOUT) {
dev_err(wcss->dev, "timed out on wait\n");
return ret;

Reviewed-by: Rishabh Bhatnagar 

Re: [PATCH v2 2/4] remoteproc: sysmon: Expose the shutdown result

[rishabhb]

On 2020-11-04 20:50, Bjorn Andersson wrote:

A graceful shutdown of the Qualcomm remote processors where
traditionally performed by invoking a shared memory state signal and
waiting for the associated ack.

This was later superseded by the "sysmon" mechanism, where some form of
shared memory bus is used to send a "graceful shutdown request" message
and one of more signals comes back to indicate its success.

But when this newer mechanism is in effect the firmware is shut down by
the time the older mechanism, implemented in the remoteproc drivers,
attempts to perform a graceful shutdown - and as such it will never
receive an ack back.

This patch therefor track the success of the latest shutdown attempt in
sysmon and exposes a new function in the API that the remoteproc driver
can use to query the success and the necessity of invoking the older
mechanism.

Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- New patch

 drivers/remoteproc/qcom_common.h |  6 +++
 drivers/remoteproc/qcom_sysmon.c | 82 
 2 files changed, 69 insertions(+), 19 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.h 
b/drivers/remoteproc/qcom_common.h

index dfc641c3a98b..8ba9052955bd 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -51,6 +51,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct
rproc *rproc,
   const char *name,
   int ssctl_instance);
 void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon);
+bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon);
 #else
 static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc 
*rproc,

 const char *name,
@@ -62,6 +63,11 @@ static inline struct qcom_sysmon
*qcom_add_sysmon_subdev(struct rproc *rproc,
 static inline void qcom_remove_sysmon_subdev(struct qcom_sysmon 
*sysmon)

 {
 }
+
+static inline bool qcom_sysmon_shutdown_acked(struct qcom_sysmon 
*sysmon)

+{
+   return false;
+}
 #endif

 #endif
diff --git a/drivers/remoteproc/qcom_sysmon.c 
b/drivers/remoteproc/qcom_sysmon.c

index 38f63c968fa8..1c42f00010d3 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -44,6 +44,7 @@ struct qcom_sysmon {
struct mutex lock;

bool ssr_ack;
+   bool shutdown_acked;

struct qmi_handle qmi;
struct sockaddr_qrtr ssctl;
@@ -115,10 +116,13 @@ static void sysmon_send_event(struct qcom_sysmon 
*sysmon,

 /**
  * sysmon_request_shutdown() - request graceful shutdown of remote
  * @sysmon:sysmon context
+ *
+ * Return: boolean indicator of the remote processor acking the 
request

  */
-static void sysmon_request_shutdown(struct qcom_sysmon *sysmon)
+static bool sysmon_request_shutdown(struct qcom_sysmon *sysmon)
 {
char *req = "ssr:shutdown";
+   bool acked = false;
int ret;

mutex_lock(>lock);
@@ -141,9 +145,13 @@ static void sysmon_request_shutdown(struct
qcom_sysmon *sysmon)
if (!sysmon->ssr_ack)
dev_err(sysmon->dev,
"unexpected response to sysmon shutdown request\n");
+   else
+   acked = true;

 out_unlock:
mutex_unlock(>lock);
+
+   return acked;
 }

 static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int 
count,
@@ -297,14 +305,33 @@ static struct qmi_msg_handler 
qmi_indication_handler[] = {

{}
 };

+static bool ssctl_request_shutdown_wait(struct qcom_sysmon *sysmon)
+{
+   int ret;
+
+   ret = wait_for_completion_timeout(>shutdown_comp, 10 * HZ);
+   if (ret)
+   return true;
+
+   ret = try_wait_for_completion(>ind_comp);
+   if (ret)
+   return true;
+
+   dev_err(sysmon->dev, "timeout waiting for shutdown ack\n");
+   return false;
+}
+
 /**
  * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
  * @sysmon:sysmon context
+ *
+ * Return: boolean indicator of the remote processor acking the 
request

  */
-static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
+static bool ssctl_request_shutdown(struct qcom_sysmon *sysmon)
 {
struct ssctl_shutdown_resp resp;
struct qmi_txn txn;
+   bool acked = false;
int ret;

reinit_completion(>ind_comp);
@@ -312,7 +339,7 @@ static void ssctl_request_shutdown(struct
qcom_sysmon *sysmon)
 	ret = qmi_txn_init(>qmi, , ssctl_shutdown_resp_ei, 
);

if (ret < 0) {
dev_err(sysmon->dev, "failed to allocate QMI txn\n");
-   return;
+   return false;
}

ret = qmi_send_request(>qmi, >ssctl, ,
@@ -320,27 +347,23 @@ static void ssctl_request_shutdown(struct
qcom_sysmon *sysmon)
if (ret < 0) {
dev_err(sysmon->dev, "failed to send shutdown request\n");
qmi_txn_cancel();
-   return;
+  

Re: [PATCH v2 1/4] remoteproc: sysmon: Ensure remote notification ordering

[rishabhb]

On 2020-11-04 20:50, Bjorn Andersson wrote:

The reliance on the remoteproc's state for determining when to send
sysmon notifications to a remote processor is racy with regard to
concurrent remoteproc operations.

Further more the advertisement of the state of other remote processor 
to

a newly started remote processor might not only send the wrong state,
but might result in a stream of state changes that are out of order.

Address this by introducing state tracking within the sysmon instances
themselves and extend the locking to ensure that the notifications are
consistent with this state.

Fixes: 1f36ab3f6e3b ("remoteproc: sysmon: Inform current rproc about
all active rprocs")
Fixes: 1877f54f75ad ("remoteproc: sysmon: Add notifications for 
events")

Fixes: 1fb82ee806d1 ("remoteproc: qcom: Introduce sysmon")
Cc: sta...@vger.kernel.org
Signed-off-by: Bjorn Andersson 
---

Changes since v1:
- Reduced the locking to be per sysmon instance
- Dropped unused local "rproc" variable in sysmon_notify()

 drivers/remoteproc/qcom_sysmon.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

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

index 9eb2f6bccea6..38f63c968fa8 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -22,6 +22,9 @@ struct qcom_sysmon {
struct rproc_subdev subdev;
struct rproc *rproc;

+   int state;
+   struct mutex state_lock;
+
struct list_head node;

const char *name;
@@ -448,7 +451,10 @@ static int sysmon_prepare(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_BEFORE_POWERUP
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_BEFORE_POWERUP;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

return 0;
 }
@@ -472,22 +478,25 @@ static int sysmon_start(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_AFTER_POWERUP;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

-   mutex_lock(_lock);


We should keep the sysmon_lock to make sure sysmon_list is not modified
at the time we are doing this operation?

list_for_each_entry(target, _list, node) {
-   if (target == sysmon ||
-   target->rproc->state != RPROC_RUNNING)
+   if (target == sysmon)
continue;

+   mutex_lock(>state_lock);
event.subsys_name = target->name;
+   event.ssr_event = target->state;


Is it better to only send this event when target->state is 
"SSCTL_SSR_EVENT_AFTER_POWERUP"?


if (sysmon->ssctl_version == 2)
ssctl_send_event(sysmon, );
else if (sysmon->ept)
sysmon_send_event(sysmon, );
+   mutex_unlock(>state_lock);
}
-   mutex_unlock(_lock);

return 0;
 }
@@ -500,7 +509,10 @@ static void sysmon_stop(struct rproc_subdev
*subdev, bool crashed)
.ssr_event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);

/* Don't request graceful shutdown if we've crashed */
if (crashed)
@@ -521,7 +533,10 @@ static void sysmon_unprepare(struct rproc_subdev 
*subdev)

.ssr_event = SSCTL_SSR_EVENT_AFTER_SHUTDOWN
};

+   mutex_lock(>state_lock);
+   sysmon->state = SSCTL_SSR_EVENT_AFTER_SHUTDOWN;
blocking_notifier_call_chain(_notifiers, 0, (void *));
+   mutex_unlock(>state_lock);
 }

 /**
@@ -534,11 +549,10 @@ static int sysmon_notify(struct notifier_block
*nb, unsigned long event,
 void *data)
 {
 	struct qcom_sysmon *sysmon = container_of(nb, struct qcom_sysmon, 
nb);

-   struct rproc *rproc = sysmon->rproc;
struct sysmon_event *sysmon_event = data;

/* Skip non-running rprocs and the originating instance */
-   if (rproc->state != RPROC_RUNNING ||
+   if (sysmon->state != SSCTL_SSR_EVENT_AFTER_POWERUP ||
!strcmp(sysmon_event->subsys_name, sysmon->name)) {
dev_dbg(sysmon->dev, "not notifying %s\n", sysmon->name);
return NOTIFY_DONE;
@@ -591,6 +605,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct
rproc *rproc,
init_completion(>ind_comp);
init_completion(>shutdown_comp);
mutex_init(>lock);
+   mutex_init(>state_lock);

sysmon->shutdown_irq = of_irq_get_byname(sysmon->dev->of_node,
 "shutdown-ack");

Re: [PATCH v4 0/3] Move recovery/coredump configuration to sysfs

[rishabhb]

On 2020-09-17 16:40, Randy Dunlap wrote:

On 9/17/20 11:56 AM, Rishabh Bhatnagar wrote:

From Android R onwards Google has restricted access to debugfs in user
and user-debug builds. This restricts access to most of the features
exposed through debugfs. This patch series removes the 
recovery/coredump

entries from debugfs and moves them to sysfs.
'Coredump' and 'Recovery' are critical interfaces that are required
for remoteproc to work on Qualcomm Chipsets. Coredump configuration
needs to be set to "inline" in debug/test build and "disabled" in
production builds. Whereas recovery needs to be "disabled" for
debugging purposes and "enabled" on production builds.

Changelog:

v4 -> v3:
- Remove the feature flag to expose recovery/coredump

v3 -> v2:
- Remove the coredump/recovery entries from debugfs
- Expose recovery/coredump from sysfs under a feature flag

v1 -> v2:
- Correct the contact name in the sysfs documentation.
- Remove the redundant write documentation for coredump/recovery sysfs
- Add a feature flag to make this interface switch configurable.

Rishabh Bhatnagar (3):
  remoteproc: Expose remoteproc configuration through sysfs
  remoteproc: Add coredump configuration to sysfs
  remoteproc: Add recovery configuration to sysfs

 Documentation/ABI/testing/sysfs-class-remoteproc |  44 
 drivers/remoteproc/Kconfig   |  12 +++
 drivers/remoteproc/remoteproc_debugfs.c  |  10 +-
 drivers/remoteproc/remoteproc_sysfs.c| 126 
+++

 4 files changed, 190 insertions(+), 2 deletions(-)



Hi,

Is there a patch 3/3?
This email (reply) is patch 0/3, then I see
  patch 1/2
  patch 2/2
so I'm confused.  However, the diffstat above references a Kconfig file
and neither patch 1/2 nor patch 2/2 contains any Kconfig changes.

thanks.

Hi Randy,
The cover letter got messed up. There are only 2 patches now whereas in 
the

earlier versions there were 3 patches. I'll be sending out v5 soon.
Please review that.

Re: [PATCH] soc: qcom: pdr: Fixup array type of get_domain_list_resp message

[rishabhb]

On 2020-09-14 07:58, Sibi Sankar wrote:
The array type of get_domain_list_resp is incorrectly marked as 
NO_ARRAY.
Due to which the following error was observed when using pdr helpers 
with

the downstream proprietary pd-mapper. Fix this up by marking it as
VAR_LEN_ARRAY instead.

Err logs:
qmi_decode_struct_elem: Fault in decoding: dl(2), db(27), tl(160), 
i(1), el(1)

failed to decode incoming message
PDR: tms/servreg get domain list txn wait failed: -14
PDR: service lookup for tms/servreg failed: -14

Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart 
helpers")

Reported-by: Rishabh Bhatnagar 
Signed-off-by: Sibi Sankar 
---
 drivers/soc/qcom/pdr_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pdr_internal.h 
b/drivers/soc/qcom/pdr_internal.h

index 15b5002e4127b..ab9ae8cdfa54c 100644
--- a/drivers/soc/qcom/pdr_internal.h
+++ b/drivers/soc/qcom/pdr_internal.h
@@ -185,7 +185,7 @@ struct qmi_elem_info 
servreg_get_domain_list_resp_ei[] = {

.data_type  = QMI_STRUCT,
.elem_len   = SERVREG_DOMAIN_LIST_LENGTH,
.elem_size  = sizeof(struct servreg_location_entry),
-   .array_type = NO_ARRAY,
+   .array_type = VAR_LEN_ARRAY,
.tlv_type   = 0x12,
.offset = offsetof(struct servreg_get_domain_list_resp,
   domain_list),

Tested-by: Rishabh Bhatnagar 

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

+android kernel team

On 2020-09-09 10:27, risha...@codeaurora.org wrote:

On 2020-09-04 15:02, Mathieu Poirier wrote:

On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:

On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:

> Hi Rishabh,
>
> On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > From Android R onwards Google has restricted access to debugfs in user
> > and user-debug builds. This restricts access to most of the features
> > exposed through debugfs. This patch series adds a configurable option
> > to move the recovery/coredump interfaces to sysfs. If the feature
> > flag is selected it would move these interfaces to sysfs and remove
> > the equivalent debugfs interface.
>
> What I meant wast to move the coredump entry from debugfs to sysfs and from
> there make it available to user space using a kernel config.

Why would we not always make this available in sysfs?


At this time the options are in debugfs and vendors can decide to make 
that

available on products if they want to.  The idea behind using a kernel
configuration once moved to sysfs was to give the same kind of 
options.




> But thinking further on this it may be better to simply provide an API
> to set the coredump mode from the platform driver, the same way
> rproc_coredump_set_elf_info() works.

Being able to invoke these from the platform drivers sounds like a 
new

feature. What would trigger the platform drivers to call this? Or are
you perhaps asking for the means of the drivers to be able to select 
the

default mode?


My ultimate goal is to avoid needlessly stuffing things in sysfs.  My 
hope in
suggesting a new API was that platform drivers could recognise the 
kind of
build/environment they operate in and setup the coredump mode 
accordingly.  That

would have allowed us to leave debugfs options alone.



Regarding the default mode, I think it would make sense to make the
default "disabled", because this is the most sensible configuration 
in a

"production" environment. And the sysfs means we have a convenient
mechanism to configure it, even on production environments.



I am weary of changing something that hasn't been requested.


> That will prevent breaking a fair amount of user space code...
>

We typically don't guarantee that the debugfs interfaces are stable 
and

if I understand the beginning of you reply you still want to move it
from debugfs to sysfs - which I presume would break such scripts in 
the

first place?


Correct - I am sure that moving coredump and recovery options to sysfs 
will
break user space scripts.  Even if debugfs is not part of the ABI it 
would be

nice to avoid disrupting people as much as possible.




I would prefer to see that we don't introduce config options for 
every

little thing, unless there's good reason for it.


I totally agree.  It is with great reluctance that I asked Rishab to 
proceed
the way he did in V3.  His usecase makes sense... On the flip side 
this is
pushed down on the kernel community and I really like Christoph's 
position about

fixing Android and leaving the kernel alone.

Well, removing debugfs is conscious decision taken by android due to 
security

concerns and there is not we can fix there.
Would it be a terrible idea to have recovery and coredump exposed from 
both

sysfs and debugfs instead of choosing one and breaking userspace code?


Regards,
Bjorn

> Let me know if that can work for you.
>
> Thanks,
> Mathieu
>
> > 'Coredump' and 'Recovery' are critical
> > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > Coredump configuration needs to be set to "inline" in debug/test build
> > and "disabled" in production builds. Whereas recovery needs to be
> > "disabled" for debugging purposes and "enabled" on production builds.
> >
> > Changelog:
> >
> > v1 -> v2:
> > - Correct the contact name in the sysfs documentation.
> > - Remove the redundant write documentation for coredump/recovery sysfs
> > - Add a feature flag to make this interface switch configurable.
> >
> > Rishabh Bhatnagar (3):
> >   remoteproc: Expose remoteproc configuration through sysfs
> >   remoteproc: Add coredump configuration to sysfs
> >   remoteproc: Add recovery configuration to sysfs
> >
> >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 
> >  drivers/remoteproc/Kconfig   |  12 +++
> >  drivers/remoteproc/remoteproc_debugfs.c  |  10 +-
> >  drivers/remoteproc/remoteproc_sysfs.c| 126 
+++
> >  4 files changed, 190 insertions(+), 2 deletions(-)
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

On 2020-09-04 15:02, Mathieu Poirier wrote:

On Thu, Sep 03, 2020 at 06:59:44PM -0500, Bjorn Andersson wrote:

On Tue 01 Sep 17:05 CDT 2020, Mathieu Poirier wrote:

> Hi Rishabh,
>
> On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:
> > From Android R onwards Google has restricted access to debugfs in user
> > and user-debug builds. This restricts access to most of the features
> > exposed through debugfs. This patch series adds a configurable option
> > to move the recovery/coredump interfaces to sysfs. If the feature
> > flag is selected it would move these interfaces to sysfs and remove
> > the equivalent debugfs interface.
>
> What I meant wast to move the coredump entry from debugfs to sysfs and from
> there make it available to user space using a kernel config.

Why would we not always make this available in sysfs?


At this time the options are in debugfs and vendors can decide to make 
that

available on products if they want to.  The idea behind using a kernel
configuration once moved to sysfs was to give the same kind of options.



> But thinking further on this it may be better to simply provide an API
> to set the coredump mode from the platform driver, the same way
> rproc_coredump_set_elf_info() works.

Being able to invoke these from the platform drivers sounds like a new
feature. What would trigger the platform drivers to call this? Or are
you perhaps asking for the means of the drivers to be able to select 
the

default mode?


My ultimate goal is to avoid needlessly stuffing things in sysfs.  My 
hope in
suggesting a new API was that platform drivers could recognise the kind 
of
build/environment they operate in and setup the coredump mode 
accordingly.  That

would have allowed us to leave debugfs options alone.



Regarding the default mode, I think it would make sense to make the
default "disabled", because this is the most sensible configuration in 
a

"production" environment. And the sysfs means we have a convenient
mechanism to configure it, even on production environments.



I am weary of changing something that hasn't been requested.


> That will prevent breaking a fair amount of user space code...
>

We typically don't guarantee that the debugfs interfaces are stable 
and

if I understand the beginning of you reply you still want to move it
from debugfs to sysfs - which I presume would break such scripts in 
the

first place?


Correct - I am sure that moving coredump and recovery options to sysfs 
will
break user space scripts.  Even if debugfs is not part of the ABI it 
would be

nice to avoid disrupting people as much as possible.




I would prefer to see that we don't introduce config options for every
little thing, unless there's good reason for it.


I totally agree.  It is with great reluctance that I asked Rishab to 
proceed
the way he did in V3.  His usecase makes sense... On the flip side this 
is
pushed down on the kernel community and I really like Christoph's 
position about

fixing Android and leaving the kernel alone.

Well, removing debugfs is conscious decision taken by android due to 
security

concerns and there is not we can fix there.
Would it be a terrible idea to have recovery and coredump exposed from 
both

sysfs and debugfs instead of choosing one and breaking userspace code?


Regards,
Bjorn

> Let me know if that can work for you.
>
> Thanks,
> Mathieu
>
> > 'Coredump' and 'Recovery' are critical
> > interfaces that are required for remoteproc to work on Qualcomm Chipsets.
> > Coredump configuration needs to be set to "inline" in debug/test build
> > and "disabled" in production builds. Whereas recovery needs to be
> > "disabled" for debugging purposes and "enabled" on production builds.
> >
> > Changelog:
> >
> > v1 -> v2:
> > - Correct the contact name in the sysfs documentation.
> > - Remove the redundant write documentation for coredump/recovery sysfs
> > - Add a feature flag to make this interface switch configurable.
> >
> > Rishabh Bhatnagar (3):
> >   remoteproc: Expose remoteproc configuration through sysfs
> >   remoteproc: Add coredump configuration to sysfs
> >   remoteproc: Add recovery configuration to sysfs
> >
> >  Documentation/ABI/testing/sysfs-class-remoteproc |  44 
> >  drivers/remoteproc/Kconfig   |  12 +++
> >  drivers/remoteproc/remoteproc_debugfs.c  |  10 +-
> >  drivers/remoteproc/remoteproc_sysfs.c| 126 
+++
> >  4 files changed, 190 insertions(+), 2 deletions(-)
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >

Re: [PATCH v2 0/3] Expose recovery/coredump configuration from sysfs

[rishabhb]

On 2020-09-01 15:05, Mathieu Poirier wrote:

Hi Rishabh,

On Thu, Aug 27, 2020 at 12:48:48PM -0700, Rishabh Bhatnagar wrote:

From Android R onwards Google has restricted access to debugfs in user
and user-debug builds. This restricts access to most of the features
exposed through debugfs. This patch series adds a configurable option
to move the recovery/coredump interfaces to sysfs. If the feature
flag is selected it would move these interfaces to sysfs and remove
the equivalent debugfs interface.


What I meant wast to move the coredump entry from debugfs to sysfs and 
from
there make it available to user space using a kernel config.  But 
thinking
further on this it may be better to simply provide an API to set the 
coredump
mode from the platform driver, the same way 
rproc_coredump_set_elf_info() works.

That will prevent breaking a fair amount of user space code...

Let me know if that can work for you.

Thanks,
Mathieu


Hi Mathieu,
That works for product configuration but that would still limit internal
testing. Since there is also restriction on accessing debugfs through
userspace code, automation won't be able to run recovery/coredump tests.
Only other way for us would be to provide these sysfs entries through 
the

platform drivers locally but that would create a lot of mess/redundancy.


'Coredump' and 'Recovery' are critical
interfaces that are required for remoteproc to work on Qualcomm 
Chipsets.

Coredump configuration needs to be set to "inline" in debug/test build
and "disabled" in production builds. Whereas recovery needs to be
"disabled" for debugging purposes and "enabled" on production builds.

Changelog:

v1 -> v2:
- Correct the contact name in the sysfs documentation.
- Remove the redundant write documentation for coredump/recovery sysfs
- Add a feature flag to make this interface switch configurable.

Rishabh Bhatnagar (3):
  remoteproc: Expose remoteproc configuration through sysfs
  remoteproc: Add coredump configuration to sysfs
  remoteproc: Add recovery configuration to sysfs

 Documentation/ABI/testing/sysfs-class-remoteproc |  44 
 drivers/remoteproc/Kconfig   |  12 +++
 drivers/remoteproc/remoteproc_debugfs.c  |  10 +-
 drivers/remoteproc/remoteproc_sysfs.c| 126 
+++

 4 files changed, 190 insertions(+), 2 deletions(-)

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH v7 3/4] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-07-16 01:25, Sibi Sankar wrote:

On 2020-07-10 02:01, Rishabh Bhatnagar wrote:

The current coredump implementation uses vmalloc area to copy
all the segments. But this might put strain on low memory targets
as the firmware size sometimes is in tens of MBs. The situation
becomes worse if there are multiple remote processors undergoing
recovery at the same time. This patch adds inline coredump
functionality that avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called. Also modify the qcom_q6v5_mss driver to include
size and offset in the segment dump function.

Signed-off-by: Rishabh Bhatnagar 
Reviewed-by: Sibi Sankar 
---
 drivers/remoteproc/qcom_q6v5_mss.c   |  11 ++-
 drivers/remoteproc/remoteproc_coredump.c | 160 
+++

 include/linux/remoteproc.h   |  21 +++-
 3 files changed, 166 insertions(+), 26 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
b/drivers/remoteproc/qcom_q6v5_mss.c
index c6ce032..79df354 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1199,11 +1199,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc)

 static void qcom_q6v5_dump_segment(struct rproc *rproc,
   struct rproc_dump_segment *segment,
-  void *dest)
+  void *dest, size_t cp_offset, size_t size)
 {
int ret = 0;
struct q6v5 *qproc = rproc->priv;
int offset = segment->da - qproc->mpss_reloc;
+   size_t cp_size = size ? size : segment->size;
void *ptr = NULL;

/* Unlock mba before copying segments */
@@ -1219,16 +1220,16 @@ static void qcom_q6v5_dump_segment(struct 
rproc *rproc,

}

if (!ret)
-   ptr = ioremap_wc(qproc->mpss_phys + offset, segment->size);
+   ptr = ioremap_wc(qproc->mpss_phys + offset + cp_offset, 
cp_size);

if (ptr) {
-   memcpy(dest, ptr, segment->size);
+   memcpy(dest, ptr, cp_size);
iounmap(ptr);
} else {
-   memset(dest, 0xff, segment->size);
+   memset(dest, 0xff, cp_size);
}

-   qproc->current_dump_size += segment->size;
+   qproc->current_dump_size += cp_size;

/* Reclaim mba after copying segments */
if (qproc->current_dump_size == qproc->total_dump_size) {
diff --git a/drivers/remoteproc/remoteproc_coredump.c
b/drivers/remoteproc/remoteproc_coredump.c
index ded0244..646886f 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  */

+#include 
 #include 
 #include 
 #include 
@@ -12,6 +13,12 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"

+struct rproc_coredump_state {
+   struct rproc *rproc;
+   void *header;
+   struct completion dump_done;
+};
+
 /**
  * rproc_coredump_cleanup() - clean up dump_segments list
  * @rproc: the remote processor handle
@@ -72,7 +79,8 @@ int rproc_coredump_add_custom_segment(struct rproc 
*rproc,

  dma_addr_t da, size_t size,
  void (*dumpfn)(struct rproc *rproc,
 struct rproc_dump_segment 
*segment,
-void *dest),
+void *dest, size_t offset,
+size_t size),
  void *priv)
 {
struct rproc_dump_segment *segment;
@@ -114,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc
*rproc, u8 class, u16 machine)
 }
 EXPORT_SYMBOL(rproc_coredump_set_elf_info);

+static void rproc_coredump_free(void *data)
+{
+   struct rproc_coredump_state *dump_state = data;
+
+   complete(_state->dump_done);
+   vfree(dump_state->header);


Rishabh,

I observed a system hang when I trigger
recovery of a number of remoteproc
simultaneously (after a number of iterations
consistently). This goes away if I free the
header and  then issue the dump completion
and that does seem to be the right thing to
do.


Makes sense i'll switch it in the next patchset.

+}
+
+static void *rproc_coredump_find_segment(loff_t user_offset,
+struct list_head *segments,
+size_t *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset < segment->size) {
+   *data_left = segment->size - user_offset;
+   return segment;
+   }
+   user_offset -= segment->size;
+   }
+
+   *data_left = 0;
+   return NULL;
+}
+
+static 

Re: [PATCH v1 2/4] remoteproc: qcom_q6v5_mss: Replace mask based tracking with size

[rishabhb]

On 2020-07-16 01:10, Sibi Sankar wrote:

On 2020-07-14 22:48, Mathieu Poirier wrote:

On Thu, Jul 09, 2020 at 01:31:54PM -0700, Rishabh Bhatnagar wrote:

From: Sibi Sankar 

In order to land inline coredump support for mss, the dump_segment
function would need to support granularities less than the segment
size. This is achieved by replacing mask based tracking with size.

Signed-off-by: Sibi Sankar 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/remoteproc/qcom_q6v5_mss.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

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

index feb70283b..c6ce032 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -181,8 +181,8 @@ struct q6v5 {
bool running;

bool dump_mba_loaded;
-   unsigned long dump_segment_mask;
-   unsigned long dump_complete_mask;
+   size_t current_dump_size;
+   size_t total_dump_size;

phys_addr_t mba_phys;
void *mba_region;
@@ -1203,7 +1203,6 @@ static void qcom_q6v5_dump_segment(struct rproc 
*rproc,

 {
int ret = 0;
struct q6v5 *qproc = rproc->priv;
-   unsigned long mask = BIT((unsigned long)segment->priv);
int offset = segment->da - qproc->mpss_reloc;
void *ptr = NULL;

@@ -1229,10 +1228,10 @@ static void qcom_q6v5_dump_segment(struct 
rproc *rproc,

memset(dest, 0xff, segment->size);
}

-   qproc->dump_segment_mask |= mask;
+   qproc->current_dump_size += segment->size;

/* Reclaim mba after copying segments */
-   if (qproc->dump_segment_mask == qproc->dump_complete_mask) {
+   if (qproc->current_dump_size == qproc->total_dump_size) {
if (qproc->dump_mba_loaded) {
/* Try to reset ownership back to Q6 */
q6v5_xfer_mem_ownership(qproc, >mpss_perm,
@@ -1274,7 +1273,7 @@ static int q6v5_start(struct rproc *rproc)
"Failed to reclaim mba buffer system may become 
unstable\n");

/* Reset Dump Segment Mask */
-   qproc->dump_segment_mask = 0;
+   qproc->current_dump_size = 0;
qproc->running = true;

return 0;
@@ -1323,7 +1322,7 @@ static int 
qcom_q6v5_register_dump_segments(struct rproc *rproc,


ehdr = (struct elf32_hdr *)fw->data;
phdrs = (struct elf32_phdr *)(ehdr + 1);
-   qproc->dump_complete_mask = 0;
+   qproc->total_dump_size = 0;

for (i = 0; i < ehdr->e_phnum; i++) {
phdr = [i];
@@ -1338,7 +1337,7 @@ static int 
qcom_q6v5_register_dump_segments(struct rproc *rproc,

if (ret)
break;


There is also no longer a need to carry the 'i' in:

ret = rproc_coredump_add_custom_segment(rproc, 
phdr->p_paddr,

phdr->p_memsz,

qcom_q6v5_dump_segment,

(void *)i);


I assume Rishabh will re-spin the
series today and this will be taken
care as well.


Hi Sibi,
I'll respin and add the fixes.
Thanks


-   qproc->dump_complete_mask |= BIT(i);
+   qproc->total_dump_size += phdr->p_memsz;
}

release_firmware(fw);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH v7 3/4] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-07-14 10:53, Mathieu Poirier wrote:

On Thu, Jul 09, 2020 at 01:31:55PM -0700, Rishabh Bhatnagar wrote:

The current coredump implementation uses vmalloc area to copy
all the segments. But this might put strain on low memory targets
as the firmware size sometimes is in tens of MBs. The situation
becomes worse if there are multiple remote processors undergoing
recovery at the same time. This patch adds inline coredump
functionality that avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called. Also modify the qcom_q6v5_mss driver to include
size and offset in the segment dump function.

Signed-off-by: Rishabh Bhatnagar 
Reviewed-by: Sibi Sankar 
---
 drivers/remoteproc/qcom_q6v5_mss.c   |  11 ++-
 drivers/remoteproc/remoteproc_coredump.c | 160 
+++

 include/linux/remoteproc.h   |  21 +++-
 3 files changed, 166 insertions(+), 26 deletions(-)

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

index c6ce032..79df354 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1199,11 +1199,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc)

 static void qcom_q6v5_dump_segment(struct rproc *rproc,
   struct rproc_dump_segment *segment,
-  void *dest)
+  void *dest, size_t cp_offset, size_t size)


Now that I'm taking another look at this, refactoring 
qcom_q6v5_dump_segment()

should be in another patch - please split them.


Hi Mathieu,
Thanks for reviewing.

Since I'm changing the API signature if we just take this patch without 
q6v5_mss
changes (in a separate patch) it would cause compilation errors. Is that 
ok?

 {
int ret = 0;
struct q6v5 *qproc = rproc->priv;
int offset = segment->da - qproc->mpss_reloc;
+   size_t cp_size = size ? size : segment->size;


I can't find a scenario where size is 0 - can you explain when
such condition occurs?


There is no such case right now. I can get rid of the check.

Thanks,
Mathieu



void *ptr = NULL;

/* Unlock mba before copying segments */
@@ -1219,16 +1220,16 @@ static void qcom_q6v5_dump_segment(struct 
rproc *rproc,

}

if (!ret)
-   ptr = ioremap_wc(qproc->mpss_phys + offset, segment->size);
+   ptr = ioremap_wc(qproc->mpss_phys + offset + cp_offset, 
cp_size);

if (ptr) {
-   memcpy(dest, ptr, segment->size);
+   memcpy(dest, ptr, cp_size);
iounmap(ptr);
} else {
-   memset(dest, 0xff, segment->size);
+   memset(dest, 0xff, cp_size);
}

-   qproc->current_dump_size += segment->size;
+   qproc->current_dump_size += cp_size;

/* Reclaim mba after copying segments */
if (qproc->current_dump_size == qproc->total_dump_size) {
diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c

index ded0244..646886f 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  */

+#include 
 #include 
 #include 
 #include 
@@ -12,6 +13,12 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"

+struct rproc_coredump_state {
+   struct rproc *rproc;
+   void *header;
+   struct completion dump_done;
+};
+
 /**
  * rproc_coredump_cleanup() - clean up dump_segments list
  * @rproc: the remote processor handle
@@ -72,7 +79,8 @@ int rproc_coredump_add_custom_segment(struct rproc 
*rproc,

  dma_addr_t da, size_t size,
  void (*dumpfn)(struct rproc *rproc,
 struct rproc_dump_segment 
*segment,
-void *dest),
+void *dest, size_t offset,
+size_t size),
  void *priv)
 {
struct rproc_dump_segment *segment;
@@ -114,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc 
*rproc, u8 class, u16 machine)

 }
 EXPORT_SYMBOL(rproc_coredump_set_elf_info);

+static void rproc_coredump_free(void *data)
+{
+   struct rproc_coredump_state *dump_state = data;
+
+   complete(_state->dump_done);
+   vfree(dump_state->header);
+}
+
+static void *rproc_coredump_find_segment(loff_t user_offset,
+struct list_head *segments,
+size_t *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset < segment->size) {
+   

Re: [PATCH v7 1/5] dt-bindings: remoteproc: Add Qualcomm PIL info binding

[rishabhb]

On 2020-06-22 12:19, Bjorn Andersson wrote:

Add a devicetree binding for the Qualcomm peripheral image loader
relocation information region found in the IMEM.

Reviewed-by: Mathieu Poirier 
Reviewed-by: Rob Herring 
Reviewed-by: Stephen Boyd 
Reviewed-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v6:
- None

 .../bindings/remoteproc/qcom,pil-info.yaml| 44 +++
 1 file changed, 44 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml

diff --git
a/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
new file mode 100644
index ..87c52316ddbd
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,pil-info.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,pil-info.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm peripheral image loader relocation info binding
+
+maintainers:
+  - Bjorn Andersson 
+
+description:
+  The Qualcomm peripheral image loader relocation memory region, in 
IMEM, is
+  used for communicating remoteproc relocation information to post 
mortem

+  debugging tools.
+
+properties:
+  compatible:
+const: qcom,pil-reloc-info
+
+  reg:
+maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+imem@146bf000 {
+  compatible = "syscon", "simple-mfd";
+  reg = <0x146bf000 0x1000>;
+
+  #address-cells = <1>;
+  #size-cells = <1>;
+
+  ranges = <0 0x146bf000 0x1000>;
+
+  pil-reloc@94c {
+compatible = "qcom,pil-reloc-info";
+reg = <0x94c 0xc8>;
+  };
+};
+...

Reviewed-by: Rishabh Bhatnagar 

Re: [PATCH v7 3/5] remoteproc: qcom: Update PIL relocation info on load

[rishabhb]

On 2020-06-22 12:19, Bjorn Andersson wrote:

Update the PIL relocation information in IMEM with information about
where the firmware for various remoteprocs are loaded.

Reviewed-by: Vinod Koul 
Reviewed-by: Stephen Boyd 
Signed-off-by: Bjorn Andersson 
---

Changes since v6:
- None

 drivers/remoteproc/Kconfig  |  5 +
 drivers/remoteproc/qcom_q6v5_adsp.c | 16 +---
 drivers/remoteproc/qcom_q6v5_mss.c  |  3 +++
 drivers/remoteproc/qcom_q6v5_pas.c  | 15 ---
 drivers/remoteproc/qcom_q6v5_wcss.c | 14 +++---
 drivers/remoteproc/qcom_wcnss.c | 14 +++---
 6 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index f4bd96d1a1a3..3e8d5d1a2b9e 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -135,6 +135,7 @@ config QCOM_Q6V5_ADSP
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
+   select QCOM_PIL_INFO
select QCOM_MDT_LOADER
select QCOM_Q6V5_COMMON
select QCOM_RPROC_COMMON
@@ -151,6 +152,7 @@ config QCOM_Q6V5_MSS
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
select QCOM_MDT_LOADER
+   select QCOM_PIL_INFO
select QCOM_Q6V5_COMMON
select QCOM_Q6V5_IPA_NOTIFY
select QCOM_RPROC_COMMON
@@ -167,6 +169,7 @@ config QCOM_Q6V5_PAS
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
+   select QCOM_PIL_INFO
select QCOM_MDT_LOADER
select QCOM_Q6V5_COMMON
select QCOM_RPROC_COMMON
@@ -185,6 +188,7 @@ config QCOM_Q6V5_WCSS
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
select QCOM_MDT_LOADER
+   select QCOM_PIL_INFO
select QCOM_Q6V5_COMMON
select QCOM_RPROC_COMMON
select QCOM_SCM
@@ -218,6 +222,7 @@ config QCOM_WCNSS_PIL
depends on QCOM_SMEM
depends on QCOM_SYSMON || QCOM_SYSMON=n
select QCOM_MDT_LOADER
+   select QCOM_PIL_INFO
select QCOM_RPROC_COMMON
select QCOM_SCM
help
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
b/drivers/remoteproc/qcom_q6v5_adsp.c
index d2a2574dcf35..efb2c1aa80a3 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -26,6 +26,7 @@
 #include 

 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"

@@ -82,6 +83,7 @@ struct qcom_adsp {
unsigned int halt_lpass;

int crash_reason_smem;
+   const char *info_name;

struct completion start_done;
struct completion stop_done;
@@ -164,10 +166,17 @@ static int qcom_adsp_shutdown(struct qcom_adsp 
*adsp)

 static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 {
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+   int ret;
+
+   ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
+   adsp->mem_region, adsp->mem_phys,
+   adsp->mem_size, >mem_reloc);
+   if (ret)
+   return ret;
+
+   qcom_pil_info_store(adsp->info_name, adsp->mem_phys, adsp->mem_size);

-   return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
-adsp->mem_region, adsp->mem_phys, adsp->mem_size,
->mem_reloc);
+   return 0;
 }

 static int adsp_start(struct rproc *rproc)
@@ -436,6 +445,7 @@ static int adsp_probe(struct platform_device *pdev)
adsp = (struct qcom_adsp *)rproc->priv;
adsp->dev = >dev;
adsp->rproc = rproc;
+   adsp->info_name = desc->sysmon_name;
platform_set_drvdata(pdev, adsp);

ret = adsp_alloc_memory_region(adsp);
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
b/drivers/remoteproc/qcom_q6v5_mss.c
index 903b2bb97e12..4b8567f970f9 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -30,6 +30,7 @@

 #include "remoteproc_internal.h"
 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"

 #include 
@@ -1190,6 +1191,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
else if (ret < 0)
dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret);

+   qcom_pil_info_store("modem", qproc->mpss_phys, qproc->mpss_size);
+
 release_firmware:
release_firmware(fw);
 out:
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 61791a03f648..3837f23995e0 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -25,6 +25,7 @@
 #include 

 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"

@@ -64,6 +65,7 @@ struct qcom_adsp {
int 

Re: [PATCH v7 2/5] remoteproc: qcom: Introduce helper to store pil info in IMEM

[rishabhb]

On 2020-06-22 12:19, Bjorn Andersson wrote:

A region in IMEM is used to communicate load addresses of remoteproc to
post mortem debug tools. Implement a helper function that can be used 
to

store this information in order to enable these tools to process
collected ramdumps.

Reviewed-by: Mathieu Poirier 
Reviewed-by: Vinod Koul 
Signed-off-by: Bjorn Andersson 
---

Changes since v6:
- Replaced entry struct and usage of offset_of with a comment and
defined offsets
- Renamed pil_reloc_lock
- Write out upper 32 bits of the address
- Include header from implementation
- Add linux/types.h to the header file

 drivers/remoteproc/Kconfig |   3 +
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/qcom_pil_info.c | 129 +
 drivers/remoteproc/qcom_pil_info.h |   9 ++
 4 files changed, 142 insertions(+)
 create mode 100644 drivers/remoteproc/qcom_pil_info.c
 create mode 100644 drivers/remoteproc/qcom_pil_info.h

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index c4d1731295eb..f4bd96d1a1a3 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -116,6 +116,9 @@ config KEYSTONE_REMOTEPROC
  It's safe to say N here if you're not interested in the Keystone
  DSPs or just want to use a bare minimum kernel.

+config QCOM_PIL_INFO
+   tristate
+
 config QCOM_RPROC_COMMON
tristate

diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e8b886e511f0..fe398f82d550 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_OMAP_REMOTEPROC) += omap_remoteproc.o
 obj-$(CONFIG_WKUP_M3_RPROC)+= wkup_m3_rproc.o
 obj-$(CONFIG_DA8XX_REMOTEPROC) += da8xx_remoteproc.o
 obj-$(CONFIG_KEYSTONE_REMOTEPROC)  += keystone_remoteproc.o
+obj-$(CONFIG_QCOM_PIL_INFO)+= qcom_pil_info.o
 obj-$(CONFIG_QCOM_RPROC_COMMON)+= qcom_common.o
 obj-$(CONFIG_QCOM_Q6V5_COMMON) += qcom_q6v5.o
 obj-$(CONFIG_QCOM_Q6V5_ADSP)   += qcom_q6v5_adsp.o
diff --git a/drivers/remoteproc/qcom_pil_info.c
b/drivers/remoteproc/qcom_pil_info.c
new file mode 100644
index ..0536e3904669
--- /dev/null
+++ b/drivers/remoteproc/qcom_pil_info.c
@@ -0,0 +1,129 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2019-2020 Linaro Ltd.
+ */
+#include 
+#include 
+#include 
+#include 
+#include "qcom_pil_info.h"
+
+/*
+ * The PIL relocation information region is used to communicate memory 
regions

+ * occupied by co-processor firmware for post mortem crash analysis.
+ *
+ * It consists of an array of entries with an 8 byte textual 
identifier of the
+ * region followed by a 64 bit base address and 32 bit size, both 
little

+ * endian.
+ */
+#define PIL_RELOC_NAME_LEN 8
+#define PIL_RELOC_ENTRY_SIZE   (PIL_RELOC_NAME_LEN + sizeof(__le64) +
sizeof(__le32))
+
+struct pil_reloc {
+   void __iomem *base;
+   size_t num_entries;
+};
+
+static struct pil_reloc _reloc __read_mostly;
+static DEFINE_MUTEX(pil_reloc_lock);
+
+static int qcom_pil_info_init(void)
+{
+   struct device_node *np;
+   struct resource imem;
+   void __iomem *base;
+   int ret;
+
+   /* Already initialized? */
+   if (_reloc.base)
+   return 0;
+
+   np = of_find_compatible_node(NULL, NULL, "qcom,pil-reloc-info");
+   if (!np)
+   return -ENOENT;
+
+   ret = of_address_to_resource(np, 0, );
+   of_node_put(np);
+   if (ret < 0)
+   return ret;
+
+   base = ioremap(imem.start, resource_size());
+   if (!base) {
+   pr_err("failed to map PIL relocation info region\n");
+   return -ENOMEM;
+   }
+
+   memset_io(base, 0, resource_size());
+
+   _reloc.base = base;
+   _reloc.num_entries = resource_size() / PIL_RELOC_ENTRY_SIZE;
+
+   return 0;
+}
+
+/**
+ * qcom_pil_info_store() - store PIL information of image in IMEM
+ * @image: name of the image
+ * @base:  base address of the loaded image
+ * @size:  size of the loaded image
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+int qcom_pil_info_store(const char *image, phys_addr_t base, size_t 
size)

+{
+   char buf[PIL_RELOC_NAME_LEN];
+   void __iomem *entry;
+   int ret;
+   int i;
+
+   mutex_lock(_reloc_lock);
+   ret = qcom_pil_info_init();
+   if (ret < 0) {
+   mutex_unlock(_reloc_lock);
+   return ret;
+   }
+
+   for (i = 0; i < _reloc.num_entries; i++) {
+   entry = _reloc.base + i * PIL_RELOC_ENTRY_SIZE;
+
+   memcpy_fromio(buf, entry, PIL_RELOC_NAME_LEN);
+
+   /*
+* An empty record means we didn't find it, given that the
+* records are packed.
+*/
+   if (!buf[0])
+   goto found_unused;
+
+   if (!strncmp(buf, image, 

Re: [RESEND v1] soc: qcom: pdr: Reorder the PD state indication ack

[rishabhb]

On 2020-07-01 12:59, Sibi Sankar wrote:

The Protection Domains (PD) have a mechanism to keep its resources
enabled until the PD down indication is acked. Reorder the PD state
indication ack so that clients get to release the relevant resources
before the PD goes down.

Fixes: fbe639b44a82 ("soc: qcom: Introduce Protection Domain Restart 
helpers")

Reported-by: Rishabh Bhatnagar 
Signed-off-by: Sibi Sankar 
---

I couldn't find the previous patch on patchworks. Resending the patch
since it would need to land on stable trees as well

 drivers/soc/qcom/pdr_interface.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pdr_interface.c 
b/drivers/soc/qcom/pdr_interface.c

index a90d707da6894..088dc99f77f3f 100644
--- a/drivers/soc/qcom/pdr_interface.c
+++ b/drivers/soc/qcom/pdr_interface.c
@@ -279,13 +279,15 @@ static void pdr_indack_work(struct work_struct 
*work)


list_for_each_entry_safe(ind, tmp, >indack_list, node) {
pds = ind->pds;
-   pdr_send_indack_msg(pdr, pds, ind->transaction_id);

mutex_lock(>status_lock);
pds->state = ind->curr_state;
pdr->status(pds->state, pds->service_path, pdr->priv);
mutex_unlock(>status_lock);

+   /* Ack the indication after clients release the PD resources */
+   pdr_send_indack_msg(pdr, pds, ind->transaction_id);
+
mutex_lock(>list_lock);
list_del(>node);
mutex_unlock(>list_lock);


Reviewed-by: Rishabh Bhatnagar 

Thanks,
Rishabh

Re: [PATCH v6 2/3] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-07-06 10:47, Mathieu Poirier wrote:

On Mon, Jun 29, 2020 at 01:02:12PM -0700, Rishabh Bhatnagar wrote:

The current coredump implementation uses vmalloc area to copy
all the segments. But this might put strain on low memory targets
as the firmware size sometimes is in tens of MBs. The situation
becomes worse if there are multiple remote processors undergoing
recovery at the same time. This patch adds inline coredump
functionality that avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called.

Signed-off-by: Rishabh Bhatnagar 
---
 drivers/remoteproc/qcom_q6v5_mss.c   |   9 +-
 drivers/remoteproc/remoteproc_coredump.c | 160 
+++

 include/linux/remoteproc.h   |  21 +++-
 3 files changed, 165 insertions(+), 25 deletions(-)

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

index 903b2bb..d4ff9b8 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1200,12 +1200,13 @@ static int q6v5_mpss_load(struct q6v5 *qproc)

 static void qcom_q6v5_dump_segment(struct rproc *rproc,
   struct rproc_dump_segment *segment,
-  void *dest)
+  void *dest, size_t cp_offset, size_t size)
 {
int ret = 0;
struct q6v5 *qproc = rproc->priv;
unsigned long mask = BIT((unsigned long)segment->priv);
int offset = segment->da - qproc->mpss_reloc;
+   size_t cp_size = size ? size : segment->size;
void *ptr = NULL;


On the V4 of this set the above line was:

+   void *ptr = rproc_da_to_va(rproc, segment->da + offset, 
copy_size);


Back then both Bjorn and I had RB'ed this set and all that was required 
was a
rebase to linux-next.  On V5 and V6 our RBs have been removed, the 
above has
been changed and an iounmap() was been added below.  Yet nothing in the 
cover

letter provides an explanation that justifies the modification.

What is going on here?


Hi Mathieu,
The qcom_q6v5_dump_segment code has changed in the latest tip. That's 
why I had to
make modifications to my patch. Sibi reviewed this patch as he made the 
above
changes to mss driver and he has reviewed and tested from his side. He 
has
also created a minor fix in the driver to make inline dumps work for 
mss.

https://patchwork.kernel.org/patch/11637159/

I removed the reviewed-by because I made some modifications to this 
patch
and was not sure if the reviewed-by still applied. Let me know if i 
should add it

again.


/* Unlock mba before copying segments */
@@ -1221,13 +1222,13 @@ static void qcom_q6v5_dump_segment(struct 
rproc *rproc,

}

if (!ret)
-   ptr = ioremap_wc(qproc->mpss_phys + offset, segment->size);
+   ptr = ioremap_wc(qproc->mpss_phys + offset + cp_offset, 
cp_size);

if (ptr) {
-   memcpy(dest, ptr, segment->size);
+   memcpy(dest, ptr, cp_size);
iounmap(ptr);
} else {
-   memset(dest, 0xff, segment->size);
+   memset(dest, 0xff, cp_size);
}

qproc->dump_segment_mask |= mask;
diff --git a/drivers/remoteproc/remoteproc_coredump.c 
b/drivers/remoteproc/remoteproc_coredump.c

index ded0244..646886f 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2020, The Linux Foundation. All rights reserved.
  */

+#include 
 #include 
 #include 
 #include 
@@ -12,6 +13,12 @@
 #include "remoteproc_internal.h"
 #include "remoteproc_elf_helpers.h"

+struct rproc_coredump_state {
+   struct rproc *rproc;
+   void *header;
+   struct completion dump_done;
+};
+
 /**
  * rproc_coredump_cleanup() - clean up dump_segments list
  * @rproc: the remote processor handle
@@ -72,7 +79,8 @@ int rproc_coredump_add_custom_segment(struct rproc 
*rproc,

  dma_addr_t da, size_t size,
  void (*dumpfn)(struct rproc *rproc,
 struct rproc_dump_segment 
*segment,
-void *dest),
+void *dest, size_t offset,
+size_t size),
  void *priv)
 {
struct rproc_dump_segment *segment;
@@ -114,12 +122,110 @@ int rproc_coredump_set_elf_info(struct rproc 
*rproc, u8 class, u16 machine)

 }
 EXPORT_SYMBOL(rproc_coredump_set_elf_info);

+static void rproc_coredump_free(void *data)
+{
+   struct rproc_coredump_state *dump_state = data;
+
+   complete(_state->dump_done);
+   vfree(dump_state->header);
+}
+
+static void *rproc_coredump_find_segment(loff_t user_offset,
+struct 

Re: [PATCH v5 1/2] remoteproc: qcom: Add per subsystem SSR notification

[rishabhb]

On 2020-06-23 14:45, Alex Elder wrote:

On 6/22/20 8:04 PM, Rishabh Bhatnagar wrote:
Currently there is a single notification chain which is called 
whenever any
remoteproc shuts down. This leads to all the listeners being notified, 
and

is not an optimal design as kernel drivers might only be interested in
listening to notifications from a particular remoteproc. Create a 
global
list of remoteproc notification info data structures. This will hold 
the
name and notifier_list information for a particular remoteproc. The 
API

to register for notifications will use name argument to retrieve the
notification info data structure and the notifier block will be added 
to
that data structure's notification chain. Also move from blocking 
notifier

to srcu notifer based implementation to support dynamic notifier head
creation.

Signed-off-by: Siddharth Gupta 
Signed-off-by: Rishabh Bhatnagar 


Sorry, a few more comments, but I think your next one will
likely be fine.

General:
- SSR subsystems are added but never removed.  Note that
  "qcom_common.o" can be built as a module, and if that
  module were ever removed, memory allocated for these
  subsystems would be leaked.

Hi Alex,
Thank you for reviewing this patchset quickly. This point was
brought up by Bjorn and it was decided that I will push another patch on
top in which I'll do the cleanup during module exit.

- Will a remoteproc subdev (and in particular, an SSR subdev)
  ever be removed?  What happens to entities that have
  registered for SSR notifications in that case?

In practice it should never be removed. If it is clients will
never get notification about subsystem shutdown/powerup.


(Maybe these are issues that won't/can't occur in practice?)


---
  drivers/remoteproc/qcom_common.c  | 86 
+--

  drivers/remoteproc/qcom_common.h  |  5 +-
  include/linux/remoteproc/qcom_rproc.h | 20 ++--
  3 files changed, 91 insertions(+), 20 deletions(-)

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

index 9028cea..658f2ca 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -12,6 +12,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -23,7 +24,14 @@
  #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)

  -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
+struct qcom_ssr_subsystem {
+   const char *name;
+   struct srcu_notifier_head notifier_list;
+   struct list_head list;
+};
+
+static LIST_HEAD(qcom_ssr_subsystem_list);
+static DEFINE_MUTEX(qcom_ssr_subsys_lock);
static int glink_subdev_start(struct rproc_subdev *subdev)
  {
@@ -189,37 +197,80 @@ void qcom_remove_smd_subdev(struct rproc *rproc, 
struct qcom_rproc_subdev *smd)

  }
  EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
  +static struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char 
*name)

+{
+   struct qcom_ssr_subsystem *info;
+
+   mutex_lock(_ssr_subsys_lock);
+   /* Match in the global qcom_ssr_subsystem_list with name */
+   list_for_each_entry(info, _ssr_subsystem_list, list)
+   if (!strcmp(info->name, name))
+   return info;


You need to unlock the mutex here.  You would probably
be better off structuring this with a common exit path
below, for example:

if (!strcmp(info->name, name))
goto out_mutex_unlock;

. . .

out_mutex_unlock:
mutex_unlock(_ssr_subsys_lock);

return info;
}


+
+   info = kzalloc(sizeof(*info), GFP_KERNEL);
+   if (!info)
+   return ERR_PTR(-ENOMEM);


Here too.  Perhaps this:

if (!info) {
info = ERR_PTR(-ENOMEM);
goto out_mutex_unlock;
}


+   info->name = kstrdup_const(name, GFP_KERNEL);
+   srcu_init_notifier_head(>notifier_list);
+
+   /* Add to global notification list */
+   list_add_tail(>list, _ssr_subsystem_list);
+   mutex_unlock(_ssr_subsys_lock);
+
+   return info;
+}
+
  /**
   * qcom_register_ssr_notifier() - register SSR notification handler
- * @nb:notifier_block to notify for restart notifications
+ * @name:  Subsystem's SSR name
+ * @nb:notifier_block to be invoked upon subsystem's state 
change
   *
- * Returns 0 on success, negative errno on failure.
+ * This registers the @nb notifier block as part the notifier chain 
for a

+ * remoteproc associated with @name. The notifier block's callback
+ * will be invoked when the remote processor's SSR events occur
+ * (pre/post startup and pre/post shutdown).
   *
- * This register the @notify function as handler for restart 
notifications. As
- * remote processors are stopped this function will be called, with 
the SSR

- * name passed as a parameter.
+ * Return: a subsystem cookie on success, 

Re: [v4 PATCH 0/3] Extend coredump functionality

[rishabhb]

On 2020-06-22 15:23, Bjorn Andersson wrote:

On Wed 27 May 13:26 PDT 2020, Rishabh Bhatnagar wrote:


This patch series moves the coredump functionality to a separate
file and adds "inline" coredump feature. Inline coredump directly
copies segments from device memory during coredump to userspace.
This avoids extra memory usage at the cost of speed. Recovery is
stalled until all data is read by userspace.

Changelog:



Hi Rishabh,

This looks good to me, but it doesn't apply cleanly on linux-next. Can
you please take a look?

Regards,
Bjorn

Yes some more gerrits have been merged in the meantime. I'll rebase on 
top

of Linux-next and send out another patchset.

v4 -> v3:
- Write a helper function to copy segment memory for every dump format
- Change segment dump fn to add offset and size adn covert mss driver

v3 -> v2:
- Move entire coredump functionality to remoteproc_coredump.c
- Modify rproc_coredump to perform dump according to conf. set by 
userspace

- Move the userspace configuration to debugfs from sysfs.
- Keep the default coredump implementation as is

v2 -> v1:
- Introduce new file for coredump.
- Add userspace sysfs configuration for dump type.

Rishabh Bhatnagar (3):
  remoteproc: Move coredump functionality to a new file
  remoteproc: Add inline coredump functionality
  remoteproc: Add coredump debugfs entry

 drivers/remoteproc/Makefile  |   1 +
 drivers/remoteproc/qcom_q6v5_mss.c   |   9 +-
 drivers/remoteproc/remoteproc_core.c | 191 --
 drivers/remoteproc/remoteproc_coredump.c | 328 
+++

 drivers/remoteproc/remoteproc_debugfs.c  |  86 
 drivers/remoteproc/remoteproc_internal.h |   4 +
 include/linux/remoteproc.h   |  21 +-
 7 files changed, 443 insertions(+), 197 deletions(-)
 create mode 100644 drivers/remoteproc/remoteproc_coredump.c

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH v4 1/2] remoteproc: qcom: Add per subsystem SSR notification

[rishabhb]

On 2020-06-18 16:35, Bjorn Andersson wrote:

On Thu 18 Jun 16:00 PDT 2020, Alex Elder wrote:


On 5/27/20 10:34 PM, Rishabh Bhatnagar wrote:
> Currently there is a single notification chain which is called whenever any
> remoteproc shuts down. This leads to all the listeners being notified, and
> is not an optimal design as kernel drivers might only be interested in
> listening to notifications from a particular remoteproc. Create a global
> list of remoteproc notification info data structures. This will hold the
> name and notifier_list information for a particular remoteproc. The API
> to register for notifications will use name argument to retrieve the
> notification info data structure and the notifier block will be added to
> that data structure's notification chain. Also move from blocking notifier
> to srcu notifer based implementation to support dynamic notifier head
> creation.

I'm looking at these patches now, without having reviewed the
previous versions.  Forgive me if I contradict or duplicate
previous feedback.

I have a number of suggestions, below.

-Alex



Thanks for your review Alex, some feedback on the patch and your
response below.


> Signed-off-by: Siddharth Gupta 
> Signed-off-by: Rishabh Bhatnagar 
> ---
>   drivers/remoteproc/qcom_common.c  | 84 
++-
>   drivers/remoteproc/qcom_common.h  |  5 ++-
>   include/linux/remoteproc/qcom_rproc.h | 20 ++---
>   3 files changed, 90 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_common.c 
b/drivers/remoteproc/qcom_common.c
> index 9028cea..61ff2dd 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -12,6 +12,7 @@
>   #include 
>   #include 
>   #include 
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -23,7 +24,14 @@
>   #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)
> -static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> +struct qcom_ssr_subsystem {
> +  const char *name;
> +  struct srcu_notifier_head notifier_list;
> +  struct list_head list;
> +};
> +
> +static LIST_HEAD(qcom_ssr_subsystem_list);
> +DEFINE_MUTEX(qcom_ssr_subsys_lock);

There is no need for this mutex to be global.

>   static int glink_subdev_start(struct rproc_subdev *subdev)
>   {
> @@ -189,39 +197,79 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct 
qcom_rproc_subdev *smd)
>   }
>   EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> +struct qcom_ssr_subsystem *qcom_ssr_get_subsys(const char *name)

This function should be made private (static).



Yes.


I think the mutex should be taken in this function rather than
the caller (more on this below).  But if you leave it this way,
please mention something in a comment that indicates the caller
must hold the qcom_ssr_subsys_lock mutex.



I agree, that would simplify reasoning about the lock.


> +{
> +  struct qcom_ssr_subsystem *info;
> +
> +  /* Match in the global qcom_ssr_subsystem_list with name */
> +  list_for_each_entry(info, _ssr_subsystem_list, list) {
> +  if (!strcmp(info->name, name))
> +  return info;

This probably isn't strictly necessary, because you are
returning a void pointer, but you could do this here:
return ERR_CAST(info);


Info is a struct qcom_ssr_subsystem * and that's the function's return
type as well, so Rishabh's approach looks correct to me.



> +  }

This is purely a style thing, but the curly braces around
the loop body aren't necessary.

> +  info = kzalloc(sizeof(*info), GFP_KERNEL);
> +  if (!info)
> +  return ERR_PTR(-ENOMEM);
> +  info->name = kstrdup_const(name, GFP_KERNEL);
> +  srcu_init_notifier_head(>notifier_list);
> +
> +  /* Add to global notif list */

s/notif/notification/

> +  INIT_LIST_HEAD(>list);

No need to initialize the list element when adding it
to a list.  Both uts fields will be overwritten anyway.

> +  list_add_tail(>list, _ssr_subsystem_list);
> +
> +  return info;
> +}
> +
>   /**
>* qcom_register_ssr_notifier() - register SSR notification handler
> + * @name: name that will be searched in global ssr subsystem list

Maybe just "SSR subsystem name".

>* @nb:  notifier_block to notify for restart notifications

Drop or modify "restart" in the comment line above.

>*
> - * Returns 0 on success, negative errno on failure.
> + * Returns a subsystem cookie on success, ERR_PTR on failure.

Maybe make the above a @Return: comment.



No @ in that, but "Return: foo" is the appropriate format.


>*
> - * This register the @notify function as handler for restart notifications. 
As
> - * remote processors are stopped this function will be called, with the SSR
> - * name passed as a parameter.
> + * This registers the @nb notifier block as part the notifier chain for a
> + * remoteproc associated with @name. The notifier 

Re: [PATCH v3 1/2] remoteproc: Add remoteproc character device interface

[rishabhb]

On 2020-04-30 01:30, Arnaud POULIQUEN wrote:

Hi Rishabh,


On 4/21/20 8:10 PM, Rishabh Bhatnagar wrote:

Add the character device interface into remoteproc framework.
This interface can be used in order to boot/shutdown remote
subsystems and provides a basic ioctl based interface to implement
supplementary functionality. An ioctl call is implemented to enable
the shutdown on release feature which will allow remote processors to
be shutdown when the controlling userpsace application crashes or 
hangs.




Thanks for intruducing Ioctl, this will help for future evolutions.


Signed-off-by: Rishabh Bhatnagar 
---
 Documentation/userspace-api/ioctl/ioctl-number.rst |   1 +
 drivers/remoteproc/Kconfig |   9 ++
 drivers/remoteproc/Makefile|   1 +
 drivers/remoteproc/remoteproc_cdev.c   | 143 
+

 drivers/remoteproc/remoteproc_internal.h   |  21 +++
 include/linux/remoteproc.h |   3 +
 include/uapi/linux/remoteproc_cdev.h   |  20 +++
 7 files changed, 198 insertions(+)
 create mode 100644 drivers/remoteproc/remoteproc_cdev.c
 create mode 100644 include/uapi/linux/remoteproc_cdev.h

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst 
b/Documentation/userspace-api/ioctl/ioctl-number.rst

index 2e91370..412b2a0 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -337,6 +337,7 @@ Code  Seq#Include File 
  Comments
 0xB4  00-0F  linux/gpio.h

 0xB5  00-0F  uapi/linux/rpmsg.h  


 0xB6  alllinux/fpga-dfl.h
+0xB7  alluapi/linux/remoteproc_cdev.h			 


 0xC0  00-0F  linux/usb/iowarrior.h
 0xCA  00-0F  uapi/misc/cxl.h
 0xCA  10-2F  uapi/misc/ocxl.h
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index de3862c..6374b79 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -14,6 +14,15 @@ config REMOTEPROC

 if REMOTEPROC

+config REMOTEPROC_CDEV
+   bool "Remoteproc character device interface"
+   help
+ Say y here to have a character device interface for Remoteproc
+ framework. Userspace can boot/shutdown remote processors through
+ this interface.
+
+ It's safe to say N if you don't want to use this interface.
+
 config IMX_REMOTEPROC
tristate "IMX6/7 remoteproc support"
depends on ARCH_MXC
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index e30a1b1..b7d4f77 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -9,6 +9,7 @@ remoteproc-y+= remoteproc_debugfs.o
 remoteproc-y   += remoteproc_sysfs.o
 remoteproc-y   += remoteproc_virtio.o
 remoteproc-y   += remoteproc_elf_loader.o
+obj-$(CONFIG_REMOTEPROC_CDEV)  += remoteproc_cdev.o
 obj-$(CONFIG_IMX_REMOTEPROC)   += imx_rproc.o
 obj-$(CONFIG_MTK_SCP)  += mtk_scp.o mtk_scp_ipi.o
 obj-$(CONFIG_OMAP_REMOTEPROC)  += omap_remoteproc.o
diff --git a/drivers/remoteproc/remoteproc_cdev.c 
b/drivers/remoteproc/remoteproc_cdev.c

new file mode 100644
index 000..65142ec
--- /dev/null
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Character device interface driver for Remoteproc framework.
+ *
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "remoteproc_internal.h"
+
+#define NUM_RPROC_DEVICES  64
+static dev_t rproc_major;
+
+static ssize_t rproc_cdev_write(struct file *filp, const char __user 
*buf,

+size_t len, loff_t *pos)
+{
+   struct rproc *rproc = container_of(filp->f_inode->i_cdev,
+  struct rproc, char_dev);
+   int ret = 0;
+   char cmd[10];
+
+   if (!len || len > sizeof(cmd))
+   return -EINVAL;
+
+   ret = copy_from_user(cmd, buf, sizeof(cmd));
+   if (ret)
+   return -EFAULT;
+
+   if (sysfs_streq(cmd, "start")) {
+   if (rproc->state == RPROC_RUNNING)
+   return -EBUSY;
+
+   ret = rproc_boot(rproc);
+   if (ret)
+   dev_err(>dev, "Boot failed:%d\n", ret);
+   } else if (sysfs_streq(cmd, "stop")) {
+   if (rproc->state == RPROC_OFFLINE)
+   return -ENXIO;


returning ENXIO in this case seems to me no appropriate , what about 
EPERM or

EINVAL (rproc_sysfs) ?


+
+   rproc_shutdown(rproc);
+   } else {

Re: [PATCH v3 1/2] remoteproc: qcom: Add per subsystem SSR notification

[rishabhb]

On 2020-05-19 13:38, Bjorn Andersson wrote:

On Tue 28 Apr 15:16 PDT 2020, Rishabh Bhatnagar wrote:

Currently there is a single notification chain which is called 
whenever any
remoteproc shuts down. This leads to all the listeners being notified, 
and

is not an optimal design as kernel drivers might only be interested in
listening to notifications from a particular remoteproc. Create a 
global
list of remoteproc notification info data structures. This will hold 
the
name and notifier_list information for a particular remoteproc. The 
API

to register for notifications will use name argument to retrieve the
notification info data structure and the notifier block will be added 
to

that data structure's notification chain.

Signed-off-by: Siddharth Gupta 
Signed-off-by: Rishabh Bhatnagar 


Thanks Rishabh, design wise I think this looks good now, just some code
style things below.


---
 drivers/remoteproc/qcom_common.c  | 89 
++-

 drivers/remoteproc/qcom_common.h  | 10 +++-
 include/linux/remoteproc/qcom_rproc.h | 20 ++--
 3 files changed, 99 insertions(+), 20 deletions(-)

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

index 60650bc..7cd17be 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -15,16 +15,18 @@
 #include 
 #include 
 #include 
+#include 


Please maintain alphabetical sort order.



 #include "remoteproc_internal.h"
 #include "qcom_common.h"

+#define MAX_NAME_LEN   20
+DEFINE_MUTEX(rproc_notif_lock);


Please rename this qcom_ssr_subsystem_lock


+
 #define to_glink_subdev(d) container_of(d, struct qcom_rproc_glink, 
subdev)
 #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)


-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);


Move the definition of rproc_notif_info, the new rproc_notif_info list
head and move the two lines above here as well.


-
 static int glink_subdev_start(struct rproc_subdev *subdev)
 {
struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
@@ -174,39 +176,81 @@ void qcom_remove_smd_subdev(struct rproc *rproc, 
struct qcom_rproc_subdev *smd)

 }
 EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);

+struct rproc_notif_info *find_notif_info(const char *name)


Please make this qcom_ssr_get_subsystem(const char *name)


+{
+   struct rproc_notif_info *info;
+
+   /* Match in the global rproc_notif_list with name */
+   list_for_each_entry(info, _notif_list, list) {
+   if (!strncmp(info->name, name, strlen(name)))


strncmp(a, b, strlen(b)) is the same thing as strcmp(a, b), unless a is
shorted than b and not NUL terminated.


+   return info;
+   }
+   return NULL;


Both callers of this function will if NULL is returned allocate a new
subsystem object and attach to the list. If you do that here you can
remove the duplication between these.


+}
+
 /**
  * qcom_register_ssr_notifier() - register SSR notification handler
+ * @name:	pointer to name which will be searched in the global 
notif_list

  * @nb:notifier_block to notify for restart notifications
  *
- * Returns 0 on success, negative errno on failure.
+ * Returns pointer to srcu notifier head on success, ERR_PTR on 
failure.


This shouldn't mention that the opaque pointer is of a type standard to
the kernel. Better just say that it returns a "subsystem cookie".


  *
- * This register the @notify function as handler for restart 
notifications. As
- * remote processors are stopped this function will be called, with 
the SSR

- * name passed as a parameter.
+ * This registers the @nb notifier block as part the notifier chain 
for a

+ * remoteproc associated with @name. The notifier block's callback
+ * will be invoked when the particular remote processor is stopped.
  */
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(const char *name, struct 
notifier_block *nb)

 {
-   return blocking_notifier_chain_register(_notifiers, nb);
+   struct rproc_notif_info *info;
+
+   mutex_lock(_notif_lock);
+   info = find_notif_info(name);
+   if (!info) {
+   info = kzalloc(sizeof(*info), GFP_KERNEL);
+   if (!info) {
+   mutex_unlock(_notif_lock);
+   return ERR_PTR(-ENOMEM);
+   }
+   info->name = kstrndup(name, MAX_NAME_LEN, GFP_KERNEL);


This is going to be a constant in a lot of cases, so please use
kstrdup_const(). Also what's the purpose of limiting the length of 
this?



+   srcu_init_notifier_head(>notifier_list);
+
+   /* Add to global notif list */
+   INIT_LIST_HEAD(>list);
+   list_add_tail(>list, _notif_list);
+   }
+
+   srcu_notifier_chain_register(>notifier_list, nb);
+   mutex_unlock(_notif_lock);
+   return 

Re: [PATCH v5 3/5] remoteproc: qcom: Update PIL relocation info on load

[rishabhb]

On 2020-05-12 22:56, Bjorn Andersson wrote:

Update the PIL relocation information in IMEM with information about
where the firmware for various remoteprocs are loaded.

Signed-off-by: Bjorn Andersson 
---

Changes since v4:
- Dropped unnecessary comment about ignoring return value.

 drivers/remoteproc/Kconfig  |  3 +++
 drivers/remoteproc/qcom_q6v5_adsp.c | 16 +---
 drivers/remoteproc/qcom_q6v5_mss.c  |  3 +++
 drivers/remoteproc/qcom_q6v5_pas.c  | 15 ---
 drivers/remoteproc/qcom_q6v5_wcss.c | 14 +++---
 drivers/remoteproc/qcom_wcnss.c | 14 +++---
 6 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 8088ca4dd6dc..6bd42a411ca8 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -126,6 +126,7 @@ config QCOM_Q6V5_ADSP
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
+   select QCOM_PIL_INFO
select QCOM_MDT_LOADER
select QCOM_Q6V5_COMMON
select QCOM_RPROC_COMMON
@@ -158,6 +159,7 @@ config QCOM_Q6V5_PAS
depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
depends on QCOM_SYSMON || QCOM_SYSMON=n
select MFD_SYSCON
+   select QCOM_PIL_INFO
select QCOM_MDT_LOADER
select QCOM_Q6V5_COMMON
select QCOM_RPROC_COMMON
@@ -209,6 +211,7 @@ config QCOM_WCNSS_PIL
depends on QCOM_SMEM
depends on QCOM_SYSMON || QCOM_SYSMON=n
select QCOM_MDT_LOADER
+   select QCOM_PIL_INFO
select QCOM_RPROC_COMMON
select QCOM_SCM
help
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
b/drivers/remoteproc/qcom_q6v5_adsp.c
index d2a2574dcf35..c539e89664cb 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -26,6 +26,7 @@
 #include 

 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"

@@ -82,6 +83,7 @@ struct qcom_adsp {
unsigned int halt_lpass;

int crash_reason_smem;
+   const char *info_name;

struct completion start_done;
struct completion stop_done;
@@ -164,10 +166,17 @@ static int qcom_adsp_shutdown(struct qcom_adsp 
*adsp)

 static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 {
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+   int ret;
+
+   ret = qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
+   adsp->mem_region, adsp->mem_phys,
+   adsp->mem_size, >mem_reloc);
+   if (ret)
+   return ret;
+
+	qcom_pil_info_store(adsp->info_name, adsp->mem_reloc, 
adsp->mem_size);


-   return qcom_mdt_load_no_init(adsp->dev, fw, rproc->firmware, 0,
-adsp->mem_region, adsp->mem_phys, adsp->mem_size,
->mem_reloc);
+   return 0;
 }

 static int adsp_start(struct rproc *rproc)
@@ -436,6 +445,7 @@ static int adsp_probe(struct platform_device *pdev)
adsp = (struct qcom_adsp *)rproc->priv;
adsp->dev = >dev;
adsp->rproc = rproc;
+   adsp->info_name = desc->sysmon_name;
platform_set_drvdata(pdev, adsp);

ret = adsp_alloc_memory_region(adsp);
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
b/drivers/remoteproc/qcom_q6v5_mss.c
index c4936f4d1e80..fdbcae11ae64 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -29,6 +29,7 @@

 #include "remoteproc_internal.h"
 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"

 #include 
@@ -1221,6 +1222,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
else if (ret < 0)
dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret);

+   qcom_pil_info_store("modem", mpss_reloc, qproc->mpss_size);
+
 release_firmware:
release_firmware(fw);
 out:
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 3bb69f58e086..84cb19231c35 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -25,6 +25,7 @@
 #include 

 #include "qcom_common.h"
+#include "qcom_pil_info.h"
 #include "qcom_q6v5.h"
 #include "remoteproc_internal.h"

@@ -64,6 +65,7 @@ struct qcom_adsp {
int pas_id;
int crash_reason_smem;
bool has_aggre2_clk;
+   const char *info_name;

struct completion start_done;
struct completion stop_done;
@@ -117,11 +119,17 @@ static void adsp_pds_disable(struct qcom_adsp
*adsp, struct device **pds,
 static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 {
struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+   int ret;

-   return qcom_mdt_load(adsp->dev, fw, rproc->firmware, adsp->pas_id,
-adsp->mem_region, 

Re: [PATCH 2/3] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-05-07 13:21, Bjorn Andersson wrote:

On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:


This patch adds the inline coredump functionality. The current
coredump implementation uses vmalloc area to copy all the segments.
But this might put a lot of strain on low memory targets as the
firmware size sometimes is in ten's of MBs. The situation becomes
worse if there are multiple remote processors  undergoing recovery
at the same time. This patch directly copies the device memory to
userspace buffer and avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called.

Signed-off-by: Rishabh Bhatnagar 
---
 drivers/remoteproc/remoteproc_coredump.c | 130 
+++

 drivers/remoteproc/remoteproc_internal.h |  23 +-
 include/linux/remoteproc.h   |   2 +
 3 files changed, 153 insertions(+), 2 deletions(-)

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

index 9de0467..888b7dec91 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -12,6 +12,84 @@
 #include 
 #include "remoteproc_internal.h"

+static void rproc_free_dump(void *data)


rproc_coredump_free()


+{
+   struct rproc_coredump_state *dump_state = data;
+
+   complete(_state->dump_done);


vfree(dump_state->header);


+}
+
+static unsigned long resolve_addr(loff_t user_offset,


rproc_coredump_find_segment()


+  struct list_head *segments,
+  unsigned long *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset >= segment->size)
+   user_offset -= segment->size;
+   else
+   break;


if (user_offset < segment->size) {
*data_left = segment->size - user_offset;
return segment->da + user_offset;
}

user_offset -= segment->size;

+   }


*data_left = 0;
return 0;


+
+   if (>node == segments) {
+   *data_left = 0;
+   return 0;
+   }
+
+   *data_left = segment->size - user_offset;
+
+   return segment->da + user_offset;
+}
+
+static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t 
count,

+   void *data, size_t header_size)
+{
+   void *device_mem;
+   size_t data_left, copy_size, bytes_left = count;
+   unsigned long addr;
+   struct rproc_coredump_state *dump_state = data;
+   struct rproc *rproc = dump_state->rproc;
+   void *elfcore = dump_state->header;
+
+   /* Copy the header first */
+   if (offset < header_size) {
+   copy_size = header_size - offset;
+   copy_size = min(copy_size, bytes_left);
+
+   memcpy(buffer, elfcore + offset, copy_size);
+   offset += copy_size;
+   bytes_left -= copy_size;
+   buffer += copy_size;
+   }


Perhaps you can take inspiration from devcd_readv() here?


+
+   while (bytes_left) {
+   addr = resolve_addr(offset - header_size,
+   >dump_segments, _left);
+   /* EOF check */
+   if (data_left == 0) {


Afaict data_left denotes the amount of data left in this particular
segment, rather than in the entire core.

I think you should start by making bytes_left the minimum of the core
size and @count and then have this loop as long as bytes_left, copying
data to the buffer either from header or an appropriate segment based 
on

the current offset.


+   pr_info("Ramdump complete %lld bytes read", offset);


dev_dbg(>dev, ...)


+   break;
+   }
+
+   copy_size = min_t(size_t, bytes_left, data_left);
+
+   device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);


rproc_da_to_va()


+   if (!device_mem) {
+   pr_err("Address:%lx with size %zd out of remoteproc 
carveout\n",


dev_err(>dev, "coredump: %#lx size %#zx outside of carveouts\n",
..);


+   addr, copy_size);
+   return -ENOMEM;
+   }
+   memcpy(buffer, device_mem, copy_size);
+
+   offset += copy_size;
+   buffer += copy_size;
+   bytes_left -= copy_size;
+   }
+
+   return count - bytes_left;
+}
+
 static void create_elf_header(void *data, int phnum, struct rproc 
*rproc)

 {
struct elf32_phdr *phdr;
@@ -55,6 +133,58 @@ static void create_elf_header(void *data, int 
phnum, struct rproc *rproc)

 }

 /**
+ * rproc_inline_coredump() - perform synchronized coredump
+ * @rproc: rproc handle
+ *
+ * This function will generate an ELF header for 

Re: [PATCH 2/3] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-05-11 17:30, Bjorn Andersson wrote:

On Mon 11 May 17:11 PDT 2020, risha...@codeaurora.org wrote:


On 2020-05-07 13:21, Bjorn Andersson wrote:
> On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:
>
> > This patch adds the inline coredump functionality. The current
> > coredump implementation uses vmalloc area to copy all the segments.
> > But this might put a lot of strain on low memory targets as the
> > firmware size sometimes is in ten's of MBs. The situation becomes
> > worse if there are multiple remote processors  undergoing recovery
> > at the same time. This patch directly copies the device memory to
> > userspace buffer and avoids extra memory usage. This requires
> > recovery to be halted until data is read by userspace and free
> > function is called.
> >
> > Signed-off-by: Rishabh Bhatnagar 
> > ---
> >  drivers/remoteproc/remoteproc_coredump.c | 130
> > +++
> >  drivers/remoteproc/remoteproc_internal.h |  23 +-
> >  include/linux/remoteproc.h   |   2 +
> >  3 files changed, 153 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_coredump.c
> > b/drivers/remoteproc/remoteproc_coredump.c
> > index 9de0467..888b7dec91 100644
> > --- a/drivers/remoteproc/remoteproc_coredump.c
> > +++ b/drivers/remoteproc/remoteproc_coredump.c
> > @@ -12,6 +12,84 @@
> >  #include 
> >  #include "remoteproc_internal.h"
> >
> > +static void rproc_free_dump(void *data)
>
> rproc_coredump_free()
>
> > +{
> > + struct rproc_coredump_state *dump_state = data;
> > +
> > + complete(_state->dump_done);
>
> vfree(dump_state->header);
>
> > +}
> > +
> > +static unsigned long resolve_addr(loff_t user_offset,
>
> rproc_coredump_find_segment()
>
> > +struct list_head *segments,
> > +unsigned long *data_left)
> > +{
> > + struct rproc_dump_segment *segment;
> > +
> > + list_for_each_entry(segment, segments, node) {
> > + if (user_offset >= segment->size)
> > + user_offset -= segment->size;
> > + else
> > + break;
>
>if (user_offset < segment->size) {
>*data_left = segment->size - user_offset;
>return segment->da + user_offset;
>}
>
>user_offset -= segment->size;
> > + }
>
>*data_left = 0;
>return 0;
>
> > +
> > + if (>node == segments) {
> > + *data_left = 0;
> > + return 0;
> > + }
> > +
> > + *data_left = segment->size - user_offset;
> > +
> > + return segment->da + user_offset;
> > +}
> > +
> > +static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t
> > count,
> > + void *data, size_t header_size)
> > +{
> > + void *device_mem;
> > + size_t data_left, copy_size, bytes_left = count;
> > + unsigned long addr;
> > + struct rproc_coredump_state *dump_state = data;
> > + struct rproc *rproc = dump_state->rproc;
> > + void *elfcore = dump_state->header;
> > +
> > + /* Copy the header first */
> > + if (offset < header_size) {
> > + copy_size = header_size - offset;
> > + copy_size = min(copy_size, bytes_left);
> > +
> > + memcpy(buffer, elfcore + offset, copy_size);
> > + offset += copy_size;
> > + bytes_left -= copy_size;
> > + buffer += copy_size;
> > + }
>
> Perhaps you can take inspiration from devcd_readv() here?
>
> > +
> > + while (bytes_left) {
> > + addr = resolve_addr(offset - header_size,
> > + >dump_segments, _left);
> > + /* EOF check */
> > + if (data_left == 0) {
>
> Afaict data_left denotes the amount of data left in this particular
> segment, rather than in the entire core.
>
Yes, but it only returns 0 when the final segment has been copied
completely.  Otherwise it gives data left to copy for every segment
and moves to next segment once the current one is copied.


You're right.


> I think you should start by making bytes_left the minimum of the core
> size and @count and then have this loop as long as bytes_left, copying
> data to the buffer either from header or an appropriate segment based on
> the current offset.
>
That would require an extra function that calculates entire core size,
as its not available right now. Do you see any missed corner cases 
with this

approach?


You're looping over all the segments as you're building the header
anyways, so you could simply store this in the dump_state. I think this
depend more on the ability to reuse the read function between inline 
and

default coredump.

Regards,
Bjorn


Wouldn't the first if condition take care of "default" dump as it is?
The header_size in that case would involve the 'header + all segments'.



> > + pr_info("Ramdump complete %lld bytes read", offset);
>

Re: [PATCH 2/3] remoteproc: Add inline coredump functionality

[rishabhb]

On 2020-05-07 13:21, Bjorn Andersson wrote:

On Thu 16 Apr 11:38 PDT 2020, Rishabh Bhatnagar wrote:


This patch adds the inline coredump functionality. The current
coredump implementation uses vmalloc area to copy all the segments.
But this might put a lot of strain on low memory targets as the
firmware size sometimes is in ten's of MBs. The situation becomes
worse if there are multiple remote processors  undergoing recovery
at the same time. This patch directly copies the device memory to
userspace buffer and avoids extra memory usage. This requires
recovery to be halted until data is read by userspace and free
function is called.

Signed-off-by: Rishabh Bhatnagar 
---
 drivers/remoteproc/remoteproc_coredump.c | 130 
+++

 drivers/remoteproc/remoteproc_internal.h |  23 +-
 include/linux/remoteproc.h   |   2 +
 3 files changed, 153 insertions(+), 2 deletions(-)

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

index 9de0467..888b7dec91 100644
--- a/drivers/remoteproc/remoteproc_coredump.c
+++ b/drivers/remoteproc/remoteproc_coredump.c
@@ -12,6 +12,84 @@
 #include 
 #include "remoteproc_internal.h"

+static void rproc_free_dump(void *data)


rproc_coredump_free()


+{
+   struct rproc_coredump_state *dump_state = data;
+
+   complete(_state->dump_done);


vfree(dump_state->header);


+}
+
+static unsigned long resolve_addr(loff_t user_offset,


rproc_coredump_find_segment()


+  struct list_head *segments,
+  unsigned long *data_left)
+{
+   struct rproc_dump_segment *segment;
+
+   list_for_each_entry(segment, segments, node) {
+   if (user_offset >= segment->size)
+   user_offset -= segment->size;
+   else
+   break;


if (user_offset < segment->size) {
*data_left = segment->size - user_offset;
return segment->da + user_offset;
}

user_offset -= segment->size;

+   }


*data_left = 0;
return 0;


+
+   if (>node == segments) {
+   *data_left = 0;
+   return 0;
+   }
+
+   *data_left = segment->size - user_offset;
+
+   return segment->da + user_offset;
+}
+
+static ssize_t rproc_read_dump(char *buffer, loff_t offset, size_t 
count,

+   void *data, size_t header_size)
+{
+   void *device_mem;
+   size_t data_left, copy_size, bytes_left = count;
+   unsigned long addr;
+   struct rproc_coredump_state *dump_state = data;
+   struct rproc *rproc = dump_state->rproc;
+   void *elfcore = dump_state->header;
+
+   /* Copy the header first */
+   if (offset < header_size) {
+   copy_size = header_size - offset;
+   copy_size = min(copy_size, bytes_left);
+
+   memcpy(buffer, elfcore + offset, copy_size);
+   offset += copy_size;
+   bytes_left -= copy_size;
+   buffer += copy_size;
+   }


Perhaps you can take inspiration from devcd_readv() here?


+
+   while (bytes_left) {
+   addr = resolve_addr(offset - header_size,
+   >dump_segments, _left);
+   /* EOF check */
+   if (data_left == 0) {


Afaict data_left denotes the amount of data left in this particular
segment, rather than in the entire core.

Yes, but it only returns 0 when the final segment has been copied 
completely.
Otherwise it gives data left to copy for every segment and moves to next 
segment

once the current one is copied.

I think you should start by making bytes_left the minimum of the core
size and @count and then have this loop as long as bytes_left, copying
data to the buffer either from header or an appropriate segment based 
on

the current offset.


That would require an extra function that calculates entire core size,
as its not available right now. Do you see any missed corner cases with 
this

approach?

+   pr_info("Ramdump complete %lld bytes read", offset);


dev_dbg(>dev, ...)


+   break;
+   }
+
+   copy_size = min_t(size_t, bytes_left, data_left);
+
+   device_mem = rproc->ops->da_to_va(rproc, addr, copy_size);


rproc_da_to_va()


+   if (!device_mem) {
+   pr_err("Address:%lx with size %zd out of remoteproc 
carveout\n",


dev_err(>dev, "coredump: %#lx size %#zx outside of carveouts\n",
..);


+   addr, copy_size);
+   return -ENOMEM;
+   }
+   memcpy(buffer, device_mem, copy_size);
+
+   offset += copy_size;
+   buffer += copy_size;
+   bytes_left -= copy_size;
+   }
+
+   return count - bytes_left;
+}
+
 

Re: [PATCH 4/4] arm64: defconfig: Remove QCOM_GLINK_SSR

[rishabhb]

On 2020-04-22 17:37, Bjorn Andersson wrote:
Remove the QCOM_GLINK_SSR option from the arm64 defconfig, as the 
module

is assimilated by QCOM_GLINK - which is selected by other means.

Signed-off-by: Bjorn Andersson 
---


Acked-by: Rishabh Bhatnagar 


 arch/arm64/configs/defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/configs/defconfig 
b/arch/arm64/configs/defconfig

index f9eefb5940ca..f26a0b6ea0e8 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -838,7 +838,6 @@ CONFIG_FSL_MC_DPIO=y
 CONFIG_IMX_SCU_SOC=y
 CONFIG_QCOM_AOSS_QMP=y
 CONFIG_QCOM_GENI_SE=y
-CONFIG_QCOM_GLINK_SSR=m
 CONFIG_QCOM_RMTFS_MEM=m
 CONFIG_QCOM_RPMH=y
 CONFIG_QCOM_RPMHPD=y

Re: [PATCH 3/4] rpmsg: glink: Integrate glink_ssr in qcom_glink

[rishabhb]

On 2020-04-22 17:37, Bjorn Andersson wrote:

In all but the very special case of a system with _only_ glink_rpm,
GLINK is dependent on glink_ssr, so move it to rpmsg and combine it 
with

qcom_glink_native in the new qcom_glink kernel module.

Signed-off-by: Bjorn Andersson 
---


Acked-by: Rishabh Bhatnagar 


 drivers/rpmsg/Kconfig| 6 +++---
 drivers/rpmsg/Makefile   | 3 ++-
 drivers/{soc/qcom/glink_ssr.c => rpmsg/qcom_glink_ssr.c} | 4 
 drivers/soc/qcom/Kconfig | 9 -
 drivers/soc/qcom/Makefile| 1 -
 include/linux/rpmsg/qcom_glink.h | 7 +--
 6 files changed, 6 insertions(+), 24 deletions(-)
 rename drivers/{soc/qcom/glink_ssr.c => rpmsg/qcom_glink_ssr.c} (97%)

diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index a9108ff563dc..f96716893c2a 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -24,13 +24,13 @@ config RPMSG_MTK_SCP
  remote processors in MediaTek platforms.
  This use IPI and IPC to communicate with remote processors.

-config RPMSG_QCOM_GLINK_NATIVE
+config RPMSG_QCOM_GLINK
tristate
select RPMSG

 config RPMSG_QCOM_GLINK_RPM
tristate "Qualcomm RPM Glink driver"
-   select RPMSG_QCOM_GLINK_NATIVE
+   select RPMSG_QCOM_GLINK
depends on HAS_IOMEM
depends on MAILBOX
help
@@ -40,7 +40,7 @@ config RPMSG_QCOM_GLINK_RPM

 config RPMSG_QCOM_GLINK_SMEM
tristate "Qualcomm SMEM Glink driver"
-   select RPMSG_QCOM_GLINK_NATIVE
+   select RPMSG_QCOM_GLINK
depends on MAILBOX
depends on QCOM_SMEM
help
diff --git a/drivers/rpmsg/Makefile b/drivers/rpmsg/Makefile
index ae92a7fb08f6..ffe932ef6050 100644
--- a/drivers/rpmsg/Makefile
+++ b/drivers/rpmsg/Makefile
@@ -2,8 +2,9 @@
 obj-$(CONFIG_RPMSG)+= rpmsg_core.o
 obj-$(CONFIG_RPMSG_CHAR)   += rpmsg_char.o
 obj-$(CONFIG_RPMSG_MTK_SCP)+= mtk_rpmsg.o
+qcom_glink-objs:= qcom_glink_native.o qcom_glink_ssr.o
+obj-$(CONFIG_RPMSG_QCOM_GLINK) += qcom_glink.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_RPM) += qcom_glink_rpm.o
-obj-$(CONFIG_RPMSG_QCOM_GLINK_NATIVE) += qcom_glink_native.o
 obj-$(CONFIG_RPMSG_QCOM_GLINK_SMEM) += qcom_glink_smem.o
 obj-$(CONFIG_RPMSG_QCOM_SMD)   += qcom_smd.o
 obj-$(CONFIG_RPMSG_VIRTIO) += virtio_rpmsg_bus.o
diff --git a/drivers/soc/qcom/glink_ssr.c 
b/drivers/rpmsg/qcom_glink_ssr.c

similarity index 97%
rename from drivers/soc/qcom/glink_ssr.c
rename to drivers/rpmsg/qcom_glink_ssr.c
index 847d79c935f1..dcd1ce616974 100644
--- a/drivers/soc/qcom/glink_ssr.c
+++ b/drivers/rpmsg/qcom_glink_ssr.c
@@ -164,7 +164,3 @@ static struct rpmsg_driver qcom_glink_ssr_driver = 
{

},
 };
 module_rpmsg_driver(qcom_glink_ssr_driver);
-
-MODULE_ALIAS("rpmsg:glink_ssr");
-MODULE_DESCRIPTION("Qualcomm GLINK SSR notifier");
-MODULE_LICENSE("GPL v2");
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 19332ea40234..5140bd82f1be 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -35,15 +35,6 @@ config QCOM_GENI_SE
  driver is also used to manage the common aspects of multiple Serial
  Engines present in the QUP.

-config QCOM_GLINK_SSR
-   tristate "Qualcomm Glink SSR driver"
-   depends on RPMSG
-   depends on QCOM_RPROC_COMMON
-   help
- Say y here to enable GLINK SSR support. The GLINK SSR driver
-	  implements the SSR protocol for notifying the remote processor 
about

- neighboring subsystems going up or down.
-
 config QCOM_GSBI
tristate "QCOM General Serial Bus Interface"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 5d6b83dc58e8..e9cacc9ad401 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -3,7 +3,6 @@ CFLAGS_rpmh-rsc.o := -I$(src)
 obj-$(CONFIG_QCOM_AOSS_QMP) += qcom_aoss.o
 obj-$(CONFIG_QCOM_GENI_SE) +=  qcom-geni-se.o
 obj-$(CONFIG_QCOM_COMMAND_DB) += cmd-db.o
-obj-$(CONFIG_QCOM_GLINK_SSR) +=glink_ssr.o
 obj-$(CONFIG_QCOM_GSBI)+=  qcom_gsbi.o
 obj-$(CONFIG_QCOM_MDT_LOADER)  += mdt_loader.o
 obj-$(CONFIG_QCOM_OCMEM)   += ocmem.o
diff --git a/include/linux/rpmsg/qcom_glink.h 
b/include/linux/rpmsg/qcom_glink.h

index 09daa0acde2c..daded9fddf36 100644
--- a/include/linux/rpmsg/qcom_glink.h
+++ b/include/linux/rpmsg/qcom_glink.h
@@ -12,6 +12,7 @@ struct qcom_glink;
 struct qcom_glink *qcom_glink_smem_register(struct device *parent,
struct device_node *node);
 void qcom_glink_smem_unregister(struct qcom_glink *glink);
+void qcom_glink_ssr_notify(const char *ssr_name);

 #else

@@ -23,12 +24,6 @@ qcom_glink_smem_register(struct device *parent,
 }

 static inline void qcom_glink_smem_unregister(struct qcom_glink 
*glink) {}

-
-#endif
-
-#if 

Re: [PATCH 2/4] soc: qcom: glink_ssr: Internalize ssr_notifiers

[rishabhb]

On 2020-04-22 17:37, Bjorn Andersson wrote:

Rather than carrying a special purpose blocking notifier for glink_ssr
in remoteproc's qcom_common.c, move it into glink_ssr so allow wider
reuse of the common one.

The rpmsg glink header file is used in preparation for the next patch.

Signed-off-by: Bjorn Andersson 
---


Acked-by: Rishabh Bhatnagar 


 drivers/remoteproc/qcom_common.c |  8 
 drivers/soc/qcom/glink_ssr.c | 24 +++-
 include/linux/rpmsg/qcom_glink.h |  6 ++
 3 files changed, 33 insertions(+), 5 deletions(-)

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

index ff26f2b68752..9028cea2d81e 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -42,6 +42,13 @@ static void glink_subdev_stop(struct rproc_subdev
*subdev, bool crashed)
glink->edge = NULL;
 }

+static void glink_subdev_unprepare(struct rproc_subdev *subdev)
+{
+   struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
+
+   qcom_glink_ssr_notify(glink->ssr_name);
+}
+
 /**
  * qcom_add_glink_subdev() - try to add a GLINK subdevice to rproc
  * @rproc: rproc handle to parent the subdevice
@@ -64,6 +71,7 @@ void qcom_add_glink_subdev(struct rproc *rproc,
struct qcom_rproc_glink *glink,
glink->dev = dev;
glink->subdev.start = glink_subdev_start;
glink->subdev.stop = glink_subdev_stop;
+   glink->subdev.unprepare = glink_subdev_unprepare;

rproc_add_subdev(rproc, >subdev);
 }
diff --git a/drivers/soc/qcom/glink_ssr.c 
b/drivers/soc/qcom/glink_ssr.c

index d7babe3d67bc..847d79c935f1 100644
--- a/drivers/soc/qcom/glink_ssr.c
+++ b/drivers/soc/qcom/glink_ssr.c
@@ -54,6 +54,19 @@ struct glink_ssr {
struct completion completion;
 };

+/* Notifier list for all registered glink_ssr instances */
+static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
+
+/**
+ * qcom_glink_ssr_notify() - notify GLINK SSR about stopped remoteproc
+ * @ssr_name:  name of the remoteproc that has been stopped
+ */
+void qcom_glink_ssr_notify(const char *ssr_name)
+{
+   blocking_notifier_call_chain(_notifiers, 0, (void *)ssr_name);
+}
+EXPORT_SYMBOL_GPL(qcom_glink_ssr_notify);
+
 static int qcom_glink_ssr_callback(struct rpmsg_device *rpdev,
   void *data, int len, void *priv, u32 addr)
 {
@@ -81,8 +94,9 @@ static int qcom_glink_ssr_callback(struct 
rpmsg_device *rpdev,

return 0;
 }

-static int qcom_glink_ssr_notify(struct notifier_block *nb, unsigned
long event,
-void *data)
+static int qcom_glink_ssr_notifier_call(struct notifier_block *nb,
+   unsigned long event,
+   void *data)
 {
struct glink_ssr *ssr = container_of(nb, struct glink_ssr, nb);
struct do_cleanup_msg msg;
@@ -121,18 +135,18 @@ static int qcom_glink_ssr_probe(struct
rpmsg_device *rpdev)

ssr->dev = >dev;
ssr->ept = rpdev->ept;
-   ssr->nb.notifier_call = qcom_glink_ssr_notify;
+   ssr->nb.notifier_call = qcom_glink_ssr_notifier_call;

dev_set_drvdata(>dev, ssr);

-   return qcom_register_ssr_notifier(>nb);
+   return blocking_notifier_chain_register(_notifiers, >nb);
 }

 static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev)
 {
struct glink_ssr *ssr = dev_get_drvdata(>dev);

-   qcom_unregister_ssr_notifier(>nb);
+   blocking_notifier_chain_unregister(_notifiers, >nb);
 }

 static const struct rpmsg_device_id qcom_glink_ssr_match[] = {
diff --git a/include/linux/rpmsg/qcom_glink.h 
b/include/linux/rpmsg/qcom_glink.h

index 96e26d94719f..09daa0acde2c 100644
--- a/include/linux/rpmsg/qcom_glink.h
+++ b/include/linux/rpmsg/qcom_glink.h
@@ -26,4 +26,10 @@ static inline void
qcom_glink_smem_unregister(struct qcom_glink *glink) {}

 #endif

+#if IS_ENABLED(CONFIG_RPMSG_QCOM_GLINK_SSR)
+void qcom_glink_ssr_notify(const char *ssr_name);
+#else
+static inline void qcom_glink_ssr_notify(const char *ssr_name) {}
+#endif
+
 #endif

Re: [PATCH 1/4] remoteproc: qcom: Pass ssr_name to glink subdevice

[rishabhb]

On 2020-04-22 17:37, Bjorn Andersson wrote:

Pass ssr_name to glink subdevice in preparation for tying glink_ssr to
the glink subdevice, rather than having its own "ssr subdevice".

Signed-off-by: Bjorn Andersson 
---


Acked-by: Rishabh Bhatnagar 


 drivers/remoteproc/qcom_common.c| 9 -
 drivers/remoteproc/qcom_common.h| 5 -
 drivers/remoteproc/qcom_q6v5_adsp.c | 2 +-
 drivers/remoteproc/qcom_q6v5_mss.c  | 2 +-
 drivers/remoteproc/qcom_q6v5_pas.c  | 2 +-
 5 files changed, 15 insertions(+), 5 deletions(-)

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

index 60650bcc8c67..ff26f2b68752 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -46,8 +46,10 @@ static void glink_subdev_stop(struct rproc_subdev
*subdev, bool crashed)
  * qcom_add_glink_subdev() - try to add a GLINK subdevice to rproc
  * @rproc: rproc handle to parent the subdevice
  * @glink: reference to a GLINK subdev context
+ * @ssr_name:	identifier of the associated remoteproc for ssr 
notifications

  */
-void qcom_add_glink_subdev(struct rproc *rproc, struct 
qcom_rproc_glink *glink)
+void qcom_add_glink_subdev(struct rproc *rproc, struct 
qcom_rproc_glink *glink,

+  const char *ssr_name)
 {
struct device *dev = >dev;

@@ -55,6 +57,10 @@ void qcom_add_glink_subdev(struct rproc *rproc,
struct qcom_rproc_glink *glink)
if (!glink->node)
return;

+   glink->ssr_name = kstrdup_const(ssr_name, GFP_KERNEL);
+   if (!glink->ssr_name)
+   return;
+
glink->dev = dev;
glink->subdev.start = glink_subdev_start;
glink->subdev.stop = glink_subdev_stop;
@@ -74,6 +80,7 @@ void qcom_remove_glink_subdev(struct rproc *rproc,
struct qcom_rproc_glink *glin
return;

rproc_remove_subdev(rproc, >subdev);
+   kfree_const(glink->ssr_name);
of_node_put(glink->node);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_glink_subdev);
diff --git a/drivers/remoteproc/qcom_common.h 
b/drivers/remoteproc/qcom_common.h

index 58de71e4781c..34e5188187dc 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -11,6 +11,8 @@ struct qcom_sysmon;
 struct qcom_rproc_glink {
struct rproc_subdev subdev;

+   const char *ssr_name;
+
struct device *dev;
struct device_node *node;
struct qcom_glink *edge;
@@ -30,7 +32,8 @@ struct qcom_rproc_ssr {
const char *name;
 };

-void qcom_add_glink_subdev(struct rproc *rproc, struct
qcom_rproc_glink *glink);
+void qcom_add_glink_subdev(struct rproc *rproc, struct 
qcom_rproc_glink *glink,

+  const char *ssr_name);
 void qcom_remove_glink_subdev(struct rproc *rproc, struct
qcom_rproc_glink *glink);

 int qcom_register_dump_segments(struct rproc *rproc, const struct
firmware *fw);
diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c
b/drivers/remoteproc/qcom_q6v5_adsp.c
index c60dabc6939e..d2a2574dcf35 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -461,7 +461,7 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto disable_pm;

-   qcom_add_glink_subdev(rproc, >glink_subdev);
+   qcom_add_glink_subdev(rproc, >glink_subdev, desc->ssr_name);
qcom_add_ssr_subdev(rproc, >ssr_subdev, desc->ssr_name);
adsp->sysmon = qcom_add_sysmon_subdev(rproc,
  desc->sysmon_name,
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
b/drivers/remoteproc/qcom_q6v5_mss.c
index 7af1d0c987e0..b5dd36775b77 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -1762,7 +1762,7 @@ static int q6v5_probe(struct platform_device 
*pdev)


qproc->mpss_perm = BIT(QCOM_SCM_VMID_HLOS);
qproc->mba_perm = BIT(QCOM_SCM_VMID_HLOS);
-   qcom_add_glink_subdev(rproc, >glink_subdev);
+   qcom_add_glink_subdev(rproc, >glink_subdev, "mpss");
qcom_add_smd_subdev(rproc, >smd_subdev);
qcom_add_ssr_subdev(rproc, >ssr_subdev, "mpss");
qcom_add_ipa_notify_subdev(rproc, >ipa_notify_subdev);
diff --git a/drivers/remoteproc/qcom_q6v5_pas.c
b/drivers/remoteproc/qcom_q6v5_pas.c
index 8ecc157f1ed1..fc6658b523b6 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -436,7 +436,7 @@ static int adsp_probe(struct platform_device *pdev)
if (ret)
goto detach_proxy_pds;

-   qcom_add_glink_subdev(rproc, >glink_subdev);
+   qcom_add_glink_subdev(rproc, >glink_subdev, desc->ssr_name);
qcom_add_smd_subdev(rproc, >smd_subdev);
qcom_add_ssr_subdev(rproc, >ssr_subdev, desc->ssr_name);
adsp->sysmon = qcom_add_sysmon_subdev(rproc,

Re: Bug at kernel/cred.c +443

[rishabhb]

On 2019-09-09 15:47, risha...@codeaurora.org wrote:

Hi Miklos

In 4.19 kernel when we write to a file that doesn't exist we see the
following stack:
[  377.382745]  ovl_create_or_link+0xac/0x710
[  377.382745]  ovl_create_object+0xb8/0x110
[  377.382745]  ovl_create+0x34/0x40
[  377.382745]  path_openat+0xd44/0x15a8
[  377.382745]  do_filp_open+0x80/0x128
[  377.382745]  do_sys_open+0x140/0x250
[  377.382745]  __arm64_sys_openat+0x2c/0x38

If the override_cred flag = off, the ovl_override_cred and
ovl_revert_cred just returns NULL.
But there is another override_cred in between these two functions;
put_cred(override_creds(override_cred));
put_cred(override_cred);

This will override the credentials permanently as there is no
corresponding revert_cred associated.
So whenever we do commit_creds for this task, we see a BUG_ON at
kernel/cred.c +443.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cred.c#n443

Should this override_cred be changed to ovl_override_cred to maintain
consistency and avoid this
BUG_ON?


Thanks,
Rishabh


Corrected line number in the subject.

Bug at kernel/cred.c +432

[rishabhb]

Hi Miklos

In 4.19 kernel when we write to a file that doesn't exist we see the
following stack:
[  377.382745]  ovl_create_or_link+0xac/0x710
[  377.382745]  ovl_create_object+0xb8/0x110
[  377.382745]  ovl_create+0x34/0x40
[  377.382745]  path_openat+0xd44/0x15a8
[  377.382745]  do_filp_open+0x80/0x128
[  377.382745]  do_sys_open+0x140/0x250
[  377.382745]  __arm64_sys_openat+0x2c/0x38

If the override_cred flag = off, the ovl_override_cred and 
ovl_revert_cred just returns NULL.

But there is another override_cred in between these two functions;
put_cred(override_creds(override_cred));
put_cred(override_cred);

This will override the credentials permanently as there is no 
corresponding revert_cred associated.
So whenever we do commit_creds for this task, we see a BUG_ON at 
kernel/cred.c +443.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/cred.c#n443

Should this override_cred be changed to ovl_override_cred to maintain 
consistency and avoid this

BUG_ON?


Thanks,
Rishabh

usercopy_warn in __copy_to_user

[rishabhb]

In the 4.19 kernel, we are seeing a USERCOPY_WARN in __copy_to_user 
during bootup.

The code-flow is something like this:

(arch/arm64/kernel/signal.c)
struct sigset_t *set;
__copy_to_user(>uc.uc_sigmask, set, sizeof(*set))

(include/linux/uaccess.h)
__copy_to_user(void __user *to, const void *from, unsigned long n)
{
might_fault();
kasan_check_read(from, n);
check_object_size(from, n, true);
return raw_copy_to_user(to, from, n);
}

(include/linux/thread_info.h)
static __always_inline void check_object_size(const void *ptr,
 unsigned long n, bool to_user)
{
if (!__builtin_constant_p(n))
__check_object_size(ptr, n, to_user);
}

Since sizeof(*set) is constant, __builtin_constant_p(n) should return 
true.
But we are seeing that its returning the value as false. Because of 
which

the code goes on to __check_object_size and generates a USERCOPY_WARN
("usercopy: WARN() on slab cache usercopy
region violations").

We are using LLVM clang version 6.0 to compile the kernel and not gcc.
In clang, __builtin_constant_p is evaluated immediately, before inlining
or other optimizations run, gcc evaluates it later.
We believe that maybe causing __builtin_constant_p(n) to return false.
There’s upstream work to change LLVM, so __builtin_constant_p works more
like gcc when optimization is enabled, but its still in progress.


For this scenario is there a way to avoid the warning? Should the code 
be

written in a different to avoid dependency on compiler?

Thanks,
Rishabh

Re: [PATCH v1] dd: Invoke one probe retry cycle after some initcall levels

[rishabhb]

On 2018-08-13 10:39, Rishabh Bhatnagar wrote:

From: Rishabh Bhatnagar 

Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level after subsys initcall, allowing
these drivers to be probed earlier.
To give an example many Qualcomm drivers are dependent on the
regulator and bus driver. Both the regulator and bus driver
are probed in the subsys_initcall level. Now the probe of bus
driver requires regulator to be working. If the probe of bus
driver happens before regulator, then bus driver's probe will
be deferred and all other device's probes which depend on bus
driver will also be deferred.
The impact of this problem is reduced if we have this patch.

Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---

Changes since v0:
 * Remove arch_initcall_sync(deferred_probe_initcall) from patch. This 
is not
   really needed as none of the devices are re-probed in 
arch_initcall_sync

   level.

 drivers/base/dd.c | 32 ++--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d72..9aa41aa 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,23 +224,43 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
 }

+static void enable_trigger_defer_cycle(void)
+{
+   driver_deferred_probe_enable = true;
+   driver_deferred_probe_trigger();
+   /*
+* Sort as many dependencies as possible before the next initcall
+* level
+*/
+   flush_work(_probe_work);
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
  * We don't want to get in the way when the bulk of drivers are 
getting probed.
  * Instead, this initcall makes sure that deferred probing is delayed 
until

- * late_initcall time.
+ * all the registered initcall functions at a particular level are 
completed.

+ * This function is invoked at every *_initcall_sync level.
  */
 static int deferred_probe_initcall(void)
 {
-   driver_deferred_probe_enable = true;
-   driver_deferred_probe_trigger();
-   /* Sort as many dependencies as possible before exiting initcalls */
-   flush_work(_probe_work);
+   enable_trigger_defer_cycle();
+   driver_deferred_probe_enable = false;
+   return 0;
+}
+subsys_initcall_sync(deferred_probe_initcall);
+fs_initcall_sync(deferred_probe_initcall);
+device_initcall_sync(deferred_probe_initcall);
+
+static int deferred_probe_enable_fn(void)
+{
+   /* Enable deferred probing for all time */
+   enable_trigger_defer_cycle();
initcalls_done = true;
return 0;
 }
-late_initcall(deferred_probe_initcall);
+late_initcall(deferred_probe_enable_fn);

 /**
  * device_is_bound() - Check if device is bound to a driver


Hi Rafael
Just a reminder about this patch. Let me know if you want me to add/edit 
anything else.

-Rishabh

Re: [PATCH v1] dd: Invoke one probe retry cycle after some initcall levels

[rishabhb]

On 2018-08-13 10:39, Rishabh Bhatnagar wrote:

From: Rishabh Bhatnagar 

Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level after subsys initcall, allowing
these drivers to be probed earlier.
To give an example many Qualcomm drivers are dependent on the
regulator and bus driver. Both the regulator and bus driver
are probed in the subsys_initcall level. Now the probe of bus
driver requires regulator to be working. If the probe of bus
driver happens before regulator, then bus driver's probe will
be deferred and all other device's probes which depend on bus
driver will also be deferred.
The impact of this problem is reduced if we have this patch.

Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---

Changes since v0:
 * Remove arch_initcall_sync(deferred_probe_initcall) from patch. This 
is not
   really needed as none of the devices are re-probed in 
arch_initcall_sync

   level.

 drivers/base/dd.c | 32 ++--
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d72..9aa41aa 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,23 +224,43 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
 }

+static void enable_trigger_defer_cycle(void)
+{
+   driver_deferred_probe_enable = true;
+   driver_deferred_probe_trigger();
+   /*
+* Sort as many dependencies as possible before the next initcall
+* level
+*/
+   flush_work(_probe_work);
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
  * We don't want to get in the way when the bulk of drivers are 
getting probed.
  * Instead, this initcall makes sure that deferred probing is delayed 
until

- * late_initcall time.
+ * all the registered initcall functions at a particular level are 
completed.

+ * This function is invoked at every *_initcall_sync level.
  */
 static int deferred_probe_initcall(void)
 {
-   driver_deferred_probe_enable = true;
-   driver_deferred_probe_trigger();
-   /* Sort as many dependencies as possible before exiting initcalls */
-   flush_work(_probe_work);
+   enable_trigger_defer_cycle();
+   driver_deferred_probe_enable = false;
+   return 0;
+}
+subsys_initcall_sync(deferred_probe_initcall);
+fs_initcall_sync(deferred_probe_initcall);
+device_initcall_sync(deferred_probe_initcall);
+
+static int deferred_probe_enable_fn(void)
+{
+   /* Enable deferred probing for all time */
+   enable_trigger_defer_cycle();
initcalls_done = true;
return 0;
 }
-late_initcall(deferred_probe_initcall);
+late_initcall(deferred_probe_enable_fn);

 /**
  * device_is_bound() - Check if device is bound to a driver


Hi Rafael
Just a reminder about this patch. Let me know if you want me to add/edit 
anything else.

-Rishabh

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-08-06 01:53, Rafael J. Wysocki wrote:

On Fri, Aug 3, 2018 at 12:20 AM, Sodagudi Prasad
 wrote:

From: RAFAEL J. WYSOCKI 
Date: Wed, Aug 1, 2018 at 2:21 PM
Subject: Re: [PATCH] dd: Invoke one probe retry cycle after every
initcall level
To: Rishabh Bhatnagar 
Cc: "Rafael J. Wysocki" , Greg Kroah-Hartman
, Linux Kernel Mailing List
, ckad...@codeaurora.org,
ts...@codeaurora.org, Vikram Mulukutla 


On Wed, Aug 1, 2018 at 11:18 PM,   wrote:


On 2018-07-24 01:24, Rafael J. Wysocki wrote:



On Mon, Jul 23, 2018 at 10:22 PM,   wrote:



On 2018-07-23 04:17, Rafael J. Wysocki wrote:




On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:




Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.





Can you please say something about the actual use case this is
expected to address?




We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of


dependencies


not being met the deferral mechanism defers the probes for a later


time,


even though the dependencies might be met earlier. With this


change


display can be brought up much earlier.




OK

What runlevel brings up the display after the change?

Thanks,
Rafael


After the change the display can come up after device_initcall level
itself.
The above mentioned 3 devices are probed at 0.503253, 0.505210 and


0.523264


secs.
Only the first device is probed successfully. With the current
deferral mechanism the devices get probed again after late_initcall
at 9.19 and 9.35 secs. So display can only come up after 9.35 secs.
With this change the devices are re-probed successfully at 0.60 and
0.613 secs. Therefore display can come just after 0.613 secs.



OK, so why do you touch the initcall levels earlier than device_?


1)  re-probe probing devices in the active list on every level 
help to

avoid circular dependency pending list.
2)  There are so many devices which gets deferred in earlier init 
call
levels, so we wanted to reprobe them at every successive init call 
level.


Do you have specific examples of devices for which that helps?




This change helps in overall android bootup as well.



How exactly?


We have seen less no of re-probes at late_init and most of the 
driver's
dependency met earlier than late_init call level. It helped display 
and
couple of other drivers by executing the re probe work at every init 
level.


So I can believe that walking the deferred list on device_initcall and
maybe on device_initcall_sync may help, but I'm not quite convinced
that it matters for the earlier initcall levels.


Many of our drivers are dependent on the regulator and bus driver.
Both the regulator and bus driver are probed in the subsys_initcall 
level.
Now the probe of bus driver requires regulator to be working. If the 
probe of

bus driver happens before regulator, then bus driver's probe will be
deferred and all other device's probes which depend on bus driver will 
also be deferred.

The impact of this problem is reduced if we have this patch.

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-08-06 01:53, Rafael J. Wysocki wrote:

On Fri, Aug 3, 2018 at 12:20 AM, Sodagudi Prasad
 wrote:

From: RAFAEL J. WYSOCKI 
Date: Wed, Aug 1, 2018 at 2:21 PM
Subject: Re: [PATCH] dd: Invoke one probe retry cycle after every
initcall level
To: Rishabh Bhatnagar 
Cc: "Rafael J. Wysocki" , Greg Kroah-Hartman
, Linux Kernel Mailing List
, ckad...@codeaurora.org,
ts...@codeaurora.org, Vikram Mulukutla 


On Wed, Aug 1, 2018 at 11:18 PM,   wrote:


On 2018-07-24 01:24, Rafael J. Wysocki wrote:



On Mon, Jul 23, 2018 at 10:22 PM,   wrote:



On 2018-07-23 04:17, Rafael J. Wysocki wrote:




On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:




Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.





Can you please say something about the actual use case this is
expected to address?




We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of


dependencies


not being met the deferral mechanism defers the probes for a later


time,


even though the dependencies might be met earlier. With this


change


display can be brought up much earlier.




OK

What runlevel brings up the display after the change?

Thanks,
Rafael


After the change the display can come up after device_initcall level
itself.
The above mentioned 3 devices are probed at 0.503253, 0.505210 and


0.523264


secs.
Only the first device is probed successfully. With the current
deferral mechanism the devices get probed again after late_initcall
at 9.19 and 9.35 secs. So display can only come up after 9.35 secs.
With this change the devices are re-probed successfully at 0.60 and
0.613 secs. Therefore display can come just after 0.613 secs.



OK, so why do you touch the initcall levels earlier than device_?


1)  re-probe probing devices in the active list on every level 
help to

avoid circular dependency pending list.
2)  There are so many devices which gets deferred in earlier init 
call
levels, so we wanted to reprobe them at every successive init call 
level.


Do you have specific examples of devices for which that helps?




This change helps in overall android bootup as well.



How exactly?


We have seen less no of re-probes at late_init and most of the 
driver's
dependency met earlier than late_init call level. It helped display 
and
couple of other drivers by executing the re probe work at every init 
level.


So I can believe that walking the deferred list on device_initcall and
maybe on device_initcall_sync may help, but I'm not quite convinced
that it matters for the earlier initcall levels.


Many of our drivers are dependent on the regulator and bus driver.
Both the regulator and bus driver are probed in the subsys_initcall 
level.
Now the probe of bus driver requires regulator to be working. If the 
probe of

bus driver happens before regulator, then bus driver's probe will be
deferred and all other device's probes which depend on bus driver will 
also be deferred.

The impact of this problem is reduced if we have this patch.

Re: [PATCH] firmware: Fix security issue with request_firmware_into_buf()

[rishabhb]

On 2018-08-02 14:58, Luis Chamberlain wrote:

On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar
 wrote:


When calling request_firmware_into_buf() with the FW_OPT_NOCACHE
flag
it is expected that firmware is loaded into buffer from memory.
But inside alloc_lookup_fw_priv every new firmware that is loaded is
added to the firmware cache (fwc) list head. So if any driver
requests
a firmware that is already loaded the code iterates over the above
mentioned list and it can end up giving a pointer to other device
driver's
firmware buffer.
Also the existing copy may either be modified by drivers, remote
processors
or even freed. This causes a potential security issue with batched
requests
when using request_firmware_into_buf.

Fix alloc_lookup_fw_priv to not add to the fwc head list if
FW_OPT_NOCACHE
is set, and also don't do the lookup in the list.

Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw
caching optional")

Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---


Did you test with the tools/testing/selftests/firmware/ scripts? If
not please do so and report back and confirm no regressions are found.

Brownie points for you to add a test case to show the issue
highlighted in this patch, and which it fixes. I believe this fix
should be pushed to stable, so I'll do that after you confirm no
regressions were found.

The new selftests changed you'd make would not go to stable, however
there are Linux distributions and 0day that test the latest tools
directory against older kernels. So this test would help capture gaps
later.

  Luis


I ran the selftests and observed no regressions with this change.
I'm still working on adding a test case though.

-Rishabh

Re: [PATCH] firmware: Fix security issue with request_firmware_into_buf()

[rishabhb]

On 2018-08-02 14:58, Luis Chamberlain wrote:

On Wed, Aug 1, 2018, 4:26 PM Rishabh Bhatnagar
 wrote:


When calling request_firmware_into_buf() with the FW_OPT_NOCACHE
flag
it is expected that firmware is loaded into buffer from memory.
But inside alloc_lookup_fw_priv every new firmware that is loaded is
added to the firmware cache (fwc) list head. So if any driver
requests
a firmware that is already loaded the code iterates over the above
mentioned list and it can end up giving a pointer to other device
driver's
firmware buffer.
Also the existing copy may either be modified by drivers, remote
processors
or even freed. This causes a potential security issue with batched
requests
when using request_firmware_into_buf.

Fix alloc_lookup_fw_priv to not add to the fwc head list if
FW_OPT_NOCACHE
is set, and also don't do the lookup in the list.

Fixes: 0e742e9275 ("firmware: provide infrastructure to make fw
caching optional")

Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---


Did you test with the tools/testing/selftests/firmware/ scripts? If
not please do so and report back and confirm no regressions are found.

Brownie points for you to add a test case to show the issue
highlighted in this patch, and which it fixes. I believe this fix
should be pushed to stable, so I'll do that after you confirm no
regressions were found.

The new selftests changed you'd make would not go to stable, however
there are Linux distributions and 0day that test the latest tools
directory against older kernels. So this test would help capture gaps
later.

  Luis


I ran the selftests and observed no regressions with this change.
I'm still working on adding a test case though.

-Rishabh

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-07-24 01:24, Rafael J. Wysocki wrote:

On Mon, Jul 23, 2018 at 10:22 PM,   wrote:

On 2018-07-23 04:17, Rafael J. Wysocki wrote:


On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:


Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.



Can you please say something about the actual use case this is
expected to address?


We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of dependencies
not being met the deferral mechanism defers the probes for a later 
time,

even though the dependencies might be met earlier. With this change
display can be brought up much earlier.


OK

What runlevel brings up the display after the change?

Thanks,
Rafael

After the change the display can come up after device_initcall level
itself.
The above mentioned 3 devices are probed at 0.503253, 0.505210 and 
0.523264 secs.

Only the first device is probed successfully. With the current
deferral mechanism the devices get probed again after late_initcall
at 9.19 and 9.35 secs. So display can only come up after 9.35 secs.
With this change the devices are re-probed successfully at 0.60 and
0.613 secs. Therefore display can come just after 0.613 secs.
This change helps in overall android bootup as well.

-Rishabh

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-07-24 01:24, Rafael J. Wysocki wrote:

On Mon, Jul 23, 2018 at 10:22 PM,   wrote:

On 2018-07-23 04:17, Rafael J. Wysocki wrote:


On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:


Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.



Can you please say something about the actual use case this is
expected to address?


We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of dependencies
not being met the deferral mechanism defers the probes for a later 
time,

even though the dependencies might be met earlier. With this change
display can be brought up much earlier.


OK

What runlevel brings up the display after the change?

Thanks,
Rafael

After the change the display can come up after device_initcall level
itself.
The above mentioned 3 devices are probed at 0.503253, 0.505210 and 
0.523264 secs.

Only the first device is probed successfully. With the current
deferral mechanism the devices get probed again after late_initcall
at 9.19 and 9.35 secs. So display can only come up after 9.35 secs.
With this change the devices are re-probed successfully at 0.60 and
0.613 secs. Therefore display can come just after 0.613 secs.
This change helps in overall android bootup as well.

-Rishabh

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-07-23 04:17, Rafael J. Wysocki wrote:

On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:

Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.


Can you please say something about the actual use case this is
expected to address?

We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of dependencies
not being met the deferral mechanism defers the probes for a later time,
even though the dependencies might be met earlier. With this change
display can be brought up much earlier.



Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/base/dd.c | 33 +++--
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d72..e6a6821 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,23 +224,44 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
 }

+static void enable_trigger_defer_cycle(void)
+{
+   driver_deferred_probe_enable = true;
+   driver_deferred_probe_trigger();
+   /*
+* Sort as many dependencies as possible before the next 
initcall

+* level
+*/
+   flush_work(_probe_work);
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
  * We don't want to get in the way when the bulk of drivers are 
getting probed.
  * Instead, this initcall makes sure that deferred probing is delayed 
until

- * late_initcall time.
+ * all the registered initcall functions at a particular level are 
completed.

+ * This function is invoked at every *_initcall_sync level.
  */
 static int deferred_probe_initcall(void)
 {
-   driver_deferred_probe_enable = true;
-   driver_deferred_probe_trigger();
-   /* Sort as many dependencies as possible before exiting 
initcalls */

-   flush_work(_probe_work);
+   enable_trigger_defer_cycle();
+   driver_deferred_probe_enable = false;
+   return 0;
+}
+arch_initcall_sync(deferred_probe_initcall);
+subsys_initcall_sync(deferred_probe_initcall);
+fs_initcall_sync(deferred_probe_initcall);
+device_initcall_sync(deferred_probe_initcall);
+
+static int deferred_probe_enable_fn(void)
+{
+   /* Enable deferred probing for all time */
+   enable_trigger_defer_cycle();
initcalls_done = true;
return 0;
 }
-late_initcall(deferred_probe_initcall);
+late_initcall(deferred_probe_enable_fn);

 /**
  * device_is_bound() - Check if device is bound to a driver
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH] dd: Invoke one probe retry cycle after every initcall level

[rishabhb]

On 2018-07-23 04:17, Rafael J. Wysocki wrote:

On Thu, Jul 19, 2018 at 11:24 PM, Rishabh Bhatnagar
 wrote:

Drivers that are registered at an initcall level may have to
wait until late_init before the probe deferral mechanism can
retry their probe functions. It is possible that their
dependencies were resolved much earlier, in some cases even
before the next initcall level. Invoke one probe retry cycle
at every _sync initcall level, allowing these drivers to be
probed earlier.


Can you please say something about the actual use case this is
expected to address?

We have a display driver that depends 3 other devices to be
probed so that it can bring-up the display. Because of dependencies
not being met the deferral mechanism defers the probes for a later time,
even though the dependencies might be met earlier. With this change
display can be brought up much earlier.



Signed-off-by: Vikram Mulukutla 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/base/dd.c | 33 +++--
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d72..e6a6821 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -224,23 +224,44 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
 }

+static void enable_trigger_defer_cycle(void)
+{
+   driver_deferred_probe_enable = true;
+   driver_deferred_probe_trigger();
+   /*
+* Sort as many dependencies as possible before the next 
initcall

+* level
+*/
+   flush_work(_probe_work);
+}
+
 /**
  * deferred_probe_initcall() - Enable probing of deferred devices
  *
  * We don't want to get in the way when the bulk of drivers are 
getting probed.
  * Instead, this initcall makes sure that deferred probing is delayed 
until

- * late_initcall time.
+ * all the registered initcall functions at a particular level are 
completed.

+ * This function is invoked at every *_initcall_sync level.
  */
 static int deferred_probe_initcall(void)
 {
-   driver_deferred_probe_enable = true;
-   driver_deferred_probe_trigger();
-   /* Sort as many dependencies as possible before exiting 
initcalls */

-   flush_work(_probe_work);
+   enable_trigger_defer_cycle();
+   driver_deferred_probe_enable = false;
+   return 0;
+}
+arch_initcall_sync(deferred_probe_initcall);
+subsys_initcall_sync(deferred_probe_initcall);
+fs_initcall_sync(deferred_probe_initcall);
+device_initcall_sync(deferred_probe_initcall);
+
+static int deferred_probe_enable_fn(void)
+{
+   /* Enable deferred probing for all time */
+   enable_trigger_defer_cycle();
initcalls_done = true;
return 0;
 }
-late_initcall(deferred_probe_initcall);
+late_initcall(deferred_probe_enable_fn);

 /**
  * device_is_bound() - Check if device is bound to a driver
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

Re: [PATCH v8 0/2] SDM845 System Cache Driver

[rishabhb]

On 2018-05-23 17:35, Rishabh Bhatnagar wrote:
This series implements system cache or LLCC(Last Level Cache 
Controller)

driver for SDM845 SOC. The purpose of the driver is to partition the
system cache and program the settings such as priortiy, lines to probe
while doing a look up in the system cache, low power related settings 
etc.

The partitions are called cache slices. Each cache slice is associated
with size and SCID(System Cache ID). The driver also provides API for
clients to query the cache slice details,activate and deactivate them.

The driver can be broadly classified into:
* SOC specific driver: llcc-sdm845.c: Cache partitioning and cache 
slice

properties for usecases on sdm845 that need to use system cache.

* API : llcc-slice.c: Exports APIs to clients to query cache slice 
details,

activate and deactivate cache slices.

Changes since v7:
* Change the DT node name to cache-controller.
* Use the module_platform_driver_macro
* Use GENMASK and SZ_* macros
* Correct indentation, and remove unnecessary assignemnts.
* Addresed all comments by Andy Schevchenko except the comment to 
ignore some

  lines of code going over 80 characters.

Changes since v6:
* Remove the max-slices property from DT.
* Make client's slice_ids as macros.
* Unlock mutex while returning from function in case of error.

Changes since v5:
* Remove client information from DT.
* Make the llcc driver data as global.
* Check return value of llcc_update_act_ctrl function
* Change error returned from -EFAULT to -EINVAL

Changes since v4:
* Remove null pointer checks as per comments.
* Remove extra blank lines.

Changes since v3:
* Use the regmap_read_poll_timeout function
* Check for regmap read/write errors.
* Remove memory barrier after regmap write
* Derive memory bank offsets using stride macro variable
* Remove debug statements from code
* Remove the qcom_llcc_remove function
* Use if IS_ENABLED in place of ifdef for built-in module
* Change EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
* Remove unnecessary free functions
* Change the variable names as per review comments

Changes since v2:
* Corrected the Makefile to fix compilation.

Changes since v1:
* Added Makefile and Kconfig.

Changes since v0:
* Removed the syscon and simple-mfd approach
* Updated the device tree nodes to mention LLCC as a single HW block
* Moved llcc bank offsets from device tree and handled the offset
  in the driver.

ckad...@codeaurora.org (2):
  dt-bindings: Documentation for qcom, llcc
  drivers: soc: Add LLCC driver

 .../devicetree/bindings/arm/msm/qcom,llcc.txt  |  26 ++
 drivers/soc/qcom/Kconfig   |  17 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c |  94 ++
 drivers/soc/qcom/llcc-slice.c  | 335 
+

 include/linux/soc/qcom/llcc-qcom.h | 180 +++
 6 files changed, 654 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h


Does this spin look fine to everyone? If yes can we go ahead and merge 
this?

Re: [PATCH v8 0/2] SDM845 System Cache Driver

[rishabhb]

On 2018-05-23 17:35, Rishabh Bhatnagar wrote:
This series implements system cache or LLCC(Last Level Cache 
Controller)

driver for SDM845 SOC. The purpose of the driver is to partition the
system cache and program the settings such as priortiy, lines to probe
while doing a look up in the system cache, low power related settings 
etc.

The partitions are called cache slices. Each cache slice is associated
with size and SCID(System Cache ID). The driver also provides API for
clients to query the cache slice details,activate and deactivate them.

The driver can be broadly classified into:
* SOC specific driver: llcc-sdm845.c: Cache partitioning and cache 
slice

properties for usecases on sdm845 that need to use system cache.

* API : llcc-slice.c: Exports APIs to clients to query cache slice 
details,

activate and deactivate cache slices.

Changes since v7:
* Change the DT node name to cache-controller.
* Use the module_platform_driver_macro
* Use GENMASK and SZ_* macros
* Correct indentation, and remove unnecessary assignemnts.
* Addresed all comments by Andy Schevchenko except the comment to 
ignore some

  lines of code going over 80 characters.

Changes since v6:
* Remove the max-slices property from DT.
* Make client's slice_ids as macros.
* Unlock mutex while returning from function in case of error.

Changes since v5:
* Remove client information from DT.
* Make the llcc driver data as global.
* Check return value of llcc_update_act_ctrl function
* Change error returned from -EFAULT to -EINVAL

Changes since v4:
* Remove null pointer checks as per comments.
* Remove extra blank lines.

Changes since v3:
* Use the regmap_read_poll_timeout function
* Check for regmap read/write errors.
* Remove memory barrier after regmap write
* Derive memory bank offsets using stride macro variable
* Remove debug statements from code
* Remove the qcom_llcc_remove function
* Use if IS_ENABLED in place of ifdef for built-in module
* Change EXPORT_SYMBOL to EXPORT_SYMBOL_GPL
* Remove unnecessary free functions
* Change the variable names as per review comments

Changes since v2:
* Corrected the Makefile to fix compilation.

Changes since v1:
* Added Makefile and Kconfig.

Changes since v0:
* Removed the syscon and simple-mfd approach
* Updated the device tree nodes to mention LLCC as a single HW block
* Moved llcc bank offsets from device tree and handled the offset
  in the driver.

ckad...@codeaurora.org (2):
  dt-bindings: Documentation for qcom, llcc
  drivers: soc: Add LLCC driver

 .../devicetree/bindings/arm/msm/qcom,llcc.txt  |  26 ++
 drivers/soc/qcom/Kconfig   |  17 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c |  94 ++
 drivers/soc/qcom/llcc-slice.c  | 335 
+

 include/linux/soc/qcom/llcc-qcom.h | 180 +++
 6 files changed, 654 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h


Does this spin look fine to everyone? If yes can we go ahead and merge 
this?

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-22 12:38, Andy Shevchenko wrote:

On Tue, May 22, 2018 at 9:33 PM,   wrote:

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1



Are these bits? Perhaps BIT() ?

isn't it just better to use fixed size as u suggest in the next 
comment?


If the are bits, use BIT() macro.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+




+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);



if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


do {
  if (!cfg || count == sz)
   return ...(-ENODEV);
 ...
} while (...);

Though, as I said for-loop will look slightly better I think.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);



Perhaps one line (~83 characters here is OK) ?


The checkpatch script complains about such lines.


So what if it just 3 characters out?

Many upstream reviewers have objection to lines crossing over 80 
characters

I have gotten reviews to reduce the line length even if its like 81~82
characters. Can we keep this as it is? I have addressed all other
comments and will send out the next patch by today.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);



Ditto.



+   attr1_cfg = bcast_off +
+
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+   attr0_cfg = bcast_off +
+
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);



Ditto.



+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority <<
ATTR1_PRIORITY_SHIFT;



foo |=
  bar << SHIFT;

would look slightly better.


Did you consider this option ?

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-22 12:38, Andy Shevchenko wrote:

On Tue, May 22, 2018 at 9:33 PM,   wrote:

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1



Are these bits? Perhaps BIT() ?

isn't it just better to use fixed size as u suggest in the next 
comment?


If the are bits, use BIT() macro.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+




+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);



if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


do {
  if (!cfg || count == sz)
   return ...(-ENODEV);
 ...
} while (...);

Though, as I said for-loop will look slightly better I think.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);



Perhaps one line (~83 characters here is OK) ?


The checkpatch script complains about such lines.


So what if it just 3 characters out?

Many upstream reviewers have objection to lines crossing over 80 
characters

I have gotten reviews to reduce the line length even if its like 81~82
characters. Can we keep this as it is? I have addressed all other
comments and will send out the next patch by today.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);



Ditto.



+   attr1_cfg = bcast_off +
+
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+   attr0_cfg = bcast_off +
+
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);



Ditto.



+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority <<
ATTR1_PRIORITY_SHIFT;



foo |=
  bar << SHIFT;

would look slightly better.


Did you consider this option ?

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-22 12:38, Andy Shevchenko wrote:

On Tue, May 22, 2018 at 9:33 PM,   wrote:

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1



Are these bits? Perhaps BIT() ?

isn't it just better to use fixed size as u suggest in the next 
comment?


If the are bits, use BIT() macro.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+




+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);



if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


do {
  if (!cfg || count == sz)
   return ...(-ENODEV);
 ...
} while (...);

Though, as I said for-loop will look slightly better I think.

Is this fine?
for (count = 0; count < sz; count++) {
   if (!cfg)
return ERR_PTR(-ENODEV);
   if (cfg->usecase_id == uid)
break;
   cfg++;
}
if (count == sz)
   return ERR_PTR(-ENODEV);




+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);



Perhaps one line (~83 characters here is OK) ?


The checkpatch script complains about such lines.


So what if it just 3 characters out?


Other reviewers sometimes are not okay if the checkpatch complains.
Because I have gotten many reviews previously concerning about line
length. Not sure how to proceed here.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);



Ditto.



+   attr1_cfg = bcast_off +
+
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+   attr0_cfg = bcast_off +
+
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);



Ditto.



+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority <<
ATTR1_PRIORITY_SHIFT;



foo |=
  bar << SHIFT;

would look slightly better.


Did you consider this option ?

Yes, forgot to mention.

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-22 12:38, Andy Shevchenko wrote:

On Tue, May 22, 2018 at 9:33 PM,   wrote:

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1



Are these bits? Perhaps BIT() ?

isn't it just better to use fixed size as u suggest in the next 
comment?


If the are bits, use BIT() macro.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+




+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);



if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


do {
  if (!cfg || count == sz)
   return ...(-ENODEV);
 ...
} while (...);

Though, as I said for-loop will look slightly better I think.

Is this fine?
for (count = 0; count < sz; count++) {
   if (!cfg)
return ERR_PTR(-ENODEV);
   if (cfg->usecase_id == uid)
break;
   cfg++;
}
if (count == sz)
   return ERR_PTR(-ENODEV);




+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);



Perhaps one line (~83 characters here is OK) ?


The checkpatch script complains about such lines.


So what if it just 3 characters out?


Other reviewers sometimes are not okay if the checkpatch complains.
Because I have gotten many reviews previously concerning about line
length. Not sure how to proceed here.


+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);



Ditto.



+   attr1_cfg = bcast_off +
+
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);
+   attr0_cfg = bcast_off +
+
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);



Ditto.



+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority <<
ATTR1_PRIORITY_SHIFT;



foo |=
  bar << SHIFT;

would look slightly better.


Did you consider this option ?

Yes, forgot to mention.

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into multiple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API for the clients to perform these operations.



+static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
+   { .compatible = "qcom,sdm845-llcc", },



+   { },


Slightly better w/o comma

Changed



+};



+static struct platform_driver sdm845_qcom_llcc_driver = {
+   .driver = {
+   .name = "sdm845-llcc",



+   .owner = THIS_MODULE,


No need. See below.


+   .of_match_table = sdm845_qcom_llcc_of_match,
+   },
+   .probe = sdm845_qcom_llcc_probe,
+};



+
+static int __init sdm845_init_qcom_llcc_init(void)
+{
+   return platform_driver_register(_qcom_llcc_driver);
+}
+module_init(sdm845_init_qcom_llcc_init);
+
+static void __exit sdm845_exit_qcom_llcc_exit(void)
+{
+   platform_driver_unregister(_qcom_llcc_driver);
+}
+module_exit(sdm845_exit_qcom_llcc_exit);


Why not to use module_platform_driver() macro?

Done



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1


Are these bits? Perhaps BIT() ?


isn't it just better to use fixed size as u suggest in the next comment?


+#define ACT_CTRL_OPCODE_SHIFT 0x1
+#define ATTR1_PROBE_TARGET_WAYS_SHIFT 0x2
+#define ATTR1_FIXED_SIZE_SHIFT0x3
+#define ATTR1_PRIORITY_SHIFT  0x4
+#define ATTR1_MAX_CAP_SHIFT   0x10


Better to use fixed size pattern, i.e. 0x01, 0x02, 0x03, 0x04, 0x10.


+#define ATTR0_RES_WAYS_MASK   0x0fff
+#define ATTR0_BONUS_WAYS_MASK 0x0fff


GENMASK()

Done



+#define LLCC_LB_CNT_MASK   0xf000


Ditto.


+#define MAX_CAP_TO_BYTES(n) (n * 1024)


(n * SZ_1K) ?

Done



+#define LLCC_TRP_ACT_CTRLn(n) (n * 0x1000)


SZ_4K ?


+#define LLCC_TRP_STATUSn(n)   (4 + n * 0x1000)


Ditto.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+



+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);


if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


+static int llcc_update_act_ctrl(u32 sid,
+   u32 act_ctrl_reg_val, u32 status)
+{
+   u32 act_ctrl_reg;
+   u32 status_reg;
+   u32 slice_status;



+   int ret = 0;


Useless assignment. Check entire patch series for a such.

Checked and  removed these.



+   ret = regmap_read_poll_timeout(drv_data->regmap, status_reg,
+   slice_status, !(slice_status & status), 0, 
LLCC_STATUS_READ_DELAY);


Wrong indentation.

Corrected



+   return ret;
+}



+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);


Perhaps one line (~83 characters here is OK) ?

The checkpatch script complains about such lines.



+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);


Ditto.


+   attr1_cfg = bcast_off +
+   
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);

+   attr0_cfg = bcast_off +
+   
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);


Ditto.


+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority << 
ATTR1_PRIORITY_SHIFT;


foo |=
  bar << SHIFT;

would look slightly better.


+int qcom_llcc_probe(struct platform_device *pdev,
+ const struct llcc_slice_config *llcc_cfg, u32 
sz)

+{



+   drv_data->offsets = devm_kzalloc(dev, num_banks * sizeof(u32),
+   

Re: [PATCH v7 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-05-18 14:01, Andy Shevchenko wrote:

On Wed, May 16, 2018 at 8:43 PM, Rishabh Bhatnagar
 wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into multiple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API for the clients to perform these operations.



+static const struct of_device_id sdm845_qcom_llcc_of_match[] = {
+   { .compatible = "qcom,sdm845-llcc", },



+   { },


Slightly better w/o comma

Changed



+};



+static struct platform_driver sdm845_qcom_llcc_driver = {
+   .driver = {
+   .name = "sdm845-llcc",



+   .owner = THIS_MODULE,


No need. See below.


+   .of_match_table = sdm845_qcom_llcc_of_match,
+   },
+   .probe = sdm845_qcom_llcc_probe,
+};



+
+static int __init sdm845_init_qcom_llcc_init(void)
+{
+   return platform_driver_register(_qcom_llcc_driver);
+}
+module_init(sdm845_init_qcom_llcc_init);
+
+static void __exit sdm845_exit_qcom_llcc_exit(void)
+{
+   platform_driver_unregister(_qcom_llcc_driver);
+}
+module_exit(sdm845_exit_qcom_llcc_exit);


Why not to use module_platform_driver() macro?

Done



+#define ACTIVATE  0x1
+#define DEACTIVATE0x2
+#define ACT_CTRL_OPCODE_ACTIVATE  0x1
+#define ACT_CTRL_OPCODE_DEACTIVATE0x2
+#define ACT_CTRL_ACT_TRIG 0x1


Are these bits? Perhaps BIT() ?


isn't it just better to use fixed size as u suggest in the next comment?


+#define ACT_CTRL_OPCODE_SHIFT 0x1
+#define ATTR1_PROBE_TARGET_WAYS_SHIFT 0x2
+#define ATTR1_FIXED_SIZE_SHIFT0x3
+#define ATTR1_PRIORITY_SHIFT  0x4
+#define ATTR1_MAX_CAP_SHIFT   0x10


Better to use fixed size pattern, i.e. 0x01, 0x02, 0x03, 0x04, 0x10.


+#define ATTR0_RES_WAYS_MASK   0x0fff
+#define ATTR0_BONUS_WAYS_MASK 0x0fff


GENMASK()

Done



+#define LLCC_LB_CNT_MASK   0xf000


Ditto.


+#define MAX_CAP_TO_BYTES(n) (n * 1024)


(n * SZ_1K) ?

Done



+#define LLCC_TRP_ACT_CTRLn(n) (n * 0x1000)


SZ_4K ?


+#define LLCC_TRP_STATUSn(n)   (4 + n * 0x1000)


Ditto.


+struct llcc_slice_desc *llcc_slice_getd(u32 uid)
+{
+   const struct llcc_slice_config *cfg;
+   struct llcc_slice_desc *desc;
+   u32 sz, count = 0;
+
+   cfg = drv_data->cfg;
+   sz = drv_data->cfg_size;
+



+   while (cfg && count < sz) {
+   if (cfg->usecase_id == uid)
+   break;
+   cfg++;
+   count++;
+   }
+   if (cfg == NULL || count == sz)
+   return ERR_PTR(-ENODEV);


if (!cfg)
  return ERR_PTR(-ENODEV);

while (cfg->... != uid) {
  cfg++;
  count++;
}

if (count == sz)
 return ...

Though I would rather put it to for () loop.


In each while loop iteration the cfg pointer needs to be checked for
NULL. What if the usecase id never matches the uid passed by client
and we keep iterating. At some point it will crash.


+static int llcc_update_act_ctrl(u32 sid,
+   u32 act_ctrl_reg_val, u32 status)
+{
+   u32 act_ctrl_reg;
+   u32 status_reg;
+   u32 slice_status;



+   int ret = 0;


Useless assignment. Check entire patch series for a such.

Checked and  removed these.



+   ret = regmap_read_poll_timeout(drv_data->regmap, status_reg,
+   slice_status, !(slice_status & status), 0, 
LLCC_STATUS_READ_DELAY);


Wrong indentation.

Corrected



+   return ret;
+}



+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ DEACTIVATE);


Perhaps one line (~83 characters here is OK) ?

The checkpatch script complains about such lines.



+   ret = llcc_update_act_ctrl(desc->slice_id, act_ctrl_val,
+ ACTIVATE);


Ditto.


+   attr1_cfg = bcast_off +
+   
LLCC_TRP_ATTR1_CFGn(llcc_table[i].slice_id);

+   attr0_cfg = bcast_off +
+   
LLCC_TRP_ATTR0_CFGn(llcc_table[i].slice_id);


Ditto.


+   attr1_val |= llcc_table[i].probe_target_ways <<
+   ATTR1_PROBE_TARGET_WAYS_SHIFT;
+   attr1_val |= llcc_table[i].fixed_size <<
+   ATTR1_FIXED_SIZE_SHIFT;
+   attr1_val |= llcc_table[i].priority << 
ATTR1_PRIORITY_SHIFT;


foo |=
  bar << SHIFT;

would look slightly better.


+int qcom_llcc_probe(struct platform_device *pdev,
+ const struct llcc_slice_config *llcc_cfg, u32 
sz)

+{



+   drv_data->offsets = devm_kzalloc(dev, num_banks * sizeof(u32),
+   

Re: [PATCH v6 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-05-16 11:08, Stephen Boyd wrote:

Quoting risha...@codeaurora.org (2018-05-16 10:33:14)

On 2018-05-16 10:03, Stephen Boyd wrote:
> Quoting Rishabh Bhatnagar (2018-05-08 13:22:00)

>> +
>> +- max-slices:
>> +   usage: required
>> +   Value Type: 
>> +   Definition: Number of cache slices supported by hardware
>> +
>> +Example:
>> +
>> +   llcc: qcom,llcc@110 {
>
> cache-controller@110 ?
>
We have tried to use consistent naming convention as in llcc_*
everywhere.
Using cache-controller will mix and match the naming convention. Also 
in

the documentation it is explained what llcc is and its full form.



DT prefers standard node names as opposed to vendor specific node 
names.

Isn't it a cache controller? I fail to see why this can't be done.

Hi Stephen,
The driver is vendor specific and also for uniformity purposes we 
preferred

to go with this name.

Re: [PATCH v6 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-05-16 11:08, Stephen Boyd wrote:

Quoting risha...@codeaurora.org (2018-05-16 10:33:14)

On 2018-05-16 10:03, Stephen Boyd wrote:
> Quoting Rishabh Bhatnagar (2018-05-08 13:22:00)

>> +
>> +- max-slices:
>> +   usage: required
>> +   Value Type: 
>> +   Definition: Number of cache slices supported by hardware
>> +
>> +Example:
>> +
>> +   llcc: qcom,llcc@110 {
>
> cache-controller@110 ?
>
We have tried to use consistent naming convention as in llcc_*
everywhere.
Using cache-controller will mix and match the naming convention. Also 
in

the documentation it is explained what llcc is and its full form.



DT prefers standard node names as opposed to vendor specific node 
names.

Isn't it a cache controller? I fail to see why this can't be done.

Hi Stephen,
The driver is vendor specific and also for uniformity purposes we 
preferred

to go with this name.

Re: [PATCH v6 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-05-16 10:03, Stephen Boyd wrote:

Quoting Rishabh Bhatnagar (2018-05-08 13:22:00)
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..a586a17
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,32 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to
+common pool of memory. Cache memory is divided into partitions called 
slices
+which are assigned to clients. Clients can query the slice details, 
activate

+and deactivate them.
+
+Properties:
+- compatible:
+   Usage: required
+   Value type: 
+   Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+   Usage: required
+   Value Type: 
+   Definition: Start address and the range of the LLCC registers.


Start address and size?


Yes i'll change it to Start address and size of the register region.


+
+- max-slices:
+   usage: required
+   Value Type: 
+   Definition: Number of cache slices supported by hardware
+
+Example:
+
+   llcc: qcom,llcc@110 {


cache-controller@110 ?

We have tried to use consistent naming convention as in llcc_* 
everywhere.

Using cache-controller will mix and match the naming convention. Also in
the documentation it is explained what llcc is and its full form.


+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   max-slices = <32>;
+   };
--

Re: [PATCH v6 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-05-16 10:03, Stephen Boyd wrote:

Quoting Rishabh Bhatnagar (2018-05-08 13:22:00)
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..a586a17
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,32 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to
+common pool of memory. Cache memory is divided into partitions called 
slices
+which are assigned to clients. Clients can query the slice details, 
activate

+and deactivate them.
+
+Properties:
+- compatible:
+   Usage: required
+   Value type: 
+   Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+   Usage: required
+   Value Type: 
+   Definition: Start address and the range of the LLCC registers.


Start address and size?


Yes i'll change it to Start address and size of the register region.


+
+- max-slices:
+   usage: required
+   Value Type: 
+   Definition: Number of cache slices supported by hardware
+
+Example:
+
+   llcc: qcom,llcc@110 {


cache-controller@110 ?

We have tried to use consistent naming convention as in llcc_* 
everywhere.

Using cache-controller will mix and match the naming convention. Also in
the documentation it is explained what llcc is and its full form.


+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   max-slices = <32>;
+   };
--

Re: [PATCH v5 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-04-30 07:33, Rob Herring wrote:

On Fri, Apr 27, 2018 at 5:57 PM,   wrote:

On 2018-04-27 07:21, Rob Herring wrote:


On Mon, Apr 23, 2018 at 04:09:31PM -0700, Rishabh Bhatnagar wrote:


Documentation for last level cache controller device tree bindings,
client bindings usage examples.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 60
++
 1 file changed, 60 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt



My comments on v4 still apply.

Rob



Hi Rob,
Reposting our replies to your comments on v4:

This is partially true, a bunch of SoCs would support this design but
clients IDs are not expected to change. So Ideally client drivers 
could

hard code these IDs.

However I have other concerns of moving the client Ids in the driver.
The way the APIs implemented today are as follows:
#1. Client calls into system cache driver to get cache slice handle
with the usecase Id as input.
#2. System cache driver gets the phandle of system cache instance from
the client device to obtain the private data.
#3. Based on the usecase Id perform look up in the private data to get
cache slice handle.
#4. Return the cache slice handle to client

If we don't have the connection between client & system cache then the
 private data needs to declared as static global in the system cache 
driver,

that limits us to have just once instance of system cache block.


How many instances do you have?

It is easier to put the data into the kernel and move it to DT later
than vice-versa. I don't think it is a good idea to do a custom
binding here and one that only addresses caches and nothing else in
the interconnect. So either we define an extensible and future-proof
binding or put the data into the kernel for now.

Rob

Hi rob,
Currently we have only instance but how do you propose we handle 
multiple

instances in future?
Currently we do a lookup in the private data of the driver to get the 
slice
handle but, if we were to remove the client connection we will have to 
make

lookup table as global and we can't have more than one instance.
Also, can you suggest any extensible interconnect binding that we can 
refer

to?

Re: [PATCH v5 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-04-30 07:33, Rob Herring wrote:

On Fri, Apr 27, 2018 at 5:57 PM,   wrote:

On 2018-04-27 07:21, Rob Herring wrote:


On Mon, Apr 23, 2018 at 04:09:31PM -0700, Rishabh Bhatnagar wrote:


Documentation for last level cache controller device tree bindings,
client bindings usage examples.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 60
++
 1 file changed, 60 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt



My comments on v4 still apply.

Rob



Hi Rob,
Reposting our replies to your comments on v4:

This is partially true, a bunch of SoCs would support this design but
clients IDs are not expected to change. So Ideally client drivers 
could

hard code these IDs.

However I have other concerns of moving the client Ids in the driver.
The way the APIs implemented today are as follows:
#1. Client calls into system cache driver to get cache slice handle
with the usecase Id as input.
#2. System cache driver gets the phandle of system cache instance from
the client device to obtain the private data.
#3. Based on the usecase Id perform look up in the private data to get
cache slice handle.
#4. Return the cache slice handle to client

If we don't have the connection between client & system cache then the
 private data needs to declared as static global in the system cache 
driver,

that limits us to have just once instance of system cache block.


How many instances do you have?

It is easier to put the data into the kernel and move it to DT later
than vice-versa. I don't think it is a good idea to do a custom
binding here and one that only addresses caches and nothing else in
the interconnect. So either we define an extensible and future-proof
binding or put the data into the kernel for now.

Rob

Hi rob,
Currently we have only instance but how do you propose we handle 
multiple

instances in future?
Currently we do a lookup in the private data of the driver to get the 
slice
handle but, if we were to remove the client connection we will have to 
make

lookup table as global and we can't have more than one instance.
Also, can you suggest any extensible interconnect binding that we can 
refer

to?

Re: [PATCH v5 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-04-27 07:21, Rob Herring wrote:

On Mon, Apr 23, 2018 at 04:09:31PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 60 
++

 1 file changed, 60 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


My comments on v4 still apply.

Rob


Hi Rob,
Reposting our replies to your comments on v4:

This is partially true, a bunch of SoCs would support this design but
clients IDs are not expected to change. So Ideally client drivers could
hard code these IDs.

However I have other concerns of moving the client Ids in the driver.
The way the APIs implemented today are as follows:
#1. Client calls into system cache driver to get cache slice handle
with the usecase Id as input.
#2. System cache driver gets the phandle of system cache instance from
the client device to obtain the private data.
#3. Based on the usecase Id perform look up in the private data to get
cache slice handle.
#4. Return the cache slice handle to client

If we don't have the connection between client & system cache then the
 private data needs to declared as static global in the system cache 
driver,

that limits us to have just once instance of system cache block.

Please let us know what you think.

Re: [PATCH v5 1/2] dt-bindings: Documentation for qcom, llcc

[rishabhb]

On 2018-04-27 07:21, Rob Herring wrote:

On Mon, Apr 23, 2018 at 04:09:31PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 60 
++

 1 file changed, 60 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


My comments on v4 still apply.

Rob


Hi Rob,
Reposting our replies to your comments on v4:

This is partially true, a bunch of SoCs would support this design but
clients IDs are not expected to change. So Ideally client drivers could
hard code these IDs.

However I have other concerns of moving the client Ids in the driver.
The way the APIs implemented today are as follows:
#1. Client calls into system cache driver to get cache slice handle
with the usecase Id as input.
#2. System cache driver gets the phandle of system cache instance from
the client device to obtain the private data.
#3. Based on the usecase Id perform look up in the private data to get
cache slice handle.
#4. Return the cache slice handle to client

If we don't have the connection between client & system cache then the
 private data needs to declared as static global in the system cache 
driver,

that limits us to have just once instance of system cache block.

Please let us know what you think.

Re: [PATCH v4 1/2] Documentation: Documentation for qcom, llcc

[rishabhb]

On 2018-04-17 10:43, risha...@codeaurora.org wrote:

On 2018-04-16 07:59, Rob Herring wrote:

On Tue, Apr 10, 2018 at 01:08:12PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.


"Documentation: Documentation ..."? That wastes a lot of the subject
line... The preferred prefix is "dt-bindings: ..."



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 58 
++

 1 file changed, 58 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..497cf0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,58 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to

+common pool of memory
+
+Properties:
+- compatible:
+Usage: required
+Value type: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+Definition: must be addresses and sizes of the LLCC registers


How many address ranges?


It consists of just one address range. I'll edit the definition to make
it more clear.

+
+- #cache-cells:


This is all written as it is a common binding, but it is not one.

You already have most of the configuration data for each client in the
driver, I think I'd just put the client connection there too. Is there
any variation of this for a given SoC?

#cache-cells and max-slices won't change for a given SOC. So you want 
me

to hard-code in the driver itself?

I can use of_parse_phandle_with_fixed_args function and fix the number 
of
args as 1 instead of keeping #cache-cells here in DT. Does that look 
fine?

+Usage: required
+Value Type: 
+Definition: Number of cache cells, must be 1
+
+- max-slices:
+usage: required
+Value Type: 
+Definition: Number of cache slices supported by hardware


What's a slice?


System cache memory provided by LLCC is divided into smaller chunks
called slices. Each slice has its associated size and ID. Clients can
query slice details, activate and deactivate them.

+
+Example:
+
+   llcc: qcom,llcc@110 {
+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+Usage: required
+Value type: 
+Definition: A set of names that identify the usecase names of a
+   client that uses cache slice. These strings are
+   used to look up the cache slice entries by name.
+
+- cache-slices:
+Usage: required
+Value type: 
+Definition: The tuple has phandle to llcc device as the first
+   argument and the second argument is the usecase
+   id of the client.
+For Example:
+   venus {
+   cache-slice-names = "vidsc0", "vidsc1";
+   cache-slices = < VIDSC0_ID>, < VIDSC1_ID>;
+   };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] Documentation: Documentation for qcom, llcc

[rishabhb]

On 2018-04-17 10:43, risha...@codeaurora.org wrote:

On 2018-04-16 07:59, Rob Herring wrote:

On Tue, Apr 10, 2018 at 01:08:12PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.


"Documentation: Documentation ..."? That wastes a lot of the subject
line... The preferred prefix is "dt-bindings: ..."



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 58 
++

 1 file changed, 58 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..497cf0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,58 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to

+common pool of memory
+
+Properties:
+- compatible:
+Usage: required
+Value type: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+Definition: must be addresses and sizes of the LLCC registers


How many address ranges?


It consists of just one address range. I'll edit the definition to make
it more clear.

+
+- #cache-cells:


This is all written as it is a common binding, but it is not one.

You already have most of the configuration data for each client in the
driver, I think I'd just put the client connection there too. Is there
any variation of this for a given SoC?

#cache-cells and max-slices won't change for a given SOC. So you want 
me

to hard-code in the driver itself?

I can use of_parse_phandle_with_fixed_args function and fix the number 
of
args as 1 instead of keeping #cache-cells here in DT. Does that look 
fine?

+Usage: required
+Value Type: 
+Definition: Number of cache cells, must be 1
+
+- max-slices:
+usage: required
+Value Type: 
+Definition: Number of cache slices supported by hardware


What's a slice?


System cache memory provided by LLCC is divided into smaller chunks
called slices. Each slice has its associated size and ID. Clients can
query slice details, activate and deactivate them.

+
+Example:
+
+   llcc: qcom,llcc@110 {
+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+Usage: required
+Value type: 
+Definition: A set of names that identify the usecase names of a
+   client that uses cache slice. These strings are
+   used to look up the cache slice entries by name.
+
+- cache-slices:
+Usage: required
+Value type: 
+Definition: The tuple has phandle to llcc device as the first
+   argument and the second argument is the usecase
+   id of the client.
+For Example:
+   venus {
+   cache-slice-names = "vidsc0", "vidsc1";
+   cache-slices = < VIDSC0_ID>, < VIDSC1_ID>;
+   };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] Documentation: Documentation for qcom, llcc

[rishabhb]

On 2018-04-16 07:59, Rob Herring wrote:

On Tue, Apr 10, 2018 at 01:08:12PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.


"Documentation: Documentation ..."? That wastes a lot of the subject
line... The preferred prefix is "dt-bindings: ..."



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 58 
++

 1 file changed, 58 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..497cf0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,58 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to

+common pool of memory
+
+Properties:
+- compatible:
+Usage: required
+Value type: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+Definition: must be addresses and sizes of the LLCC registers


How many address ranges?


It consists of just one address range. I'll edit the definition to make
it more clear.

+
+- #cache-cells:


This is all written as it is a common binding, but it is not one.

You already have most of the configuration data for each client in the
driver, I think I'd just put the client connection there too. Is there
any variation of this for a given SoC?


#cache-cells and max-slices won't change for a given SOC. So you want me
to hard-code in the driver itself?


+Usage: required
+Value Type: 
+Definition: Number of cache cells, must be 1
+
+- max-slices:
+usage: required
+Value Type: 
+Definition: Number of cache slices supported by hardware


What's a slice?


System cache memory provided by LLCC is divided into smaller chunks
called slices. Each slice has its associated size and ID. Clients can
query slice details, activate and deactivate them.

+
+Example:
+
+   llcc: qcom,llcc@110 {
+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+Usage: required
+Value type: 
+Definition: A set of names that identify the usecase names of a
+   client that uses cache slice. These strings are
+   used to look up the cache slice entries by name.
+
+- cache-slices:
+Usage: required
+Value type: 
+Definition: The tuple has phandle to llcc device as the first
+   argument and the second argument is the usecase
+   id of the client.
+For Example:
+   venus {
+   cache-slice-names = "vidsc0", "vidsc1";
+   cache-slices = < VIDSC0_ID>, < VIDSC1_ID>;
+   };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] Documentation: Documentation for qcom, llcc

[rishabhb]

On 2018-04-16 07:59, Rob Herring wrote:

On Tue, Apr 10, 2018 at 01:08:12PM -0700, Rishabh Bhatnagar wrote:

Documentation for last level cache controller device tree bindings,
client bindings usage examples.


"Documentation: Documentation ..."? That wastes a lot of the subject
line... The preferred prefix is "dt-bindings: ..."



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 .../devicetree/bindings/arm/msm/qcom,llcc.txt  | 58 
++

 1 file changed, 58 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt


diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt 
b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt

new file mode 100644
index 000..497cf0f
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.txt
@@ -0,0 +1,58 @@
+== Introduction==
+
+LLCC (Last Level Cache Controller) provides last level of cache 
memory in SOC,
+that can be shared by multiple clients. Clients here are different 
cores in the
+SOC, the idea is to minimize the local caches at the clients and 
migrate to

+common pool of memory
+
+Properties:
+- compatible:
+Usage: required
+Value type: 
+Definition: must be "qcom,sdm845-llcc"
+
+- reg:
+Usage: required
+Value Type: 
+Definition: must be addresses and sizes of the LLCC registers


How many address ranges?


It consists of just one address range. I'll edit the definition to make
it more clear.

+
+- #cache-cells:


This is all written as it is a common binding, but it is not one.

You already have most of the configuration data for each client in the
driver, I think I'd just put the client connection there too. Is there
any variation of this for a given SoC?


#cache-cells and max-slices won't change for a given SOC. So you want me
to hard-code in the driver itself?


+Usage: required
+Value Type: 
+Definition: Number of cache cells, must be 1
+
+- max-slices:
+usage: required
+Value Type: 
+Definition: Number of cache slices supported by hardware


What's a slice?


System cache memory provided by LLCC is divided into smaller chunks
called slices. Each slice has its associated size and ID. Clients can
query slice details, activate and deactivate them.

+
+Example:
+
+   llcc: qcom,llcc@110 {
+   compatible = "qcom,sdm845-llcc";
+   reg = <0x110 0x25>;
+   #cache-cells = <1>;
+   max-slices = <32>;
+   };
+
+== Client ==
+
+Properties:
+- cache-slice-names:
+Usage: required
+Value type: 
+Definition: A set of names that identify the usecase names of a
+   client that uses cache slice. These strings are
+   used to look up the cache slice entries by name.
+
+- cache-slices:
+Usage: required
+Value type: 
+Definition: The tuple has phandle to llcc device as the first
+   argument and the second argument is the usecase
+   id of the client.
+For Example:
+   venus {
+   cache-slice-names = "vidsc0", "vidsc1";
+   cache-slices = < VIDSC0_ID>, < VIDSC1_ID>;
+   };
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,

a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-04-16 10:14, Evan Green wrote:

On Fri, Apr 13, 2018 at 4:08 PM  wrote:


On 2018-04-12 15:02, Evan Green wrote:
> Hi Rishabh,
>
> On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar
> 
> wrote:
>
>> LLCC (Last Level Cache Controller) provides additional cache memory
>> in the system. LLCC is partitioned into multiple slices and each
>> slice gets its own priority, size, ID and other config parameters.
>> LLCC driver programs these parameters for each slice. Clients that
>> are assigned to use LLCC need to get information such size & ID of the
>> slice they get and activate or deactivate the slice as needed. LLCC
>> driver
>> provides API for the clients to perform these operations.
>
>> Signed-off-by: Channagoud Kadabi 
>> Signed-off-by: Rishabh Bhatnagar 
>> ---
>>   drivers/soc/qcom/Kconfig   |  17 ++
>>   drivers/soc/qcom/Makefile  |   2 +
>>   drivers/soc/qcom/llcc-sdm845.c | 110 ++
>>   drivers/soc/qcom/llcc-slice.c  | 404
> +
>>   include/linux/soc/qcom/llcc-qcom.h | 168 +++
>>   5 files changed, 701 insertions(+)
>>   create mode 100644 drivers/soc/qcom/llcc-sdm845.c
>>   create mode 100644 drivers/soc/qcom/llcc-slice.c
>>   create mode 100644 include/linux/soc/qcom/llcc-qcom.h
>
> [...]
>> diff --git a/drivers/soc/qcom/llcc-sdm845.c
> b/drivers/soc/qcom/llcc-sdm845.c
>> new file mode 100644
>> index 000..619b226
>> --- /dev/null
>> +++ b/drivers/soc/qcom/llcc-sdm845.c
>> @@ -0,0 +1,110 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights
>> reserved.
>> + *
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * SCT(System Cache Table) entry contains of the following parameters
>
> contains the following members:
>
>> + * name: Name of the client's use case for which the llcc slice is
>> used
>> + * uid: Unique id for the client's use case
>
> s/uid/usecase_id/
>
>> + * slice_id: llcc slice id for each client
>> + * max_cap: The maximum capacity of the cache slice provided in KB
>> + * priority: Priority of the client used to select victim line for
> replacement
>> + * fixed_size: Determine if the slice has a fixed capacity
>
> "Boolean indicating if the slice has a fixed capacity" might be better
>
>> diff --git a/drivers/soc/qcom/llcc-slice.c
>> b/drivers/soc/qcom/llcc-slice.c
>> new file mode 100644
>> index 000..67a81b0
>> --- /dev/null
>> +++ b/drivers/soc/qcom/llcc-slice.c
> ...
>> +static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid,
>> +   u32 act_ctrl_reg_val, u32 status)
>> +{
>> +   u32 act_ctrl_reg;
>> +   u32 status_reg;
>> +   u32 slice_status;
>> +   int ret = 0;
>> +
>> +   act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid);
>> +   status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid);
>> +
>> +   /*Set the ACTIVE trigger*/
>
> Add spaces around /* */
>
>> +   act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG;
>> +   ret = regmap_write(drv->regmap, act_ctrl_reg,
>> act_ctrl_reg_val);
>> +   if (ret)
>> +   return ret;
>> +
>> +   /* Clear the ACTIVE trigger */
>> +   act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG;
>> +   ret = regmap_write(drv->regmap, act_ctrl_reg,
>> act_ctrl_reg_val);
>> +   if (ret)
>> +   return ret;
>> +
>> +   ret = regmap_read_poll_timeout(drv->regmap, status_reg,
> slice_status,
>> +   !(slice_status & status), 0,
> LLCC_STATUS_READ_DELAY);
>> +   return ret;
>> +}
>> +
>> +/**
>> + * llcc_slice_activate - Activate the llcc slice
>> + * @desc: Pointer to llcc slice descriptor
>> + *
>> + * A value zero will be returned on success and a negative errno will
>
> a value of zero
>
>> + * be returned in error cases
>> + */
>> +int llcc_slice_activate(struct llcc_slice_desc *desc)
>> +{
>> +   int ret;
>> +   u32 act_ctrl_val;
>> +   struct llcc_drv_data *drv;
>> +
>> +   if (desc == NULL)
>> +   return -EINVAL;
>
> I think we can remove this check, right?
>
>> +
>> +   drv = dev_get_drvdata(desc->dev);
>> +   if (!drv)
>> +   return -EINVAL;
>> +
>> +   mutex_lock(>lock);
>> +   if (test_bit(desc->slice_id, drv->bitmap)) {
>> +   mutex_unlock(>lock);
>> +   return 0;
>> +   }
>> +
>> +   act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE <<
>> ACT_CTRL_OPCODE_SHIFT;
>> +
>> +   ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
>> + DEACTIVATE);
>> +
>> +   __set_bit(desc->slice_id, drv->bitmap);
>> +   mutex_unlock(>lock);
>> +
>> +   return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(llcc_slice_activate);
>> +
>> +/**
>> + * llcc_slice_deactivate - Deactivate the llcc slice
>> + * @desc: Pointer to llcc 

Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-04-16 10:14, Evan Green wrote:

On Fri, Apr 13, 2018 at 4:08 PM  wrote:


On 2018-04-12 15:02, Evan Green wrote:
> Hi Rishabh,
>
> On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar
> 
> wrote:
>
>> LLCC (Last Level Cache Controller) provides additional cache memory
>> in the system. LLCC is partitioned into multiple slices and each
>> slice gets its own priority, size, ID and other config parameters.
>> LLCC driver programs these parameters for each slice. Clients that
>> are assigned to use LLCC need to get information such size & ID of the
>> slice they get and activate or deactivate the slice as needed. LLCC
>> driver
>> provides API for the clients to perform these operations.
>
>> Signed-off-by: Channagoud Kadabi 
>> Signed-off-by: Rishabh Bhatnagar 
>> ---
>>   drivers/soc/qcom/Kconfig   |  17 ++
>>   drivers/soc/qcom/Makefile  |   2 +
>>   drivers/soc/qcom/llcc-sdm845.c | 110 ++
>>   drivers/soc/qcom/llcc-slice.c  | 404
> +
>>   include/linux/soc/qcom/llcc-qcom.h | 168 +++
>>   5 files changed, 701 insertions(+)
>>   create mode 100644 drivers/soc/qcom/llcc-sdm845.c
>>   create mode 100644 drivers/soc/qcom/llcc-slice.c
>>   create mode 100644 include/linux/soc/qcom/llcc-qcom.h
>
> [...]
>> diff --git a/drivers/soc/qcom/llcc-sdm845.c
> b/drivers/soc/qcom/llcc-sdm845.c
>> new file mode 100644
>> index 000..619b226
>> --- /dev/null
>> +++ b/drivers/soc/qcom/llcc-sdm845.c
>> @@ -0,0 +1,110 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights
>> reserved.
>> + *
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +/*
>> + * SCT(System Cache Table) entry contains of the following parameters
>
> contains the following members:
>
>> + * name: Name of the client's use case for which the llcc slice is
>> used
>> + * uid: Unique id for the client's use case
>
> s/uid/usecase_id/
>
>> + * slice_id: llcc slice id for each client
>> + * max_cap: The maximum capacity of the cache slice provided in KB
>> + * priority: Priority of the client used to select victim line for
> replacement
>> + * fixed_size: Determine if the slice has a fixed capacity
>
> "Boolean indicating if the slice has a fixed capacity" might be better
>
>> diff --git a/drivers/soc/qcom/llcc-slice.c
>> b/drivers/soc/qcom/llcc-slice.c
>> new file mode 100644
>> index 000..67a81b0
>> --- /dev/null
>> +++ b/drivers/soc/qcom/llcc-slice.c
> ...
>> +static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid,
>> +   u32 act_ctrl_reg_val, u32 status)
>> +{
>> +   u32 act_ctrl_reg;
>> +   u32 status_reg;
>> +   u32 slice_status;
>> +   int ret = 0;
>> +
>> +   act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid);
>> +   status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid);
>> +
>> +   /*Set the ACTIVE trigger*/
>
> Add spaces around /* */
>
>> +   act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG;
>> +   ret = regmap_write(drv->regmap, act_ctrl_reg,
>> act_ctrl_reg_val);
>> +   if (ret)
>> +   return ret;
>> +
>> +   /* Clear the ACTIVE trigger */
>> +   act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG;
>> +   ret = regmap_write(drv->regmap, act_ctrl_reg,
>> act_ctrl_reg_val);
>> +   if (ret)
>> +   return ret;
>> +
>> +   ret = regmap_read_poll_timeout(drv->regmap, status_reg,
> slice_status,
>> +   !(slice_status & status), 0,
> LLCC_STATUS_READ_DELAY);
>> +   return ret;
>> +}
>> +
>> +/**
>> + * llcc_slice_activate - Activate the llcc slice
>> + * @desc: Pointer to llcc slice descriptor
>> + *
>> + * A value zero will be returned on success and a negative errno will
>
> a value of zero
>
>> + * be returned in error cases
>> + */
>> +int llcc_slice_activate(struct llcc_slice_desc *desc)
>> +{
>> +   int ret;
>> +   u32 act_ctrl_val;
>> +   struct llcc_drv_data *drv;
>> +
>> +   if (desc == NULL)
>> +   return -EINVAL;
>
> I think we can remove this check, right?
>
>> +
>> +   drv = dev_get_drvdata(desc->dev);
>> +   if (!drv)
>> +   return -EINVAL;
>> +
>> +   mutex_lock(>lock);
>> +   if (test_bit(desc->slice_id, drv->bitmap)) {
>> +   mutex_unlock(>lock);
>> +   return 0;
>> +   }
>> +
>> +   act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE <<
>> ACT_CTRL_OPCODE_SHIFT;
>> +
>> +   ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
>> + DEACTIVATE);
>> +
>> +   __set_bit(desc->slice_id, drv->bitmap);
>> +   mutex_unlock(>lock);
>> +
>> +   return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(llcc_slice_activate);
>> +
>> +/**
>> + * llcc_slice_deactivate - Deactivate the llcc slice
>> + * @desc: Pointer to llcc slice descriptor
>> + *
>> + * A value zero will be returned on success and a negative errno will
>> + 

Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-04-12 15:02, Evan Green wrote:

Hi Rishabh,

On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar 


wrote:


LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into multiple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API for the clients to perform these operations.



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
  drivers/soc/qcom/Kconfig   |  17 ++
  drivers/soc/qcom/Makefile  |   2 +
  drivers/soc/qcom/llcc-sdm845.c | 110 ++
  drivers/soc/qcom/llcc-slice.c  | 404

+

  include/linux/soc/qcom/llcc-qcom.h | 168 +++
  5 files changed, 701 insertions(+)
  create mode 100644 drivers/soc/qcom/llcc-sdm845.c
  create mode 100644 drivers/soc/qcom/llcc-slice.c
  create mode 100644 include/linux/soc/qcom/llcc-qcom.h


[...]

diff --git a/drivers/soc/qcom/llcc-sdm845.c

b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..619b226
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * SCT(System Cache Table) entry contains of the following parameters


contains the following members:

+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case


s/uid/usecase_id/


+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for

replacement

+ * fixed_size: Determine if the slice has a fixed capacity


"Boolean indicating if the slice has a fixed capacity" might be better

diff --git a/drivers/soc/qcom/llcc-slice.c 
b/drivers/soc/qcom/llcc-slice.c

new file mode 100644
index 000..67a81b0
--- /dev/null
+++ b/drivers/soc/qcom/llcc-slice.c

...

+static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid,
+   u32 act_ctrl_reg_val, u32 status)
+{
+   u32 act_ctrl_reg;
+   u32 status_reg;
+   u32 slice_status;
+   int ret = 0;
+
+   act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid);
+   status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid);
+
+   /*Set the ACTIVE trigger*/


Add spaces around /* */


+   act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG;
+   ret = regmap_write(drv->regmap, act_ctrl_reg, 
act_ctrl_reg_val);

+   if (ret)
+   return ret;
+
+   /* Clear the ACTIVE trigger */
+   act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG;
+   ret = regmap_write(drv->regmap, act_ctrl_reg, 
act_ctrl_reg_val);

+   if (ret)
+   return ret;
+
+   ret = regmap_read_poll_timeout(drv->regmap, status_reg,

slice_status,

+   !(slice_status & status), 0,

LLCC_STATUS_READ_DELAY);

+   return ret;
+}
+
+/**
+ * llcc_slice_activate - Activate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will


a value of zero


+ * be returned in error cases
+ */
+int llcc_slice_activate(struct llcc_slice_desc *desc)
+{
+   int ret;
+   u32 act_ctrl_val;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL)
+   return -EINVAL;


I think we can remove this check, right?


+
+   drv = dev_get_drvdata(desc->dev);
+   if (!drv)
+   return -EINVAL;
+
+   mutex_lock(>lock);
+   if (test_bit(desc->slice_id, drv->bitmap)) {
+   mutex_unlock(>lock);
+   return 0;
+   }
+
+   act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE << 
ACT_CTRL_OPCODE_SHIFT;

+
+   ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
+ DEACTIVATE);
+
+   __set_bit(desc->slice_id, drv->bitmap);
+   mutex_unlock(>lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(llcc_slice_activate);
+
+/**
+ * llcc_slice_deactivate - Deactivate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will
+ * be returned in error cases
+ */
+int llcc_slice_deactivate(struct llcc_slice_desc *desc)
+{
+   u32 act_ctrl_val;
+   int ret;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL)
+   return -EINVAL;
+
+   drv = dev_get_drvdata(desc->dev);
+   if (!drv)
+   return -EINVAL;
+
+   mutex_lock(>lock);
+   if (!test_bit(desc->slice_id, 

Re: [PATCH v4 2/2] drivers: soc: Add LLCC driver

[rishabhb]

On 2018-04-12 15:02, Evan Green wrote:

Hi Rishabh,

On Tue, Apr 10, 2018 at 1:09 PM Rishabh Bhatnagar 


wrote:


LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into multiple slices and each
slice gets its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API for the clients to perform these operations.



Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
  drivers/soc/qcom/Kconfig   |  17 ++
  drivers/soc/qcom/Makefile  |   2 +
  drivers/soc/qcom/llcc-sdm845.c | 110 ++
  drivers/soc/qcom/llcc-slice.c  | 404

+

  include/linux/soc/qcom/llcc-qcom.h | 168 +++
  5 files changed, 701 insertions(+)
  create mode 100644 drivers/soc/qcom/llcc-sdm845.c
  create mode 100644 drivers/soc/qcom/llcc-slice.c
  create mode 100644 include/linux/soc/qcom/llcc-qcom.h


[...]

diff --git a/drivers/soc/qcom/llcc-sdm845.c

b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..619b226
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * SCT(System Cache Table) entry contains of the following parameters


contains the following members:

+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case


s/uid/usecase_id/


+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for

replacement

+ * fixed_size: Determine if the slice has a fixed capacity


"Boolean indicating if the slice has a fixed capacity" might be better

diff --git a/drivers/soc/qcom/llcc-slice.c 
b/drivers/soc/qcom/llcc-slice.c

new file mode 100644
index 000..67a81b0
--- /dev/null
+++ b/drivers/soc/qcom/llcc-slice.c

...

+static int llcc_update_act_ctrl(struct llcc_drv_data *drv, u32 sid,
+   u32 act_ctrl_reg_val, u32 status)
+{
+   u32 act_ctrl_reg;
+   u32 status_reg;
+   u32 slice_status;
+   int ret = 0;
+
+   act_ctrl_reg = drv->bcast_off + LLCC_TRP_ACT_CTRLn(sid);
+   status_reg = drv->bcast_off + LLCC_TRP_STATUSn(sid);
+
+   /*Set the ACTIVE trigger*/


Add spaces around /* */


+   act_ctrl_reg_val |= ACT_CTRL_ACT_TRIG;
+   ret = regmap_write(drv->regmap, act_ctrl_reg, 
act_ctrl_reg_val);

+   if (ret)
+   return ret;
+
+   /* Clear the ACTIVE trigger */
+   act_ctrl_reg_val &= ~ACT_CTRL_ACT_TRIG;
+   ret = regmap_write(drv->regmap, act_ctrl_reg, 
act_ctrl_reg_val);

+   if (ret)
+   return ret;
+
+   ret = regmap_read_poll_timeout(drv->regmap, status_reg,

slice_status,

+   !(slice_status & status), 0,

LLCC_STATUS_READ_DELAY);

+   return ret;
+}
+
+/**
+ * llcc_slice_activate - Activate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will


a value of zero


+ * be returned in error cases
+ */
+int llcc_slice_activate(struct llcc_slice_desc *desc)
+{
+   int ret;
+   u32 act_ctrl_val;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL)
+   return -EINVAL;


I think we can remove this check, right?


+
+   drv = dev_get_drvdata(desc->dev);
+   if (!drv)
+   return -EINVAL;
+
+   mutex_lock(>lock);
+   if (test_bit(desc->slice_id, drv->bitmap)) {
+   mutex_unlock(>lock);
+   return 0;
+   }
+
+   act_ctrl_val = ACT_CTRL_OPCODE_ACTIVATE << 
ACT_CTRL_OPCODE_SHIFT;

+
+   ret = llcc_update_act_ctrl(drv, desc->slice_id, act_ctrl_val,
+ DEACTIVATE);
+
+   __set_bit(desc->slice_id, drv->bitmap);
+   mutex_unlock(>lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(llcc_slice_activate);
+
+/**
+ * llcc_slice_deactivate - Deactivate the llcc slice
+ * @desc: Pointer to llcc slice descriptor
+ *
+ * A value zero will be returned on success and a negative errno will
+ * be returned in error cases
+ */
+int llcc_slice_deactivate(struct llcc_slice_desc *desc)
+{
+   u32 act_ctrl_val;
+   int ret;
+   struct llcc_drv_data *drv;
+
+   if (desc == NULL)
+   return -EINVAL;
+
+   drv = dev_get_drvdata(desc->dev);
+   if (!drv)
+   return -EINVAL;
+
+   mutex_lock(>lock);
+   if (!test_bit(desc->slice_id, drv->bitmap)) {
+   mutex_unlock(>lock);
+   return 0;
+   

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[rishabhb]

Hi Stanimir,
We incorporated all your comments except the following:
1. Removing the driver that maintains the SCT (system cache table)
per chipset. As responded earlier the data is expected to change
from chipset to chipset and would clutter the main driver if we
choose using compatible string. We think its good to keep the data
separate from core driver.
2. Changing struct llcc_slice_desc to llcc_desc:
The descriptor is specific to each slice and not the whole llcc.
All the properties such as id and size are specific to each slice
and not whole llcc.

please let me know if you are ok with the approach. Based on that
I can send my next revision of changes.



On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
 slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454 
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI, 
and I2C

   devices to the output pins.

+config QCOM_LLCC
+   tristate "Qualcomm Technologies, Inc. LLCC driver"
+   depends on ARCH_QCOM
+   help
+ Qualcomm Technologies, Inc. platform specific LLCC driver for Last
+	  Level Cache. This provides interfaces to client's that use the 
LLCC.

+ Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+   tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+   depends on QCOM_LLCC
+   help
+	  Say yes here to enable the LLCC driver for SDM845. This is 
provides
+	  data required to configure LLCC so that clients can start using 
the

+ LLCC slices.
+
 config QCOM_MDT_LOADER
tristate
select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)   += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c 
b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify

+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for 
replacement

+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is used 
only if

+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways 
cannot be used

+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the mode 
of the

+ 

Re: [PATCH v3 2/2] drivers: soc: Add LLCC driver

[rishabhb]

Hi Stanimir,
We incorporated all your comments except the following:
1. Removing the driver that maintains the SCT (system cache table)
per chipset. As responded earlier the data is expected to change
from chipset to chipset and would clutter the main driver if we
choose using compatible string. We think its good to keep the data
separate from core driver.
2. Changing struct llcc_slice_desc to llcc_desc:
The descriptor is specific to each slice and not the whole llcc.
All the properties such as id and size are specific to each slice
and not whole llcc.

please let me know if you are ok with the approach. Based on that
I can send my next revision of changes.



On 2018-03-29 01:08, Stanimir Varbanov wrote:

Hi,

On 03/27/2018 09:52 PM, Rishabh Bhatnagar wrote:

LLCC (Last Level Cache Controller) provides additional cache memory
in the system. LLCC is partitioned into muliple slices and each
slice getting its own priority, size, ID and other config parameters.
LLCC driver programs these parameters for each slice. Clients that
are assigned to use LLCC need to get information such size & ID of the
 slice they get and activate or deactivate the slice as needed. LLCC 
driver

provides API interfaces for the clients to perform these operations.

Signed-off-by: Channagoud Kadabi 
Signed-off-by: Rishabh Bhatnagar 
---
 drivers/soc/qcom/Kconfig   |  16 ++
 drivers/soc/qcom/Makefile  |   2 +
 drivers/soc/qcom/llcc-sdm845.c | 120 ++
 drivers/soc/qcom/llcc-slice.c  | 454 
+


I'd name it just llcc.c, slice suffix didn't add any value.


 include/linux/soc/qcom/llcc-qcom.h | 178 +++
 5 files changed, 770 insertions(+)
 create mode 100644 drivers/soc/qcom/llcc-sdm845.c
 create mode 100644 drivers/soc/qcom/llcc-slice.c
 create mode 100644 include/linux/soc/qcom/llcc-qcom.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index e050eb8..28237fc 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -21,6 +21,22 @@ config QCOM_GSBI
   functions for connecting the underlying serial UART, SPI, 
and I2C

   devices to the output pins.

+config QCOM_LLCC
+   tristate "Qualcomm Technologies, Inc. LLCC driver"
+   depends on ARCH_QCOM
+   help
+ Qualcomm Technologies, Inc. platform specific LLCC driver for Last
+	  Level Cache. This provides interfaces to client's that use the 
LLCC.

+ Say yes here to enable LLCC slice driver.
+
+config QCOM_SDM845_LLCC
+   tristate "Qualcomm Technologies, Inc. SDM845 LLCC driver"
+   depends on QCOM_LLCC
+   help
+	  Say yes here to enable the LLCC driver for SDM845. This is 
provides
+	  data required to configure LLCC so that clients can start using 
the

+ LLCC slices.
+
 config QCOM_MDT_LOADER
tristate
select QCOM_SCM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index dcebf28..e16d6a2 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -12,3 +12,5 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)   += smp2p.o
 obj-$(CONFIG_QCOM_SMSM)+= smsm.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_LLCC) += llcc-slice.o
+obj-$(CONFIG_QCOM_SDM845_LLCC) += llcc-sdm845.o
diff --git a/drivers/soc/qcom/llcc-sdm845.c 
b/drivers/soc/qcom/llcc-sdm845.c

new file mode 100644
index 000..cd431d9
--- /dev/null
+++ b/drivers/soc/qcom/llcc-sdm845.c
@@ -0,0 +1,120 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights 
reserved.

+ *
+ * This program is free software; you can redistribute it and/or 
modify

+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+
+/*
+ * SCT entry contains of the following parameters
+ * name: Name of the client's use case for which the llcc slice is 
used

+ * uid: Unique id for the client's use case
+ * slice_id: llcc slice id for each client
+ * max_cap: The maximum capacity of the cache slice provided in KB
+ * priority: Priority of the client used to select victim line for 
replacement

+ * fixed_size: Determine of the slice has a fixed capacity
+ * bonus_ways: Bonus ways to be used by any slice, bonus way is used 
only if

+ * it't not a reserved way.
+ * res_ways: Reserved ways for the cache slice, the reserved ways 
cannot be used

+ *   by any other client than the one its assigned to.
+ * cache_mode: Each slice operates as a cache, this controls the mode 
of the

+ * slice normal or TCM
+ *