[PATCH 0/6] remoteproc: qcom: Add callbacks for remoteproc events

[PATCH 0/6] remoteproc: qcom: Add callbacks for remoteproc events

[Siddharth Gupta]

This patch series adds the usecases for SSR and Sysmon subdevices which are
used by remoteprocs and kernel client drivers relying on those remoteprocs.

Patch 1-3 adds callbacks for prepare, start and unprepare events, and also type
          of sysmon notification.
Patch 4-6 adds a per subsystem notifier chain, callbacks for prepare, start
          and stop events for ssr subdevice.

Rishabh Bhatnagar (1):
  drivers: remoteproc: Add name field for every subdevice

Siddharth Gupta (5):
  remoteproc: sysmon: Add ability to send type of notification
  remoteproc: sysmon: Add notifications for events
  remoteproc: sysmon: Inform current rproc about all active rprocs
  remoteproc: qcom: Add per subsystem SSR notification
  remoteproc: qcom: Add notification types to SSR

 drivers/remoteproc/qcom_common.c      |  86 ++++++++++++++++++++++++----
 drivers/remoteproc/qcom_common.h      |   1 +
 drivers/remoteproc/qcom_sysmon.c      | 103 ++++++++++++++++++++++++++++------
 drivers/soc/qcom/glink_ssr.c          |  20 ++++++-
 include/linux/remoteproc.h            |  17 ++++++
 include/linux/remoteproc/qcom_rproc.h |  17 ++++--
 6 files changed, 209 insertions(+), 35 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 0/6] remoteproc: qcom: Add callbacks for remoteproc events

[Siddharth Gupta]

This patch series adds the usecases for SSR and Sysmon subdevices which are
used by remoteprocs and kernel client drivers relying on those remoteprocs.

Patch 1-3 adds callbacks for prepare, start and unprepare events, and also type
          of sysmon notification.
Patch 4-6 adds a per subsystem notifier chain, callbacks for prepare, start
          and stop events for ssr subdevice.

Rishabh Bhatnagar (1):
  drivers: remoteproc: Add name field for every subdevice

Siddharth Gupta (5):
  remoteproc: sysmon: Add ability to send type of notification
  remoteproc: sysmon: Add notifications for events
  remoteproc: sysmon: Inform current rproc about all active rprocs
  remoteproc: qcom: Add per subsystem SSR notification
  remoteproc: qcom: Add notification types to SSR

 drivers/remoteproc/qcom_common.c      |  86 ++++++++++++++++++++++++----
 drivers/remoteproc/qcom_common.h      |   1 +
 drivers/remoteproc/qcom_sysmon.c      | 103 ++++++++++++++++++++++++++++------
 drivers/soc/qcom/glink_ssr.c          |  20 ++++++-
 include/linux/remoteproc.h            |  17 ++++++
 include/linux/remoteproc/qcom_rproc.h |  17 ++++--
 6 files changed, 209 insertions(+), 35 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] remoteproc: sysmon: Add ability to send type of notification

[Siddharth Gupta]

Current implementation of the sysmon driver does not support adding
notifications for other remoteproc events - prepare, start, unprepare.
Clients on the remoteproc side might be interested in knowing when a
remoteproc boots up. This change adds the ability to send the notification
type along with the name. For example, audio DSP is interested in knowing
when modem has crashed so that it can perform cleanup and wait for modem to
boot up before it starts processing data again.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 54 +++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index faf3822..1366050 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -46,6 +46,25 @@ struct qcom_sysmon {
 	struct sockaddr_qrtr ssctl;
 };
 
+enum {
+	SSCTL_SSR_EVENT_BEFORE_POWERUP,
+	SSCTL_SSR_EVENT_AFTER_POWERUP,
+	SSCTL_SSR_EVENT_BEFORE_SHUTDOWN,
+	SSCTL_SSR_EVENT_AFTER_SHUTDOWN,
+};
+
+static const char * const sysmon_state_string[] = {
+	[SSCTL_SSR_EVENT_BEFORE_POWERUP]	= "before_powerup",
+	[SSCTL_SSR_EVENT_AFTER_POWERUP]		= "after_powerup",
+	[SSCTL_SSR_EVENT_BEFORE_SHUTDOWN]	= "before_shutdown",
+	[SSCTL_SSR_EVENT_AFTER_SHUTDOWN]	= "after_shutdown",
+};
+
+struct sysmon_event {
+	const char *subsys_name;
+	u32 ssr_event;
+};
+
 static DEFINE_MUTEX(sysmon_lock);
 static LIST_HEAD(sysmon_list);
 
@@ -54,13 +73,15 @@ static LIST_HEAD(sysmon_list);
  * @sysmon:	sysmon context
  * @name:	other remote's name
  */
-static void sysmon_send_event(struct qcom_sysmon *sysmon, const char *name)
+static void sysmon_send_event(struct qcom_sysmon *sysmon,
+			      const struct sysmon_event *event)
 {
 	char req[50];
 	int len;
 	int ret;
 
-	len = snprintf(req, sizeof(req), "ssr:%s:before_shutdown", name);
+	len = snprintf(req, sizeof(req), "ssr:%s:%s", event->subsys_name,
+		       sysmon_state_string[event->ssr_event]);
 	if (len >= sizeof(req))
 		return;
 
@@ -149,13 +170,6 @@ static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
 #define SSCTL_SUBSYS_NAME_LENGTH	15
 
 enum {
-	SSCTL_SSR_EVENT_BEFORE_POWERUP,
-	SSCTL_SSR_EVENT_AFTER_POWERUP,
-	SSCTL_SSR_EVENT_BEFORE_SHUTDOWN,
-	SSCTL_SSR_EVENT_AFTER_SHUTDOWN,
-};
-
-enum {
 	SSCTL_SSR_EVENT_FORCED,
 	SSCTL_SSR_EVENT_GRACEFUL,
 };
@@ -331,7 +345,8 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
  * @sysmon:	sysmon context
  * @name:	other remote's name
  */
-static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
+static void ssctl_send_event(struct qcom_sysmon *sysmon,
+			     const struct sysmon_event *event)
 {
 	struct ssctl_subsys_event_resp resp;
 	struct ssctl_subsys_event_req req;
@@ -346,9 +361,9 @@ static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
 	}
 
 	memset(&req, 0, sizeof(req));
-	strlcpy(req.subsys_name, name, sizeof(req.subsys_name));
+	strlcpy(req.subsys_name, event->subsys_name, sizeof(req.subsys_name));
 	req.subsys_name_len = strlen(req.subsys_name);
-	req.event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
+	req.event = event->ssr_event;
 	req.evt_driven_valid = true;
 	req.evt_driven = SSCTL_SSR_EVENT_FORCED;
 
@@ -432,8 +447,12 @@ static int sysmon_start(struct rproc_subdev *subdev)
 static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
 {
 	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon, subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN
+	};
 
-	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)sysmon->name);
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
 
 	/* Don't request graceful shutdown if we've crashed */
 	if (crashed)
@@ -456,19 +475,20 @@ static int sysmon_notify(struct notifier_block *nb, unsigned long event,
 {
 	struct qcom_sysmon *sysmon = container_of(nb, struct qcom_sysmon, nb);
 	struct rproc *rproc = sysmon->rproc;
-	const char *ssr_name = data;
+	struct sysmon_event *sysmon_event = data;
 
 	/* Skip non-running rprocs and the originating instance */
-	if (rproc->state != RPROC_RUNNING || !strcmp(data, sysmon->name)) {
+	if (rproc->state != RPROC_RUNNING ||
+	    !strcmp(sysmon_event->subsys_name, sysmon->name)) {
 		dev_dbg(sysmon->dev, "not notifying %s\n", sysmon->name);
 		return NOTIFY_DONE;
 	}
 
 	/* Only SSCTL version 2 supports SSR events */
 	if (sysmon->ssctl_version == 2)
-		ssctl_send_event(sysmon, ssr_name);
+		ssctl_send_event(sysmon, sysmon_event);
 	else if (sysmon->ept)
-		sysmon_send_event(sysmon, ssr_name);
+		sysmon_send_event(sysmon, sysmon_event);
 
 	return NOTIFY_DONE;
 }
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/6] remoteproc: sysmon: Add ability to send type of notification

[Siddharth Gupta]

Current implementation of the sysmon driver does not support adding
notifications for other remoteproc events - prepare, start, unprepare.
Clients on the remoteproc side might be interested in knowing when a
remoteproc boots up. This change adds the ability to send the notification
type along with the name. For example, audio DSP is interested in knowing
when modem has crashed so that it can perform cleanup and wait for modem to
boot up before it starts processing data again.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 54 +++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index faf3822..1366050 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -46,6 +46,25 @@ struct qcom_sysmon {
 	struct sockaddr_qrtr ssctl;
 };
 
+enum {
+	SSCTL_SSR_EVENT_BEFORE_POWERUP,
+	SSCTL_SSR_EVENT_AFTER_POWERUP,
+	SSCTL_SSR_EVENT_BEFORE_SHUTDOWN,
+	SSCTL_SSR_EVENT_AFTER_SHUTDOWN,
+};
+
+static const char * const sysmon_state_string[] = {
+	[SSCTL_SSR_EVENT_BEFORE_POWERUP]	= "before_powerup",
+	[SSCTL_SSR_EVENT_AFTER_POWERUP]		= "after_powerup",
+	[SSCTL_SSR_EVENT_BEFORE_SHUTDOWN]	= "before_shutdown",
+	[SSCTL_SSR_EVENT_AFTER_SHUTDOWN]	= "after_shutdown",
+};
+
+struct sysmon_event {
+	const char *subsys_name;
+	u32 ssr_event;
+};
+
 static DEFINE_MUTEX(sysmon_lock);
 static LIST_HEAD(sysmon_list);
 
@@ -54,13 +73,15 @@ static LIST_HEAD(sysmon_list);
  * @sysmon:	sysmon context
  * @name:	other remote's name
  */
-static void sysmon_send_event(struct qcom_sysmon *sysmon, const char *name)
+static void sysmon_send_event(struct qcom_sysmon *sysmon,
+			      const struct sysmon_event *event)
 {
 	char req[50];
 	int len;
 	int ret;
 
-	len = snprintf(req, sizeof(req), "ssr:%s:before_shutdown", name);
+	len = snprintf(req, sizeof(req), "ssr:%s:%s", event->subsys_name,
+		       sysmon_state_string[event->ssr_event]);
 	if (len >= sizeof(req))
 		return;
 
@@ -149,13 +170,6 @@ static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
 #define SSCTL_SUBSYS_NAME_LENGTH	15
 
 enum {
-	SSCTL_SSR_EVENT_BEFORE_POWERUP,
-	SSCTL_SSR_EVENT_AFTER_POWERUP,
-	SSCTL_SSR_EVENT_BEFORE_SHUTDOWN,
-	SSCTL_SSR_EVENT_AFTER_SHUTDOWN,
-};
-
-enum {
 	SSCTL_SSR_EVENT_FORCED,
 	SSCTL_SSR_EVENT_GRACEFUL,
 };
@@ -331,7 +345,8 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
  * @sysmon:	sysmon context
  * @name:	other remote's name
  */
-static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
+static void ssctl_send_event(struct qcom_sysmon *sysmon,
+			     const struct sysmon_event *event)
 {
 	struct ssctl_subsys_event_resp resp;
 	struct ssctl_subsys_event_req req;
@@ -346,9 +361,9 @@ static void ssctl_send_event(struct qcom_sysmon *sysmon, const char *name)
 	}
 
 	memset(&req, 0, sizeof(req));
-	strlcpy(req.subsys_name, name, sizeof(req.subsys_name));
+	strlcpy(req.subsys_name, event->subsys_name, sizeof(req.subsys_name));
 	req.subsys_name_len = strlen(req.subsys_name);
-	req.event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
+	req.event = event->ssr_event;
 	req.evt_driven_valid = true;
 	req.evt_driven = SSCTL_SSR_EVENT_FORCED;
 
@@ -432,8 +447,12 @@ static int sysmon_start(struct rproc_subdev *subdev)
 static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
 {
 	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon, subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN
+	};
 
-	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)sysmon->name);
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
 
 	/* Don't request graceful shutdown if we've crashed */
 	if (crashed)
@@ -456,19 +475,20 @@ static int sysmon_notify(struct notifier_block *nb, unsigned long event,
 {
 	struct qcom_sysmon *sysmon = container_of(nb, struct qcom_sysmon, nb);
 	struct rproc *rproc = sysmon->rproc;
-	const char *ssr_name = data;
+	struct sysmon_event *sysmon_event = data;
 
 	/* Skip non-running rprocs and the originating instance */
-	if (rproc->state != RPROC_RUNNING || !strcmp(data, sysmon->name)) {
+	if (rproc->state != RPROC_RUNNING ||
+	    !strcmp(sysmon_event->subsys_name, sysmon->name)) {
 		dev_dbg(sysmon->dev, "not notifying %s\n", sysmon->name);
 		return NOTIFY_DONE;
 	}
 
 	/* Only SSCTL version 2 supports SSR events */
 	if (sysmon->ssctl_version == 2)
-		ssctl_send_event(sysmon, ssr_name);
+		ssctl_send_event(sysmon, sysmon_event);
 	else if (sysmon->ept)
-		sysmon_send_event(sysmon, ssr_name);
+		sysmon_send_event(sysmon, sysmon_event);
 
 	return NOTIFY_DONE;
 }
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] remoteproc: sysmon: Add notifications for events

[Siddharth Gupta]

Add notification for other stages of remoteproc boot and shutdown. This
includes adding callback functions for the prepare and unprepare events,
and fleshing out the callback function for start.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index 1366050..851664e 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -439,8 +439,31 @@ static const struct qmi_ops ssctl_ops = {
 	.del_server = ssctl_del_server,
 };
 
+static int sysmon_prepare(struct rproc_subdev *subdev)
+{
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_BEFORE_POWERUP
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+
+	return 0;
+}
+
 static int sysmon_start(struct rproc_subdev *subdev)
 {
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+
 	return 0;
 }
 
@@ -464,6 +487,18 @@ static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
 		sysmon_request_shutdown(sysmon);
 }
 
