[PATCH 0/7] Add support for hwspinlock bust

[PATCH 0/7] Add support for hwspinlock bust

[Chris Lew]

hwspinlocks can be acquired by many devices on the SoC. If any of these
devices go into a bad state before the device releases the hwspinlock,
then that hwspinlock may end up in an unusable state.

In the case of smem, each remoteproc takes a hwspinlock before trying to
allocate an smem item. If the remoteproc were to suddenly crash without
releasing this, it would be impossible for other remoteprocs to allocate
any smem items.

We propose a new api to bust a hwspinlock. This functionality is meant
for drivers that manage the lifecycle of a device. The driver can use
the bust api if it detects the device has gone into an error state, thus
allowing other entities in the system to use the hwspinlock.

The bust API implies multiple devices in linux can get a reference to a
hwspinlock. We add the ability for multiple devices to get a reference
to a hwspinlock via hwspin_lock_request_specific().
hwspin_lock_request() will continue to provide the next unused lock.

For the smem example, the hwspinlock will now be referenced by remoteproc
and the smem driver.

These patches were tested on an sm8650 mtp using engineering cdsp
firmware that triggers a watchdog with the smem hwspinlock acquired.

Checked for error in dt-bindings with below.
 - make DT_CHECKER_FLAGS=-m DT_SCHEMA_FILES=remoteproc/qcom,pas-common.yaml dt_binding_check
 - make qcom/sm8650-mtp.dtb CHECK_DTBS=1

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
Chris Lew (2):
      dt-bindings: remoteproc: qcom,pas: Add hwlocks
      arm64: dts: qcom: sm8650: Add hwlock to remoteproc

Richard Maina (5):
      hwspinlock: Introduce refcount
      hwspinlock: Enable hwspinlock sharing
      hwspinlock: Introduce hwspin_lock_bust()
      hwspinlock: qcom: implement bust operation
      remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

 .../bindings/remoteproc/qcom,pas-common.yaml       |  3 ++
 Documentation/locking/hwspinlock.rst               | 19 ++++++--
 arch/arm64/boot/dts/qcom/sm8650.dtsi               |  3 ++
 drivers/hwspinlock/hwspinlock_core.c               | 52 ++++++++++++++++------
 drivers/hwspinlock/hwspinlock_internal.h           |  5 +++
 drivers/hwspinlock/qcom_hwspinlock.c               | 25 +++++++++++
 drivers/remoteproc/qcom_q6v5_pas.c                 | 28 ++++++++++++
 include/linux/hwspinlock.h                         |  6 +++
 8 files changed, 123 insertions(+), 18 deletions(-)
---
base-commit: e7b4ef8fffaca247809337bb78daceb406659f2d
change-id: 20240509-hwspinlock-bust-d497a70c1a3a

Best regards,
-- 
Chris Lew <quic_clew@quicinc.com>

[PATCH 1/7] hwspinlock: Introduce refcount

[Chris Lew]

From: Richard Maina <quic_rmaina@quicinc.com>

Currently each device receives a dedicated hwspinlock that is not
accesible to other device drivers. In order to allow multiple consumers
access to the same hwspinlock, introduce a refcount to hwspinlock struct
and implement refcounting infrastructure.

Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/hwspinlock/hwspinlock_core.c     | 14 +++++++++-----
 drivers/hwspinlock/hwspinlock_internal.h |  2 ++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 0c0a932c00f3..29b33072ff57 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -428,6 +428,7 @@ static int hwspin_lock_register_single(struct hwspinlock *hwlock, int id)
 	int ret;
 
 	mutex_lock(&hwspinlock_tree_lock);
+	hwlock->refcnt = 0;
 
 	ret = radix_tree_insert(&hwspinlock_tree, id, hwlock);
 	if (ret) {
@@ -671,6 +672,8 @@ static int __hwspin_lock_request(struct hwspinlock *hwlock)
 
 	ret = 0;
 
+	hwlock->refcnt++;
+
 	/* mark hwspinlock as used, should not fail */
 	tmp = radix_tree_tag_clear(&hwspinlock_tree, hwlock_to_id(hwlock),
 							HWSPINLOCK_UNUSED);
@@ -829,12 +832,13 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
 	/* notify the underlying device that power is not needed */
 	pm_runtime_put(dev);
 
-	/* mark this hwspinlock as available */
-	tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
-							HWSPINLOCK_UNUSED);
+	if (--hwlock->refcnt == 0) {
+		/* mark this hwspinlock as available */
+		tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock), HWSPINLOCK_UNUSED);
 
-	/* sanity check (this shouldn't happen) */
-	WARN_ON(tmp != hwlock);
+		/* sanity check (this shouldn't happen) */
+		WARN_ON(tmp != hwlock);
+	}
 
 	module_put(dev->driver->owner);
 
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index 29892767bb7a..12f230b40693 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -35,11 +35,13 @@ struct hwspinlock_ops {
  * struct hwspinlock - this struct represents a single hwspinlock instance
  * @bank: the hwspinlock_device structure which owns this lock
  * @lock: initialized and used by hwspinlock core
+ * @refcnt: counter for the number of existing references to the hwspinlock
  * @priv: private data, owned by the underlying platform-specific hwspinlock drv
  */
 struct hwspinlock {
 	struct hwspinlock_device *bank;
 	spinlock_t lock;
+	unsigned int refcnt;
 	void *priv;
 };
 

-- 
2.25.1

Re: [PATCH 1/7] hwspinlock: Introduce refcount

[Bryan O'Donoghue]

On 17/05/2024 00:58, Chris Lew wrote:
> +	unsigned int refcnt;

Why int and not refcount_t ?

Have you an argument for or against use of one over another ?

---
bod

Re: [PATCH 1/7] hwspinlock: Introduce refcount

[Chris Lew]

On 5/17/2024 1:58 AM, Bryan O'Donoghue wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
>> +    unsigned int refcnt;
> 
> Why int and not refcount_t ?
> 
> Have you an argument for or against use of one over another ?
> 

I wanted to avoid the warning if you try to do a refcount_inc on 0. In 
this case, 0 means the the hwlock is unused but the hwlock should 
persist while waiting for another request. It seemed like refcount_t 
expected the associated object to be released once the count hit 0.

Also the count here is serialized by hwspinlock_tree_lock so the need 
for the atomicity provided by refcount_t was unneeded. Using unsigned 
int here seemed simpler.

> ---
> bod

[PATCH 2/7] hwspinlock: Enable hwspinlock sharing

[Chris Lew]

From: Richard Maina <quic_rmaina@quicinc.com>

The hwspinlock used by qcom,smem is used to synchronize smem access
between the cpu and other remoteprocs in the soc. If one of these
remoteprocs crashes while holding the hwspinlock, then the remoteproc
driver should try to  release the lock on behalf of the rproc. This
would require rproc and smem drivers to share access to a hwspinlock
handle.

With the introduction of reference counting this is now achievable,
therefore, remove the code in hwspin_lock_request_specific() preventing
this.

The single owner condition is retained in the hwspin_lock_request()
function, as this is specifically intended for requesting an unused
hwspinlock and should not have any shared references.

Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 Documentation/locking/hwspinlock.rst | 8 ++++----
 drivers/hwspinlock/hwspinlock_core.c | 8 --------
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/Documentation/locking/hwspinlock.rst b/Documentation/locking/hwspinlock.rst
index 6f03713b7003..c1c2c827b575 100644
--- a/Documentation/locking/hwspinlock.rst
+++ b/Documentation/locking/hwspinlock.rst
@@ -53,10 +53,10 @@ Should be called from a process context (might sleep).
 
   struct hwspinlock *hwspin_lock_request_specific(unsigned int id);
 
-Assign a specific hwspinlock id and return its address, or NULL
-if that hwspinlock is already in use. Usually board code will
-be calling this function in order to reserve specific hwspinlock
-ids for predefined purposes.
+Assign a specific hwspinlock id or increment the reference count if the
+hwspinlock is already in use. Return NULL if unable to request the
+hwspinlock. Usually board code will be calling this function in order
+to reserve specific hwspinlock ids for predefined purposes.
 
 Should be called from a process context (might sleep).
 
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 29b33072ff57..73a6dff5cbac 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -774,14 +774,6 @@ struct hwspinlock *hwspin_lock_request_specific(unsigned int id)
 	/* sanity check (this shouldn't happen) */
 	WARN_ON(hwlock_to_id(hwlock) != id);
 
-	/* make sure this hwspinlock is unused */
-	ret = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_UNUSED);
-	if (ret == 0) {
-		pr_warn("hwspinlock %u is already in use\n", id);
-		hwlock = NULL;
-		goto out;
-	}
-
 	/* mark as used and power up */
 	ret = __hwspin_lock_request(hwlock);
 	if (ret < 0)

-- 
2.25.1

[PATCH 3/7] hwspinlock: Introduce hwspin_lock_bust()

[Chris Lew]

From: Richard Maina <quic_rmaina@quicinc.com>

When a remoteproc crashes or goes down unexpectedly this can result in
a state where locks held by the remoteproc will remain locked possibly
resulting in deadlock. This new API hwspin_lock_bust() allows
hwspinlock implementers to define a bust operation for freeing previously
acquired hwspinlocks after verifying ownership of the acquired lock.

Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 Documentation/locking/hwspinlock.rst     | 11 +++++++++++
 drivers/hwspinlock/hwspinlock_core.c     | 30 +++++++++++++++++++++++++++++-
 drivers/hwspinlock/hwspinlock_internal.h |  3 +++
 include/linux/hwspinlock.h               |  6 ++++++
 4 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/Documentation/locking/hwspinlock.rst b/Documentation/locking/hwspinlock.rst
index c1c2c827b575..6ee94cc6d3b7 100644
--- a/Documentation/locking/hwspinlock.rst
+++ b/Documentation/locking/hwspinlock.rst
@@ -85,6 +85,17 @@ is already free).
 
 Should be called from a process context (might sleep).
 
