* [PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets
@ 2026-10-21 3:09 shengjiu.wang
2026-10-21 13:24 ` Philipp Zabel
0 siblings, 1 reply; 3+ messages in thread
From: shengjiu.wang @ 2026-10-21 3:09 UTC (permalink / raw)
To: p.zabel, krzk, hansg, linux-kernel, broonie, alvin.sipraga
From: Shengjiu Wang <shengjiu.wang@nxp.com>
reset_control_deassert() and reset_control_reset() are documented to
guarantee that the reset line has been deasserted, respectively that a
reset pulse has been issued, by the time they return.
For a shared reset only the first caller runs the hardware op; the other
callers just bump the refcount and return. When the op can sleep this
breaks the guarantee: a second caller can observe the bumped refcount and
return before the first caller's op has finished, and then touch hardware
that is still in reset.
Add a reset_controller_dev::can_sleep flag that a provider sets when its
reset ops may sleep. When it is set, serialize the shared refcount update
and the hardware op in reset_control_reset() and reset_control_deassert()
under a new per-control mutex, taken inside the SRCU read-side critical
section. Providers that leave the flag clear keep the existing lockless
path, so callers in atomic context are unaffected.
Set can_sleep in the reset-gpio driver, which drives the line with
gpiod_set_value_cansleep() and may therefore sleep. Other providers whose
reset ops may sleep still need to opt in.
Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Assisted-by: Anthropic:claude-opus-4-8
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
---
Changes in v3:
- Drop the reset_control_reset_atomic()/reset_control_deassert_atomic()
variants and their reset.h declarations/stubs; gate the mutex on
rcdev->can_sleep instead, so non-sleeping controllers keep the lockless
path and need no new API.
- Use a dedicated rstc->shared_lock taken inside the SRCU read-side section
instead of reusing rstc->lock before SRCU.
- Set can_sleep in reset-gpio; no other provider is converted.
- Drop the lockdep_assert_not_held() additions and the reset.rst / kerneldoc
text about sleeping-vs-atomic variants.
Changes in v2:
- Serialize only the shared may-sleep path under rstc->lock (taken before
SRCU to match reset_control_acquire()'s order); exclusive path stays
lockless, and reset_control_assert() is left unchanged.
- Add lockless reset_control_reset_atomic()/reset_control_deassert_atomic()
for atomic callers; gate the mutex on can_sleep so no shared consumer is
regressed and so it works on all configs.
- Factor the body into __reset_control_*()/__reset_control_*_locked()
helpers, and lockdep_assert_not_held() at each lock site to catch a
controller op re-entering the API on the same control.
- Declare the new variants and stubs in reset.h, and document the sleeping
vs atomic variants in the kerneldoc and reset.rst.
drivers/reset/core.c | 125 +++++++++++++++++++++----------
drivers/reset/reset-gpio.c | 1 +
include/linux/reset-controller.h | 3 +
3 files changed, 89 insertions(+), 40 deletions(-)
diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 38e189d04d09..dd6e47163f6c 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -52,6 +52,9 @@ static DEFINE_IDA(reset_gpio_ida);
* only used for shared resets, which means that the value
* will be either 0 or 1.
* @lock: serializes the internals of reset_control_acquire()
+ * @shared_lock: serializes the shared refcount update and the hardware op for
+ * controllers whose ops may sleep (rcdev->can_sleep); taken only
+ * inside the SRCU read-side critical section
*/
struct reset_control {
struct reset_controller_dev __rcu *rcdev;
@@ -65,6 +68,8 @@ struct reset_control {
atomic_t deassert_count;
atomic_t triggered_count;
struct mutex lock;
+ /* serializes the shared refcount update with its hardware op */
+ struct mutex shared_lock;
};
/**
@@ -355,6 +360,32 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
return rstc->array;
}
+static int __reset_control_reset(struct reset_control *rstc,
+ struct reset_controller_dev *rcdev)
+{
+ int ret;
+
+ if (!rcdev->ops->reset)
+ return -ENOTSUPP;
+
+ if (rstc->shared) {
+ if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
+ return -EINVAL;
+
+ if (atomic_inc_return(&rstc->triggered_count) != 1)
+ return 0;
+ } else {
+ if (!rstc->acquired)
+ return -EPERM;
+ }
+
+ ret = rcdev->ops->reset(rcdev, rstc->id);
+ if (rstc->shared && ret)
+ atomic_dec(&rstc->triggered_count);
+
+ return ret;
+}
+
/**
* reset_control_reset - reset the controlled device
* @rstc: reset controller
@@ -362,6 +393,9 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
* On a shared reset line the actual reset pulse is only triggered once for the
* lifetime of the reset_control instance: for all but the first caller this is
* a no-op.
+ *
+ * After calling this function, the reset is guaranteed to have seen a reset
+ * pulse at any point in the past, not necessarily during the function call.
* Consumers must not use reset_control_(de)assert on shared reset lines when
* reset_control_reset has been used.
*
@@ -371,7 +405,6 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
int reset_control_reset(struct reset_control *rstc)
{
struct reset_controller_dev *rcdev;
- int ret;
if (!rstc)
return 0;
@@ -388,25 +421,18 @@ int reset_control_reset(struct reset_control *rstc)
if (!rcdev)
return -ENODEV;
- if (!rcdev->ops->reset)
- return -ENOTSUPP;
-
- if (rstc->shared) {
- if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
- return -EINVAL;
-
- if (atomic_inc_return(&rstc->triggered_count) != 1)
- return 0;
- } else {
- if (!rstc->acquired)
- return -EPERM;
+ /*
+ * For a shared reset only the first caller runs the hardware op while
+ * concurrent callers bump the refcount and return. If the op may sleep
+ * (rcdev->can_sleep), serialize the refcount update and the op so a
+ * concurrent caller cannot return before the op has finished.
+ */
+ if (rstc->shared && rcdev->can_sleep) {
+ guard(mutex)(&rstc->shared_lock);
+ return __reset_control_reset(rstc, rcdev);
}
- ret = rcdev->ops->reset(rcdev, rstc->id);
- if (rstc->shared && ret)
- atomic_dec(&rstc->triggered_count);
-
- return ret;
+ return __reset_control_reset(rstc, rcdev);
}
EXPORT_SYMBOL_GPL(reset_control_reset);
@@ -575,6 +601,36 @@ int reset_control_bulk_assert(int num_rstcs,
}
EXPORT_SYMBOL_GPL(reset_control_bulk_assert);
+static int __reset_control_deassert(struct reset_control *rstc,
+ struct reset_controller_dev *rcdev)
+{
+ if (rstc->shared) {
+ if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
+ return -EINVAL;
+
+ if (atomic_inc_return(&rstc->deassert_count) != 1)
+ return 0;
+ } else {
+ if (!rstc->acquired) {
+ WARN(1, "reset %s (ID: %u) is not acquired\n",
+ rcdev_name(rcdev), rstc->id);
+ return -EPERM;
+ }
+ }
+
+ /*
+ * If the reset controller does not implement .deassert(), we assume
+ * that it handles self-deasserting reset lines via .reset(). In that
+ * case, the reset lines are deasserted by default. If that is not the
+ * case, the reset controller driver should implement .deassert() and
+ * return -ENOTSUPP.
+ */
+ if (!rcdev->ops->deassert)
+ return 0;
+
+ return rcdev->ops->deassert(rcdev, rstc->id);
+}
+
/**
* reset_control_deassert - deasserts the reset line
* @rstc: reset controller
@@ -605,31 +661,18 @@ int reset_control_deassert(struct reset_control *rstc)
if (!rcdev)
return -ENODEV;
- if (rstc->shared) {
- if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
- return -EINVAL;
-
- if (atomic_inc_return(&rstc->deassert_count) != 1)
- return 0;
- } else {
- if (!rstc->acquired) {
- WARN(1, "reset %s (ID: %u) is not acquired\n",
- rcdev_name(rcdev), rstc->id);
- return -EPERM;
- }
- }
-
/*
- * If the reset controller does not implement .deassert(), we assume
- * that it handles self-deasserting reset lines via .reset(). In that
- * case, the reset lines are deasserted by default. If that is not the
- * case, the reset controller driver should implement .deassert() and
- * return -ENOTSUPP.
+ * For a shared reset only the first caller runs the hardware op while
+ * concurrent callers bump the refcount and return. If the op may sleep
+ * (rcdev->can_sleep), serialize the refcount update and the op so a
+ * concurrent caller cannot return before the op has finished.
*/
- if (!rcdev->ops->deassert)
- return 0;
+ if (rstc->shared && rcdev->can_sleep) {
+ guard(mutex)(&rstc->shared_lock);
+ return __reset_control_deassert(rstc, rcdev);
+ }
- return rcdev->ops->deassert(rcdev, rstc->id);
+ return __reset_control_deassert(rstc, rcdev);
}
EXPORT_SYMBOL_GPL(reset_control_deassert);
@@ -879,6 +922,7 @@ __reset_control_get_internal(struct reset_controller_dev *rcdev,
rstc->id = index;
kref_init(&rstc->refcnt);
mutex_init(&rstc->lock);
+ mutex_init(&rstc->shared_lock);
rstc->acquired = acquired;
rstc->shared = shared;
get_device(rcdev->dev);
@@ -901,6 +945,7 @@ static void __reset_control_release(struct kref *kref)
}
mutex_destroy(&rstc->lock);
+ mutex_destroy(&rstc->shared_lock);
}
static void reset_control_put_internal(struct reset_control *rstc)
diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index bf95296a512e..b4a198792107 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -67,6 +67,7 @@ static int reset_gpio_probe(struct auxiliary_device *adev,
"Could not get reset gpios\n");
priv->rc.ops = &reset_gpio_ops;
+ priv->rc.can_sleep = true;
priv->rc.owner = THIS_MODULE;
priv->rc.dev = dev;
diff --git a/include/linux/reset-controller.h b/include/linux/reset-controller.h
index 52a5a4e81f18..7318c07b80e1 100644
--- a/include/linux/reset-controller.h
+++ b/include/linux/reset-controller.h
@@ -47,6 +47,8 @@ struct of_phandle_args;
* id as given to the reset control ops, defaults to
* :c:func:`fwnode_reset_simple_xlate`
* @nr_resets: number of reset controls in this reset controller device
+ * @can_sleep: set by the provider if its reset ops may sleep; makes the core
+ * serialize the shared reset and deassert paths under a mutex
* @lock: protects the reset control list from concurrent access
*/
struct reset_controller_dev {
@@ -64,6 +66,7 @@ struct reset_controller_dev {
int (*fwnode_xlate)(struct reset_controller_dev *rcdev,
const struct fwnode_reference_args *reset_spec);
unsigned int nr_resets;
+ bool can_sleep;
struct mutex lock;
};
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets
2026-10-21 3:09 [PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets shengjiu.wang
@ 2026-10-21 13:24 ` Philipp Zabel
2026-10-21 14:16 ` Alvin Šipraga
0 siblings, 1 reply; 3+ messages in thread
From: Philipp Zabel @ 2026-10-21 13:24 UTC (permalink / raw)
To: shengjiu.wang, krzk, hansg, linux-kernel, broonie, alvin.sipraga
On Mi, 2026-10-21 at 11:09 +0800, shengjiu.wang@oss.nxp.com wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
>
> reset_control_deassert() and reset_control_reset() are documented to
> guarantee that the reset line has been deasserted, respectively that a
> reset pulse has been issued, by the time they return.
>
> For a shared reset only the first caller runs the hardware op; the other
> callers just bump the refcount and return. When the op can sleep this
> breaks the guarantee: a second caller can observe the bumped refcount and
> return before the first caller's op has finished, and then touch hardware
> that is still in reset.
>
> Add a reset_controller_dev::can_sleep flag that a provider sets when its
> reset ops may sleep. When it is set, serialize the shared refcount update
> and the hardware op in reset_control_reset() and reset_control_deassert()
> under a new per-control mutex, taken inside the SRCU read-side critical
> section. Providers that leave the flag clear keep the existing lockless
> path, so callers in atomic context are unaffected.
I think this paragraph should go first.
> Set can_sleep in the reset-gpio driver, which drives the line with
> gpiod_set_value_cansleep() and may therefore sleep. Other providers whose
> reset ops may sleep still need to opt in.
Please split this out into a separate patch.
> Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
This is a partial fix for an issue introduced by 0b52297f2288, but only
for reset-gpio, which didn't exist back then.
On the other hand, the patch doesn't apply before commit 8c91302a29bc1
("reset: protect struct reset_control with its own mutex"), which only
allows this to be trivially backported to v7.1, with adjustments needed
for older kernels.
The commit this is supposed to fix, 8a0de73cf9dc ("ASoC: ak4458: add
optional reset control to instead of gpio"), is in v6.2. I'm not sure
if something changed between v6.2 and now that exposed this issue for
ak4458?
In short, do you have an opinion how (far) this should be backported,
and does this warrant some Cc:stable tag?
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Assisted-by: Anthropic:claude-opus-4-8
Drop, see 1e035297ffda ("docs: remove Assisted-by references").
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> ---
> Changes in v3:
> - Drop the reset_control_reset_atomic()/reset_control_deassert_atomic()
> variants and their reset.h declarations/stubs; gate the mutex on
> rcdev->can_sleep instead, so non-sleeping controllers keep the lockless
> path and need no new API.
> - Use a dedicated rstc->shared_lock taken inside the SRCU read-side section
> instead of reusing rstc->lock before SRCU.
> - Set can_sleep in reset-gpio; no other provider is converted.
> - Drop the lockdep_assert_not_held() additions and the reset.rst / kerneldoc
> text about sleeping-vs-atomic variants.
>
> Changes in v2:
> - Serialize only the shared may-sleep path under rstc->lock (taken before
> SRCU to match reset_control_acquire()'s order); exclusive path stays
> lockless, and reset_control_assert() is left unchanged.
> - Add lockless reset_control_reset_atomic()/reset_control_deassert_atomic()
> for atomic callers; gate the mutex on can_sleep so no shared consumer is
> regressed and so it works on all configs.
> - Factor the body into __reset_control_*()/__reset_control_*_locked()
> helpers, and lockdep_assert_not_held() at each lock site to catch a
> controller op re-entering the API on the same control.
> - Declare the new variants and stubs in reset.h, and document the sleeping
> vs atomic variants in the kerneldoc and reset.rst.
>
> drivers/reset/core.c | 125 +++++++++++++++++++++----------
> drivers/reset/reset-gpio.c | 1 +
> include/linux/reset-controller.h | 3 +
> 3 files changed, 89 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 38e189d04d09..dd6e47163f6c 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -52,6 +52,9 @@ static DEFINE_IDA(reset_gpio_ida);
> * only used for shared resets, which means that the value
> * will be either 0 or 1.
> * @lock: serializes the internals of reset_control_acquire()
> + * @shared_lock: serializes the shared refcount update and the hardware op for
> + * controllers whose ops may sleep (rcdev->can_sleep); taken only
> + * inside the SRCU read-side critical section
> */
> struct reset_control {
> struct reset_controller_dev __rcu *rcdev;
> @@ -65,6 +68,8 @@ struct reset_control {
> atomic_t deassert_count;
> atomic_t triggered_count;
> struct mutex lock;
> + /* serializes the shared refcount update with its hardware op */
> + struct mutex shared_lock;
> };
>
> /**
> @@ -355,6 +360,32 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> return rstc->array;
> }
>
> +static int __reset_control_reset(struct reset_control *rstc,
> + struct reset_controller_dev *rcdev)
> +{
> + int ret;
> +
> + if (!rcdev->ops->reset)
> + return -ENOTSUPP;
> +
> + if (rstc->shared) {
> + if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
> + return -EINVAL;
> +
> + if (atomic_inc_return(&rstc->triggered_count) != 1)
> + return 0;
> + } else {
> + if (!rstc->acquired)
> + return -EPERM;
> + }
> +
> + ret = rcdev->ops->reset(rcdev, rstc->id);
> + if (rstc->shared && ret)
> + atomic_dec(&rstc->triggered_count);
> +
> + return ret;
> +}
> +
> /**
> * reset_control_reset - reset the controlled device
> * @rstc: reset controller
> @@ -362,6 +393,9 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> * On a shared reset line the actual reset pulse is only triggered once for the
> * lifetime of the reset_control instance: for all but the first caller this is
> * a no-op.
> + *
> + * After calling this function, the reset is guaranteed to have seen a reset
s/reset/reset line/
The sashiko bot constructed an unlikely but theoretically possible
scenario that requires to lock _assert() after all:
https://sashiko.dev/#/patchset/20261021030925.3543841-1-shengjiu.wang%40oss.nxp.com
Disregard the other issue, deassert_count leaking on ops->deassert()
errors. It is pre-existing and can't be as easily fixed because
consumers adapted to it.
regards
Philipp
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets
2026-10-21 13:24 ` Philipp Zabel
@ 2026-10-21 14:16 ` Alvin Šipraga
0 siblings, 0 replies; 3+ messages in thread
From: Alvin Šipraga @ 2026-10-21 14:16 UTC (permalink / raw)
To: Philipp Zabel; +Cc: shengjiu.wang, krzk, hansg, linux-kernel, broonie
On Wed, Oct 21, 2026 at 03:24:23PM +0200, Philipp Zabel wrote:
> On Mi, 2026-10-21 at 11:09 +0800, shengjiu.wang@oss.nxp.com wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > reset_control_deassert() and reset_control_reset() are documented to
> > guarantee that the reset line has been deasserted, respectively that a
> > reset pulse has been issued, by the time they return.
> >
> > For a shared reset only the first caller runs the hardware op; the other
> > callers just bump the refcount and return. When the op can sleep this
> > breaks the guarantee: a second caller can observe the bumped refcount and
> > return before the first caller's op has finished, and then touch hardware
> > that is still in reset.
> >
> > Add a reset_controller_dev::can_sleep flag that a provider sets when its
> > reset ops may sleep. When it is set, serialize the shared refcount update
> > and the hardware op in reset_control_reset() and reset_control_deassert()
> > under a new per-control mutex, taken inside the SRCU read-side critical
> > section. Providers that leave the flag clear keep the existing lockless
> > path, so callers in atomic context are unaffected.
>
> I think this paragraph should go first.
>
> > Set can_sleep in the reset-gpio driver, which drives the line with
> > gpiod_set_value_cansleep() and may therefore sleep. Other providers whose
> > reset ops may sleep still need to opt in.
>
> Please split this out into a separate patch.
>
> > Fixes: 0b52297f2288 ("reset: Add support for shared reset controls")
>
> This is a partial fix for an issue introduced by 0b52297f2288, but only
> for reset-gpio, which didn't exist back then.
>
> On the other hand, the patch doesn't apply before commit 8c91302a29bc1
> ("reset: protect struct reset_control with its own mutex"), which only
> allows this to be trivially backported to v7.1, with adjustments needed
> for older kernels.
>
> The commit this is supposed to fix, 8a0de73cf9dc ("ASoC: ak4458: add
> optional reset control to instead of gpio"), is in v6.2. I'm not sure
> if something changed between v6.2 and now that exposed this issue for
> ak4458?
>
> In short, do you have an opinion how (far) this should be backported,
> and does this warrant some Cc:stable tag?
>
> > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Assisted-by: Anthropic:claude-opus-4-8
>
> Drop, see 1e035297ffda ("docs: remove Assisted-by references").
>
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > Link: https://lore.kernel.org/all/aszDZlJ0KahNwgZO@analog.com/
> > ---
> > Changes in v3:
> > - Drop the reset_control_reset_atomic()/reset_control_deassert_atomic()
> > variants and their reset.h declarations/stubs; gate the mutex on
> > rcdev->can_sleep instead, so non-sleeping controllers keep the lockless
> > path and need no new API.
> > - Use a dedicated rstc->shared_lock taken inside the SRCU read-side section
> > instead of reusing rstc->lock before SRCU.
> > - Set can_sleep in reset-gpio; no other provider is converted.
> > - Drop the lockdep_assert_not_held() additions and the reset.rst / kerneldoc
> > text about sleeping-vs-atomic variants.
> >
> > Changes in v2:
> > - Serialize only the shared may-sleep path under rstc->lock (taken before
> > SRCU to match reset_control_acquire()'s order); exclusive path stays
> > lockless, and reset_control_assert() is left unchanged.
> > - Add lockless reset_control_reset_atomic()/reset_control_deassert_atomic()
> > for atomic callers; gate the mutex on can_sleep so no shared consumer is
> > regressed and so it works on all configs.
> > - Factor the body into __reset_control_*()/__reset_control_*_locked()
> > helpers, and lockdep_assert_not_held() at each lock site to catch a
> > controller op re-entering the API on the same control.
> > - Declare the new variants and stubs in reset.h, and document the sleeping
> > vs atomic variants in the kerneldoc and reset.rst.
> >
> > drivers/reset/core.c | 125 +++++++++++++++++++++----------
> > drivers/reset/reset-gpio.c | 1 +
> > include/linux/reset-controller.h | 3 +
> > 3 files changed, 89 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 38e189d04d09..dd6e47163f6c 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -52,6 +52,9 @@ static DEFINE_IDA(reset_gpio_ida);
> > * only used for shared resets, which means that the value
> > * will be either 0 or 1.
> > * @lock: serializes the internals of reset_control_acquire()
> > + * @shared_lock: serializes the shared refcount update and the hardware op for
> > + * controllers whose ops may sleep (rcdev->can_sleep); taken only
> > + * inside the SRCU read-side critical section
> > */
> > struct reset_control {
> > struct reset_controller_dev __rcu *rcdev;
> > @@ -65,6 +68,8 @@ struct reset_control {
> > atomic_t deassert_count;
> > atomic_t triggered_count;
> > struct mutex lock;
> > + /* serializes the shared refcount update with its hardware op */
> > + struct mutex shared_lock;
> > };
> >
> > /**
> > @@ -355,6 +360,32 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> > return rstc->array;
> > }
> >
> > +static int __reset_control_reset(struct reset_control *rstc,
> > + struct reset_controller_dev *rcdev)
> > +{
> > + int ret;
> > +
> > + if (!rcdev->ops->reset)
> > + return -ENOTSUPP;
> > +
> > + if (rstc->shared) {
> > + if (WARN_ON(atomic_read(&rstc->deassert_count) != 0))
> > + return -EINVAL;
> > +
> > + if (atomic_inc_return(&rstc->triggered_count) != 1)
> > + return 0;
> > + } else {
> > + if (!rstc->acquired)
> > + return -EPERM;
> > + }
> > +
> > + ret = rcdev->ops->reset(rcdev, rstc->id);
> > + if (rstc->shared && ret)
> > + atomic_dec(&rstc->triggered_count);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * reset_control_reset - reset the controlled device
> > * @rstc: reset controller
> > @@ -362,6 +393,9 @@ static inline bool reset_control_is_array(struct reset_control *rstc)
> > * On a shared reset line the actual reset pulse is only triggered once for the
> > * lifetime of the reset_control instance: for all but the first caller this is
> > * a no-op.
> > + *
> > + * After calling this function, the reset is guaranteed to have seen a reset
>
> s/reset/reset line/
>
> The sashiko bot constructed an unlikely but theoretically possible
> scenario that requires to lock _assert() after all:
>
> https://sashiko.dev/#/patchset/20261021030925.3543841-1-shengjiu.wang%40oss.nxp.com
Looking forward a bit to the second phase where we have everything
serialized, maybe it will make sense to also lock around the call to the
.status driver op too? Not suggesting we do it here (as this is just a
bugfix), but in general I like when subsystems offer a guarantee that
driver ops can't be called concurrently. Also the status is kind of
undefined if you're currently in the process of manipulating the reset
line.
What I had in mind later on was to rename what Shengjiu has called
shared_lock to ops_lock instead. With the following primitives:
can_sleep? CONTROL TYPE LOCKING PRIMITIVE
--------------------------------------------------
no shared spin_lock_irqsave
yes shared mutex
no exclusive none? see [*]
yes exclusive none? see [*]
Where all reset control operation functions look like:
reset_control_op() {
...
lock(ops_lock);
__reset_control_op(); /* inc/dec, ->op() call, etc. here */
unlock(ops_lock);
}
[*] I'm not sure if it makes sense to add any locking here? I can only
see it as API misuse if somebody is manipulating an exclusive reset
control from two places at once.
Just some thoughts, not particularly relevant to Shengjiu's change :)
Kind regards,
Alvin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-21 14:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-21 3:09 [PATCH v3] reset: core: Serialize concurrent deassert/reset of shared resets shengjiu.wang
2026-10-21 13:24 ` Philipp Zabel
2026-10-21 14:16 ` Alvin Šipraga
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®