+static void sysmon_unprepare(struct rproc_subdev *subdev)
+{
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_AFTER_SHUTDOWN
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+}
+
 /**
  * sysmon_notify() - notify sysmon target of another's SSR
  * @nb:		notifier_block associated with sysmon instance
@@ -563,8 +598,10 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
 
 	qmi_add_lookup(&sysmon->qmi, 43, 0, 0);
 
+	sysmon->subdev.prepare = sysmon_prepare;
 	sysmon->subdev.start = sysmon_start;
 	sysmon->subdev.stop = sysmon_stop;
+	sysmon->subdev.unprepare = sysmon_unprepare;
 
 	rproc_add_subdev(rproc, &sysmon->subdev);
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/6] remoteproc: sysmon: Add notifications for events

[Siddharth Gupta]

Add notification for other stages of remoteproc boot and shutdown. This
includes adding callback functions for the prepare and unprepare events,
and fleshing out the callback function for start.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index 1366050..851664e 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -439,8 +439,31 @@ static const struct qmi_ops ssctl_ops = {
 	.del_server = ssctl_del_server,
 };
 
+static int sysmon_prepare(struct rproc_subdev *subdev)
+{
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_BEFORE_POWERUP
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+
+	return 0;
+}
+
 static int sysmon_start(struct rproc_subdev *subdev)
 {
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+
 	return 0;
 }
 
@@ -464,6 +487,18 @@ static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
 		sysmon_request_shutdown(sysmon);
 }
 
+static void sysmon_unprepare(struct rproc_subdev *subdev)
+{
+	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
+						  subdev);
+	struct sysmon_event event = {
+		.subsys_name = sysmon->name,
+		.ssr_event = SSCTL_SSR_EVENT_AFTER_SHUTDOWN
+	};
+
+	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
+}
+
 /**
  * sysmon_notify() - notify sysmon target of another's SSR
  * @nb:		notifier_block associated with sysmon instance
@@ -563,8 +598,10 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
 
 	qmi_add_lookup(&sysmon->qmi, 43, 0, 0);
 
+	sysmon->subdev.prepare = sysmon_prepare;
 	sysmon->subdev.start = sysmon_start;
 	sysmon->subdev.stop = sysmon_stop;
+	sysmon->subdev.unprepare = sysmon_unprepare;
 
 	rproc_add_subdev(rproc, &sysmon->subdev);
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] remoteproc: sysmon: Inform current rproc about all active rprocs

[Siddharth Gupta]

A remoteproc that has just recovered from a crash will not be aware of the
state of other remoteprocs. Send sysmon notifications on behalf of all the
active/online remoteprocs to the one that just booted up.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index 851664e..d0d59d5 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -457,6 +457,7 @@ static int sysmon_start(struct rproc_subdev *subdev)
 {
 	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
 						  subdev);
+	struct qcom_sysmon *target;
 	struct sysmon_event event = {
 		.subsys_name = sysmon->name,
 		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
@@ -464,6 +465,17 @@ static int sysmon_start(struct rproc_subdev *subdev)
 
 	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
 
+	mutex_lock(&sysmon_lock);
+	list_for_each_entry(target, &sysmon_list, node) {
+		if (target == sysmon ||
+		    target->rproc->state != RPROC_RUNNING)
+			continue;
+
+		event.subsys_name = target->name;
+		ssctl_send_event(sysmon, &event);
+	}
+	mutex_unlock(&sysmon_lock);
+
 	return 0;
 }
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/6] remoteproc: sysmon: Inform current rproc about all active rprocs

[Siddharth Gupta]

A remoteproc that has just recovered from a crash will not be aware of the
state of other remoteprocs. Send sysmon notifications on behalf of all the
active/online remoteprocs to the one that just booted up.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_sysmon.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
index 851664e..d0d59d5 100644
--- a/drivers/remoteproc/qcom_sysmon.c
+++ b/drivers/remoteproc/qcom_sysmon.c
@@ -457,6 +457,7 @@ static int sysmon_start(struct rproc_subdev *subdev)
 {
 	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
 						  subdev);
+	struct qcom_sysmon *target;
 	struct sysmon_event event = {
 		.subsys_name = sysmon->name,
 		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
@@ -464,6 +465,17 @@ static int sysmon_start(struct rproc_subdev *subdev)
 
 	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
 
+	mutex_lock(&sysmon_lock);
+	list_for_each_entry(target, &sysmon_list, node) {
+		if (target == sysmon ||
+		    target->rproc->state != RPROC_RUNNING)
+			continue;
+
+		event.subsys_name = target->name;
+		ssctl_send_event(sysmon, &event);
+	}
+	mutex_unlock(&sysmon_lock);
+
 	return 0;
 }
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] remoteproc: sysmon: Inform current rproc about all active rprocs

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:42PM -0800, Siddharth Gupta wrote:
> A remoteproc that has just recovered from a crash will not be aware of the
> state of other remoteprocs. Send sysmon notifications on behalf of all the
> active/online remoteprocs to the one that just booted up.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_sysmon.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index 851664e..d0d59d5 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
> @@ -457,6 +457,7 @@ static int sysmon_start(struct rproc_subdev *subdev)
>  {
>  	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
>  						  subdev);
> +	struct qcom_sysmon *target;
>  	struct sysmon_event event = {
>  		.subsys_name = sysmon->name,
>  		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
> @@ -464,6 +465,17 @@ static int sysmon_start(struct rproc_subdev *subdev)
>  
>  	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
>  
> +	mutex_lock(&sysmon_lock);
> +	list_for_each_entry(target, &sysmon_list, node) {
> +		if (target == sysmon ||
> +		    target->rproc->state != RPROC_RUNNING)
> +			continue;
> +
> +		event.subsys_name = target->name;
> +		ssctl_send_event(sysmon, &event);
> +	}
> +	mutex_unlock(&sysmon_lock);
> +

The changelog is specific about crash recovery but to me this code will run
every time an MCU is switched on.  If I understand correctly, in a crash
recovery situation or simply an MCU coming on line, you want to make sure the
subdevices associated to the booting (or recovering) MCU knows about subdevices
that were registered with the sysmon_notifiers before it.

If that is the case, the changelog needs to be modified and a good chunk of
comments need to be added to this patch.

Lastly, shouldn't there be a provision for sysmon->ssctl_version == 2?

Thanks,
Mathieu 

>  	return 0;
>  }
>  
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH 3/6] remoteproc: sysmon: Inform current rproc about all active rprocs

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:42PM -0800, Siddharth Gupta wrote:
> A remoteproc that has just recovered from a crash will not be aware of the
> state of other remoteprocs. Send sysmon notifications on behalf of all the
> active/online remoteprocs to the one that just booted up.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_sysmon.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index 851664e..d0d59d5 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
> @@ -457,6 +457,7 @@ static int sysmon_start(struct rproc_subdev *subdev)
>  {
>  	struct qcom_sysmon *sysmon = container_of(subdev, struct qcom_sysmon,
>  						  subdev);
> +	struct qcom_sysmon *target;
>  	struct sysmon_event event = {
>  		.subsys_name = sysmon->name,
>  		.ssr_event = SSCTL_SSR_EVENT_AFTER_POWERUP
> @@ -464,6 +465,17 @@ static int sysmon_start(struct rproc_subdev *subdev)
>  
>  	blocking_notifier_call_chain(&sysmon_notifiers, 0, (void *)&event);
>  
> +	mutex_lock(&sysmon_lock);
> +	list_for_each_entry(target, &sysmon_list, node) {
> +		if (target == sysmon ||
> +		    target->rproc->state != RPROC_RUNNING)
> +			continue;
> +
> +		event.subsys_name = target->name;
> +		ssctl_send_event(sysmon, &event);
> +	}
> +	mutex_unlock(&sysmon_lock);
> +

The changelog is specific about crash recovery but to me this code will run
every time an MCU is switched on.  If I understand correctly, in a crash
recovery situation or simply an MCU coming on line, you want to make sure the
subdevices associated to the booting (or recovering) MCU knows about subdevices
that were registered with the sysmon_notifiers before it.

If that is the case, the changelog needs to be modified and a good chunk of
comments need to be added to this patch.

Lastly, shouldn't there be a provision for sysmon->ssctl_version == 2?

Thanks,
Mathieu 

>  	return 0;
>  }
>  
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] drivers: remoteproc: Add name field for every subdevice

[Siddharth Gupta]

From: Rishabh Bhatnagar <rishabhb@codeaurora.org>

When a client driver wishes to utilize functionality from a particular
subdevice of a remoteproc, it cannot differentiate between the subdevices
that have been added. This patch allows the client driver to distinguish
between subdevices and thus utilize their functionality.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c | 6 ++++++
 include/linux/remoteproc.h       | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 60650bc..5d59538 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -58,6 +58,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.name = kstrdup("glink", GFP_KERNEL);
 
 	rproc_add_subdev(rproc, &glink->subdev);
 }
@@ -73,6 +74,7 @@ void qcom_remove_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glin
 	if (!glink->node)
 		return;
 
+	kfree(glink->subdev.name);
 	rproc_remove_subdev(rproc, &glink->subdev);
 	of_node_put(glink->node);
 }
@@ -154,6 +156,7 @@ void qcom_add_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
 	smd->dev = dev;
 	smd->subdev.start = smd_subdev_start;
 	smd->subdev.stop = smd_subdev_stop;
+	smd->subdev.name = kstrdup("smd", GFP_KERNEL);
 
 	rproc_add_subdev(rproc, &smd->subdev);
 }
@@ -169,6 +172,7 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
 	if (!smd->node)
 		return;
 
+	kfree(smd->subdev.name);
 	rproc_remove_subdev(rproc, &smd->subdev);
 	of_node_put(smd->node);
 }
@@ -220,6 +224,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 			 const char *ssr_name)
 {
 	ssr->name = ssr_name;
+	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);
@@ -233,6 +238,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
  */
 void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
 {
+	kfree(ssr->subdev.name);
 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e2eaba9..e2f60cc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -519,6 +519,7 @@ struct rproc {
 /**
  * struct rproc_subdev - subdevice tied to a remoteproc
  * @node: list node related to the rproc subdevs list
+ * @name: name of the subdevice
  * @prepare: prepare function, called before the rproc is started
  * @start: start function, called after the rproc has been started
  * @stop: stop function, called before the rproc is stopped; the @crashed
@@ -527,6 +528,7 @@ struct rproc {
  */
 struct rproc_subdev {
 	struct list_head node;
+	char *name;
 
 	int (*prepare)(struct rproc_subdev *subdev);
 	int (*start)(struct rproc_subdev *subdev);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/6] drivers: remoteproc: Add name field for every subdevice

[Siddharth Gupta]

From: Rishabh Bhatnagar <rishabhb@codeaurora.org>

When a client driver wishes to utilize functionality from a particular
subdevice of a remoteproc, it cannot differentiate between the subdevices
that have been added. This patch allows the client driver to distinguish
between subdevices and thus utilize their functionality.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c | 6 ++++++
 include/linux/remoteproc.h       | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 60650bc..5d59538 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -58,6 +58,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.name = kstrdup("glink", GFP_KERNEL);
 
 	rproc_add_subdev(rproc, &glink->subdev);
 }
@@ -73,6 +74,7 @@ void qcom_remove_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glin
 	if (!glink->node)
 		return;
 
+	kfree(glink->subdev.name);
 	rproc_remove_subdev(rproc, &glink->subdev);
 	of_node_put(glink->node);
 }
@@ -154,6 +156,7 @@ void qcom_add_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
 	smd->dev = dev;
 	smd->subdev.start = smd_subdev_start;
 	smd->subdev.stop = smd_subdev_stop;
+	smd->subdev.name = kstrdup("smd", GFP_KERNEL);
 
 	rproc_add_subdev(rproc, &smd->subdev);
 }
@@ -169,6 +172,7 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
 	if (!smd->node)
 		return;
 
+	kfree(smd->subdev.name);
 	rproc_remove_subdev(rproc, &smd->subdev);
 	of_node_put(smd->node);
 }
@@ -220,6 +224,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 			 const char *ssr_name)
 {
 	ssr->name = ssr_name;
+	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 
 	rproc_add_subdev(rproc, &ssr->subdev);
@@ -233,6 +238,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
  */
 void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
 {
+	kfree(ssr->subdev.name);
 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e2eaba9..e2f60cc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -519,6 +519,7 @@ struct rproc {
 /**
  * struct rproc_subdev - subdevice tied to a remoteproc
  * @node: list node related to the rproc subdevs list
+ * @name: name of the subdevice
  * @prepare: prepare function, called before the rproc is started
  * @start: start function, called after the rproc has been started
  * @stop: stop function, called before the rproc is stopped; the @crashed
@@ -527,6 +528,7 @@ struct rproc {
  */
 struct rproc_subdev {
 	struct list_head node;
+	char *name;
 
 	int (*prepare)(struct rproc_subdev *subdev);
 	int (*start)(struct rproc_subdev *subdev);
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] drivers: remoteproc: Add name field for every subdevice

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:43PM -0800, Siddharth Gupta wrote:
> From: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> 
> When a client driver wishes to utilize functionality from a particular
> subdevice of a remoteproc, it cannot differentiate between the subdevices
> that have been added. This patch allows the client driver to distinguish
> between subdevices and thus utilize their functionality.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_common.c | 6 ++++++
>  include/linux/remoteproc.h       | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 60650bc..5d59538 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -58,6 +58,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.name = kstrdup("glink", GFP_KERNEL);

Because @subdev is a member of qcom_rproc_glink (rather than a pointer), it is
possible to get to glink with container_of().  From there edge->name is
available - would that work?

>  
>  	rproc_add_subdev(rproc, &glink->subdev);
>  }
> @@ -73,6 +74,7 @@ void qcom_remove_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glin
>  	if (!glink->node)
>  		return;
>  
> +	kfree(glink->subdev.name);
>  	rproc_remove_subdev(rproc, &glink->subdev);
>  	of_node_put(glink->node);
>  }
> @@ -154,6 +156,7 @@ void qcom_add_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
>  	smd->dev = dev;
>  	smd->subdev.start = smd_subdev_start;
>  	smd->subdev.stop = smd_subdev_stop;
> +	smd->subdev.name = kstrdup("smd", GFP_KERNEL);

Same as above - qcom_smd_edge has a name.

Worse case scenario, both qcom_rproc_glink and qcom_smd_edge have a device_node
that can be used as well.

>  
>  	rproc_add_subdev(rproc, &smd->subdev);
>  }
> @@ -169,6 +172,7 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
>  	if (!smd->node)
>  		return;
>  
> +	kfree(smd->subdev.name);
>  	rproc_remove_subdev(rproc, &smd->subdev);
>  	of_node_put(smd->node);
>  }
> @@ -220,6 +224,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
>  			 const char *ssr_name)
>  {
>  	ssr->name = ssr_name;
> +	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>  
>  	rproc_add_subdev(rproc, &ssr->subdev);
> @@ -233,6 +238,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
>   */
>  void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
>  {
> +	kfree(ssr->subdev.name);
>  	rproc_remove_subdev(rproc, &ssr->subdev);
>  }
>  EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e2eaba9..e2f60cc 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -519,6 +519,7 @@ struct rproc {
>  /**
>   * struct rproc_subdev - subdevice tied to a remoteproc
>   * @node: list node related to the rproc subdevs list
> + * @name: name of the subdevice
>   * @prepare: prepare function, called before the rproc is started
>   * @start: start function, called after the rproc has been started
>   * @stop: stop function, called before the rproc is stopped; the @crashed
> @@ -527,6 +528,7 @@ struct rproc {
>   */
>  struct rproc_subdev {
>  	struct list_head node;
> +	char *name;
>  
>  	int (*prepare)(struct rproc_subdev *subdev);
>  	int (*start)(struct rproc_subdev *subdev);
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH 4/6] drivers: remoteproc: Add name field for every subdevice

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:43PM -0800, Siddharth Gupta wrote:
> From: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> 
> When a client driver wishes to utilize functionality from a particular
> subdevice of a remoteproc, it cannot differentiate between the subdevices
> that have been added. This patch allows the client driver to distinguish
> between subdevices and thus utilize their functionality.
> 
> Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_common.c | 6 ++++++
>  include/linux/remoteproc.h       | 2 ++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 60650bc..5d59538 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -58,6 +58,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.name = kstrdup("glink", GFP_KERNEL);

Because @subdev is a member of qcom_rproc_glink (rather than a pointer), it is
possible to get to glink with container_of().  From there edge->name is
available - would that work?

>  
>  	rproc_add_subdev(rproc, &glink->subdev);
>  }
> @@ -73,6 +74,7 @@ void qcom_remove_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink *glin
>  	if (!glink->node)
>  		return;
>  
> +	kfree(glink->subdev.name);
>  	rproc_remove_subdev(rproc, &glink->subdev);
>  	of_node_put(glink->node);
>  }
> @@ -154,6 +156,7 @@ void qcom_add_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
>  	smd->dev = dev;
>  	smd->subdev.start = smd_subdev_start;
>  	smd->subdev.stop = smd_subdev_stop;
> +	smd->subdev.name = kstrdup("smd", GFP_KERNEL);

Same as above - qcom_smd_edge has a name.

Worse case scenario, both qcom_rproc_glink and qcom_smd_edge have a device_node
that can be used as well.

>  
>  	rproc_add_subdev(rproc, &smd->subdev);
>  }
> @@ -169,6 +172,7 @@ void qcom_remove_smd_subdev(struct rproc *rproc, struct qcom_rproc_subdev *smd)
>  	if (!smd->node)
>  		return;
>  
> +	kfree(smd->subdev.name);
>  	rproc_remove_subdev(rproc, &smd->subdev);
>  	of_node_put(smd->node);
>  }
> @@ -220,6 +224,7 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
>  			 const char *ssr_name)
>  {
>  	ssr->name = ssr_name;
> +	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>  
>  	rproc_add_subdev(rproc, &ssr->subdev);
> @@ -233,6 +238,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
>   */
>  void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
>  {
> +	kfree(ssr->subdev.name);
>  	rproc_remove_subdev(rproc, &ssr->subdev);
>  }
>  EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e2eaba9..e2f60cc 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -519,6 +519,7 @@ struct rproc {
>  /**
>   * struct rproc_subdev - subdevice tied to a remoteproc
>   * @node: list node related to the rproc subdevs list
> + * @name: name of the subdevice
>   * @prepare: prepare function, called before the rproc is started
>   * @start: start function, called after the rproc has been started
>   * @stop: stop function, called before the rproc is stopped; the @crashed
> @@ -527,6 +528,7 @@ struct rproc {
>   */
>  struct rproc_subdev {
>  	struct list_head node;
> +	char *name;
>  
>  	int (*prepare)(struct rproc_subdev *subdev);
>  	int (*start)(struct rproc_subdev *subdev);
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] remoteproc: qcom: Add per subsystem SSR notification

[Siddharth Gupta]

Currently there is a global 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 an
individual notifier chain for every SSR subdevice, and modify the
notification registration API to include the remoteproc struct as an
argument. Update the existing user of the registration API to get the
phandle of the remoteproc dt node to register for SSR notifications.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c      | 49 +++++++++++++++++++++++++++--------
 drivers/remoteproc/qcom_common.h      |  1 +
 drivers/soc/qcom/glink_ssr.c          | 20 ++++++++++++--
 include/linux/remoteproc/qcom_rproc.h | 17 ++++++++----
 4 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 5d59538..6714f27 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -23,8 +23,6 @@
 #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
 #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
 
-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
-
 static int glink_subdev_start(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
@@ -180,27 +178,52 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
 
 /**
  * qcom_register_ssr_notifier() - register SSR notification handler
+ * @rproc:	pointer to the remoteproc structure
  * @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 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 @notify function as handler for restart notifications. As
+ * remote processors are stopped this function will be called, with the rproc
+ * pointer passed as a parameter.
  */
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
 {
-	return blocking_notifier_chain_register(&ssr_notifiers, nb);
+	struct rproc_subdev *subdev;
+	struct qcom_rproc_ssr *ssr;
+	int ret;
+
+	if (!rproc)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&rproc->lock);
+	list_for_each_entry(subdev, &rproc->subdevs, node) {
+		ret = strcmp(subdev->name, "ssr_notifs");
+		if (!ret)
+			break;
+	}
+	mutex_unlock(&rproc->lock);
+	if (ret)
+		return ERR_PTR(-ENOENT);
+
+	ssr = to_ssr_subdev(subdev);
+	srcu_notifier_chain_register(ssr->rproc_notif_list, nb);
+
+	return ssr->rproc_notif_list;
 }
 EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier);
 
 /**
  * qcom_unregister_ssr_notifier() - unregister SSR notification handler
+ * @notify:	pointer to srcu notifier head
  * @nb:		notifier_block to unregister
  */
-void qcom_unregister_ssr_notifier(struct notifier_block *nb)
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
 {
-	blocking_notifier_chain_unregister(&ssr_notifiers, nb);
+	if (!notify)
+		return -EINVAL;
+
+	return srcu_notifier_chain_unregister(notify, nb);
 }
 EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
 
@@ -208,7 +231,7 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
 
-	blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name);
+	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
 }
 
 /**
@@ -226,6 +249,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 	ssr->name = ssr_name;
 	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
 	ssr->subdev.unprepare = ssr_notify_unprepare;
+	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
+								GFP_KERNEL);
+	srcu_init_notifier_head(ssr->rproc_notif_list);
 
 	rproc_add_subdev(rproc, &ssr->subdev);
 }
@@ -239,6 +265,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
 void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
 {
 	kfree(ssr->subdev.name);
+	kfree(ssr->rproc_notif_list);
 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 58de71e..7792691 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -27,6 +27,7 @@ struct qcom_rproc_subdev {
 struct qcom_rproc_ssr {
 	struct rproc_subdev subdev;
 
+	struct srcu_notifier_head *rproc_notif_list;
 	const char *name;
 };
 
diff --git a/drivers/soc/qcom/glink_ssr.c b/drivers/soc/qcom/glink_ssr.c
index d7babe3..2b39683 100644
--- a/drivers/soc/qcom/glink_ssr.c
+++ b/drivers/soc/qcom/glink_ssr.c
@@ -7,6 +7,7 @@
 #include <linux/completion.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/remoteproc.h>
 #include <linux/rpmsg.h>
 #include <linux/remoteproc/qcom_rproc.h>
 
@@ -49,6 +50,7 @@ struct glink_ssr {
 	struct rpmsg_endpoint *ept;
 
 	struct notifier_block nb;
+	void *notifier_head;
 
 	u32 seq_num;
 	struct completion completion;
@@ -112,6 +114,7 @@ static int qcom_glink_ssr_notify(struct notifier_block *nb, unsigned long event,
 static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
 {
 	struct glink_ssr *ssr;
+	struct rproc *rproc;
 
 	ssr = devm_kzalloc(&rpdev->dev, sizeof(*ssr), GFP_KERNEL);
 	if (!ssr)
@@ -125,14 +128,27 @@ static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, ssr);
 
-	return qcom_register_ssr_notifier(&ssr->nb);
+	rproc = rproc_get_by_child(&rpdev->dev);
+	if (!rproc) {
+		dev_err(&rpdev->dev, "glink device not child of rproc\n");
+		return -EINVAL;
+	}
+
+	ssr->notifier_head = qcom_register_ssr_notifier(rproc, &ssr->nb);
+	if (IS_ERR(ssr->notifier_head)) {
+		dev_err(&rpdev->dev,
+			"failed to register for ssr notifications\n");
+		return PTR_ERR(ssr->notifier_head);
+	}
+
+	return 0;
 }
 
 static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev)
 {
 	struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev);
 
-	qcom_unregister_ssr_notifier(&ssr->nb);
+	qcom_unregister_ssr_notifier(ssr->notifier_head, &ssr->nb);
 }
 
 static const struct rpmsg_device_id qcom_glink_ssr_match[] = {
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index fa8e386..89e830a 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -2,20 +2,27 @@
 #define __QCOM_RPROC_H__
 
 struct notifier_block;
+struct rproc;
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
-int qcom_register_ssr_notifier(struct notifier_block *nb);
-void qcom_unregister_ssr_notifier(struct notifier_block *nb);
+void *qcom_register_ssr_notifier(struct rproc *rproc,
+				 struct notifier_block *nb);
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
 
 #else
 
-static inline int qcom_register_ssr_notifier(struct notifier_block *nb)
+static inline void *qcom_register_ssr_notifier(struct rproc *rproc,
+					       struct notifier_block *nb)
 {
-	return 0;
+	return NULL;
 }
 
-static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {}
+static inline int qcom_unregister_ssr_notifier(void *notify,
+					       struct notifier_block *nb)
+{
+	return 0;
+}
 
 #endif
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 5/6] remoteproc: qcom: Add per subsystem SSR notification

[Siddharth Gupta]

Currently there is a global 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 an
individual notifier chain for every SSR subdevice, and modify the
notification registration API to include the remoteproc struct as an
argument. Update the existing user of the registration API to get the
phandle of the remoteproc dt node to register for SSR notifications.

Signed-off-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c      | 49 +++++++++++++++++++++++++++--------
 drivers/remoteproc/qcom_common.h      |  1 +
 drivers/soc/qcom/glink_ssr.c          | 20 ++++++++++++--
 include/linux/remoteproc/qcom_rproc.h | 17 ++++++++----
 4 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 5d59538..6714f27 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -23,8 +23,6 @@
 #define to_smd_subdev(d) container_of(d, struct qcom_rproc_subdev, subdev)
 #define to_ssr_subdev(d) container_of(d, struct qcom_rproc_ssr, subdev)
 
-static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
-
 static int glink_subdev_start(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
@@ -180,27 +178,52 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
 
 /**
  * qcom_register_ssr_notifier() - register SSR notification handler
+ * @rproc:	pointer to the remoteproc structure
  * @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 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 @notify function as handler for restart notifications. As
+ * remote processors are stopped this function will be called, with the rproc
+ * pointer passed as a parameter.
  */