+::
+
+  int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id);
+
+After verifying the owner of the hwspinlock, release a previously acquired
+hwspinlock; returns 0 on success, or an appropriate error code on failure
+(e.g. -EOPNOTSUPP if the bust operation is not defined for the specific
+hwspinlock).
+
+Should be called from a process context (might sleep).
+
 ::
 
   int hwspin_lock_timeout(struct hwspinlock *hwlock, unsigned int timeout);
diff --git a/drivers/hwspinlock/hwspinlock_core.c b/drivers/hwspinlock/hwspinlock_core.c
index 73a6dff5cbac..a7851262f36f 100644
--- a/drivers/hwspinlock/hwspinlock_core.c
+++ b/drivers/hwspinlock/hwspinlock_core.c
@@ -305,6 +305,34 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 }
 EXPORT_SYMBOL_GPL(__hwspin_unlock);
 
+/**
+ * hwspin_lock_bust() - bust a specific hwspinlock
+ * @hwlock: a previously-acquired hwspinlock which we want to bust
+ * @id: identifier of the remote lock holder, if applicable
+ *
+ * This function will bust a hwspinlock that was previously acquired as
+ * long as the current owner of the lock matches the id given by the caller.
+ *
+ * Should be called from a process context (might sleep).
+ *
+ * Returns: 0 on success, or -EINVAL if the hwspinlock does not exist, or
+ * the bust operation fails, and -EOPNOTSUPP if the bust operation is not
+ * defined for the hwspinlock.
+ */
+int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id)
+{
+	if (WARN_ON(!hwlock))
+		return -EINVAL;
+
+	if (!hwlock->bank->ops->bust) {
+		pr_err("bust operation not defined\n");
+		return -EOPNOTSUPP;
+	}
+
+	return hwlock->bank->ops->bust(hwlock, id);
+}
+EXPORT_SYMBOL_GPL(hwspin_lock_bust);
+
 /**
  * of_hwspin_lock_simple_xlate - translate hwlock_spec to return a lock id
  * @hwlock_spec: hwlock specifier as found in the device tree
@@ -610,7 +638,7 @@ EXPORT_SYMBOL_GPL(devm_hwspin_lock_unregister);
  * This function should be called from the underlying platform-specific
  * implementation, to register a new hwspinlock device instance.
  *
- * Should be called from a process context (might sleep)
+ * Context: Process context. GFP_KERNEL allocation.
  *
  * Returns: %0 on success, or an appropriate error code on failure
  */
diff --git a/drivers/hwspinlock/hwspinlock_internal.h b/drivers/hwspinlock/hwspinlock_internal.h
index 12f230b40693..2202f2c9a62e 100644
--- a/drivers/hwspinlock/hwspinlock_internal.h
+++ b/drivers/hwspinlock/hwspinlock_internal.h
@@ -21,6 +21,8 @@ struct hwspinlock_device;
  * @trylock: make a single attempt to take the lock. returns 0 on
  *	     failure and true on success. may _not_ sleep.
  * @unlock:  release the lock. always succeed. may _not_ sleep.
+ * @bust:    optional, platform-specific bust handler, called by hwspinlock
+ *	     core to bust a specific lock.
  * @relax:   optional, platform-specific relax handler, called by hwspinlock
  *	     core while spinning on a lock, between two successive
  *	     invocations of @trylock. may _not_ sleep.
@@ -28,6 +30,7 @@ struct hwspinlock_device;
 struct hwspinlock_ops {
 	int (*trylock)(struct hwspinlock *lock);
 	void (*unlock)(struct hwspinlock *lock);
+	int (*bust)(struct hwspinlock *lock, unsigned int id);
 	void (*relax)(struct hwspinlock *lock);
 };
 
diff --git a/include/linux/hwspinlock.h b/include/linux/hwspinlock.h
index bfe7c1f1ac6d..f0231dbc4777 100644
--- a/include/linux/hwspinlock.h
+++ b/include/linux/hwspinlock.h
@@ -68,6 +68,7 @@ int __hwspin_lock_timeout(struct hwspinlock *, unsigned int, int,
 int __hwspin_trylock(struct hwspinlock *, int, unsigned long *);
 void __hwspin_unlock(struct hwspinlock *, int, unsigned long *);
 int of_hwspin_lock_get_id_byname(struct device_node *np, const char *name);
+int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id);
 int devm_hwspin_lock_free(struct device *dev, struct hwspinlock *hwlock);
 struct hwspinlock *devm_hwspin_lock_request(struct device *dev);
 struct hwspinlock *devm_hwspin_lock_request_specific(struct device *dev,
@@ -127,6 +128,11 @@ void __hwspin_unlock(struct hwspinlock *hwlock, int mode, unsigned long *flags)
 {
 }
 
+static inline int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id)
+{
+	return 0;
+}
+
 static inline int of_hwspin_lock_get_id(struct device_node *np, int index)
 {
 	return 0;

-- 
2.25.1

Re: [PATCH 3/7] hwspinlock: Introduce hwspin_lock_bust()

[Bryan O'Donoghue]

On 17/05/2024 00:58, Chris Lew wrote:
> From: Richard Maina <quic_rmaina@quicinc.com>
> 
> When a remoteproc crashes or goes down unexpectedly this can result in
> a state where locks held by the remoteproc will remain locked possibly
> resulting in deadlock. This new API hwspin_lock_bust() allows
> hwspinlock implementers to define a bust operation for freeing previously
> acquired hwspinlocks after verifying ownership of the acquired lock.
> 
> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>   Documentation/locking/hwspinlock.rst     | 11 +++++++++++
>   drivers/hwspinlock/hwspinlock_core.c     | 30 +++++++++++++++++++++++++++++-

Shouldn't this be added to drivers/hwspinlock/qcom_hwspinlock.c ?

>   drivers/hwspinlock/hwspinlock_internal.h |  3 +++
>   include/linux/hwspinlock.h               |  6 ++++++
>   4 files changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/locking/hwspinlock.rst b/Documentation/locking/hwspinlock.rst
> index c1c2c827b575..6ee94cc6d3b7 100644
> --- a/Documentation/locking/hwspinlock.rst
> +++ b/Documentation/locking/hwspinlock.rst
> @@ -85,6 +85,17 @@ is already free).
>   
>   Should be called from a process context (might sleep).
>   
> +::
> +
> +  int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id);

