| Message ID | 20240516-hwspinlock-bust-v1-3-47a90a859238@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for hwspinlock bust | expand |
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
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
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;