-int qcom_register_ssr_notifier(struct notifier_block *nb)
+void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
 {
-	return blocking_notifier_chain_register(&ssr_notifiers, nb);
+	struct rproc_subdev *subdev;
+	struct qcom_rproc_ssr *ssr;
+	int ret;
+
+	if (!rproc)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&rproc->lock);
+	list_for_each_entry(subdev, &rproc->subdevs, node) {
+		ret = strcmp(subdev->name, "ssr_notifs");
+		if (!ret)
+			break;
+	}
+	mutex_unlock(&rproc->lock);
+	if (ret)
+		return ERR_PTR(-ENOENT);
+
+	ssr = to_ssr_subdev(subdev);
+	srcu_notifier_chain_register(ssr->rproc_notif_list, nb);
+
+	return ssr->rproc_notif_list;
 }
 EXPORT_SYMBOL_GPL(qcom_register_ssr_notifier);
 
 /**
  * qcom_unregister_ssr_notifier() - unregister SSR notification handler
+ * @notify:	pointer to srcu notifier head
  * @nb:		notifier_block to unregister
  */
-void qcom_unregister_ssr_notifier(struct notifier_block *nb)
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
 {
-	blocking_notifier_chain_unregister(&ssr_notifiers, nb);
+	if (!notify)
+		return -EINVAL;
+
+	return srcu_notifier_chain_unregister(notify, nb);
 }
 EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
 
@@ -208,7 +231,7 @@ static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
 
-	blocking_notifier_call_chain(&ssr_notifiers, 0, (void *)ssr->name);
+	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
 }
 
 /**
@@ -226,6 +249,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 	ssr->name = ssr_name;
 	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
 	ssr->subdev.unprepare = ssr_notify_unprepare;
+	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
+								GFP_KERNEL);
+	srcu_init_notifier_head(ssr->rproc_notif_list);
 
 	rproc_add_subdev(rproc, &ssr->subdev);
 }
@@ -239,6 +265,7 @@ EXPORT_SYMBOL_GPL(qcom_add_ssr_subdev);
 void qcom_remove_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr)
 {
 	kfree(ssr->subdev.name);
+	kfree(ssr->rproc_notif_list);
 	rproc_remove_subdev(rproc, &ssr->subdev);
 }
 EXPORT_SYMBOL_GPL(qcom_remove_ssr_subdev);
diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
index 58de71e..7792691 100644
--- a/drivers/remoteproc/qcom_common.h
+++ b/drivers/remoteproc/qcom_common.h
@@ -27,6 +27,7 @@ struct qcom_rproc_subdev {
 struct qcom_rproc_ssr {
 	struct rproc_subdev subdev;
 
+	struct srcu_notifier_head *rproc_notif_list;
 	const char *name;
 };
 
diff --git a/drivers/soc/qcom/glink_ssr.c b/drivers/soc/qcom/glink_ssr.c
index d7babe3..2b39683 100644
--- a/drivers/soc/qcom/glink_ssr.c
+++ b/drivers/soc/qcom/glink_ssr.c
@@ -7,6 +7,7 @@
 #include <linux/completion.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
+#include <linux/remoteproc.h>
 #include <linux/rpmsg.h>
 #include <linux/remoteproc/qcom_rproc.h>
 
@@ -49,6 +50,7 @@ struct glink_ssr {
 	struct rpmsg_endpoint *ept;
 
 	struct notifier_block nb;
+	void *notifier_head;
 
 	u32 seq_num;
 	struct completion completion;
@@ -112,6 +114,7 @@ static int qcom_glink_ssr_notify(struct notifier_block *nb, unsigned long event,
 static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
 {
 	struct glink_ssr *ssr;
+	struct rproc *rproc;
 
 	ssr = devm_kzalloc(&rpdev->dev, sizeof(*ssr), GFP_KERNEL);
 	if (!ssr)
@@ -125,14 +128,27 @@ static int qcom_glink_ssr_probe(struct rpmsg_device *rpdev)
 
 	dev_set_drvdata(&rpdev->dev, ssr);
 
-	return qcom_register_ssr_notifier(&ssr->nb);
+	rproc = rproc_get_by_child(&rpdev->dev);
+	if (!rproc) {
+		dev_err(&rpdev->dev, "glink device not child of rproc\n");
+		return -EINVAL;
+	}
+
+	ssr->notifier_head = qcom_register_ssr_notifier(rproc, &ssr->nb);
+	if (IS_ERR(ssr->notifier_head)) {
+		dev_err(&rpdev->dev,
+			"failed to register for ssr notifications\n");
+		return PTR_ERR(ssr->notifier_head);
+	}
+
+	return 0;
 }
 
 static void qcom_glink_ssr_remove(struct rpmsg_device *rpdev)
 {
 	struct glink_ssr *ssr = dev_get_drvdata(&rpdev->dev);
 
-	qcom_unregister_ssr_notifier(&ssr->nb);
+	qcom_unregister_ssr_notifier(ssr->notifier_head, &ssr->nb);
 }
 
 static const struct rpmsg_device_id qcom_glink_ssr_match[] = {
diff --git a/include/linux/remoteproc/qcom_rproc.h b/include/linux/remoteproc/qcom_rproc.h
index fa8e386..89e830a 100644
--- a/include/linux/remoteproc/qcom_rproc.h
+++ b/include/linux/remoteproc/qcom_rproc.h
@@ -2,20 +2,27 @@
 #define __QCOM_RPROC_H__
 
 struct notifier_block;
+struct rproc;
 
 #if IS_ENABLED(CONFIG_QCOM_RPROC_COMMON)
 
-int qcom_register_ssr_notifier(struct notifier_block *nb);
-void qcom_unregister_ssr_notifier(struct notifier_block *nb);
+void *qcom_register_ssr_notifier(struct rproc *rproc,
+				 struct notifier_block *nb);
+int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb);
 
 #else
 
-static inline int qcom_register_ssr_notifier(struct notifier_block *nb)
+static inline void *qcom_register_ssr_notifier(struct rproc *rproc,
+					       struct notifier_block *nb)
 {
-	return 0;
+	return NULL;
 }
 
-static inline void qcom_unregister_ssr_notifier(struct notifier_block *nb) {}
+static inline int qcom_unregister_ssr_notifier(void *notify,
+					       struct notifier_block *nb)
+{
+	return 0;
+}
 
 #endif
 
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Siddharth Gupta]

The SSR subdevice only adds callback for the unprepare event. Add callbacks
for unprepare, start and prepare events. The client driver for a particular
remoteproc might be interested in knowing the status of the remoteproc
while undergoing SSR, not just when the remoteproc has finished shutting
down.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++----
 include/linux/remoteproc.h       | 15 +++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 6714f27..6f04a5b 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
  *
  * Returns pointer to srcu notifier head on success, ERR_PTR on failure.
  *
- * This registers the @notify function as handler for restart notifications. As
- * remote processors are stopped this function will be called, with the rproc
- * pointer passed as a parameter.
+ * This registers the @notify function as handler for powerup/shutdown
+ * notifications. This function will be invoked inside the callbacks registered
+ * for the ssr subdevice, with the rproc pointer passed as a parameter.
  */
 void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
 {
@@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
 
+static int ssr_notify_prepare(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
+	return 0;
+}
+
+static int ssr_notify_start(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_AFTER_POWERUP, (void *)ssr->name);
+	return 0;
+}
+
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
+}
+
+
 static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
 