I don't think this is a geat name "bust" looks alot like "burst" and I 
don't think aligns well with the established namespace.

Why not simply qcom_hwspinlock_unlock_force() - which is what you are 
doing - forcing the hw spinlock to unlock.

---
bod

Re: [PATCH 3/7] hwspinlock: Introduce hwspin_lock_bust()

[Bryan O'Donoghue]

On 17/05/2024 10:07, Bryan O'Donoghue wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
>> From: Richard Maina <quic_rmaina@quicinc.com>
>>
>> When a remoteproc crashes or goes down unexpectedly this can result in
>> a state where locks held by the remoteproc will remain locked possibly
>> resulting in deadlock. This new API hwspin_lock_bust() allows
>> hwspinlock implementers to define a bust operation for freeing previously
>> acquired hwspinlocks after verifying ownership of the acquired lock.
>>
>> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> ---
>>   Documentation/locking/hwspinlock.rst     | 11 +++++++++++
>>   drivers/hwspinlock/hwspinlock_core.c     | 30 
>> +++++++++++++++++++++++++++++-
> 
> Shouldn't this be added to drivers/hwspinlock/qcom_hwspinlock.c ?
> 
>>   drivers/hwspinlock/hwspinlock_internal.h |  3 +++
>>   include/linux/hwspinlock.h               |  6 ++++++
>>   4 files changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/locking/hwspinlock.rst 
>> b/Documentation/locking/hwspinlock.rst
>> index c1c2c827b575..6ee94cc6d3b7 100644
>> --- a/Documentation/locking/hwspinlock.rst
>> +++ b/Documentation/locking/hwspinlock.rst
>> @@ -85,6 +85,17 @@ is already free).
>>   Should be called from a process context (might sleep).
>> +::
>> +
>> +  int hwspin_lock_bust(struct hwspinlock *hwlock, unsigned int id);
> 
> I don't think this is a geat name "bust" looks alot like "burst" and I 
> don't think aligns well with the established namespace.
> 
> Why not simply qcom_hwspinlock_unlock_force() - which is what you are 
> doing - forcing the hw spinlock to unlock.

hmm looking again, I think _core is the right place and bust() is 
consistent with bust_spinlocks();

meh

---
bod

[PATCH 4/7] hwspinlock: qcom: implement bust operation

[Chris Lew]

From: Richard Maina <quic_rmaina@quicinc.com>

Implement a new operation qcom_hwspinlock_bust() which can be invoked
to free any locks that are in use when a remoteproc is stopped or
crashed.

Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/hwspinlock/qcom_hwspinlock.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/hwspinlock/qcom_hwspinlock.c b/drivers/hwspinlock/qcom_hwspinlock.c
index 814dfe8697bf..0390979fd765 100644
--- a/drivers/hwspinlock/qcom_hwspinlock.c
+++ b/drivers/hwspinlock/qcom_hwspinlock.c
@@ -64,9 +64,34 @@ static void qcom_hwspinlock_unlock(struct hwspinlock *lock)
 		pr_err("%s: failed to unlock spinlock\n", __func__);
 }
 
+static int qcom_hwspinlock_bust(struct hwspinlock *lock, unsigned int id)
+{
+	struct regmap_field *field = lock->priv;
+	u32 owner;
+	int ret;
+
+	ret = regmap_field_read(field, &owner);
+	if (ret) {
+		dev_err(lock->bank->dev, "unable to query spinlock owner\n");
+		return ret;
+	}
+
+	if (owner != id)
+		return 0;
+
+	ret = regmap_field_write(field, 0);
+	if (ret) {
+		dev_err(lock->bank->dev, "failed to bust spinlock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static const struct hwspinlock_ops qcom_hwspinlock_ops = {
 	.trylock	= qcom_hwspinlock_trylock,
 	.unlock		= qcom_hwspinlock_unlock,
+	.bust		= qcom_hwspinlock_bust,
 };
 
 static const struct regmap_config sfpb_mutex_config = {

-- 
2.25.1

[PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Chris Lew]

Add hwlocks property to describe the hwspinlock that remoteproc can try
to bust on behalf of the remoteproc's smem.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
index 63a82e7a8bf8..483a8ff5f47e 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,pas-common.yaml
@@ -77,6 +77,9 @@ properties:
       and devices related to the ADSP.
     unevaluatedProperties: false
 
+  hwlocks:
+    maxItems: 1
+
 required:
   - clocks
   - clock-names

-- 
2.25.1

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Krzysztof Kozlowski]

On 17/05/2024 00:58, Chris Lew wrote:
> Add hwlocks property to describe the hwspinlock that remoteproc can try
> to bust on behalf of the remoteproc's smem.

Sorry, as you wrote, the lock is part of smem, not here. Drivers do not
crash, so if your crashes as you imply in the cover letter, then first
fix the driver.

Best regards,
Krzysztof

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Chris Lew]

On 5/19/2024 10:36 AM, Krzysztof Kozlowski wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
>> Add hwlocks property to describe the hwspinlock that remoteproc can try
>> to bust on behalf of the remoteproc's smem.
> 
> Sorry, as you wrote, the lock is part of smem, not here. Drivers do not
> crash, so if your crashes as you imply in the cover letter, then first
> fix the driver.
>

Hi Krzysztof,

Sorry for the confusion, I dont think I meant that the smem driver will 
ever crash. The referred to crash in the cover letter is a crash in the 
firmware running on the remoteproc. The remoteproc could crash for any 
unexpected reason, related or unrelated to smem, while holding the tcsr 
mutex. I want to ensure that all resources that a remoteproc might be 
using are released as part of remoteproc stop.

The SMEM driver manages the lock/unlock operations on the tcsr mutex 
from the Linux CPU's perspective. This case is for cleaning up from the 
remote side's perspective.

In this case it's the hwspinlock used to synchronize SMEM, but it's 
conceivable that firmware running on the remoteproc has additional locks 
that need to be busted in order for the system to continue executing 
until the firmware is reinitialized.

We did consider tying this to the SMEM instance, but the entitiy 
relating to firmware is the remoteproc instance.

> Best regards,
> Krzysztof
> 

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Krzysztof Kozlowski]

On 21/05/2024 06:08, Chris Lew wrote:
> 
> 
> On 5/19/2024 10:36 AM, Krzysztof Kozlowski wrote:
>> On 17/05/2024 00:58, Chris Lew wrote:
>>> Add hwlocks property to describe the hwspinlock that remoteproc can try
>>> to bust on behalf of the remoteproc's smem.
>>
>> Sorry, as you wrote, the lock is part of smem, not here. Drivers do not
>> crash, so if your crashes as you imply in the cover letter, then first
>> fix the driver.
>>
> 
> Hi Krzysztof,
> 
> Sorry for the confusion, I dont think I meant that the smem driver will 
> ever crash. The referred to crash in the cover letter is a crash in the 
> firmware running on the remoteproc. The remoteproc could crash for any 
> unexpected reason, related or unrelated to smem, while holding the tcsr 
> mutex. I want to ensure that all resources that a remoteproc might be 
> using are released as part of remoteproc stop.
> 
> The SMEM driver manages the lock/unlock operations on the tcsr mutex 
> from the Linux CPU's perspective. This case is for cleaning up from the 
> remote side's perspective.
> 
> In this case it's the hwspinlock used to synchronize SMEM, but it's 
> conceivable that firmware running on the remoteproc has additional locks 
> that need to be busted in order for the system to continue executing 
> until the firmware is reinitialized.
> 
> We did consider tying this to the SMEM instance, but the entitiy 
> relating to firmware is the remoteproc instance.

I still do not understand why you have to add hwlock to remoteproc, even
though it is not directly used. Your driver problem looks like lack of
proper driver architecture - you want to control the locks not from the
layer took the lock, but one layer up. Sorry, no, fix the driver
architecture.

Best regards,
Krzysztof

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Bjorn Andersson]

On Tue, May 21, 2024 at 09:36:03AM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 06:08, Chris Lew wrote:
> > 
> > 
> > On 5/19/2024 10:36 AM, Krzysztof Kozlowski wrote:
> >> On 17/05/2024 00:58, Chris Lew wrote:
> >>> Add hwlocks property to describe the hwspinlock that remoteproc can try
> >>> to bust on behalf of the remoteproc's smem.
> >>
> >> Sorry, as you wrote, the lock is part of smem, not here. Drivers do not
> >> crash, so if your crashes as you imply in the cover letter, then first
> >> fix the driver.
> >>
> > 
> > Hi Krzysztof,
> > 
> > Sorry for the confusion, I dont think I meant that the smem driver will 
> > ever crash. The referred to crash in the cover letter is a crash in the 
> > firmware running on the remoteproc. The remoteproc could crash for any 
> > unexpected reason, related or unrelated to smem, while holding the tcsr 
> > mutex. I want to ensure that all resources that a remoteproc might be 
> > using are released as part of remoteproc stop.
> > 
> > The SMEM driver manages the lock/unlock operations on the tcsr mutex 
> > from the Linux CPU's perspective. This case is for cleaning up from the 
> > remote side's perspective.
> > 
> > In this case it's the hwspinlock used to synchronize SMEM, but it's 
> > conceivable that firmware running on the remoteproc has additional locks 
> > that need to be busted in order for the system to continue executing 
> > until the firmware is reinitialized.
> > 
> > We did consider tying this to the SMEM instance, but the entitiy 
> > relating to firmware is the remoteproc instance.
> 
> I still do not understand why you have to add hwlock to remoteproc, even
> though it is not directly used. Your driver problem looks like lack of
> proper driver architecture - you want to control the locks not from the
> layer took the lock, but one layer up. Sorry, no, fix the driver
> architecture.
> 

No, it is the firmware's reference to the lock that is represented in
the remoteproc node, while SMEM deals with Linux's reference to the lock.

This reference would be used to release the lock - on behalf of the
firmware - in the event that the firmware held it when it
stopped/crashed.

Regards,
Bjorn

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Krzysztof Kozlowski]

On 21/05/2024 21:17, Bjorn Andersson wrote:
>>>
>>> Hi Krzysztof,
>>>
>>> Sorry for the confusion, I dont think I meant that the smem driver will 
>>> ever crash. The referred to crash in the cover letter is a crash in the 
>>> firmware running on the remoteproc. The remoteproc could crash for any 
>>> unexpected reason, related or unrelated to smem, while holding the tcsr 
>>> mutex. I want to ensure that all resources that a remoteproc might be 
>>> using are released as part of remoteproc stop.
>>>
>>> The SMEM driver manages the lock/unlock operations on the tcsr mutex 
>>> from the Linux CPU's perspective. This case is for cleaning up from the 
>>> remote side's perspective.
>>>
>>> In this case it's the hwspinlock used to synchronize SMEM, but it's 
>>> conceivable that firmware running on the remoteproc has additional locks 
>>> that need to be busted in order for the system to continue executing 
>>> until the firmware is reinitialized.
>>>
>>> We did consider tying this to the SMEM instance, but the entitiy 
>>> relating to firmware is the remoteproc instance.
>>
>> I still do not understand why you have to add hwlock to remoteproc, even
>> though it is not directly used. Your driver problem looks like lack of
>> proper driver architecture - you want to control the locks not from the
>> layer took the lock, but one layer up. Sorry, no, fix the driver
>> architecture.
>>
> 
> No, it is the firmware's reference to the lock that is represented in
> the remoteproc node, while SMEM deals with Linux's reference to the lock.
> 
> This reference would be used to release the lock - on behalf of the
> firmware - in the event that the firmware held it when it
> stopped/crashed.

I understood, but the remoteproc driver did not acquire the hardware
lock. It was taken by smem, if I got it correctly, so you should poke
smem to bust the spinlock.

The hwlock is not a property of remote proc, because remote proc does
not care, right? Other device cares... and now for every smem user you
will add new binding property?

No, you are adding a binding based on your driver solution.

Best regards,
Krzysztof

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Bjorn Andersson]

On Wed, May 22, 2024 at 09:26:00AM +0200, Krzysztof Kozlowski wrote:
> On 21/05/2024 21:17, Bjorn Andersson wrote:
> >>>
> >>> Hi Krzysztof,
> >>>
> >>> Sorry for the confusion, I dont think I meant that the smem driver will 
> >>> ever crash. The referred to crash in the cover letter is a crash in the 
> >>> firmware running on the remoteproc. The remoteproc could crash for any 
> >>> unexpected reason, related or unrelated to smem, while holding the tcsr 
> >>> mutex. I want to ensure that all resources that a remoteproc might be 
> >>> using are released as part of remoteproc stop.
> >>>
> >>> The SMEM driver manages the lock/unlock operations on the tcsr mutex 
> >>> from the Linux CPU's perspective. This case is for cleaning up from the 
> >>> remote side's perspective.
> >>>
> >>> In this case it's the hwspinlock used to synchronize SMEM, but it's 
> >>> conceivable that firmware running on the remoteproc has additional locks 
> >>> that need to be busted in order for the system to continue executing 
> >>> until the firmware is reinitialized.
> >>>
> >>> We did consider tying this to the SMEM instance, but the entitiy 
> >>> relating to firmware is the remoteproc instance.
> >>
> >> I still do not understand why you have to add hwlock to remoteproc, even
> >> though it is not directly used. Your driver problem looks like lack of
> >> proper driver architecture - you want to control the locks not from the
> >> layer took the lock, but one layer up. Sorry, no, fix the driver
> >> architecture.
> >>
> > 
> > No, it is the firmware's reference to the lock that is represented in
> > the remoteproc node, while SMEM deals with Linux's reference to the lock.
> > 
> > This reference would be used to release the lock - on behalf of the
> > firmware - in the event that the firmware held it when it
> > stopped/crashed.
> 
> I understood, but the remoteproc driver did not acquire the hardware
> lock. It was taken by smem, if I got it correctly, so you should poke
> smem to bust the spinlock.
> 

The remoteproc instance is the closest representation of the entity that
took the lock (i.e. the firmware). SMEM here is just another consumer of
the same lock.

> The hwlock is not a property of remote proc, because remote proc does
> not care, right? Other device cares... and now for every smem user you
> will add new binding property?
> 

Right, the issue seen relates to SMEM, because the remote processor (not
the remoteproc driver) took the lock.

> No, you are adding a binding based on your driver solution.

Similar to how hwspinlocks are used in other platforms (e.g. TI) the
firmware could take multiple locks, e.g. to synchronize access to other
shared memory mechanism (i.e. not SMEM). While I am not aware of such
use case today, my expectation was that in such case we just list all
the hwlocks related to the firmware and bust those from the remoteproc
instance.

Having to export APIs from each one of such drivers and make the
remoteproc identify the relevant instances and call those APIs does
indeed seem inconvenient.
SMEM is special here because it's singleton, but this would not
necessarily be true for other cases.

Regards,
Bjorn

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Krzysztof Kozlowski]

On 22/05/2024 19:50, Bjorn Andersson wrote:
>>>>>
>>>>> We did consider tying this to the SMEM instance, but the entitiy 
>>>>> relating to firmware is the remoteproc instance.
>>>>
>>>> I still do not understand why you have to add hwlock to remoteproc, even
>>>> though it is not directly used. Your driver problem looks like lack of
>>>> proper driver architecture - you want to control the locks not from the
>>>> layer took the lock, but one layer up. Sorry, no, fix the driver
>>>> architecture.
>>>>
>>>
>>> No, it is the firmware's reference to the lock that is represented in
>>> the remoteproc node, while SMEM deals with Linux's reference to the lock.
>>>
>>> This reference would be used to release the lock - on behalf of the
>>> firmware - in the event that the firmware held it when it
>>> stopped/crashed.
>>
>> I understood, but the remoteproc driver did not acquire the hardware
>> lock. It was taken by smem, if I got it correctly, so you should poke
>> smem to bust the spinlock.
>>
> 
> The remoteproc instance is the closest representation of the entity that
> took the lock (i.e. the firmware). SMEM here is just another consumer of
> the same lock.
> 
>> The hwlock is not a property of remote proc, because remote proc does
>> not care, right? Other device cares... and now for every smem user you
>> will add new binding property?
>>
> 
> Right, the issue seen relates to SMEM, because the remote processor (not
> the remoteproc driver) took the lock.
> 
>> No, you are adding a binding based on your driver solution.
> 
> Similar to how hwspinlocks are used in other platforms (e.g. TI) the
> firmware could take multiple locks, e.g. to synchronize access to other
> shared memory mechanism (i.e. not SMEM). While I am not aware of such
> use case today, my expectation was that in such case we just list all
> the hwlocks related to the firmware and bust those from the remoteproc
> instance.
> 
> Having to export APIs from each one of such drivers and make the
> remoteproc identify the relevant instances and call those APIs does
> indeed seem inconvenient.
> SMEM is special here because it's singleton, but this would not
> necessarily be true for other cases.

I don't think that exporting such API is unreasonable, but quite
opposite - expected. The remote processor crashed, so the remoteproc
driver is supposed to call some sort of smem_cleanup() or
smem_cleanup_on_crash() and call would bust/release the lock. That way
lock handling is encapsulated entirely in one driver which already takes
and releases the lock.

Just like freeing any memory. remoteproc driver does not free other
driver's memory only because processor crashed.


Best regards,
Krzysztof

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Bjorn Andersson]

On Thu, May 23, 2024 at 08:15:54AM +0200, Krzysztof Kozlowski wrote:
> On 22/05/2024 19:50, Bjorn Andersson wrote:
> >>>>>
> >>>>> We did consider tying this to the SMEM instance, but the entitiy 
> >>>>> relating to firmware is the remoteproc instance.
> >>>>
> >>>> I still do not understand why you have to add hwlock to remoteproc, even
> >>>> though it is not directly used. Your driver problem looks like lack of
> >>>> proper driver architecture - you want to control the locks not from the
> >>>> layer took the lock, but one layer up. Sorry, no, fix the driver
> >>>> architecture.
> >>>>
> >>>
> >>> No, it is the firmware's reference to the lock that is represented in
> >>> the remoteproc node, while SMEM deals with Linux's reference to the lock.
> >>>
> >>> This reference would be used to release the lock - on behalf of the
> >>> firmware - in the event that the firmware held it when it
> >>> stopped/crashed.
> >>
> >> I understood, but the remoteproc driver did not acquire the hardware
> >> lock. It was taken by smem, if I got it correctly, so you should poke
> >> smem to bust the spinlock.
> >>
> > 
> > The remoteproc instance is the closest representation of the entity that
> > took the lock (i.e. the firmware). SMEM here is just another consumer of
> > the same lock.
> > 
> >> The hwlock is not a property of remote proc, because remote proc does
> >> not care, right? Other device cares... and now for every smem user you
> >> will add new binding property?
> >>
> > 
> > Right, the issue seen relates to SMEM, because the remote processor (not
> > the remoteproc driver) took the lock.
> > 
> >> No, you are adding a binding based on your driver solution.
> > 
> > Similar to how hwspinlocks are used in other platforms (e.g. TI) the
> > firmware could take multiple locks, e.g. to synchronize access to other
> > shared memory mechanism (i.e. not SMEM). While I am not aware of such
> > use case today, my expectation was that in such case we just list all
> > the hwlocks related to the firmware and bust those from the remoteproc
> > instance.
> > 
> > Having to export APIs from each one of such drivers and make the
> > remoteproc identify the relevant instances and call those APIs does
> > indeed seem inconvenient.
> > SMEM is special here because it's singleton, but this would not
> > necessarily be true for other cases.
> 
> I don't think that exporting such API is unreasonable, but quite
> opposite - expected. The remote processor crashed, so the remoteproc
> driver is supposed to call some sort of smem_cleanup() or
> smem_cleanup_on_crash() and call would bust/release the lock. That way
> lock handling is encapsulated entirely in one driver which already takes
> and releases the lock.
> 

I don't agree.

SMEM does indeed acquire and release the same, shared, lock. But the
SMEM driver instance on our side is not involved in taking the lock for
the firmware.

There exist an equivalent SMEM driver instance in the firmware that
crashed and that's the thing that needs to be released.


We're also not tearing down, or cleaning up anything in our SMEM
instance. It is simply "when remoteproc id N died, check if N is holding
the lock and if so force a release of the lock - so that others can grab
it".

> Just like freeing any memory. remoteproc driver does not free other
> driver's memory only because processor crashed.
> 

That's a good comparison. Because when the firmware running on the
remote processor crashes, it is indeed the job of the remoteproc driver
to clean up the memory allocated to run the remote processor.

Regards,
Bjorn

Re: [PATCH 5/7] dt-bindings: remoteproc: qcom,pas: Add hwlocks

[Krzysztof Kozlowski]

