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 <[email protected]>
---
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 <[email protected]>
From: Richard Maina <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Lew <[email protected]>
---
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
From: Richard Maina <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Lew <[email protected]>
---
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
From: Richard Maina <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Lew <[email protected]>
---
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
From: Richard Maina <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Lew <[email protected]>
---
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
On 17/05/2024 00:58, Chris Lew wrote:
> From: Richard Maina <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chris Lew <[email protected]>
> ---
> 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
On 17/05/2024 10:07, Bryan O'Donoghue wrote:
> On 17/05/2024 00:58, Chris Lew wrote:
>> From: Richard Maina <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chris Lew <[email protected]>
>> ---
>> 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
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
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