-	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
 }
 
 /**
@@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 {
 	ssr->name = ssr_name;
 	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
+	ssr->subdev.prepare = ssr_notify_prepare;
+	ssr->subdev.start = ssr_notify_start;
+	ssr->subdev.stop = ssr_notify_stop;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
 								GFP_KERNEL);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e2f60cc..4be4478 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -449,6 +449,21 @@ struct rproc_dump_segment {
 };
 
 /**
+ * enum rproc_notif_type - Different stages of remoteproc notifications
+ * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
+ * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
+ * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
+ * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
+ */
+enum rproc_notif_type {
+	RPROC_BEFORE_SHUTDOWN,
+	RPROC_AFTER_SHUTDOWN,
+	RPROC_BEFORE_POWERUP,
+	RPROC_AFTER_POWERUP,
+	RPROC_MAX
+};
+
+/**
  * struct rproc - represents a physical remote processor device
  * @node: list node of this rproc object
  * @domain: iommu domain
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Siddharth Gupta]

The SSR subdevice only adds callback for the unprepare event. Add callbacks
for unprepare, start and prepare events. The client driver for a particular
remoteproc might be interested in knowing the status of the remoteproc
while undergoing SSR, not just when the remoteproc has finished shutting
down.

Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
---
 drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++----
 include/linux/remoteproc.h       | 15 +++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
index 6714f27..6f04a5b 100644
--- a/drivers/remoteproc/qcom_common.c
+++ b/drivers/remoteproc/qcom_common.c
@@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
  *
  * Returns pointer to srcu notifier head on success, ERR_PTR on failure.
  *
- * This registers the @notify function as handler for restart notifications. As
- * remote processors are stopped this function will be called, with the rproc
- * pointer passed as a parameter.
+ * This registers the @notify function as handler for powerup/shutdown
+ * notifications. This function will be invoked inside the callbacks registered
+ * for the ssr subdevice, with the rproc pointer passed as a parameter.
  */
 void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
 {
@@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
 
+static int ssr_notify_prepare(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
+	return 0;
+}
+
+static int ssr_notify_start(struct rproc_subdev *subdev)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_AFTER_POWERUP, (void *)ssr->name);
+	return 0;
+}
+
+static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
+{
+	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
+
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
+}
+
+
 static void ssr_notify_unprepare(struct rproc_subdev *subdev)
 {
 	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
 
-	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
+	srcu_notifier_call_chain(ssr->rproc_notif_list,
+				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
 }
 
 /**
@@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
 {
 	ssr->name = ssr_name;
 	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
+	ssr->subdev.prepare = ssr_notify_prepare;
+	ssr->subdev.start = ssr_notify_start;
+	ssr->subdev.stop = ssr_notify_stop;
 	ssr->subdev.unprepare = ssr_notify_unprepare;
 	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
 								GFP_KERNEL);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e2f60cc..4be4478 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -449,6 +449,21 @@ struct rproc_dump_segment {
 };
 
 /**
+ * enum rproc_notif_type - Different stages of remoteproc notifications
+ * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
+ * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
+ * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
+ * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
+ */
+enum rproc_notif_type {
+	RPROC_BEFORE_SHUTDOWN,
+	RPROC_AFTER_SHUTDOWN,
+	RPROC_BEFORE_POWERUP,
+	RPROC_AFTER_POWERUP,
+	RPROC_MAX
+};
+
+/**
  * struct rproc - represents a physical remote processor device
  * @node: list node of this rproc object
  * @domain: iommu domain
-- 
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> The SSR subdevice only adds callback for the unprepare event. Add callbacks
> for unprepare, start and prepare events. The client driver for a particular
> remoteproc might be interested in knowing the status of the remoteproc
> while undergoing SSR, not just when the remoteproc has finished shutting
> down.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++----
>  include/linux/remoteproc.h       | 15 +++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 6714f27..6f04a5b 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>   *
>   * Returns pointer to srcu notifier head on success, ERR_PTR on failure.
>   *
> - * This registers the @notify function as handler for restart notifications. As
> - * remote processors are stopped this function will be called, with the rproc
> - * pointer passed as a parameter.
> + * This registers the @notify function as handler for powerup/shutdown
> + * notifications. This function will be invoked inside the callbacks registered
> + * for the ssr subdevice, with the rproc pointer passed as a parameter.
>   */
>  void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
>  {
> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>  
> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> +	return 0;
> +}
> +
> +static int ssr_notify_start(struct rproc_subdev *subdev)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
> +	return 0;
> +}
> +
> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> +}
> +
> +
>  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>  {
>  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>  
> -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>  }
>  
>  /**
> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
>  {
>  	ssr->name = ssr_name;
>  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> +	ssr->subdev.prepare = ssr_notify_prepare;
> +	ssr->subdev.start = ssr_notify_start;
> +	ssr->subdev.stop = ssr_notify_stop;

Now that I have a better understanding of what this patchset is doing, I realise
my comments in patch 04 won't work.  To differentiate the subdevs of an rproc I
suggest to wrap them in a generic structure with a type and an enum.  That way
you can differenciate between subdevices without having to add to the core.

That being said, I don't understand what patches 5 and 6 are doing...
Registering with the global ssr_notifiers allowed to gracefully shutdown all the
MCUs in the system when one of them would go down.  But now that we are using
the notifier on a per MCU, I really don't see why each subdev couldn't implement
the right prepare/start/stop functions.

Am I missing something here?
 

>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>  								GFP_KERNEL);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e2f60cc..4be4478 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>  };
>  
>  /**
> + * enum rproc_notif_type - Different stages of remoteproc notifications
> + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
> + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
> + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
> + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
> + */
> +enum rproc_notif_type {
> +	RPROC_BEFORE_SHUTDOWN,
> +	RPROC_AFTER_SHUTDOWN,
> +	RPROC_BEFORE_POWERUP,
> +	RPROC_AFTER_POWERUP,
> +	RPROC_MAX
> +};
> +
> +/**
>   * struct rproc - represents a physical remote processor device
>   * @node: list node of this rproc object
>   * @domain: iommu domain
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> The SSR subdevice only adds callback for the unprepare event. Add callbacks
> for unprepare, start and prepare events. The client driver for a particular
> remoteproc might be interested in knowing the status of the remoteproc
> while undergoing SSR, not just when the remoteproc has finished shutting
> down.
> 
> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_common.c | 39 +++++++++++++++++++++++++++++++++++----
>  include/linux/remoteproc.h       | 15 +++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_common.c b/drivers/remoteproc/qcom_common.c
> index 6714f27..6f04a5b 100644
> --- a/drivers/remoteproc/qcom_common.c
> +++ b/drivers/remoteproc/qcom_common.c
> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>   *
>   * Returns pointer to srcu notifier head on success, ERR_PTR on failure.
>   *
> - * This registers the @notify function as handler for restart notifications. As
> - * remote processors are stopped this function will be called, with the rproc
> - * pointer passed as a parameter.
> + * This registers the @notify function as handler for powerup/shutdown
> + * notifications. This function will be invoked inside the callbacks registered
> + * for the ssr subdevice, with the rproc pointer passed as a parameter.
>   */
>  void *qcom_register_ssr_notifier(struct rproc *rproc, struct notifier_block *nb)
>  {
> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, struct notifier_block *nb)
>  }
>  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>  
> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> +	return 0;
> +}
> +
> +static int ssr_notify_start(struct rproc_subdev *subdev)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
> +	return 0;
> +}
> +
> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool crashed)
> +{
> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> +
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> +}
> +
> +
>  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>  {
>  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>  
> -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void *)ssr->name);
> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>  }
>  
>  /**
> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, struct qcom_rproc_ssr *ssr,
>  {
>  	ssr->name = ssr_name;
>  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> +	ssr->subdev.prepare = ssr_notify_prepare;
> +	ssr->subdev.start = ssr_notify_start;
> +	ssr->subdev.stop = ssr_notify_stop;

Now that I have a better understanding of what this patchset is doing, I realise
my comments in patch 04 won't work.  To differentiate the subdevs of an rproc I
suggest to wrap them in a generic structure with a type and an enum.  That way
you can differenciate between subdevices without having to add to the core.

That being said, I don't understand what patches 5 and 6 are doing...
Registering with the global ssr_notifiers allowed to gracefully shutdown all the
MCUs in the system when one of them would go down.  But now that we are using
the notifier on a per MCU, I really don't see why each subdev couldn't implement
the right prepare/start/stop functions.

Am I missing something here?
 

>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>  								GFP_KERNEL);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e2f60cc..4be4478 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>  };
>  
>  /**
> + * enum rproc_notif_type - Different stages of remoteproc notifications
> + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
> + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
> + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
> + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
> + */
> +enum rproc_notif_type {
> +	RPROC_BEFORE_SHUTDOWN,
> +	RPROC_AFTER_SHUTDOWN,
> +	RPROC_BEFORE_POWERUP,
> +	RPROC_AFTER_POWERUP,
> +	RPROC_MAX
> +};
> +
> +/**
>   * struct rproc - represents a physical remote processor device
>   * @node: list node of this rproc object
>   * @domain: iommu domain
> -- 
> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-02-27 13:59, Mathieu Poirier wrote:
> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> The SSR subdevice only adds callback for the unprepare event. Add 
>> callbacks
>> for unprepare, start and prepare events. The client driver for a 
>> particular
>> remoteproc might be interested in knowing the status of the remoteproc
>> while undergoing SSR, not just when the remoteproc has finished 
>> shutting
>> down.
>> 
>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_common.c | 39 
>> +++++++++++++++++++++++++++++++++++----
>>  include/linux/remoteproc.h       | 15 +++++++++++++++
>>  2 files changed, 50 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 6714f27..6f04a5b 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>>   *
>>   * Returns pointer to srcu notifier head on success, ERR_PTR on 
>> failure.
>>   *
>> - * This registers the @notify function as handler for restart 
>> notifications. As
>> - * remote processors are stopped this function will be called, with 
>> the rproc
>> - * pointer passed as a parameter.
>> + * This registers the @notify function as handler for 
>> powerup/shutdown
>> + * notifications. This function will be invoked inside the callbacks 
>> registered
>> + * for the ssr subdevice, with the rproc pointer passed as a 
>> parameter.
>>   */
>>  void *qcom_register_ssr_notifier(struct rproc *rproc, struct 
>> notifier_block *nb)
>>  {
>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, 
>> struct notifier_block *nb)
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> 
>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> +	return 0;
>> +}
>> +
>> +static int ssr_notify_start(struct rproc_subdev *subdev)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> +	return 0;
>> +}
>> +
>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool 
>> crashed)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> +}
>> +
>> +
>>  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>>  {
>>  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> 
>> -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void 
>> *)ssr->name);
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>>  }
>> 
>>  /**
>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, 
>> struct qcom_rproc_ssr *ssr,
>>  {
>>  	ssr->name = ssr_name;
>>  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> +	ssr->subdev.prepare = ssr_notify_prepare;
>> +	ssr->subdev.start = ssr_notify_start;
>> +	ssr->subdev.stop = ssr_notify_stop;
> 
> Now that I have a better understanding of what this patchset is doing, 
> I realise
> my comments in patch 04 won't work.  To differentiate the subdevs of an 
> rproc I
> suggest to wrap them in a generic structure with a type and an enum.  
> That way
> you can differenciate between subdevices without having to add to the 
> core.
Ok. I can try that.
> 
> That being said, I don't understand what patches 5 and 6 are doing...
> Registering with the global ssr_notifiers allowed to gracefully 
> shutdown all the
> MCUs in the system when one of them would go down.  But now that we are 
> using
> the notifier on a per MCU, I really don't see why each subdev couldn't 
> implement
> the right prepare/start/stop functions.
> 
> Am I missing something here?
We only want kernel clients to be notified when the Remoteproc they are 
interested
in changes state. For e.g. audio kernel driver should be notified when 
audio
processor goes down but it does not care about any other remoteproc.
If you are suggesting that these kernel clients be added as subdevices 
then
we will end up having many subdevices registered to each remoteproc. So 
we
implemented a notifier chain per Remoteproc. This keeps the SSR 
notifications as
the subdevice per remoteproc, and all interested clients can register to 
it.
> 
> 
>>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>>  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>>  								GFP_KERNEL);
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index e2f60cc..4be4478 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>>  };
>> 
>>  /**
>> + * enum rproc_notif_type - Different stages of remoteproc 
>> notifications
>> + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
>> + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
>> + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
>> + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
>> + */
>> +enum rproc_notif_type {
>> +	RPROC_BEFORE_SHUTDOWN,
>> +	RPROC_AFTER_SHUTDOWN,
>> +	RPROC_BEFORE_POWERUP,
>> +	RPROC_AFTER_POWERUP,
>> +	RPROC_MAX
>> +};
>> +
>> +/**
>>   * struct rproc - represents a physical remote processor device
>>   * @node: list node of this rproc object
>>   * @domain: iommu domain
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-02-27 13:59, Mathieu Poirier wrote:
> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> The SSR subdevice only adds callback for the unprepare event. Add 
>> callbacks
>> for unprepare, start and prepare events. The client driver for a 
>> particular
>> remoteproc might be interested in knowing the status of the remoteproc
>> while undergoing SSR, not just when the remoteproc has finished 
>> shutting
>> down.
>> 
>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_common.c | 39 
>> +++++++++++++++++++++++++++++++++++----
>>  include/linux/remoteproc.h       | 15 +++++++++++++++
>>  2 files changed, 50 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_common.c 
>> b/drivers/remoteproc/qcom_common.c
>> index 6714f27..6f04a5b 100644
>> --- a/drivers/remoteproc/qcom_common.c
>> +++ b/drivers/remoteproc/qcom_common.c
>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>>   *
>>   * Returns pointer to srcu notifier head on success, ERR_PTR on 
>> failure.
>>   *
>> - * This registers the @notify function as handler for restart 
>> notifications. As
>> - * remote processors are stopped this function will be called, with 
>> the rproc
>> - * pointer passed as a parameter.
>> + * This registers the @notify function as handler for 
>> powerup/shutdown
>> + * notifications. This function will be invoked inside the callbacks 
>> registered
>> + * for the ssr subdevice, with the rproc pointer passed as a 
>> parameter.
>>   */
>>  void *qcom_register_ssr_notifier(struct rproc *rproc, struct 
>> notifier_block *nb)
>>  {
>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify, 
>> struct notifier_block *nb)
>>  }
>>  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> 
>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> +	return 0;
>> +}
>> +
>> +static int ssr_notify_start(struct rproc_subdev *subdev)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> +	return 0;
>> +}
>> +
>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool 
>> crashed)
>> +{
>> +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> +
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> +}
>> +
>> +
>>  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>>  {
>>  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> 
>> -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void 
>> *)ssr->name);
>> +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>>  }
>> 
>>  /**
>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc, 
>> struct qcom_rproc_ssr *ssr,
>>  {
>>  	ssr->name = ssr_name;
>>  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> +	ssr->subdev.prepare = ssr_notify_prepare;
>> +	ssr->subdev.start = ssr_notify_start;
>> +	ssr->subdev.stop = ssr_notify_stop;
> 
> Now that I have a better understanding of what this patchset is doing, 
> I realise
> my comments in patch 04 won't work.  To differentiate the subdevs of an 
> rproc I
> suggest to wrap them in a generic structure with a type and an enum.  
> That way
> you can differenciate between subdevices without having to add to the 
> core.
Ok. I can try that.
> 
> That being said, I don't understand what patches 5 and 6 are doing...
> Registering with the global ssr_notifiers allowed to gracefully 
> shutdown all the
> MCUs in the system when one of them would go down.  But now that we are 
> using
> the notifier on a per MCU, I really don't see why each subdev couldn't 
> implement
> the right prepare/start/stop functions.
> 
> Am I missing something here?
We only want kernel clients to be notified when the Remoteproc they are 
interested
in changes state. For e.g. audio kernel driver should be notified when 
audio
processor goes down but it does not care about any other remoteproc.
If you are suggesting that these kernel clients be added as subdevices 
then
we will end up having many subdevices registered to each remoteproc. So 
we
implemented a notifier chain per Remoteproc. This keeps the SSR 
notifications as
the subdevice per remoteproc, and all interested clients can register to 
it.
> 
> 
>>  	ssr->subdev.unprepare = ssr_notify_unprepare;
>>  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>>  								GFP_KERNEL);
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index e2f60cc..4be4478 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>>  };
>> 
>>  /**
>> + * enum rproc_notif_type - Different stages of remoteproc 
>> notifications
>> + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
>> + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
>> + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
>> + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
>> + */
>> +enum rproc_notif_type {
>> +	RPROC_BEFORE_SHUTDOWN,
>> +	RPROC_AFTER_SHUTDOWN,
>> +	RPROC_BEFORE_POWERUP,
>> +	RPROC_AFTER_POWERUP,
>> +	RPROC_MAX
>> +};
>> +
>> +/**
>>   * struct rproc - represents a physical remote processor device
>>   * @node: list node of this rproc object
>>   * @domain: iommu domain
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org wrote:
> On 2020-02-27 13:59, Mathieu Poirier wrote:
> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> > > The SSR subdevice only adds callback for the unprepare event. Add
> > > callbacks
> > > for unprepare, start and prepare events. The client driver for a
> > > particular
> > > remoteproc might be interested in knowing the status of the remoteproc
> > > while undergoing SSR, not just when the remoteproc has finished
> > > shutting
> > > down.
> > > 
> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> > > ---
> > >  drivers/remoteproc/qcom_common.c | 39
> > > +++++++++++++++++++++++++++++++++++----
> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_common.c
> > > b/drivers/remoteproc/qcom_common.c
> > > index 6714f27..6f04a5b 100644
> > > --- a/drivers/remoteproc/qcom_common.c
> > > +++ b/drivers/remoteproc/qcom_common.c
> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> > >   *
> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> > > failure.
> > >   *
> > > - * This registers the @notify function as handler for restart
> > > notifications. As
> > > - * remote processors are stopped this function will be called, with
> > > the rproc
> > > - * pointer passed as a parameter.
> > > + * This registers the @notify function as handler for
> > > powerup/shutdown
> > > + * notifications. This function will be invoked inside the
> > > callbacks registered
> > > + * for the ssr subdevice, with the rproc pointer passed as a
> > > parameter.
> > >   */
> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> > > notifier_block *nb)
> > >  {
> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> > > struct notifier_block *nb)
> > >  }
> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> > > 
> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> > > +	return 0;
> > > +}
> > > +
> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
> > > +	return 0;
> > > +}
> > > +
> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> > > crashed)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> > > +}
> > > +
> > > +
> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> > >  {
> > >  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > 
> > > -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> > > *)ssr->name);
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> > >  }
> > > 
> > >  /**
> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> > > struct qcom_rproc_ssr *ssr,
> > >  {
> > >  	ssr->name = ssr_name;
> > >  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> > > +	ssr->subdev.prepare = ssr_notify_prepare;
> > > +	ssr->subdev.start = ssr_notify_start;
> > > +	ssr->subdev.stop = ssr_notify_stop;
> > 
> > Now that I have a better understanding of what this patchset is doing, I
> > realise
> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> > rproc I
> > suggest to wrap them in a generic structure with a type and an enum.
> > That way
> > you can differenciate between subdevices without having to add to the
> > core.
> Ok. I can try that.
> > 
> > That being said, I don't understand what patches 5 and 6 are doing...
> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> > all the
> > MCUs in the system when one of them would go down.  But now that we are
> > using
> > the notifier on a per MCU, I really don't see why each subdev couldn't
> > implement
> > the right prepare/start/stop functions.
> > 
> > Am I missing something here?
> We only want kernel clients to be notified when the Remoteproc they are
> interested
> in changes state. For e.g. audio kernel driver should be notified when audio
> processor goes down but it does not care about any other remoteproc.
> If you are suggesting that these kernel clients be added as subdevices then
> we will end up having many subdevices registered to each remoteproc. So we
> implemented a notifier chain per Remoteproc. This keeps the SSR
> notifications as
> the subdevice per remoteproc, and all interested clients can register to it.

It seems like I am missing information...  Your are referring to "kernel
clients" and as such I must assume some drivers that are not part of the 
remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().  I must
also assume these drivers (or that functionality) are not yet upsream because
all I can see calling qcom_register_ssr_notifier() is qcom_glink_ssr_probe(). 

Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is the glink
device that driver is handling the same as the glink device registed in
adsp_probe() and q6v5_probe()? 

> > 
> > 
> > >  	ssr->subdev.unprepare = ssr_notify_unprepare;
> > >  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> > >  								GFP_KERNEL);
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index e2f60cc..4be4478 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> > >  };
> > > 
> > >  /**
> > > + * enum rproc_notif_type - Different stages of remoteproc
> > > notifications
> > > + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
> > > + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
> > > + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
> > > + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
> > > + */
> > > +enum rproc_notif_type {
> > > +	RPROC_BEFORE_SHUTDOWN,
> > > +	RPROC_AFTER_SHUTDOWN,
> > > +	RPROC_BEFORE_POWERUP,
> > > +	RPROC_AFTER_POWERUP,
> > > +	RPROC_MAX
> > > +};
> > > +
> > > +/**
> > >   * struct rproc - represents a physical remote processor device
> > >   * @node: list node of this rproc object
> > >   * @domain: iommu domain
> > > --
> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org wrote:
> On 2020-02-27 13:59, Mathieu Poirier wrote:
> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> > > The SSR subdevice only adds callback for the unprepare event. Add
> > > callbacks
> > > for unprepare, start and prepare events. The client driver for a
> > > particular
> > > remoteproc might be interested in knowing the status of the remoteproc
> > > while undergoing SSR, not just when the remoteproc has finished
> > > shutting
> > > down.
> > > 
> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> > > ---
> > >  drivers/remoteproc/qcom_common.c | 39
> > > +++++++++++++++++++++++++++++++++++----
> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/qcom_common.c
> > > b/drivers/remoteproc/qcom_common.c
> > > index 6714f27..6f04a5b 100644
> > > --- a/drivers/remoteproc/qcom_common.c
> > > +++ b/drivers/remoteproc/qcom_common.c
> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> > >   *
> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> > > failure.
> > >   *
> > > - * This registers the @notify function as handler for restart
> > > notifications. As
> > > - * remote processors are stopped this function will be called, with
> > > the rproc
> > > - * pointer passed as a parameter.
> > > + * This registers the @notify function as handler for
> > > powerup/shutdown
> > > + * notifications. This function will be invoked inside the
> > > callbacks registered
> > > + * for the ssr subdevice, with the rproc pointer passed as a
> > > parameter.
> > >   */
> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> > > notifier_block *nb)
> > >  {
> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> > > struct notifier_block *nb)
> > >  }
> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> > > 
> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> > > +	return 0;
> > > +}
> > > +
> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
> > > +	return 0;
> > > +}
> > > +
> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> > > crashed)
> > > +{
> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > +
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> > > +}
> > > +
> > > +
> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> > >  {
> > >  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> > > 
> > > -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> > > *)ssr->name);
> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
> > > +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> > >  }
> > > 
> > >  /**
> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> > > struct qcom_rproc_ssr *ssr,
> > >  {
> > >  	ssr->name = ssr_name;
> > >  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> > > +	ssr->subdev.prepare = ssr_notify_prepare;
> > > +	ssr->subdev.start = ssr_notify_start;
> > > +	ssr->subdev.stop = ssr_notify_stop;
> > 
> > Now that I have a better understanding of what this patchset is doing, I
> > realise
> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> > rproc I
> > suggest to wrap them in a generic structure with a type and an enum.
> > That way
> > you can differenciate between subdevices without having to add to the
> > core.
> Ok. I can try that.
> > 
> > That being said, I don't understand what patches 5 and 6 are doing...
> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> > all the
> > MCUs in the system when one of them would go down.  But now that we are
> > using
> > the notifier on a per MCU, I really don't see why each subdev couldn't
> > implement
> > the right prepare/start/stop functions.
> > 
> > Am I missing something here?
> We only want kernel clients to be notified when the Remoteproc they are
> interested
> in changes state. For e.g. audio kernel driver should be notified when audio
> processor goes down but it does not care about any other remoteproc.
> If you are suggesting that these kernel clients be added as subdevices then
> we will end up having many subdevices registered to each remoteproc. So we
> implemented a notifier chain per Remoteproc. This keeps the SSR
> notifications as
> the subdevice per remoteproc, and all interested clients can register to it.

It seems like I am missing information...  Your are referring to "kernel
clients" and as such I must assume some drivers that are not part of the 
remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().  I must
also assume these drivers (or that functionality) are not yet upsream because
all I can see calling qcom_register_ssr_notifier() is qcom_glink_ssr_probe(). 

Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is the glink
device that driver is handling the same as the glink device registed in
adsp_probe() and q6v5_probe()? 

> > 
> > 
> > >  	ssr->subdev.unprepare = ssr_notify_unprepare;
> > >  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> > >  								GFP_KERNEL);
> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > > index e2f60cc..4be4478 100644
> > > --- a/include/linux/remoteproc.h
> > > +++ b/include/linux/remoteproc.h
> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> > >  };
> > > 
> > >  /**
> > > + * enum rproc_notif_type - Different stages of remoteproc
> > > notifications
> > > + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
> > > + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
> > > + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
> > > + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
> > > + */
> > > +enum rproc_notif_type {
> > > +	RPROC_BEFORE_SHUTDOWN,
> > > +	RPROC_AFTER_SHUTDOWN,
> > > +	RPROC_BEFORE_POWERUP,
> > > +	RPROC_AFTER_POWERUP,
> > > +	RPROC_MAX
> > > +};
> > > +
> > > +/**
> > >   * struct rproc - represents a physical remote processor device
> > >   * @node: list node of this rproc object
> > >   * @domain: iommu domain
> > > --
> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > > a Linux Foundation Collaborative Project
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-02-28 10:38, Mathieu Poirier wrote:
> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org 
> wrote:
>> On 2020-02-27 13:59, Mathieu Poirier wrote:
>> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> > > The SSR subdevice only adds callback for the unprepare event. Add
>> > > callbacks
>> > > for unprepare, start and prepare events. The client driver for a
>> > > particular
>> > > remoteproc might be interested in knowing the status of the remoteproc
>> > > while undergoing SSR, not just when the remoteproc has finished
>> > > shutting
>> > > down.
>> > >
>> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> > > ---
>> > >  drivers/remoteproc/qcom_common.c | 39
>> > > +++++++++++++++++++++++++++++++++++----
>> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
>> > >  2 files changed, 50 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/remoteproc/qcom_common.c
>> > > b/drivers/remoteproc/qcom_common.c
>> > > index 6714f27..6f04a5b 100644
>> > > --- a/drivers/remoteproc/qcom_common.c
>> > > +++ b/drivers/remoteproc/qcom_common.c
>> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>> > >   *
>> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
>> > > failure.
>> > >   *
>> > > - * This registers the @notify function as handler for restart
>> > > notifications. As
>> > > - * remote processors are stopped this function will be called, with
>> > > the rproc
>> > > - * pointer passed as a parameter.
>> > > + * This registers the @notify function as handler for
>> > > powerup/shutdown
>> > > + * notifications. This function will be invoked inside the
>> > > callbacks registered
>> > > + * for the ssr subdevice, with the rproc pointer passed as a
>> > > parameter.
>> > >   */
>> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>> > > notifier_block *nb)
>> > >  {
>> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>> > > struct notifier_block *nb)
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> > >
>> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>> > > crashed)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> > > +}
>> > > +
>> > > +
>> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>> > >  {
>> > >  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > >
>> > > -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>> > > *)ssr->name);
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>> > >  }
>> > >
>> > >  /**
>> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>> > > struct qcom_rproc_ssr *ssr,
>> > >  {
>> > >  	ssr->name = ssr_name;
>> > >  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> > > +	ssr->subdev.prepare = ssr_notify_prepare;
>> > > +	ssr->subdev.start = ssr_notify_start;
>> > > +	ssr->subdev.stop = ssr_notify_stop;
>> >
>> > Now that I have a better understanding of what this patchset is doing, I
>> > realise
>> > my comments in patch 04 won't work.  To differentiate the subdevs of an
>> > rproc I
>> > suggest to wrap them in a generic structure with a type and an enum.
>> > That way
>> > you can differenciate between subdevices without having to add to the
>> > core.
>> Ok. I can try that.
>> >
>> > That being said, I don't understand what patches 5 and 6 are doing...
>> > Registering with the global ssr_notifiers allowed to gracefully shutdown
>> > all the
>> > MCUs in the system when one of them would go down.  But now that we are
>> > using
>> > the notifier on a per MCU, I really don't see why each subdev couldn't
>> > implement
>> > the right prepare/start/stop functions.
>> >
>> > Am I missing something here?
>> We only want kernel clients to be notified when the Remoteproc they 
>> are
>> interested
>> in changes state. For e.g. audio kernel driver should be notified when 
>> audio
>> processor goes down but it does not care about any other remoteproc.
>> If you are suggesting that these kernel clients be added as subdevices 
>> then
>> we will end up having many subdevices registered to each remoteproc. 
>> So we
>> implemented a notifier chain per Remoteproc. This keeps the SSR
>> notifications as
>> the subdevice per remoteproc, and all interested clients can register 
>> to it.
> 
> It seems like I am missing information...  Your are referring to 
> "kernel
> clients" and as such I must assume some drivers that are not part of 
> the
> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().  
> I must
Yes these are not part of remoteproc framework and they will register 
for notifications.
> also assume these drivers (or that functionality) are not yet upsream 
> because
> all I can see calling qcom_register_ssr_notifier() is 
> qcom_glink_ssr_probe().
Correct.These are not upstreamed.
> 
> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is 
> the glink
> device that driver is handling the same as the glink device registed in
> adsp_probe() and q6v5_probe()?
glink ssr driver will send out notifications to remoteprocs that have 
opened the
"glink_ssr" channel that some subsystem has gone down or booted up. This 
helps notify
neighboring subsystems about change in state of any other subsystem.
> 
>> >
>> >
>> > >  	ssr->subdev.unprepare = ssr_notify_unprepare;
>> > >  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>> > >  								GFP_KERNEL);
>> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> > > index e2f60cc..4be4478 100644
>> > > --- a/include/linux/remoteproc.h
>> > > +++ b/include/linux/remoteproc.h
>> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>> > >  };
>> > >
>> > >  /**
>> > > + * enum rproc_notif_type - Different stages of remoteproc
>> > > notifications
>> > > + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
>> > > + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
>> > > + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
>> > > + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
>> > > + */
>> > > +enum rproc_notif_type {
>> > > +	RPROC_BEFORE_SHUTDOWN,
>> > > +	RPROC_AFTER_SHUTDOWN,
>> > > +	RPROC_BEFORE_POWERUP,
>> > > +	RPROC_AFTER_POWERUP,
>> > > +	RPROC_MAX
>> > > +};
>> > > +
>> > > +/**
>> > >   * struct rproc - represents a physical remote processor device
>> > >   * @node: list node of this rproc object
>> > >   * @domain: iommu domain
>> > > --
>> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> > > a Linux Foundation Collaborative Project
>> > >
>> > > _______________________________________________
>> > > linux-arm-kernel mailing list
>> > > linux-arm-kernel@lists.infradead.org
>> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-02-28 10:38, Mathieu Poirier wrote:
> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org 
> wrote:
>> On 2020-02-27 13:59, Mathieu Poirier wrote:
>> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> > > The SSR subdevice only adds callback for the unprepare event. Add
>> > > callbacks
>> > > for unprepare, start and prepare events. The client driver for a
>> > > particular
>> > > remoteproc might be interested in knowing the status of the remoteproc
>> > > while undergoing SSR, not just when the remoteproc has finished
>> > > shutting
>> > > down.
>> > >
>> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> > > ---
>> > >  drivers/remoteproc/qcom_common.c | 39
>> > > +++++++++++++++++++++++++++++++++++----
>> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
>> > >  2 files changed, 50 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/drivers/remoteproc/qcom_common.c
>> > > b/drivers/remoteproc/qcom_common.c
>> > > index 6714f27..6f04a5b 100644
>> > > --- a/drivers/remoteproc/qcom_common.c
>> > > +++ b/drivers/remoteproc/qcom_common.c
>> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>> > >   *
>> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
>> > > failure.
>> > >   *
>> > > - * This registers the @notify function as handler for restart
>> > > notifications. As
>> > > - * remote processors are stopped this function will be called, with
>> > > the rproc
>> > > - * pointer passed as a parameter.
>> > > + * This registers the @notify function as handler for
>> > > powerup/shutdown
>> > > + * notifications. This function will be invoked inside the
>> > > callbacks registered
>> > > + * for the ssr subdevice, with the rproc pointer passed as a
>> > > parameter.
>> > >   */
>> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>> > > notifier_block *nb)
>> > >  {
>> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>> > > struct notifier_block *nb)
>> > >  }
>> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> > >
>> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>> > > crashed)
>> > > +{
>> > > +	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > > +
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> > > +}
>> > > +
>> > > +
>> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>> > >  {
>> > >  	struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> > >
>> > > -	srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>> > > *)ssr->name);
>> > > +	srcu_notifier_call_chain(ssr->rproc_notif_list,
>> > > +				 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>> > >  }
>> > >
>> > >  /**
>> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>> > > struct qcom_rproc_ssr *ssr,
>> > >  {
>> > >  	ssr->name = ssr_name;
>> > >  	ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> > > +	ssr->subdev.prepare = ssr_notify_prepare;
>> > > +	ssr->subdev.start = ssr_notify_start;
>> > > +	ssr->subdev.stop = ssr_notify_stop;
>> >
>> > Now that I have a better understanding of what this patchset is doing, I
>> > realise
>> > my comments in patch 04 won't work.  To differentiate the subdevs of an
>> > rproc I
>> > suggest to wrap them in a generic structure with a type and an enum.
>> > That way
>> > you can differenciate between subdevices without having to add to the
>> > core.
>> Ok. I can try that.
>> >
>> > That being said, I don't understand what patches 5 and 6 are doing...
>> > Registering with the global ssr_notifiers allowed to gracefully shutdown
>> > all the
>> > MCUs in the system when one of them would go down.  But now that we are
>> > using
>> > the notifier on a per MCU, I really don't see why each subdev couldn't
>> > implement
>> > the right prepare/start/stop functions.
>> >
>> > Am I missing something here?
>> We only want kernel clients to be notified when the Remoteproc they 
>> are
>> interested
>> in changes state. For e.g. audio kernel driver should be notified when 
>> audio
>> processor goes down but it does not care about any other remoteproc.
>> If you are suggesting that these kernel clients be added as subdevices 
>> then
>> we will end up having many subdevices registered to each remoteproc. 
>> So we
>> implemented a notifier chain per Remoteproc. This keeps the SSR
>> notifications as
>> the subdevice per remoteproc, and all interested clients can register 
>> to it.
> 
> It seems like I am missing information...  Your are referring to 
> "kernel
> clients" and as such I must assume some drivers that are not part of 
> the
> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().  
> I must
Yes these are not part of remoteproc framework and they will register 
for notifications.
> also assume these drivers (or that functionality) are not yet upsream 
> because
> all I can see calling qcom_register_ssr_notifier() is 
> qcom_glink_ssr_probe().
Correct.These are not upstreamed.
> 
> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is 
> the glink
> device that driver is handling the same as the glink device registed in
> adsp_probe() and q6v5_probe()?
glink ssr driver will send out notifications to remoteprocs that have 
opened the
"glink_ssr" channel that some subsystem has gone down or booted up. This 
helps notify
neighboring subsystems about change in state of any other subsystem.
> 
>> >
>> >
>> > >  	ssr->subdev.unprepare = ssr_notify_unprepare;
>> > >  	ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>> > >  								GFP_KERNEL);
>> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> > > index e2f60cc..4be4478 100644
>> > > --- a/include/linux/remoteproc.h
>> > > +++ b/include/linux/remoteproc.h
>> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>> > >  };
>> > >
>> > >  /**
>> > > + * enum rproc_notif_type - Different stages of remoteproc
>> > > notifications
>> > > + * @RPROC_BEFORE_SHUTDOWN:	unprepare stage of  remoteproc
>> > > + * @RPROC_AFTER_SHUTDOWN:	stop stage of  remoteproc
>> > > + * @RPROC_BEFORE_POWERUP:	prepare stage of  remoteproc
>> > > + * @RPROC_AFTER_POWERUP:	start stage of  remoteproc
>> > > + */
>> > > +enum rproc_notif_type {
>> > > +	RPROC_BEFORE_SHUTDOWN,
>> > > +	RPROC_AFTER_SHUTDOWN,
>> > > +	RPROC_BEFORE_POWERUP,
>> > > +	RPROC_AFTER_POWERUP,
>> > > +	RPROC_MAX
>> > > +};
>> > > +
>> > > +/**
>> > >   * struct rproc - represents a physical remote processor device
>> > >   * @node: list node of this rproc object
>> > >   * @domain: iommu domain
>> > > --
>> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> > > a Linux Foundation Collaborative Project
>> > >
>> > > _______________________________________________
>> > > linux-arm-kernel mailing list
>> > > linux-arm-kernel@lists.infradead.org
>> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>
> On 2020-02-28 10:38, Mathieu Poirier wrote:
> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> > wrote:
> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >> > > The SSR subdevice only adds callback for the unprepare event. Add
> >> > > callbacks
> >> > > for unprepare, start and prepare events. The client driver for a
> >> > > particular
> >> > > remoteproc might be interested in knowing the status of the remoteproc
> >> > > while undergoing SSR, not just when the remoteproc has finished
> >> > > shutting
> >> > > down.
> >> > >
> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >> > > ---
> >> > >  drivers/remoteproc/qcom_common.c | 39
> >> > > +++++++++++++++++++++++++++++++++++----
> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/remoteproc/qcom_common.c
> >> > > b/drivers/remoteproc/qcom_common.c
> >> > > index 6714f27..6f04a5b 100644
> >> > > --- a/drivers/remoteproc/qcom_common.c
> >> > > +++ b/drivers/remoteproc/qcom_common.c
> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >> > >   *
> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> >> > > failure.
> >> > >   *
> >> > > - * This registers the @notify function as handler for restart
> >> > > notifications. As
> >> > > - * remote processors are stopped this function will be called, with
> >> > > the rproc
> >> > > - * pointer passed as a parameter.
> >> > > + * This registers the @notify function as handler for
> >> > > powerup/shutdown
> >> > > + * notifications. This function will be invoked inside the
> >> > > callbacks registered
> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
> >> > > parameter.
> >> > >   */
> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >> > > notifier_block *nb)
> >> > >  {
> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >> > > struct notifier_block *nb)
> >> > >  }
> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >> > >
> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >> > > +        return 0;
> >> > > +}
> >> > > +
> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >> > > +        return 0;
> >> > > +}
> >> > > +
> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >> > > crashed)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >> > > +}
> >> > > +
> >> > > +
> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >> > >  {
> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > >
> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >> > > *)ssr->name);
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >> > >  }
> >> > >
> >> > >  /**
> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >> > > struct qcom_rproc_ssr *ssr,
> >> > >  {
> >> > >          ssr->name = ssr_name;
> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
> >> > > +        ssr->subdev.start = ssr_notify_start;
> >> > > +        ssr->subdev.stop = ssr_notify_stop;
> >> >
> >> > Now that I have a better understanding of what this patchset is doing, I
> >> > realise
> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> >> > rproc I
> >> > suggest to wrap them in a generic structure with a type and an enum.
> >> > That way
> >> > you can differenciate between subdevices without having to add to the
> >> > core.
> >> Ok. I can try that.
> >> >
> >> > That being said, I don't understand what patches 5 and 6 are doing...
> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> >> > all the
> >> > MCUs in the system when one of them would go down.  But now that we are
> >> > using
> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
> >> > implement
> >> > the right prepare/start/stop functions.
> >> >
> >> > Am I missing something here?
> >> We only want kernel clients to be notified when the Remoteproc they
> >> are
> >> interested
> >> in changes state. For e.g. audio kernel driver should be notified when
> >> audio
> >> processor goes down but it does not care about any other remoteproc.
> >> If you are suggesting that these kernel clients be added as subdevices
> >> then
> >> we will end up having many subdevices registered to each remoteproc.
> >> So we
> >> implemented a notifier chain per Remoteproc. This keeps the SSR
> >> notifications as
> >> the subdevice per remoteproc, and all interested clients can register
> >> to it.
> >
> > It seems like I am missing information...  Your are referring to
> > "kernel
> > clients" and as such I must assume some drivers that are not part of
> > the
> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> > I must
> Yes these are not part of remoteproc framework and they will register
> for notifications.
> > also assume these drivers (or that functionality) are not yet upsream
> > because
> > all I can see calling qcom_register_ssr_notifier() is
> > qcom_glink_ssr_probe().
> Correct.These are not upstreamed.

Ok, things are starting to make sense.

> >
> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> > the glink
> > device that driver is handling the same as the glink device registed in
> > adsp_probe() and q6v5_probe()?
> glink ssr driver will send out notifications to remoteprocs that have
> opened the
> "glink_ssr" channel that some subsystem has gone down or booted up. This
> helps notify
> neighboring subsystems about change in state of any other subsystem.

I am still looking for an answer to my second question.

> >
> >> >
> >> >
> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >> > >                                                                  GFP_KERNEL);
> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> > > index e2f60cc..4be4478 100644
> >> > > --- a/include/linux/remoteproc.h
> >> > > +++ b/include/linux/remoteproc.h
> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >> > >  };
> >> > >
> >> > >  /**
> >> > > + * enum rproc_notif_type - Different stages of remoteproc
> >> > > notifications
> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >> > > + */
> >> > > +enum rproc_notif_type {
> >> > > +        RPROC_BEFORE_SHUTDOWN,
> >> > > +        RPROC_AFTER_SHUTDOWN,
> >> > > +        RPROC_BEFORE_POWERUP,
> >> > > +        RPROC_AFTER_POWERUP,
> >> > > +        RPROC_MAX
> >> > > +};
> >> > > +
> >> > > +/**
> >> > >   * struct rproc - represents a physical remote processor device
> >> > >   * @node: list node of this rproc object
> >> > >   * @domain: iommu domain
> >> > > --
> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> > > a Linux Foundation Collaborative Project
> >> > >
> >> > > _______________________________________________
> >> > > linux-arm-kernel mailing list
> >> > > linux-arm-kernel@lists.infradead.org
> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>
> On 2020-02-28 10:38, Mathieu Poirier wrote:
> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> > wrote:
> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >> > > The SSR subdevice only adds callback for the unprepare event. Add
> >> > > callbacks
> >> > > for unprepare, start and prepare events. The client driver for a
> >> > > particular
> >> > > remoteproc might be interested in knowing the status of the remoteproc
> >> > > while undergoing SSR, not just when the remoteproc has finished
> >> > > shutting
> >> > > down.
> >> > >
> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >> > > ---
> >> > >  drivers/remoteproc/qcom_common.c | 39
> >> > > +++++++++++++++++++++++++++++++++++----
> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/drivers/remoteproc/qcom_common.c
> >> > > b/drivers/remoteproc/qcom_common.c
> >> > > index 6714f27..6f04a5b 100644
> >> > > --- a/drivers/remoteproc/qcom_common.c
> >> > > +++ b/drivers/remoteproc/qcom_common.c
> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >> > >   *
> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> >> > > failure.
> >> > >   *
> >> > > - * This registers the @notify function as handler for restart
> >> > > notifications. As
> >> > > - * remote processors are stopped this function will be called, with
> >> > > the rproc
> >> > > - * pointer passed as a parameter.
> >> > > + * This registers the @notify function as handler for
> >> > > powerup/shutdown
> >> > > + * notifications. This function will be invoked inside the
> >> > > callbacks registered
> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
> >> > > parameter.
> >> > >   */
> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >> > > notifier_block *nb)
> >> > >  {
> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >> > > struct notifier_block *nb)
> >> > >  }
> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >> > >
> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >> > > +        return 0;
> >> > > +}
> >> > > +
> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >> > > +        return 0;
> >> > > +}
> >> > > +
> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >> > > crashed)
> >> > > +{
> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > > +
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >> > > +}
> >> > > +
> >> > > +
> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >> > >  {
> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> > >
> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >> > > *)ssr->name);
> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >> > >  }
> >> > >
> >> > >  /**
> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >> > > struct qcom_rproc_ssr *ssr,
> >> > >  {
> >> > >          ssr->name = ssr_name;
> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
> >> > > +        ssr->subdev.start = ssr_notify_start;
> >> > > +        ssr->subdev.stop = ssr_notify_stop;
> >> >
> >> > Now that I have a better understanding of what this patchset is doing, I
> >> > realise
> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> >> > rproc I
> >> > suggest to wrap them in a generic structure with a type and an enum.
> >> > That way
> >> > you can differenciate between subdevices without having to add to the
> >> > core.
> >> Ok. I can try that.
> >> >
> >> > That being said, I don't understand what patches 5 and 6 are doing...
> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> >> > all the
> >> > MCUs in the system when one of them would go down.  But now that we are
> >> > using
> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
> >> > implement
> >> > the right prepare/start/stop functions.
> >> >
> >> > Am I missing something here?
> >> We only want kernel clients to be notified when the Remoteproc they
> >> are
> >> interested
> >> in changes state. For e.g. audio kernel driver should be notified when
> >> audio
> >> processor goes down but it does not care about any other remoteproc.
> >> If you are suggesting that these kernel clients be added as subdevices
> >> then
> >> we will end up having many subdevices registered to each remoteproc.
> >> So we
> >> implemented a notifier chain per Remoteproc. This keeps the SSR
> >> notifications as
> >> the subdevice per remoteproc, and all interested clients can register
> >> to it.
> >
> > It seems like I am missing information...  Your are referring to
> > "kernel
> > clients" and as such I must assume some drivers that are not part of
> > the
> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> > I must
> Yes these are not part of remoteproc framework and they will register
> for notifications.
> > also assume these drivers (or that functionality) are not yet upsream
> > because
> > all I can see calling qcom_register_ssr_notifier() is
> > qcom_glink_ssr_probe().
> Correct.These are not upstreamed.

Ok, things are starting to make sense.

> >
> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> > the glink
> > device that driver is handling the same as the glink device registed in
> > adsp_probe() and q6v5_probe()?
> glink ssr driver will send out notifications to remoteprocs that have
> opened the
> "glink_ssr" channel that some subsystem has gone down or booted up. This
> helps notify
> neighboring subsystems about change in state of any other subsystem.

I am still looking for an answer to my second question.

> >
> >> >
> >> >
> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >> > >                                                                  GFP_KERNEL);
> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> > > index e2f60cc..4be4478 100644
> >> > > --- a/include/linux/remoteproc.h
> >> > > +++ b/include/linux/remoteproc.h
> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >> > >  };
> >> > >
> >> > >  /**
> >> > > + * enum rproc_notif_type - Different stages of remoteproc
> >> > > notifications
> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >> > > + */
> >> > > +enum rproc_notif_type {
> >> > > +        RPROC_BEFORE_SHUTDOWN,
> >> > > +        RPROC_AFTER_SHUTDOWN,
> >> > > +        RPROC_BEFORE_POWERUP,
> >> > > +        RPROC_AFTER_POWERUP,
> >> > > +        RPROC_MAX
> >> > > +};
> >> > > +
> >> > > +/**
> >> > >   * struct rproc - represents a physical remote processor device
> >> > >   * @node: list node of this rproc object
> >> > >   * @domain: iommu domain
> >> > > --
> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> > > a Linux Foundation Collaborative Project
> >> > >
> >> > > _______________________________________________
> >> > > linux-arm-kernel mailing list
> >> > > linux-arm-kernel@lists.infradead.org
> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-03-03 10:05, Mathieu Poirier wrote:
> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>> 
>> On 2020-02-28 10:38, Mathieu Poirier wrote:
>> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
>> > wrote:
>> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
>> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> >> > > The SSR subdevice only adds callback for the unprepare event. Add
>> >> > > callbacks
>> >> > > for unprepare, start and prepare events. The client driver for a
>> >> > > particular
>> >> > > remoteproc might be interested in knowing the status of the remoteproc
>> >> > > while undergoing SSR, not just when the remoteproc has finished
>> >> > > shutting
>> >> > > down.
>> >> > >
>> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> >> > > ---
>> >> > >  drivers/remoteproc/qcom_common.c | 39
>> >> > > +++++++++++++++++++++++++++++++++++----
>> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
>> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/remoteproc/qcom_common.c
>> >> > > b/drivers/remoteproc/qcom_common.c
>> >> > > index 6714f27..6f04a5b 100644
>> >> > > --- a/drivers/remoteproc/qcom_common.c
>> >> > > +++ b/drivers/remoteproc/qcom_common.c
>> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>> >> > >   *
>> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
>> >> > > failure.
>> >> > >   *
>> >> > > - * This registers the @notify function as handler for restart
>> >> > > notifications. As
>> >> > > - * remote processors are stopped this function will be called, with
>> >> > > the rproc
>> >> > > - * pointer passed as a parameter.
>> >> > > + * This registers the @notify function as handler for
>> >> > > powerup/shutdown
>> >> > > + * notifications. This function will be invoked inside the
>> >> > > callbacks registered
>> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
>> >> > > parameter.
>> >> > >   */
>> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>> >> > > notifier_block *nb)
>> >> > >  {
>> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>> >> > > struct notifier_block *nb)
>> >> > >  }
>> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> >> > >
>> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> >> > > +        return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> >> > > +        return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>> >> > > crashed)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> >> > > +}
>> >> > > +
>> >> > > +
>> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>> >> > >  {
>> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > >
>> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>> >> > > *)ssr->name);
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>> >> > >  }
>> >> > >
>> >> > >  /**
>> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>> >> > > struct qcom_rproc_ssr *ssr,
>> >> > >  {
>> >> > >          ssr->name = ssr_name;
>> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
>> >> > > +        ssr->subdev.start = ssr_notify_start;
>> >> > > +        ssr->subdev.stop = ssr_notify_stop;
>> >> >
>> >> > Now that I have a better understanding of what this patchset is doing, I
>> >> > realise
>> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
>> >> > rproc I
>> >> > suggest to wrap them in a generic structure with a type and an enum.
>> >> > That way
>> >> > you can differenciate between subdevices without having to add to the
>> >> > core.
>> >> Ok. I can try that.
>> >> >
>> >> > That being said, I don't understand what patches 5 and 6 are doing...
>> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
>> >> > all the
>> >> > MCUs in the system when one of them would go down.  But now that we are
>> >> > using
>> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
>> >> > implement
>> >> > the right prepare/start/stop functions.
>> >> >
>> >> > Am I missing something here?
>> >> We only want kernel clients to be notified when the Remoteproc they
>> >> are
>> >> interested
>> >> in changes state. For e.g. audio kernel driver should be notified when
>> >> audio
>> >> processor goes down but it does not care about any other remoteproc.
>> >> If you are suggesting that these kernel clients be added as subdevices
>> >> then
>> >> we will end up having many subdevices registered to each remoteproc.
>> >> So we
>> >> implemented a notifier chain per Remoteproc. This keeps the SSR
>> >> notifications as
>> >> the subdevice per remoteproc, and all interested clients can register
>> >> to it.
>> >
>> > It seems like I am missing information...  Your are referring to
>> > "kernel
>> > clients" and as such I must assume some drivers that are not part of
>> > the
>> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
>> > I must
>> Yes these are not part of remoteproc framework and they will register
>> for notifications.
>> > also assume these drivers (or that functionality) are not yet upsream
>> > because
>> > all I can see calling qcom_register_ssr_notifier() is
>> > qcom_glink_ssr_probe().
>> Correct.These are not upstreamed.
> 
> Ok, things are starting to make sense.
> 
>> >
>> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
>> > the glink
>> > device that driver is handling the same as the glink device registed in
>> > adsp_probe() and q6v5_probe()?
>> glink ssr driver will send out notifications to remoteprocs that have
>> opened the
>> "glink_ssr" channel that some subsystem has gone down or booted up. 
>> This
>> helps notify
>> neighboring subsystems about change in state of any other subsystem.
> 
> I am still looking for an answer to my second question.
Yes its the subdevice of the glink device that is registered in 
adsp_probe.
It uses the "glink_ssr" glink channel.
> 
>> >
>> >> >
>> >> >
>> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
>> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>> >> > >                                                                  GFP_KERNEL);
>> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> >> > > index e2f60cc..4be4478 100644
>> >> > > --- a/include/linux/remoteproc.h
>> >> > > +++ b/include/linux/remoteproc.h
>> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>> >> > >  };
>> >> > >
>> >> > >  /**
>> >> > > + * enum rproc_notif_type - Different stages of remoteproc
>> >> > > notifications
>> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
>> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
>> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
>> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
>> >> > > + */
>> >> > > +enum rproc_notif_type {
>> >> > > +        RPROC_BEFORE_SHUTDOWN,
>> >> > > +        RPROC_AFTER_SHUTDOWN,
>> >> > > +        RPROC_BEFORE_POWERUP,
>> >> > > +        RPROC_AFTER_POWERUP,
>> >> > > +        RPROC_MAX
>> >> > > +};
>> >> > > +
>> >> > > +/**
>> >> > >   * struct rproc - represents a physical remote processor device
>> >> > >   * @node: list node of this rproc object
>> >> > >   * @domain: iommu domain
>> >> > > --
>> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> >> > > a Linux Foundation Collaborative Project
>> >> > >
>> >> > > _______________________________________________
>> >> > > linux-arm-kernel mailing list
>> >> > > linux-arm-kernel@lists.infradead.org
>> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[rishabhb]

On 2020-03-03 10:05, Mathieu Poirier wrote:
> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>> 
>> On 2020-02-28 10:38, Mathieu Poirier wrote:
>> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
>> > wrote:
>> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
>> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>> >> > > The SSR subdevice only adds callback for the unprepare event. Add
>> >> > > callbacks
>> >> > > for unprepare, start and prepare events. The client driver for a
>> >> > > particular
>> >> > > remoteproc might be interested in knowing the status of the remoteproc
>> >> > > while undergoing SSR, not just when the remoteproc has finished
>> >> > > shutting
>> >> > > down.
>> >> > >
>> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>> >> > > ---
>> >> > >  drivers/remoteproc/qcom_common.c | 39
>> >> > > +++++++++++++++++++++++++++++++++++----
>> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
>> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
>> >> > >
>> >> > > diff --git a/drivers/remoteproc/qcom_common.c
>> >> > > b/drivers/remoteproc/qcom_common.c
>> >> > > index 6714f27..6f04a5b 100644
>> >> > > --- a/drivers/remoteproc/qcom_common.c
>> >> > > +++ b/drivers/remoteproc/qcom_common.c
>> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>> >> > >   *
>> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
>> >> > > failure.
>> >> > >   *
>> >> > > - * This registers the @notify function as handler for restart
>> >> > > notifications. As
>> >> > > - * remote processors are stopped this function will be called, with
>> >> > > the rproc
>> >> > > - * pointer passed as a parameter.
>> >> > > + * This registers the @notify function as handler for
>> >> > > powerup/shutdown
>> >> > > + * notifications. This function will be invoked inside the
>> >> > > callbacks registered
>> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
>> >> > > parameter.
>> >> > >   */
>> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>> >> > > notifier_block *nb)
>> >> > >  {
>> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>> >> > > struct notifier_block *nb)
>> >> > >  }
>> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>> >> > >
>> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>> >> > > +        return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
>> >> > > +        return 0;
>> >> > > +}
>> >> > > +
>> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>> >> > > crashed)
>> >> > > +{
>> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > > +
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>> >> > > +}
>> >> > > +
>> >> > > +
>> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>> >> > >  {
>> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>> >> > >
>> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>> >> > > *)ssr->name);
>> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>> >> > >  }
>> >> > >
>> >> > >  /**
>> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>> >> > > struct qcom_rproc_ssr *ssr,
>> >> > >  {
>> >> > >          ssr->name = ssr_name;
>> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
>> >> > > +        ssr->subdev.start = ssr_notify_start;
>> >> > > +        ssr->subdev.stop = ssr_notify_stop;
>> >> >
>> >> > Now that I have a better understanding of what this patchset is doing, I
>> >> > realise
>> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
>> >> > rproc I
>> >> > suggest to wrap them in a generic structure with a type and an enum.
>> >> > That way
>> >> > you can differenciate between subdevices without having to add to the
>> >> > core.
>> >> Ok. I can try that.
>> >> >
>> >> > That being said, I don't understand what patches 5 and 6 are doing...
>> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
>> >> > all the
>> >> > MCUs in the system when one of them would go down.  But now that we are
>> >> > using
>> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
>> >> > implement
>> >> > the right prepare/start/stop functions.
>> >> >
>> >> > Am I missing something here?
>> >> We only want kernel clients to be notified when the Remoteproc they
>> >> are
>> >> interested
>> >> in changes state. For e.g. audio kernel driver should be notified when
>> >> audio
>> >> processor goes down but it does not care about any other remoteproc.
>> >> If you are suggesting that these kernel clients be added as subdevices
>> >> then
>> >> we will end up having many subdevices registered to each remoteproc.
>> >> So we
>> >> implemented a notifier chain per Remoteproc. This keeps the SSR
>> >> notifications as
>> >> the subdevice per remoteproc, and all interested clients can register
>> >> to it.
>> >
>> > It seems like I am missing information...  Your are referring to
>> > "kernel
>> > clients" and as such I must assume some drivers that are not part of
>> > the
>> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
>> > I must
>> Yes these are not part of remoteproc framework and they will register
>> for notifications.
>> > also assume these drivers (or that functionality) are not yet upsream
>> > because
>> > all I can see calling qcom_register_ssr_notifier() is
>> > qcom_glink_ssr_probe().
>> Correct.These are not upstreamed.
> 
> Ok, things are starting to make sense.
> 
>> >
>> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
>> > the glink
>> > device that driver is handling the same as the glink device registed in
>> > adsp_probe() and q6v5_probe()?
>> glink ssr driver will send out notifications to remoteprocs that have
>> opened the
>> "glink_ssr" channel that some subsystem has gone down or booted up. 
>> This
>> helps notify
>> neighboring subsystems about change in state of any other subsystem.
> 
> I am still looking for an answer to my second question.
Yes its the subdevice of the glink device that is registered in 
adsp_probe.
It uses the "glink_ssr" glink channel.
> 
>> >
>> >> >
>> >> >
>> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
>> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>> >> > >                                                                  GFP_KERNEL);
>> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> >> > > index e2f60cc..4be4478 100644
>> >> > > --- a/include/linux/remoteproc.h
>> >> > > +++ b/include/linux/remoteproc.h
>> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>> >> > >  };
>> >> > >
>> >> > >  /**
>> >> > > + * enum rproc_notif_type - Different stages of remoteproc
>> >> > > notifications
>> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
>> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
>> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
>> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
>> >> > > + */
>> >> > > +enum rproc_notif_type {
>> >> > > +        RPROC_BEFORE_SHUTDOWN,
>> >> > > +        RPROC_AFTER_SHUTDOWN,
>> >> > > +        RPROC_BEFORE_POWERUP,
>> >> > > +        RPROC_AFTER_POWERUP,
>> >> > > +        RPROC_MAX
>> >> > > +};
>> >> > > +
>> >> > > +/**
>> >> > >   * struct rproc - represents a physical remote processor device
>> >> > >   * @node: list node of this rproc object
>> >> > >   * @domain: iommu domain
>> >> > > --
>> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> >> > > a Linux Foundation Collaborative Project
>> >> > >
>> >> > > _______________________________________________
>> >> > > linux-arm-kernel mailing list
>> >> > > linux-arm-kernel@lists.infradead.org
>> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
>
> On 2020-03-03 10:05, Mathieu Poirier wrote:
> > On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
> >>
> >> On 2020-02-28 10:38, Mathieu Poirier wrote:
> >> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> >> > wrote:
> >> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >> >> > > The SSR subdevice only adds callback for the unprepare event. Add
> >> >> > > callbacks
> >> >> > > for unprepare, start and prepare events. The client driver for a
> >> >> > > particular
> >> >> > > remoteproc might be interested in knowing the status of the remoteproc
> >> >> > > while undergoing SSR, not just when the remoteproc has finished
> >> >> > > shutting
> >> >> > > down.
> >> >> > >
> >> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >> >> > > ---
> >> >> > >  drivers/remoteproc/qcom_common.c | 39
> >> >> > > +++++++++++++++++++++++++++++++++++----
> >> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> >> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> >> >> > >
> >> >> > > diff --git a/drivers/remoteproc/qcom_common.c
> >> >> > > b/drivers/remoteproc/qcom_common.c
> >> >> > > index 6714f27..6f04a5b 100644
> >> >> > > --- a/drivers/remoteproc/qcom_common.c
> >> >> > > +++ b/drivers/remoteproc/qcom_common.c
> >> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >> >> > >   *
> >> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> >> >> > > failure.
> >> >> > >   *
> >> >> > > - * This registers the @notify function as handler for restart
> >> >> > > notifications. As
> >> >> > > - * remote processors are stopped this function will be called, with
> >> >> > > the rproc
> >> >> > > - * pointer passed as a parameter.
> >> >> > > + * This registers the @notify function as handler for
> >> >> > > powerup/shutdown
> >> >> > > + * notifications. This function will be invoked inside the
> >> >> > > callbacks registered
> >> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
> >> >> > > parameter.
> >> >> > >   */
> >> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >> >> > > notifier_block *nb)
> >> >> > >  {
> >> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >> >> > > struct notifier_block *nb)
> >> >> > >  }
> >> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >> >> > >
> >> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >> >> > > +        return 0;
> >> >> > > +}
> >> >> > > +
> >> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >> >> > > +        return 0;
> >> >> > > +}
> >> >> > > +
> >> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >> >> > > crashed)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >> >> > > +}
> >> >> > > +
> >> >> > > +
> >> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >> >> > >  {
> >> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > >
> >> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >> >> > > *)ssr->name);
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >> >> > >  }
> >> >> > >
> >> >> > >  /**
> >> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >> >> > > struct qcom_rproc_ssr *ssr,
> >> >> > >  {
> >> >> > >          ssr->name = ssr_name;
> >> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
> >> >> > > +        ssr->subdev.start = ssr_notify_start;
> >> >> > > +        ssr->subdev.stop = ssr_notify_stop;
> >> >> >
> >> >> > Now that I have a better understanding of what this patchset is doing, I
> >> >> > realise
> >> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> >> >> > rproc I
> >> >> > suggest to wrap them in a generic structure with a type and an enum.
> >> >> > That way
> >> >> > you can differenciate between subdevices without having to add to the
> >> >> > core.
> >> >> Ok. I can try that.
> >> >> >
> >> >> > That being said, I don't understand what patches 5 and 6 are doing...
> >> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> >> >> > all the
> >> >> > MCUs in the system when one of them would go down.  But now that we are
> >> >> > using
> >> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
> >> >> > implement
> >> >> > the right prepare/start/stop functions.
> >> >> >
> >> >> > Am I missing something here?
> >> >> We only want kernel clients to be notified when the Remoteproc they
> >> >> are
> >> >> interested
> >> >> in changes state. For e.g. audio kernel driver should be notified when
> >> >> audio
> >> >> processor goes down but it does not care about any other remoteproc.
> >> >> If you are suggesting that these kernel clients be added as subdevices
> >> >> then
> >> >> we will end up having many subdevices registered to each remoteproc.
> >> >> So we
> >> >> implemented a notifier chain per Remoteproc. This keeps the SSR
> >> >> notifications as
> >> >> the subdevice per remoteproc, and all interested clients can register
> >> >> to it.
> >> >
> >> > It seems like I am missing information...  Your are referring to
> >> > "kernel
> >> > clients" and as such I must assume some drivers that are not part of
> >> > the
> >> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> >> > I must
> >> Yes these are not part of remoteproc framework and they will register
> >> for notifications.
> >> > also assume these drivers (or that functionality) are not yet upsream
> >> > because
> >> > all I can see calling qcom_register_ssr_notifier() is
> >> > qcom_glink_ssr_probe().
> >> Correct.These are not upstreamed.
> >
> > Ok, things are starting to make sense.
> >
> >> >
> >> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> >> > the glink
> >> > device that driver is handling the same as the glink device registed in
> >> > adsp_probe() and q6v5_probe()?
> >> glink ssr driver will send out notifications to remoteprocs that have
> >> opened the
> >> "glink_ssr" channel that some subsystem has gone down or booted up.
> >> This
> >> helps notify
> >> neighboring subsystems about change in state of any other subsystem.
> >
> > I am still looking for an answer to my second question.
> Yes its the subdevice of the glink device that is registered in
> adsp_probe.
> It uses the "glink_ssr" glink channel.

Since this is confining events to a single MCU, I was mostly worried
about opening the "glink_ssr" channel for nothing but taking a step
back and thinking further on this, there might be other purposes for
the channel than only receiving notifications of other MCUs in the
system going down.

Please spin off a new revision of this set and I will take another look.

Thanks,
Mathieu

> >
> >> >
> >> >> >
> >> >> >
> >> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
> >> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >> >> > >                                                                  GFP_KERNEL);
> >> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> >> > > index e2f60cc..4be4478 100644
> >> >> > > --- a/include/linux/remoteproc.h
> >> >> > > +++ b/include/linux/remoteproc.h
> >> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >> >> > >  };
> >> >> > >
> >> >> > >  /**
> >> >> > > + * enum rproc_notif_type - Different stages of remoteproc
> >> >> > > notifications
> >> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >> >> > > + */
> >> >> > > +enum rproc_notif_type {
> >> >> > > +        RPROC_BEFORE_SHUTDOWN,
> >> >> > > +        RPROC_AFTER_SHUTDOWN,
> >> >> > > +        RPROC_BEFORE_POWERUP,
> >> >> > > +        RPROC_AFTER_POWERUP,
> >> >> > > +        RPROC_MAX
> >> >> > > +};
> >> >> > > +
> >> >> > > +/**
> >> >> > >   * struct rproc - represents a physical remote processor device
> >> >> > >   * @node: list node of this rproc object
> >> >> > >   * @domain: iommu domain
> >> >> > > --
> >> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> >> > > a Linux Foundation Collaborative Project
> >> >> > >
> >> >> > > _______________________________________________
> >> >> > > linux-arm-kernel mailing list
> >> >> > > linux-arm-kernel@lists.infradead.org
> >> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
>
> On 2020-03-03 10:05, Mathieu Poirier wrote:
> > On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
> >>
> >> On 2020-02-28 10:38, Mathieu Poirier wrote:
> >> > On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> >> > wrote:
> >> >> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >> >> > On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >> >> > > The SSR subdevice only adds callback for the unprepare event. Add
> >> >> > > callbacks
> >> >> > > for unprepare, start and prepare events. The client driver for a
> >> >> > > particular
> >> >> > > remoteproc might be interested in knowing the status of the remoteproc
> >> >> > > while undergoing SSR, not just when the remoteproc has finished
> >> >> > > shutting
> >> >> > > down.
> >> >> > >
> >> >> > > Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >> >> > > ---
> >> >> > >  drivers/remoteproc/qcom_common.c | 39
> >> >> > > +++++++++++++++++++++++++++++++++++----
> >> >> > >  include/linux/remoteproc.h       | 15 +++++++++++++++
> >> >> > >  2 files changed, 50 insertions(+), 4 deletions(-)
> >> >> > >
> >> >> > > diff --git a/drivers/remoteproc/qcom_common.c
> >> >> > > b/drivers/remoteproc/qcom_common.c
> >> >> > > index 6714f27..6f04a5b 100644
> >> >> > > --- a/drivers/remoteproc/qcom_common.c
> >> >> > > +++ b/drivers/remoteproc/qcom_common.c
> >> >> > > @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >> >> > >   *
> >> >> > >   * Returns pointer to srcu notifier head on success, ERR_PTR on
> >> >> > > failure.
> >> >> > >   *
> >> >> > > - * This registers the @notify function as handler for restart
> >> >> > > notifications. As
> >> >> > > - * remote processors are stopped this function will be called, with
> >> >> > > the rproc
> >> >> > > - * pointer passed as a parameter.
> >> >> > > + * This registers the @notify function as handler for
> >> >> > > powerup/shutdown
> >> >> > > + * notifications. This function will be invoked inside the
> >> >> > > callbacks registered
> >> >> > > + * for the ssr subdevice, with the rproc pointer passed as a
> >> >> > > parameter.
> >> >> > >   */
> >> >> > >  void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >> >> > > notifier_block *nb)
> >> >> > >  {
> >> >> > > @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >> >> > > struct notifier_block *nb)
> >> >> > >  }
> >> >> > >  EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >> >> > >
> >> >> > > +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >> >> > > +        return 0;
> >> >> > > +}
> >> >> > > +
> >> >> > > +static int ssr_notify_start(struct rproc_subdev *subdev)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >> >> > > +        return 0;
> >> >> > > +}
> >> >> > > +
> >> >> > > +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >> >> > > crashed)
> >> >> > > +{
> >> >> > > +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > > +
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >> >> > > +}
> >> >> > > +
> >> >> > > +
> >> >> > >  static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >> >> > >  {
> >> >> > >          struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >> >> > >
> >> >> > > -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >> >> > > *)ssr->name);
> >> >> > > +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >> >> > > +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >> >> > >  }
> >> >> > >
> >> >> > >  /**
> >> >> > > @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >> >> > > struct qcom_rproc_ssr *ssr,
> >> >> > >  {
> >> >> > >          ssr->name = ssr_name;
> >> >> > >          ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >> >> > > +        ssr->subdev.prepare = ssr_notify_prepare;
> >> >> > > +        ssr->subdev.start = ssr_notify_start;
> >> >> > > +        ssr->subdev.stop = ssr_notify_stop;
> >> >> >
> >> >> > Now that I have a better understanding of what this patchset is doing, I
> >> >> > realise
> >> >> > my comments in patch 04 won't work.  To differentiate the subdevs of an
> >> >> > rproc I
> >> >> > suggest to wrap them in a generic structure with a type and an enum.
> >> >> > That way
> >> >> > you can differenciate between subdevices without having to add to the
> >> >> > core.
> >> >> Ok. I can try that.
> >> >> >
> >> >> > That being said, I don't understand what patches 5 and 6 are doing...
> >> >> > Registering with the global ssr_notifiers allowed to gracefully shutdown
> >> >> > all the
> >> >> > MCUs in the system when one of them would go down.  But now that we are
> >> >> > using
> >> >> > the notifier on a per MCU, I really don't see why each subdev couldn't
> >> >> > implement
> >> >> > the right prepare/start/stop functions.
> >> >> >
> >> >> > Am I missing something here?
> >> >> We only want kernel clients to be notified when the Remoteproc they
> >> >> are
> >> >> interested
> >> >> in changes state. For e.g. audio kernel driver should be notified when
> >> >> audio
> >> >> processor goes down but it does not care about any other remoteproc.
> >> >> If you are suggesting that these kernel clients be added as subdevices
> >> >> then
> >> >> we will end up having many subdevices registered to each remoteproc.
> >> >> So we
> >> >> implemented a notifier chain per Remoteproc. This keeps the SSR
> >> >> notifications as
> >> >> the subdevice per remoteproc, and all interested clients can register
> >> >> to it.
> >> >
> >> > It seems like I am missing information...  Your are referring to
> >> > "kernel
> >> > clients" and as such I must assume some drivers that are not part of
> >> > the
> >> > remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> >> > I must
> >> Yes these are not part of remoteproc framework and they will register
> >> for notifications.
> >> > also assume these drivers (or that functionality) are not yet upsream
> >> > because
> >> > all I can see calling qcom_register_ssr_notifier() is
> >> > qcom_glink_ssr_probe().
> >> Correct.These are not upstreamed.
> >
> > Ok, things are starting to make sense.
> >
> >> >
> >> > Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> >> > the glink
> >> > device that driver is handling the same as the glink device registed in
> >> > adsp_probe() and q6v5_probe()?
> >> glink ssr driver will send out notifications to remoteprocs that have
> >> opened the
> >> "glink_ssr" channel that some subsystem has gone down or booted up.
> >> This
> >> helps notify
> >> neighboring subsystems about change in state of any other subsystem.
> >
> > I am still looking for an answer to my second question.
> Yes its the subdevice of the glink device that is registered in
> adsp_probe.
> It uses the "glink_ssr" glink channel.

Since this is confining events to a single MCU, I was mostly worried
about opening the "glink_ssr" channel for nothing but taking a step
back and thinking further on this, there might be other purposes for
the channel than only receiving notifications of other MCUs in the
system going down.

Please spin off a new revision of this set and I will take another look.

Thanks,
Mathieu

> >
> >> >
> >> >> >
> >> >> >
> >> >> > >          ssr->subdev.unprepare = ssr_notify_unprepare;
> >> >> > >          ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >> >> > >                                                                  GFP_KERNEL);
> >> >> > > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> >> > > index e2f60cc..4be4478 100644
> >> >> > > --- a/include/linux/remoteproc.h
> >> >> > > +++ b/include/linux/remoteproc.h
> >> >> > > @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >> >> > >  };
> >> >> > >
> >> >> > >  /**
> >> >> > > + * enum rproc_notif_type - Different stages of remoteproc
> >> >> > > notifications
> >> >> > > + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >> >> > > + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >> >> > > + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >> >> > > + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >> >> > > + */
> >> >> > > +enum rproc_notif_type {
> >> >> > > +        RPROC_BEFORE_SHUTDOWN,
> >> >> > > +        RPROC_AFTER_SHUTDOWN,
> >> >> > > +        RPROC_BEFORE_POWERUP,
> >> >> > > +        RPROC_AFTER_POWERUP,
> >> >> > > +        RPROC_MAX
> >> >> > > +};
> >> >> > > +
> >> >> > > +/**
> >> >> > >   * struct rproc - represents a physical remote processor device
> >> >> > >   * @node: list node of this rproc object
> >> >> > >   * @domain: iommu domain
> >> >> > > --
> >> >> > > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >> >> > > a Linux Foundation Collaborative Project
> >> >> > >
> >> >> > > _______________________________________________
> >> >> > > linux-arm-kernel mailing list
> >> >> > > linux-arm-kernel@lists.infradead.org
> >> >> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Siddharth Gupta]

On 3/9/2020 10:34 AM, Mathieu Poirier wrote:

> On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
>> On 2020-03-03 10:05, Mathieu Poirier wrote:
>>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>>>> On 2020-02-28 10:38, Mathieu Poirier wrote:
>>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
>>>>> wrote:
>>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote:
>>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add
>>>>>>>> callbacks
>>>>>>>> for unprepare, start and prepare events. The client driver for a
>>>>>>>> particular
>>>>>>>> remoteproc might be interested in knowing the status of the remoteproc
>>>>>>>> while undergoing SSR, not just when the remoteproc has finished
>>>>>>>> shutting
>>>>>>>> down.
>>>>>>>>
>>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>>>>>>>> ---
>>>>>>>>   drivers/remoteproc/qcom_common.c | 39
>>>>>>>> +++++++++++++++++++++++++++++++++++----
>>>>>>>>   include/linux/remoteproc.h       | 15 +++++++++++++++
>>>>>>>>   2 files changed, 50 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c
>>>>>>>> b/drivers/remoteproc/qcom_common.c
>>>>>>>> index 6714f27..6f04a5b 100644
>>>>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>>>>>>>>    *
>>>>>>>>    * Returns pointer to srcu notifier head on success, ERR_PTR on
>>>>>>>> failure.
>>>>>>>>    *
>>>>>>>> - * This registers the @notify function as handler for restart
>>>>>>>> notifications. As
>>>>>>>> - * remote processors are stopped this function will be called, with
>>>>>>>> the rproc
>>>>>>>> - * pointer passed as a parameter.
>>>>>>>> + * This registers the @notify function as handler for
>>>>>>>> powerup/shutdown
>>>>>>>> + * notifications. This function will be invoked inside the
>>>>>>>> callbacks registered
>>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a
>>>>>>>> parameter.
>>>>>>>>    */
>>>>>>>>   void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>>>>>>>> notifier_block *nb)
>>>>>>>>   {
>>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>>>>>>>> struct notifier_block *nb)
>>>>>>>>   }
>>>>>>>>   EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>>>>>>>>
>>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>>>>>>>> +        return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
>>>>>>>> +        return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>>>>>>>> crashed)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +
>>>>>>>>   static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>>>>>>>>   {
>>>>>>>>           struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>>
>>>>>>>> -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>>>>>>>> *)ssr->name);
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>>>>>>>>   }
>>>>>>>>
>>>>>>>>   /**
>>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>>>>>>>> struct qcom_rproc_ssr *ssr,
>>>>>>>>   {
>>>>>>>>           ssr->name = ssr_name;
>>>>>>>>           ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>>>>>>>> +        ssr->subdev.prepare = ssr_notify_prepare;
>>>>>>>> +        ssr->subdev.start = ssr_notify_start;
>>>>>>>> +        ssr->subdev.stop = ssr_notify_stop;
>>>>>>> Now that I have a better understanding of what this patchset is doing, I
>>>>>>> realise
>>>>>>> my comments in patch 04 won't work.  To differentiate the subdevs of an
>>>>>>> rproc I
>>>>>>> suggest to wrap them in a generic structure with a type and an enum.
>>>>>>> That way
>>>>>>> you can differenciate between subdevices without having to add to the
>>>>>>> core.

While creating a new revision of the patchset we tried to implement 
this, but a similar issue comes
up. If at a later point we wish to utilize the functionality of some 
common subdevice (not the case
right now, but potentially), we might run into a similar problem of 
accessing illegal memory using
container_of. I think it might be a better idea to introduce the name in 
the subdevice structure over
having a potential security bug. What do you think?

Thanks,
Siddharth

>>>>>> Ok. I can try that.
>>>>>>> That being said, I don't understand what patches 5 and 6 are doing...
>>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown
>>>>>>> all the
>>>>>>> MCUs in the system when one of them would go down.  But now that we are
>>>>>>> using
>>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't
>>>>>>> implement
>>>>>>> the right prepare/start/stop functions.
>>>>>>>
>>>>>>> Am I missing something here?
>>>>>> We only want kernel clients to be notified when the Remoteproc they
>>>>>> are
>>>>>> interested
>>>>>> in changes state. For e.g. audio kernel driver should be notified when
>>>>>> audio
>>>>>> processor goes down but it does not care about any other remoteproc.
>>>>>> If you are suggesting that these kernel clients be added as subdevices
>>>>>> then
>>>>>> we will end up having many subdevices registered to each remoteproc.
>>>>>> So we
>>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR
>>>>>> notifications as
>>>>>> the subdevice per remoteproc, and all interested clients can register
>>>>>> to it.
>>>>> It seems like I am missing information...  Your are referring to
>>>>> "kernel
>>>>> clients" and as such I must assume some drivers that are not part of
>>>>> the
>>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
>>>>> I must
>>>> Yes these are not part of remoteproc framework and they will register
>>>> for notifications.
>>>>> also assume these drivers (or that functionality) are not yet upsream
>>>>> because
>>>>> all I can see calling qcom_register_ssr_notifier() is
>>>>> qcom_glink_ssr_probe().
>>>> Correct.These are not upstreamed.
>>> Ok, things are starting to make sense.
>>>
>>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
>>>>> the glink
>>>>> device that driver is handling the same as the glink device registed in
>>>>> adsp_probe() and q6v5_probe()?
>>>> glink ssr driver will send out notifications to remoteprocs that have
>>>> opened the
>>>> "glink_ssr" channel that some subsystem has gone down or booted up.
>>>> This
>>>> helps notify
>>>> neighboring subsystems about change in state of any other subsystem.
>>> I am still looking for an answer to my second question.
>> Yes its the subdevice of the glink device that is registered in
>> adsp_probe.
>> It uses the "glink_ssr" glink channel.
> Since this is confining events to a single MCU, I was mostly worried
> about opening the "glink_ssr" channel for nothing but taking a step
> back and thinking further on this, there might be other purposes for
> the channel than only receiving notifications of other MCUs in the
> system going down.
>
> Please spin off a new revision of this set and I will take another look.
>
> Thanks,
> Mathieu
>
>>>>>>>
>>>>>>>>           ssr->subdev.unprepare = ssr_notify_unprepare;
>>>>>>>>           ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>>>>>>>>                                                                   GFP_KERNEL);
>>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>>>> index e2f60cc..4be4478 100644
>>>>>>>> --- a/include/linux/remoteproc.h
>>>>>>>> +++ b/include/linux/remoteproc.h
>>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>>>>>>>>   };
>>>>>>>>
>>>>>>>>   /**
>>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc
>>>>>>>> notifications
>>>>>>>> + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
>>>>>>>> + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
>>>>>>>> + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
>>>>>>>> + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
>>>>>>>> + */
>>>>>>>> +enum rproc_notif_type {
>>>>>>>> +        RPROC_BEFORE_SHUTDOWN,
>>>>>>>> +        RPROC_AFTER_SHUTDOWN,
>>>>>>>> +        RPROC_BEFORE_POWERUP,
>>>>>>>> +        RPROC_AFTER_POWERUP,
>>>>>>>> +        RPROC_MAX
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>>    * struct rproc - represents a physical remote processor device
>>>>>>>>    * @node: list node of this rproc object
>>>>>>>>    * @domain: iommu domain
>>>>>>>> --
>>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>>>>>> a Linux Foundation Collaborative Project
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> linux-arm-kernel mailing list
>>>>>>>> linux-arm-kernel@lists.infradead.org
>>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Siddharth Gupta]