On 24/05/2024 21:23, Bjorn Andersson wrote:
> On Thu, May 23, 2024 at 08:15:54AM +0200, Krzysztof Kozlowski wrote:
>> On 22/05/2024 19:50, Bjorn Andersson wrote:
>>>>>>>
>>>>>>> We did consider tying this to the SMEM instance, but the entitiy 
>>>>>>> relating to firmware is the remoteproc instance.
>>>>>>
>>>>>> I still do not understand why you have to add hwlock to remoteproc, even
>>>>>> though it is not directly used. Your driver problem looks like lack of
>>>>>> proper driver architecture - you want to control the locks not from the
>>>>>> layer took the lock, but one layer up. Sorry, no, fix the driver
>>>>>> architecture.
>>>>>>
>>>>>
>>>>> No, it is the firmware's reference to the lock that is represented in
>>>>> the remoteproc node, while SMEM deals with Linux's reference to the lock.
>>>>>
>>>>> This reference would be used to release the lock - on behalf of the
>>>>> firmware - in the event that the firmware held it when it
>>>>> stopped/crashed.
>>>>
>>>> I understood, but the remoteproc driver did not acquire the hardware
>>>> lock. It was taken by smem, if I got it correctly, so you should poke
>>>> smem to bust the spinlock.
>>>>
>>>
>>> The remoteproc instance is the closest representation of the entity that
>>> took the lock (i.e. the firmware). SMEM here is just another consumer of
>>> the same lock.
>>>
>>>> The hwlock is not a property of remote proc, because remote proc does
>>>> not care, right? Other device cares... and now for every smem user you
>>>> will add new binding property?
>>>>
>>>
>>> Right, the issue seen relates to SMEM, because the remote processor (not
>>> the remoteproc driver) took the lock.
>>>
>>>> No, you are adding a binding based on your driver solution.
>>>
>>> Similar to how hwspinlocks are used in other platforms (e.g. TI) the
>>> firmware could take multiple locks, e.g. to synchronize access to other
>>> shared memory mechanism (i.e. not SMEM). While I am not aware of such
>>> use case today, my expectation was that in such case we just list all
>>> the hwlocks related to the firmware and bust those from the remoteproc
>>> instance.
>>>
>>> Having to export APIs from each one of such drivers and make the
>>> remoteproc identify the relevant instances and call those APIs does
>>> indeed seem inconvenient.
>>> SMEM is special here because it's singleton, but this would not
>>> necessarily be true for other cases.
>>
>> I don't think that exporting such API is unreasonable, but quite
>> opposite - expected. The remote processor crashed, so the remoteproc
>> driver is supposed to call some sort of smem_cleanup() or
>> smem_cleanup_on_crash() and call would bust/release the lock. That way
>> lock handling is encapsulated entirely in one driver which already takes
>> and releases the lock.
>>
> 
> I don't agree.
> 
> SMEM does indeed acquire and release the same, shared, lock. But the
> SMEM driver instance on our side is not involved in taking the lock for
> the firmware.
> 
> There exist an equivalent SMEM driver instance in the firmware that
> crashed and that's the thing that needs to be released.

Then please include relevant explanation in the commit msg.

> 
> 
> We're also not tearing down, or cleaning up anything in our SMEM
> instance. It is simply "when remoteproc id N died, check if N is holding
> the lock and if so force a release of the lock - so that others can grab
> it".
> 
>> Just like freeing any memory. remoteproc driver does not free other
>> driver's memory only because processor crashed.
>>
> 
> That's a good comparison. Because when the firmware running on the
> remote processor crashes, it is indeed the job of the remoteproc driver
> to clean up the memory allocated to run the remote processor.


Best regards,
Krzysztof

[PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Chris Lew]

From: Richard Maina <quic_rmaina@quicinc.com>

When remoteproc goes down unexpectedly this results in a state where any
acquired hwspinlocks will remain locked possibly resulting in deadlock.
In order to ensure all locks are freed we include a call to
hwspin_lock_bust() during remoteproc shutdown.

For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
is used to take the hwspinlock. Remoteproc should use this id to try and
bust the lock on remoteproc stop.

This edge case only occurs with q6v5_pas watchdog crashes. The error
fatal case has handling to clear the hwspinlock before the error fatal
interrupt is triggered.

Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
index 54d8005d40a3..57178fcb9aa3 100644
--- a/drivers/remoteproc/qcom_q6v5_pas.c
+++ b/drivers/remoteproc/qcom_q6v5_pas.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
+#include <linux/hwspinlock.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -52,6 +53,7 @@ struct adsp_data {
 	const char *ssr_name;
 	const char *sysmon_name;
 	int ssctl_id;
+	int hwlock_id;
 
 	int region_assign_idx;
 	int region_assign_count;
@@ -84,6 +86,9 @@ struct qcom_adsp {
 	bool decrypt_shutdown;
 	const char *info_name;
 
+	struct hwspinlock *hwlock;
+	int hwlock_id;
+
 	const struct firmware *firmware;
 	const struct firmware *dtb_firmware;
 
@@ -399,6 +404,12 @@ static int adsp_stop(struct rproc *rproc)
 	if (handover)
 		qcom_pas_handover(&adsp->q6v5);
 
+	if (adsp->hwlock) {
+		ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
+		if (ret)
+			dev_info(adsp->dev, "failed to bust hwspinlock\n");
+	}
+
 	return ret;
 }
 
@@ -684,6 +695,7 @@ static int adsp_probe(struct platform_device *pdev)
 	struct rproc *rproc;
 	const char *fw_name, *dtb_fw_name = NULL;
 	const struct rproc_ops *ops = &adsp_ops;
+	int hwlock_idx;
 	int ret;
 
 	desc = of_device_get_match_data(&pdev->dev);
@@ -736,6 +748,17 @@ static int adsp_probe(struct platform_device *pdev)
 		adsp->dtb_firmware_name = dtb_fw_name;
 		adsp->dtb_pas_id = desc->dtb_pas_id;
 	}
+
+	if (desc->hwlock_id) {
+		adsp->hwlock_id = desc->hwlock_id;
+		hwlock_idx = of_hwspin_lock_get_id(pdev->dev.of_node, 0);
+		if (hwlock_idx >= 0) {
+			adsp->hwlock = hwspin_lock_request_specific(hwlock_idx);
+			if (!adsp->hwlock)
+				dev_err(&pdev->dev, "failed to request hwspinlock\n");
+		}
+	}
+
 	platform_set_drvdata(pdev, adsp);
 
 	ret = device_init_wakeup(adsp->dev, true);
@@ -1196,6 +1219,7 @@ static const struct adsp_data sm8550_adsp_resource = {
 	.ssr_name = "lpass",
 	.sysmon_name = "adsp",
 	.ssctl_id = 0x14,
+	.hwlock_id = 3,
 };
 
 static const struct adsp_data sm8550_cdsp_resource = {
@@ -1216,6 +1240,7 @@ static const struct adsp_data sm8550_cdsp_resource = {
 	.ssr_name = "cdsp",
 	.sysmon_name = "cdsp",
 	.ssctl_id = 0x17,
+	.hwlock_id = 6,
 };
 
 static const struct adsp_data sm8550_mpss_resource = {
@@ -1236,6 +1261,7 @@ static const struct adsp_data sm8550_mpss_resource = {
 	.ssr_name = "mpss",
 	.sysmon_name = "modem",
 	.ssctl_id = 0x12,
+	.hwlock_id = 2,
 	.region_assign_idx = 2,
 	.region_assign_count = 1,
 	.region_assign_vmid = QCOM_SCM_VMID_MSS_MSA,
@@ -1275,6 +1301,7 @@ static const struct adsp_data sm8650_cdsp_resource = {
 	.ssr_name = "cdsp",
 	.sysmon_name = "cdsp",
 	.ssctl_id = 0x17,
+	.hwlock_id = 6,
 	.region_assign_idx = 2,
 	.region_assign_count = 1,
 	.region_assign_shared = true,
@@ -1299,6 +1326,7 @@ static const struct adsp_data sm8650_mpss_resource = {
 	.ssr_name = "mpss",
 	.sysmon_name = "modem",
 	.ssctl_id = 0x12,
+	.hwlock_id = 2,
 	.region_assign_idx = 2,
 	.region_assign_count = 3,
 	.region_assign_vmid = QCOM_SCM_VMID_MSS_MSA,

-- 
2.25.1

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Mukesh Ojha]

On 5/17/2024 4:28 AM, Chris Lew wrote:
> From: Richard Maina <quic_rmaina@quicinc.com>
> 
> When remoteproc goes down unexpectedly this results in a state where any
> acquired hwspinlocks will remain locked possibly resulting in deadlock.
> In order to ensure all locks are freed we include a call to
> hwspin_lock_bust() during remoteproc shutdown.
> 
> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
> is used to take the hwspinlock. Remoteproc should use this id to try and
> bust the lock on remoteproc stop.
> 
> This edge case only occurs with q6v5_pas watchdog crashes. The error
> fatal case has handling to clear the hwspinlock before the error fatal
> interrupt is triggered.
> 
> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>   drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 54d8005d40a3..57178fcb9aa3 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -10,6 +10,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/firmware.h>
> +#include <linux/hwspinlock.h>
>   #include <linux/interrupt.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -52,6 +53,7 @@ struct adsp_data {
>   	const char *ssr_name;
>   	const char *sysmon_name;
>   	int ssctl_id;
> +	int hwlock_id;
>   
>   	int region_assign_idx;
>   	int region_assign_count;
> @@ -84,6 +86,9 @@ struct qcom_adsp {
>   	bool decrypt_shutdown;
>   	const char *info_name;
>   
> +	struct hwspinlock *hwlock;
> +	int hwlock_id;
> +
>   	const struct firmware *firmware;
>   	const struct firmware *dtb_firmware;
>   
> @@ -399,6 +404,12 @@ static int adsp_stop(struct rproc *rproc)
>   	if (handover)
>   		qcom_pas_handover(&adsp->q6v5);
>   
> +	if (adsp->hwlock) {
> +		ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
> +		if (ret)
> +			dev_info(adsp->dev, "failed to bust hwspinlock\n");
> +	}
> +

What if recovery is disabled, fatal case is clearing the hwspinlock but
wdog case is not, right ?

-Mukesh

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Mukesh Ojha]

On 5/17/2024 4:28 AM, Chris Lew wrote:
> From: Richard Maina <quic_rmaina@quicinc.com>
> 
> When remoteproc goes down unexpectedly this results in a state where any
> acquired hwspinlocks will remain locked possibly resulting in deadlock.
> In order to ensure all locks are freed we include a call to
> hwspin_lock_bust() during remoteproc shutdown.
> 
> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
> is used to take the hwspinlock. Remoteproc should use this id to try and
> bust the lock on remoteproc stop.
> 
> This edge case only occurs with q6v5_pas watchdog crashes. The error
> fatal case has handling to clear the hwspinlock before the error fatal
> interrupt is triggered.
> 
> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>   drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 54d8005d40a3..57178fcb9aa3 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -10,6 +10,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/firmware.h>
> +#include <linux/hwspinlock.h>
>   #include <linux/interrupt.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -52,6 +53,7 @@ struct adsp_data {
>   	const char *ssr_name;
>   	const char *sysmon_name;
>   	int ssctl_id;
> +	int hwlock_id;
>   
>   	int region_assign_idx;
>   	int region_assign_count;
> @@ -84,6 +86,9 @@ struct qcom_adsp {
>   	bool decrypt_shutdown;
>   	const char *info_name;
>   
> +	struct hwspinlock *hwlock;
> +	int hwlock_id;
> +
>   	const struct firmware *firmware;
>   	const struct firmware *dtb_firmware;
>   
> @@ -399,6 +404,12 @@ static int adsp_stop(struct rproc *rproc)
>   	if (handover)
>   		qcom_pas_handover(&adsp->q6v5);
>   
> +	if (adsp->hwlock) {
> +		ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
> +		if (ret)
> +			dev_info(adsp->dev, "failed to bust hwspinlock\n");
> +	}
> +

As you said above, you seem to cover only wdog case and fatal cases
are already handled at remote;

You are clearing here for both ? is this right understanding ?

-Mukesh

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Chris Lew]

On 5/17/2024 12:21 AM, Mukesh Ojha wrote:
> 
> 
> On 5/17/2024 4:28 AM, Chris Lew wrote:
>> From: Richard Maina <quic_rmaina@quicinc.com>
>>
>> When remoteproc goes down unexpectedly this results in a state where any
>> acquired hwspinlocks will remain locked possibly resulting in deadlock.
>> In order to ensure all locks are freed we include a call to
>> hwspin_lock_bust() during remoteproc shutdown.
>>
>> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
>> is used to take the hwspinlock. Remoteproc should use this id to try and
>> bust the lock on remoteproc stop.
>>
>> This edge case only occurs with q6v5_pas watchdog crashes. The error
>> fatal case has handling to clear the hwspinlock before the error fatal
>> interrupt is triggered.
>>
>> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> ---
>>   drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
>> b/drivers/remoteproc/qcom_q6v5_pas.c
>> index 54d8005d40a3..57178fcb9aa3 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/firmware.h>
>> +#include <linux/hwspinlock.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>> @@ -52,6 +53,7 @@ struct adsp_data {
>>       const char *ssr_name;
>>       const char *sysmon_name;
>>       int ssctl_id;
>> +    int hwlock_id;
>>       int region_assign_idx;
>>       int region_assign_count;
>> @@ -84,6 +86,9 @@ struct qcom_adsp {
>>       bool decrypt_shutdown;
>>       const char *info_name;
>> +    struct hwspinlock *hwlock;
>> +    int hwlock_id;
>> +
>>       const struct firmware *firmware;
>>       const struct firmware *dtb_firmware;
>> @@ -399,6 +404,12 @@ static int adsp_stop(struct rproc *rproc)
>>       if (handover)
>>           qcom_pas_handover(&adsp->q6v5);
>> +    if (adsp->hwlock) {
>> +        ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
>> +        if (ret)
>> +            dev_info(adsp->dev, "failed to bust hwspinlock\n");
>> +    }
>> +
> 
> As you said above, you seem to cover only wdog case and fatal cases
> are already handled at remote;
> 
> You are clearing here for both ? is this right understanding ?
> 

Yes that is correct. While the firmware is able to handle error fatal 
cases, I think it is still the responsibility of the remoteproc driver 
to try and bust the lock on behalf of the rproc in both cases.

The bust will only clear the spinlock if it has the token (specific id 
for that rproc) that is passed into hwspin_lock_bust, so it is safe to 
attempt this even if the value has been cleared by the rproc in the 
error fatal case.

> -Mukesh

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Bryan O'Donoghue]

On 17/05/2024 00:58, Chris Lew wrote:
> From: Richard Maina <quic_rmaina@quicinc.com>
> 
> When remoteproc goes down unexpectedly this results in a state where any
> acquired hwspinlocks will remain locked possibly resulting in deadlock.
> In order to ensure all locks are freed we include a call to
> hwspin_lock_bust() during remoteproc shutdown.
> 
> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
> is used to take the hwspinlock. Remoteproc should use this id to try and
> bust the lock on remoteproc stop.
> 
> This edge case only occurs with q6v5_pas watchdog crashes. The error
> fatal case has handling to clear the hwspinlock before the error fatal
> interrupt is triggered.
> 
> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---

> +	if (adsp->hwlock) {
> +		ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
> +		if (ret)
> +			dev_info(adsp->dev, "failed to bust hwspinlock\n");

qcom_hwspinlock_bust() already prints an error on failure, you're 
printing a second error here.

Choose at most one.

---
bod

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Chris Lew]

On 5/17/2024 2:08 AM, Bryan O'Donoghue wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
>> From: Richard Maina <quic_rmaina@quicinc.com>
>>
>> When remoteproc goes down unexpectedly this results in a state where any
>> acquired hwspinlocks will remain locked possibly resulting in deadlock.
>> In order to ensure all locks are freed we include a call to
>> hwspin_lock_bust() during remoteproc shutdown.
>>
>> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
>> is used to take the hwspinlock. Remoteproc should use this id to try and
>> bust the lock on remoteproc stop.
>>
>> This edge case only occurs with q6v5_pas watchdog crashes. The error
>> fatal case has handling to clear the hwspinlock before the error fatal
>> interrupt is triggered.
>>
>> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> ---
> 
>> +    if (adsp->hwlock) {
>> +        ret = hwspin_lock_bust(adsp->hwlock, adsp->hwlock_id);
>> +        if (ret)
>> +            dev_info(adsp->dev, "failed to bust hwspinlock\n");
> 
> qcom_hwspinlock_bust() already prints an error on failure, you're 
> printing a second error here.
> 
> Choose at most one.
> 

Ack, will remove the error print here and leave the one in 
qcom_hwspinlock_bust()

> ---
> bod

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Konrad Dybcio]

On 5/17/24 00:58, Chris Lew wrote:
> From: Richard Maina <quic_rmaina@quicinc.com>
> 
> When remoteproc goes down unexpectedly this results in a state where any
> acquired hwspinlocks will remain locked possibly resulting in deadlock.
> In order to ensure all locks are freed we include a call to
> hwspin_lock_bust() during remoteproc shutdown.
> 
> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
> is used to take the hwspinlock. Remoteproc should use this id to try and
> bust the lock on remoteproc stop.
> 
> This edge case only occurs with q6v5_pas watchdog crashes. The error
> fatal case has handling to clear the hwspinlock before the error fatal
> interrupt is triggered.
> 
> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---


>   drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 54d8005d40a3..57178fcb9aa3 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -10,6 +10,7 @@
>   #include <linux/clk.h>
>   #include <linux/delay.h>
>   #include <linux/firmware.h>
> +#include <linux/hwspinlock.h>
>   #include <linux/interrupt.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
> @@ -52,6 +53,7 @@ struct adsp_data {
>   	const char *ssr_name;
>   	const char *sysmon_name;
>   	int ssctl_id;
> +	int hwlock_id;
>   
>   	int region_assign_idx;
>   	int region_assign_count;
> @@ -84,6 +86,9 @@ struct qcom_adsp {
>   	bool decrypt_shutdown;
>   	const char *info_name;
>   
> +	struct hwspinlock *hwlock;
> +	int hwlock_id;

IIRC, this is the same one that is passed in the DT.

Can we get it dynamically from there?

Konrad

Re: [PATCH 6/7] remoteproc: qcom_q6v5_pas: Add hwspinlock bust on stop

[Chris Lew]

On 5/21/2024 10:38 AM, Konrad Dybcio wrote:
> 
> 
> On 5/17/24 00:58, Chris Lew wrote:
>> From: Richard Maina <quic_rmaina@quicinc.com>
>>
>> When remoteproc goes down unexpectedly this results in a state where any
>> acquired hwspinlocks will remain locked possibly resulting in deadlock.
>> In order to ensure all locks are freed we include a call to
>> hwspin_lock_bust() during remoteproc shutdown.
>>
>> For qcom_q6v5_pas remoteprocs, each remoteproc has an assigned id that
>> is used to take the hwspinlock. Remoteproc should use this id to try and
>> bust the lock on remoteproc stop.
>>
>> This edge case only occurs with q6v5_pas watchdog crashes. The error
>> fatal case has handling to clear the hwspinlock before the error fatal
>> interrupt is triggered.
>>
>> Signed-off-by: Richard Maina <quic_rmaina@quicinc.com>
>> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
>> ---
> 
> 
>>   drivers/remoteproc/qcom_q6v5_pas.c | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c 
>> b/drivers/remoteproc/qcom_q6v5_pas.c
>> index 54d8005d40a3..57178fcb9aa3 100644
>> --- a/drivers/remoteproc/qcom_q6v5_pas.c
>> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/clk.h>
>>   #include <linux/delay.h>
>>   #include <linux/firmware.h>
>> +#include <linux/hwspinlock.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>> @@ -52,6 +53,7 @@ struct adsp_data {
>>       const char *ssr_name;
>>       const char *sysmon_name;
>>       int ssctl_id;
>> +    int hwlock_id;
>>       int region_assign_idx;
>>       int region_assign_count;
>> @@ -84,6 +86,9 @@ struct qcom_adsp {
>>       bool decrypt_shutdown;
>>       const char *info_name;
>> +    struct hwspinlock *hwlock;
>> +    int hwlock_id;
> 
> IIRC, this is the same one that is passed in the DT.
> 
> Can we get it dynamically from there?
> 

The argument passed in DT is the index of the hwlock in the TCSR mutex 
region. The index determines use of hwlock[0..n]

This id is supposed to be the identifier that is passed into 
hwspin_lock_bust(). The actual value that we would read from 
hwlock[0..n] to see if we need to bust the lock.

Maybe the naming of this variable is confusing. Do you have any 
suggestions to make it clearer? Could call it hwlock_bust_id.

We could also try increasing the #hwlock-cells to 2 and have something 
like <&phandle index bust_id>. To me this seemed odd for clients that 
weren't planning on using the bust_id.

> Konrad

[PATCH 7/7] arm64: dts: qcom: sm8650: Add hwlock to remoteproc

[Chris Lew]

Add the hwlock property to remoteproc. This enables the remoteproc to
try and bust the smem hwspinlock if the remoteproc has crashed while
holding the hwspinlock.

Signed-off-by: Chris Lew <quic_clew@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 62a6e77730bc..a65a1679f003 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2878,6 +2878,7 @@ remoteproc_mpss: remoteproc@4080000 {
 
 			qcom,smem-states = <&smp2p_modem_out 0>;
 			qcom,smem-state-names = "stop";
+			hwlocks = <&tcsr_mutex 3>;
 
 			status = "disabled";
 
@@ -5024,6 +5025,7 @@ remoteproc_adsp: remoteproc@30000000 {
 
 			qcom,smem-states = <&smp2p_adsp_out 0>;
 			qcom,smem-state-names = "stop";
+			hwlocks = <&tcsr_mutex 3>;
 
 			status = "disabled";
 
@@ -5183,6 +5185,7 @@ remoteproc_cdsp: remoteproc@32300000 {
 
 			qcom,smem-states = <&smp2p_cdsp_out 0>;
 			qcom,smem-state-names = "stop";
+			hwlocks = <&tcsr_mutex 3>;
 
 			status = "disabled";
 

-- 
2.25.1

Re: [PATCH 7/7] arm64: dts: qcom: sm8650: Add hwlock to remoteproc

[Krzysztof Kozlowski]

On 17/05/2024 00:58, Chris Lew wrote:
> Add the hwlock property to remoteproc. This enables the remoteproc to
> try and bust the smem hwspinlock if the remoteproc has crashed while
> holding the hwspinlock.
> 
> Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 62a6e77730bc..a65a1679f003 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2878,6 +2878,7 @@ remoteproc_mpss: remoteproc@4080000 {
>  
>  			qcom,smem-states = <&smp2p_modem_out 0>;
>  			qcom,smem-state-names = "stop";
> +			hwlocks = <&tcsr_mutex 3>;

lock #3 is used by smem, so this proves you are taking someone else's
lock. I commented on this in the binding, but let's be specific:

NAK, please carry:

Nacked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH 7/7] arm64: dts: qcom: sm8650: Add hwlock to remoteproc

[Bjorn Andersson]

On Wed, May 22, 2024 at 09:27:29AM +0200, Krzysztof Kozlowski wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
> > Add the hwlock property to remoteproc. This enables the remoteproc to
> > try and bust the smem hwspinlock if the remoteproc has crashed while
> > holding the hwspinlock.
> > 
> > Signed-off-by: Chris Lew <quic_clew@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> > index 62a6e77730bc..a65a1679f003 100644
> > --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> > @@ -2878,6 +2878,7 @@ remoteproc_mpss: remoteproc@4080000 {
> >  
> >  			qcom,smem-states = <&smp2p_modem_out 0>;
> >  			qcom,smem-state-names = "stop";
> > +			hwlocks = <&tcsr_mutex 3>;
> 
> lock #3 is used by smem, so this proves you are taking someone else's
> lock.

The lock is a shared resource, it's not "some else's lock".

Regards,
Bjorn

> I commented on this in the binding, but let's be specific:
> 
> NAK, please carry:
> 
> Nacked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Best regards,
> Krzysztof
>