On 3/9/2020 10:34 AM, Mathieu Poirier wrote:

> On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
>> On 2020-03-03 10:05, Mathieu Poirier wrote:
>>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
>>>> On 2020-02-28 10:38, Mathieu Poirier wrote:
>>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
>>>>> wrote:
>>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote:
>>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
>>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add
>>>>>>>> callbacks
>>>>>>>> for unprepare, start and prepare events. The client driver for a
>>>>>>>> particular
>>>>>>>> remoteproc might be interested in knowing the status of the remoteproc
>>>>>>>> while undergoing SSR, not just when the remoteproc has finished
>>>>>>>> shutting
>>>>>>>> down.
>>>>>>>>
>>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
>>>>>>>> ---
>>>>>>>>   drivers/remoteproc/qcom_common.c | 39
>>>>>>>> +++++++++++++++++++++++++++++++++++----
>>>>>>>>   include/linux/remoteproc.h       | 15 +++++++++++++++
>>>>>>>>   2 files changed, 50 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c
>>>>>>>> b/drivers/remoteproc/qcom_common.c
>>>>>>>> index 6714f27..6f04a5b 100644
>>>>>>>> --- a/drivers/remoteproc/qcom_common.c
>>>>>>>> +++ b/drivers/remoteproc/qcom_common.c
>>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
>>>>>>>>    *
>>>>>>>>    * Returns pointer to srcu notifier head on success, ERR_PTR on
>>>>>>>> failure.
>>>>>>>>    *
>>>>>>>> - * This registers the @notify function as handler for restart
>>>>>>>> notifications. As
>>>>>>>> - * remote processors are stopped this function will be called, with
>>>>>>>> the rproc
>>>>>>>> - * pointer passed as a parameter.
>>>>>>>> + * This registers the @notify function as handler for
>>>>>>>> powerup/shutdown
>>>>>>>> + * notifications. This function will be invoked inside the
>>>>>>>> callbacks registered
>>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a
>>>>>>>> parameter.
>>>>>>>>    */
>>>>>>>>   void *qcom_register_ssr_notifier(struct rproc *rproc, struct
>>>>>>>> notifier_block *nb)
>>>>>>>>   {
>>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
>>>>>>>> struct notifier_block *nb)
>>>>>>>>   }
>>>>>>>>   EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
>>>>>>>>
>>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
>>>>>>>> +        return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
>>>>>>>> +        return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
>>>>>>>> crashed)
>>>>>>>> +{
>>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>> +
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +
>>>>>>>>   static void ssr_notify_unprepare(struct rproc_subdev *subdev)
>>>>>>>>   {
>>>>>>>>           struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
>>>>>>>>
>>>>>>>> -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
>>>>>>>> *)ssr->name);
>>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
>>>>>>>> +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
>>>>>>>>   }
>>>>>>>>
>>>>>>>>   /**
>>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
>>>>>>>> struct qcom_rproc_ssr *ssr,
>>>>>>>>   {
>>>>>>>>           ssr->name = ssr_name;
>>>>>>>>           ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
>>>>>>>> +        ssr->subdev.prepare = ssr_notify_prepare;
>>>>>>>> +        ssr->subdev.start = ssr_notify_start;
>>>>>>>> +        ssr->subdev.stop = ssr_notify_stop;
>>>>>>> Now that I have a better understanding of what this patchset is doing, I
>>>>>>> realise
>>>>>>> my comments in patch 04 won't work.  To differentiate the subdevs of an
>>>>>>> rproc I
>>>>>>> suggest to wrap them in a generic structure with a type and an enum.
>>>>>>> That way
>>>>>>> you can differenciate between subdevices without having to add to the
>>>>>>> core.

While creating a new revision of the patchset we tried to implement 
this, but a similar issue comes
up. If at a later point we wish to utilize the functionality of some 
common subdevice (not the case
right now, but potentially), we might run into a similar problem of 
accessing illegal memory using
container_of. I think it might be a better idea to introduce the name in 
the subdevice structure over
having a potential security bug. What do you think?

Thanks,
Siddharth

>>>>>> Ok. I can try that.
>>>>>>> That being said, I don't understand what patches 5 and 6 are doing...
>>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown
>>>>>>> all the
>>>>>>> MCUs in the system when one of them would go down.  But now that we are
>>>>>>> using
>>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't
>>>>>>> implement
>>>>>>> the right prepare/start/stop functions.
>>>>>>>
>>>>>>> Am I missing something here?
>>>>>> We only want kernel clients to be notified when the Remoteproc they
>>>>>> are
>>>>>> interested
>>>>>> in changes state. For e.g. audio kernel driver should be notified when
>>>>>> audio
>>>>>> processor goes down but it does not care about any other remoteproc.
>>>>>> If you are suggesting that these kernel clients be added as subdevices
>>>>>> then
>>>>>> we will end up having many subdevices registered to each remoteproc.
>>>>>> So we
>>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR
>>>>>> notifications as
>>>>>> the subdevice per remoteproc, and all interested clients can register
>>>>>> to it.
>>>>> It seems like I am missing information...  Your are referring to
>>>>> "kernel
>>>>> clients" and as such I must assume some drivers that are not part of
>>>>> the
>>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
>>>>> I must
>>>> Yes these are not part of remoteproc framework and they will register
>>>> for notifications.
>>>>> also assume these drivers (or that functionality) are not yet upsream
>>>>> because
>>>>> all I can see calling qcom_register_ssr_notifier() is
>>>>> qcom_glink_ssr_probe().
>>>> Correct.These are not upstreamed.
>>> Ok, things are starting to make sense.
>>>
>>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
>>>>> the glink
>>>>> device that driver is handling the same as the glink device registed in
>>>>> adsp_probe() and q6v5_probe()?
>>>> glink ssr driver will send out notifications to remoteprocs that have
>>>> opened the
>>>> "glink_ssr" channel that some subsystem has gone down or booted up.
>>>> This
>>>> helps notify
>>>> neighboring subsystems about change in state of any other subsystem.
>>> I am still looking for an answer to my second question.
>> Yes its the subdevice of the glink device that is registered in
>> adsp_probe.
>> It uses the "glink_ssr" glink channel.
> Since this is confining events to a single MCU, I was mostly worried
> about opening the "glink_ssr" channel for nothing but taking a step
> back and thinking further on this, there might be other purposes for
> the channel than only receiving notifications of other MCUs in the
> system going down.
>
> Please spin off a new revision of this set and I will take another look.
>
> Thanks,
> Mathieu
>
>>>>>>>
>>>>>>>>           ssr->subdev.unprepare = ssr_notify_unprepare;
>>>>>>>>           ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
>>>>>>>>                                                                   GFP_KERNEL);
>>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>>>> index e2f60cc..4be4478 100644
>>>>>>>> --- a/include/linux/remoteproc.h
>>>>>>>> +++ b/include/linux/remoteproc.h
>>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
>>>>>>>>   };
>>>>>>>>
>>>>>>>>   /**
>>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc
>>>>>>>> notifications
>>>>>>>> + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
>>>>>>>> + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
>>>>>>>> + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
>>>>>>>> + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
>>>>>>>> + */
>>>>>>>> +enum rproc_notif_type {
>>>>>>>> +        RPROC_BEFORE_SHUTDOWN,
>>>>>>>> +        RPROC_AFTER_SHUTDOWN,
>>>>>>>> +        RPROC_BEFORE_POWERUP,
>>>>>>>> +        RPROC_AFTER_POWERUP,
>>>>>>>> +        RPROC_MAX
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>>    * struct rproc - represents a physical remote processor device
>>>>>>>>    * @node: list node of this rproc object
>>>>>>>>    * @domain: iommu domain
>>>>>>>> --
>>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>>>>>>>> a Linux Foundation Collaborative Project
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> linux-arm-kernel mailing list
>>>>>>>> linux-arm-kernel@lists.infradead.org
>>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Wed, 1 Apr 2020 at 19:01, Siddharth Gupta <sidgup@codeaurora.org> wrote:
>
> On 3/9/2020 10:34 AM, Mathieu Poirier wrote:
>
> > On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
> >> On 2020-03-03 10:05, Mathieu Poirier wrote:
> >>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
> >>>> On 2020-02-28 10:38, Mathieu Poirier wrote:
> >>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> >>>>> wrote:
> >>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add
> >>>>>>>> callbacks
> >>>>>>>> for unprepare, start and prepare events. The client driver for a
> >>>>>>>> particular
> >>>>>>>> remoteproc might be interested in knowing the status of the remoteproc
> >>>>>>>> while undergoing SSR, not just when the remoteproc has finished
> >>>>>>>> shutting
> >>>>>>>> down.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >>>>>>>> ---
> >>>>>>>>   drivers/remoteproc/qcom_common.c | 39
> >>>>>>>> +++++++++++++++++++++++++++++++++++----
> >>>>>>>>   include/linux/remoteproc.h       | 15 +++++++++++++++
> >>>>>>>>   2 files changed, 50 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c
> >>>>>>>> b/drivers/remoteproc/qcom_common.c
> >>>>>>>> index 6714f27..6f04a5b 100644
> >>>>>>>> --- a/drivers/remoteproc/qcom_common.c
> >>>>>>>> +++ b/drivers/remoteproc/qcom_common.c
> >>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >>>>>>>>    *
> >>>>>>>>    * Returns pointer to srcu notifier head on success, ERR_PTR on
> >>>>>>>> failure.
> >>>>>>>>    *
> >>>>>>>> - * This registers the @notify function as handler for restart
> >>>>>>>> notifications. As
> >>>>>>>> - * remote processors are stopped this function will be called, with
> >>>>>>>> the rproc
> >>>>>>>> - * pointer passed as a parameter.
> >>>>>>>> + * This registers the @notify function as handler for
> >>>>>>>> powerup/shutdown
> >>>>>>>> + * notifications. This function will be invoked inside the
> >>>>>>>> callbacks registered
> >>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a
> >>>>>>>> parameter.
> >>>>>>>>    */
> >>>>>>>>   void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >>>>>>>> notifier_block *nb)
> >>>>>>>>   {
> >>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >>>>>>>> struct notifier_block *nb)
> >>>>>>>>   }
> >>>>>>>>   EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >>>>>>>>
> >>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >>>>>>>> crashed)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>>   static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >>>>>>>>   {
> >>>>>>>>           struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>>
> >>>>>>>> -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >>>>>>>> *)ssr->name);
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >>>>>>>>   }
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >>>>>>>> struct qcom_rproc_ssr *ssr,
> >>>>>>>>   {
> >>>>>>>>           ssr->name = ssr_name;
> >>>>>>>>           ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >>>>>>>> +        ssr->subdev.prepare = ssr_notify_prepare;
> >>>>>>>> +        ssr->subdev.start = ssr_notify_start;
> >>>>>>>> +        ssr->subdev.stop = ssr_notify_stop;
> >>>>>>> Now that I have a better understanding of what this patchset is doing, I
> >>>>>>> realise
> >>>>>>> my comments in patch 04 won't work.  To differentiate the subdevs of an
> >>>>>>> rproc I
> >>>>>>> suggest to wrap them in a generic structure with a type and an enum.
> >>>>>>> That way
> >>>>>>> you can differenciate between subdevices without having to add to the
> >>>>>>> core.
>
> While creating a new revision of the patchset we tried to implement
> this, but a similar issue comes
> up. If at a later point we wish to utilize the functionality of some
> common subdevice (not the case
> right now, but potentially), we might run into a similar problem of
> accessing illegal memory using
> container_of. I think it might be a better idea to introduce the name in
> the subdevice structure over
> having a potential security bug. What do you think?

I trust that you have given this an honest try but found potential
problems that I can't foresee due to the lack of insight on your
operating environment.  Please move forward with the addition of a new
"name" field to the rproc_subdev structure.

>
> Thanks,
> Siddharth
>
> >>>>>> Ok. I can try that.
> >>>>>>> That being said, I don't understand what patches 5 and 6 are doing...
> >>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown
> >>>>>>> all the
> >>>>>>> MCUs in the system when one of them would go down.  But now that we are
> >>>>>>> using
> >>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't
> >>>>>>> implement
> >>>>>>> the right prepare/start/stop functions.
> >>>>>>>
> >>>>>>> Am I missing something here?
> >>>>>> We only want kernel clients to be notified when the Remoteproc they
> >>>>>> are
> >>>>>> interested
> >>>>>> in changes state. For e.g. audio kernel driver should be notified when
> >>>>>> audio
> >>>>>> processor goes down but it does not care about any other remoteproc.
> >>>>>> If you are suggesting that these kernel clients be added as subdevices
> >>>>>> then
> >>>>>> we will end up having many subdevices registered to each remoteproc.
> >>>>>> So we
> >>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR
> >>>>>> notifications as
> >>>>>> the subdevice per remoteproc, and all interested clients can register
> >>>>>> to it.
> >>>>> It seems like I am missing information...  Your are referring to
> >>>>> "kernel
> >>>>> clients" and as such I must assume some drivers that are not part of
> >>>>> the
> >>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> >>>>> I must
> >>>> Yes these are not part of remoteproc framework and they will register
> >>>> for notifications.
> >>>>> also assume these drivers (or that functionality) are not yet upsream
> >>>>> because
> >>>>> all I can see calling qcom_register_ssr_notifier() is
> >>>>> qcom_glink_ssr_probe().
> >>>> Correct.These are not upstreamed.
> >>> Ok, things are starting to make sense.
> >>>
> >>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> >>>>> the glink
> >>>>> device that driver is handling the same as the glink device registed in
> >>>>> adsp_probe() and q6v5_probe()?
> >>>> glink ssr driver will send out notifications to remoteprocs that have
> >>>> opened the
> >>>> "glink_ssr" channel that some subsystem has gone down or booted up.
> >>>> This
> >>>> helps notify
> >>>> neighboring subsystems about change in state of any other subsystem.
> >>> I am still looking for an answer to my second question.
> >> Yes its the subdevice of the glink device that is registered in
> >> adsp_probe.
> >> It uses the "glink_ssr" glink channel.
> > Since this is confining events to a single MCU, I was mostly worried
> > about opening the "glink_ssr" channel for nothing but taking a step
> > back and thinking further on this, there might be other purposes for
> > the channel than only receiving notifications of other MCUs in the
> > system going down.
> >
> > Please spin off a new revision of this set and I will take another look.
> >
> > Thanks,
> > Mathieu
> >
> >>>>>>>
> >>>>>>>>           ssr->subdev.unprepare = ssr_notify_unprepare;
> >>>>>>>>           ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >>>>>>>>                                                                   GFP_KERNEL);
> >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>>>>> index e2f60cc..4be4478 100644
> >>>>>>>> --- a/include/linux/remoteproc.h
> >>>>>>>> +++ b/include/linux/remoteproc.h
> >>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >>>>>>>>   };
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc
> >>>>>>>> notifications
> >>>>>>>> + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >>>>>>>> + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >>>>>>>> + */
> >>>>>>>> +enum rproc_notif_type {
> >>>>>>>> +        RPROC_BEFORE_SHUTDOWN,
> >>>>>>>> +        RPROC_AFTER_SHUTDOWN,
> >>>>>>>> +        RPROC_BEFORE_POWERUP,
> >>>>>>>> +        RPROC_AFTER_POWERUP,
> >>>>>>>> +        RPROC_MAX
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>>    * struct rproc - represents a physical remote processor device
> >>>>>>>>    * @node: list node of this rproc object
> >>>>>>>>    * @domain: iommu domain
> >>>>>>>> --
> >>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >>>>>>>> a Linux Foundation Collaborative Project
> >>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> linux-arm-kernel mailing list
> >>>>>>>> linux-arm-kernel@lists.infradead.org
> >>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] remoteproc: qcom: Add notification types to SSR

[Mathieu Poirier]

On Wed, 1 Apr 2020 at 19:01, Siddharth Gupta <sidgup@codeaurora.org> wrote:
>
> On 3/9/2020 10:34 AM, Mathieu Poirier wrote:
>
> > On Tue, 3 Mar 2020 at 16:30, <rishabhb@codeaurora.org> wrote:
> >> On 2020-03-03 10:05, Mathieu Poirier wrote:
> >>> On Mon, 2 Mar 2020 at 13:54, <rishabhb@codeaurora.org> wrote:
> >>>> On 2020-02-28 10:38, Mathieu Poirier wrote:
> >>>>> On Thu, Feb 27, 2020 at 04:00:21PM -0800, rishabhb@codeaurora.org
> >>>>> wrote:
> >>>>>> On 2020-02-27 13:59, Mathieu Poirier wrote:
> >>>>>>> On Wed, Feb 19, 2020 at 06:57:45PM -0800, Siddharth Gupta wrote:
> >>>>>>>> The SSR subdevice only adds callback for the unprepare event. Add
> >>>>>>>> callbacks
> >>>>>>>> for unprepare, start and prepare events. The client driver for a
> >>>>>>>> particular
> >>>>>>>> remoteproc might be interested in knowing the status of the remoteproc
> >>>>>>>> while undergoing SSR, not just when the remoteproc has finished
> >>>>>>>> shutting
> >>>>>>>> down.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Siddharth Gupta <sidgup@codeaurora.org>
> >>>>>>>> ---
> >>>>>>>>   drivers/remoteproc/qcom_common.c | 39
> >>>>>>>> +++++++++++++++++++++++++++++++++++----
> >>>>>>>>   include/linux/remoteproc.h       | 15 +++++++++++++++
> >>>>>>>>   2 files changed, 50 insertions(+), 4 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/remoteproc/qcom_common.c
> >>>>>>>> b/drivers/remoteproc/qcom_common.c
> >>>>>>>> index 6714f27..6f04a5b 100644
> >>>>>>>> --- a/drivers/remoteproc/qcom_common.c
> >>>>>>>> +++ b/drivers/remoteproc/qcom_common.c
> >>>>>>>> @@ -183,9 +183,9 @@ EXPORT_SYMBOL_GPL(qcom_remove_smd_subdev);
> >>>>>>>>    *
> >>>>>>>>    * Returns pointer to srcu notifier head on success, ERR_PTR on
> >>>>>>>> failure.
> >>>>>>>>    *
> >>>>>>>> - * This registers the @notify function as handler for restart
> >>>>>>>> notifications. As
> >>>>>>>> - * remote processors are stopped this function will be called, with
> >>>>>>>> the rproc
> >>>>>>>> - * pointer passed as a parameter.
> >>>>>>>> + * This registers the @notify function as handler for
> >>>>>>>> powerup/shutdown
> >>>>>>>> + * notifications. This function will be invoked inside the
> >>>>>>>> callbacks registered
> >>>>>>>> + * for the ssr subdevice, with the rproc pointer passed as a
> >>>>>>>> parameter.
> >>>>>>>>    */
> >>>>>>>>   void *qcom_register_ssr_notifier(struct rproc *rproc, struct
> >>>>>>>> notifier_block *nb)
> >>>>>>>>   {
> >>>>>>>> @@ -227,11 +227,39 @@ int qcom_unregister_ssr_notifier(void *notify,
> >>>>>>>> struct notifier_block *nb)
> >>>>>>>>   }
> >>>>>>>>   EXPORT_SYMBOL_GPL(qcom_unregister_ssr_notifier);
> >>>>>>>>
> >>>>>>>> +static int ssr_notify_prepare(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static int ssr_notify_start(struct rproc_subdev *subdev)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_POWERUP, (void *)ssr->name);
> >>>>>>>> +        return 0;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +static void ssr_notify_stop(struct rproc_subdev *subdev, bool
> >>>>>>>> crashed)
> >>>>>>>> +{
> >>>>>>>> +        struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>> +
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_BEFORE_SHUTDOWN, (void *)ssr->name);
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +
> >>>>>>>>   static void ssr_notify_unprepare(struct rproc_subdev *subdev)
> >>>>>>>>   {
> >>>>>>>>           struct qcom_rproc_ssr *ssr = to_ssr_subdev(subdev);
> >>>>>>>>
> >>>>>>>> -        srcu_notifier_call_chain(ssr->rproc_notif_list, 0, (void
> >>>>>>>> *)ssr->name);
> >>>>>>>> +        srcu_notifier_call_chain(ssr->rproc_notif_list,
> >>>>>>>> +                                 RPROC_AFTER_SHUTDOWN, (void *)ssr->name);
> >>>>>>>>   }
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> @@ -248,6 +276,9 @@ void qcom_add_ssr_subdev(struct rproc *rproc,
> >>>>>>>> struct qcom_rproc_ssr *ssr,
> >>>>>>>>   {
> >>>>>>>>           ssr->name = ssr_name;
> >>>>>>>>           ssr->subdev.name = kstrdup("ssr_notifs", GFP_KERNEL);
> >>>>>>>> +        ssr->subdev.prepare = ssr_notify_prepare;
> >>>>>>>> +        ssr->subdev.start = ssr_notify_start;
> >>>>>>>> +        ssr->subdev.stop = ssr_notify_stop;
> >>>>>>> Now that I have a better understanding of what this patchset is doing, I
> >>>>>>> realise
> >>>>>>> my comments in patch 04 won't work.  To differentiate the subdevs of an
> >>>>>>> rproc I
> >>>>>>> suggest to wrap them in a generic structure with a type and an enum.
> >>>>>>> That way
> >>>>>>> you can differenciate between subdevices without having to add to the
> >>>>>>> core.
>
> While creating a new revision of the patchset we tried to implement
> this, but a similar issue comes
> up. If at a later point we wish to utilize the functionality of some
> common subdevice (not the case
> right now, but potentially), we might run into a similar problem of
> accessing illegal memory using
> container_of. I think it might be a better idea to introduce the name in
> the subdevice structure over
> having a potential security bug. What do you think?

I trust that you have given this an honest try but found potential
problems that I can't foresee due to the lack of insight on your
operating environment.  Please move forward with the addition of a new
"name" field to the rproc_subdev structure.

>
> Thanks,
> Siddharth
>
> >>>>>> Ok. I can try that.
> >>>>>>> That being said, I don't understand what patches 5 and 6 are doing...
> >>>>>>> Registering with the global ssr_notifiers allowed to gracefully shutdown
> >>>>>>> all the
> >>>>>>> MCUs in the system when one of them would go down.  But now that we are
> >>>>>>> using
> >>>>>>> the notifier on a per MCU, I really don't see why each subdev couldn't
> >>>>>>> implement
> >>>>>>> the right prepare/start/stop functions.
> >>>>>>>
> >>>>>>> Am I missing something here?
> >>>>>> We only want kernel clients to be notified when the Remoteproc they
> >>>>>> are
> >>>>>> interested
> >>>>>> in changes state. For e.g. audio kernel driver should be notified when
> >>>>>> audio
> >>>>>> processor goes down but it does not care about any other remoteproc.
> >>>>>> If you are suggesting that these kernel clients be added as subdevices
> >>>>>> then
> >>>>>> we will end up having many subdevices registered to each remoteproc.
> >>>>>> So we
> >>>>>> implemented a notifier chain per Remoteproc. This keeps the SSR
> >>>>>> notifications as
> >>>>>> the subdevice per remoteproc, and all interested clients can register
> >>>>>> to it.
> >>>>> It seems like I am missing information...  Your are referring to
> >>>>> "kernel
> >>>>> clients" and as such I must assume some drivers that are not part of
> >>>>> the
> >>>>> remoteproc/rpmsg subsystems are calling qcom_register_ssr_notifier().
> >>>>> I must
> >>>> Yes these are not part of remoteproc framework and they will register
> >>>> for notifications.
> >>>>> also assume these drivers (or that functionality) are not yet upsream
> >>>>> because
> >>>>> all I can see calling qcom_register_ssr_notifier() is
> >>>>> qcom_glink_ssr_probe().
> >>>> Correct.These are not upstreamed.
> >>> Ok, things are starting to make sense.
> >>>
> >>>>> Speaking of which, what is the role of the qcom_glink_ssr_driver?  Is
> >>>>> the glink
> >>>>> device that driver is handling the same as the glink device registed in
> >>>>> adsp_probe() and q6v5_probe()?
> >>>> glink ssr driver will send out notifications to remoteprocs that have
> >>>> opened the
> >>>> "glink_ssr" channel that some subsystem has gone down or booted up.
> >>>> This
> >>>> helps notify
> >>>> neighboring subsystems about change in state of any other subsystem.
> >>> I am still looking for an answer to my second question.
> >> Yes its the subdevice of the glink device that is registered in
> >> adsp_probe.
> >> It uses the "glink_ssr" glink channel.
> > Since this is confining events to a single MCU, I was mostly worried
> > about opening the "glink_ssr" channel for nothing but taking a step
> > back and thinking further on this, there might be other purposes for
> > the channel than only receiving notifications of other MCUs in the
> > system going down.
> >
> > Please spin off a new revision of this set and I will take another look.
> >
> > Thanks,
> > Mathieu
> >
> >>>>>>>
> >>>>>>>>           ssr->subdev.unprepare = ssr_notify_unprepare;
> >>>>>>>>           ssr->rproc_notif_list = kzalloc(sizeof(struct srcu_notifier_head),
> >>>>>>>>                                                                   GFP_KERNEL);
> >>>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>>>>>>> index e2f60cc..4be4478 100644
> >>>>>>>> --- a/include/linux/remoteproc.h
> >>>>>>>> +++ b/include/linux/remoteproc.h
> >>>>>>>> @@ -449,6 +449,21 @@ struct rproc_dump_segment {
> >>>>>>>>   };
> >>>>>>>>
> >>>>>>>>   /**
> >>>>>>>> + * enum rproc_notif_type - Different stages of remoteproc
> >>>>>>>> notifications
> >>>>>>>> + * @RPROC_BEFORE_SHUTDOWN:      unprepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_SHUTDOWN:       stop stage of  remoteproc
> >>>>>>>> + * @RPROC_BEFORE_POWERUP:       prepare stage of  remoteproc
> >>>>>>>> + * @RPROC_AFTER_POWERUP:        start stage of  remoteproc
> >>>>>>>> + */
> >>>>>>>> +enum rproc_notif_type {
> >>>>>>>> +        RPROC_BEFORE_SHUTDOWN,
> >>>>>>>> +        RPROC_AFTER_SHUTDOWN,
> >>>>>>>> +        RPROC_BEFORE_POWERUP,
> >>>>>>>> +        RPROC_AFTER_POWERUP,
> >>>>>>>> +        RPROC_MAX
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>> +/**
> >>>>>>>>    * struct rproc - represents a physical remote processor device
> >>>>>>>>    * @node: list node of this rproc object
> >>>>>>>>    * @domain: iommu domain
> >>>>>>>> --
> >>>>>>>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> >>>>>>>> a Linux Foundation Collaborative Project
> >>>>>>>>
> >>>>>>>> _______________________________________________
> >>>>>>>> linux-arm-kernel mailing list
> >>>>>>>> linux-arm-kernel@lists.infradead.org
> >>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel