[PATCH] revocable: hide the implementation details from users

[PATCH] revocable: hide the implementation details from users

[Bartosz Golaszewski]

Revocable stacks two layers of SRCU on top of each other: one to protect
the actual revocable resource and another to synchronize the revoking.
While this design itself is questionable, it also forces the user of
revokable to think about the implementation details and annotate the
pointer holding the address of the revocable_provider struct with __rcu.
Hide the real type of struct revokable_provider behind a typedef to free
the users from this responsability. While adding new typedefs goes
against current guidelines, it's still better than the current
requirement.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
I realized that one important person was missing from the whole review
process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
at the SRCU usage in GPIO and I think he should have also signed off on
revocable before it got queued.

Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
The series that implemented it and made its way into v7.0 is here:

  https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/

Could you please take a look and say if the design looks sane to you?
Especially the double SRCU on the revocable_provider.

 drivers/base/revocable.c                           |  8 ++++----
 drivers/base/revocable_test.c                      | 14 +++++++-------
 include/linux/revocable.h                          |  8 +++++---
 .../base/revocable/test_modules/revocable_test.c   |  4 ++--
 4 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
index 8532ca6a371c..02dd3ec2c9ad 100644
--- a/drivers/base/revocable.c
+++ b/drivers/base/revocable.c
@@ -82,7 +82,7 @@ struct revocable_provider {
  * Return: The pointer of struct revocable_provider.  NULL on errors.
  * It enforces the caller handles the returned pointer in RCU ways.
  */
-struct revocable_provider __rcu *revocable_provider_alloc(void *res)
+revocable_provider_t revocable_provider_alloc(void *res)
 {
 	struct revocable_provider *rp;
 
@@ -94,7 +94,7 @@ struct revocable_provider __rcu *revocable_provider_alloc(void *res)
 	RCU_INIT_POINTER(rp->res, res);
 	kref_init(&rp->kref);
 
-	return (struct revocable_provider __rcu *)rp;
+	return (revocable_provider_t)rp;
 }
 EXPORT_SYMBOL_GPL(revocable_provider_alloc);
 
@@ -120,7 +120,7 @@ static void revocable_provider_release(struct kref *kref)
  * It enforces the caller to pass a pointer of pointer of resource provider so
  * that it sets \*rp_ptr to NULL to prevent from keeping a dangling pointer.
  */
-void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
+void revocable_provider_revoke(revocable_provider_t *rp_ptr)
 {
 	struct revocable_provider *rp;
 
@@ -143,7 +143,7 @@ EXPORT_SYMBOL_GPL(revocable_provider_revoke);
  *
  * Return: 0 on success, -errno otherwise.
  */
-int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
+int revocable_init(revocable_provider_t _rp, struct revocable *rev)
 {
 	struct revocable_provider *rp;
 
diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
index 27f5d7d96f4b..732197c887ef 100644
--- a/drivers/base/revocable_test.c
+++ b/drivers/base/revocable_test.c
@@ -30,7 +30,7 @@
 
 static void revocable_test_basic(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	struct revocable rev;
 	void *real_res = (void *)0x12345678, *res;
 	int ret;
@@ -52,7 +52,7 @@ static void revocable_test_basic(struct kunit *test)
 
 static void revocable_test_revocation(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	struct revocable rev;
 	void *real_res = (void *)0x12345678, *res;
 	int ret;
@@ -79,7 +79,7 @@ static void revocable_test_revocation(struct kunit *test)
 
 static void revocable_test_try_access_macro(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	void *real_res = (void *)0x12345678, *res;
 
 	rp = revocable_provider_alloc(real_res);
@@ -101,7 +101,7 @@ static void revocable_test_try_access_macro(struct kunit *test)
 
 static void revocable_test_try_access_macro2(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	void *real_res = (void *)0x12345678, *res;
 	bool accessed;
 
@@ -128,7 +128,7 @@ static void revocable_test_try_access_macro2(struct kunit *test)
 
 static void revocable_test_provider_use_after_free(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	struct revocable_provider *old_rp;
 	struct revocable rev;
 	void *real_res = (void *)0x12345678;
@@ -167,7 +167,7 @@ static void revocable_test_provider_use_after_free(struct kunit *test)
 
 struct test_concurrent_access_context {
 	struct kunit *test;
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	struct revocable rev;
 	struct completion started, enter, exit;
 	struct task_struct *thread;
@@ -206,7 +206,7 @@ static int test_concurrent_access_consumer(void *data)
 
 static void revocable_test_concurrent_access(struct kunit *test)
 {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	void *real_res = (void *)0x12345678;
 	struct test_concurrent_access_context *ctx;
 	int ret, i;
diff --git a/include/linux/revocable.h b/include/linux/revocable.h
index e3d6d2c953a3..f0aea6f56a25 100644
--- a/include/linux/revocable.h
+++ b/include/linux/revocable.h
@@ -12,6 +12,8 @@
 struct device;
 struct revocable_provider;
 
+typedef struct revocable_provider __rcu *revocable_provider_t;
+
 /**
  * struct revocable - A handle for resource consumer.
  * @rp: The pointer of resource provider.
@@ -22,10 +24,10 @@ struct revocable {
 	int idx;
 };
 
-struct revocable_provider __rcu *revocable_provider_alloc(void *res);
-void revocable_provider_revoke(struct revocable_provider __rcu **rp);
+revocable_provider_t revocable_provider_alloc(void *res);
+void revocable_provider_revoke(revocable_provider_t *rp);
 
-int revocable_init(struct revocable_provider __rcu *rp, struct revocable *rev);
+int revocable_init(revocable_provider_t rp, struct revocable *rev);
 void revocable_deinit(struct revocable *rev);
 void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu);
 void revocable_withdraw_access(struct revocable *rev) __releases(&rev->rp->srcu);
diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
index a560ceda7318..30cc9c145725 100644
--- a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
+++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
@@ -17,7 +17,7 @@
 static struct dentry *debugfs_dir;
 
 struct revocable_test_provider_priv {
-	struct revocable_provider __rcu *rp;
+	revocable_provider_t rp;
 	struct dentry *dentry;
 	char res[16];
 };
@@ -37,7 +37,7 @@ static ssize_t revocable_test_consumer_read(struct file *filp,
 	char data[16];
 	size_t len;
 	struct revocable rev;
-	struct revocable_provider __rcu *rp = filp->private_data;
+	revocable_provider_t rp = filp->private_data;
 
 	switch (*offset) {
 	case 0:
-- 
2.47.3

Re: [PATCH] revocable: hide the implementation details from users

[Greg Kroah-Hartman]

On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
> -struct revocable_provider __rcu *revocable_provider_alloc(void *res)
> +revocable_provider_t revocable_provider_alloc(void *res)

While I understand why you did this, and it does save us the "__rcu"
usage which is essencial, it still makes me feel dirty seeing it :)

Also, as the __rcu pointer is now "hidden", what is that going to mean
for sparse usage?  Will this accidentally trigger problems if we do
anything with the pointer incorrectly?

thanks,

greg k-h

Re: [PATCH] revocable: hide the implementation details from users

[Paul E. McKenney]

On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
> Revocable stacks two layers of SRCU on top of each other: one to protect
> the actual revocable resource and another to synchronize the revoking.
> While this design itself is questionable, it also forces the user of
> revokable to think about the implementation details and annotate the
> pointer holding the address of the revocable_provider struct with __rcu.
> Hide the real type of struct revokable_provider behind a typedef to free
> the users from this responsability. While adding new typedefs goes
> against current guidelines, it's still better than the current
> requirement.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
> I realized that one important person was missing from the whole review
> process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
> at the SRCU usage in GPIO and I think he should have also signed off on
> revocable before it got queued.
> 
> Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
> The series that implemented it and made its way into v7.0 is here:
> 
>   https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/
> 
> Could you please take a look and say if the design looks sane to you?
> Especially the double SRCU on the revocable_provider.

The first patch in the above URL adds SRCU, and the other
two add various tests.  I do not see a double SRCU, just an
srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
in revocable_withdraw_access().

You are allowed to nest srcu_read_lock(), if that is what you are asking.
*However*, nesting revocable_try_access() on the same revocable structure
is buggy because the second call to revocable_try_access() would overwrite
the rp->srcu value written by the first call.  This could result in both
SRCU grace-period hangs and too-short SRCU grace periods, more likely
the former than the latter.

Or do you mean something else by "double SRCU"?

							Thanx, Paul

>  drivers/base/revocable.c                           |  8 ++++----
>  drivers/base/revocable_test.c                      | 14 +++++++-------
>  include/linux/revocable.h                          |  8 +++++---
>  .../base/revocable/test_modules/revocable_test.c   |  4 ++--
>  4 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/base/revocable.c b/drivers/base/revocable.c
> index 8532ca6a371c..02dd3ec2c9ad 100644
> --- a/drivers/base/revocable.c
> +++ b/drivers/base/revocable.c
> @@ -82,7 +82,7 @@ struct revocable_provider {
>   * Return: The pointer of struct revocable_provider.  NULL on errors.
>   * It enforces the caller handles the returned pointer in RCU ways.
>   */
> -struct revocable_provider __rcu *revocable_provider_alloc(void *res)
> +revocable_provider_t revocable_provider_alloc(void *res)
>  {
>  	struct revocable_provider *rp;
>  
> @@ -94,7 +94,7 @@ struct revocable_provider __rcu *revocable_provider_alloc(void *res)
>  	RCU_INIT_POINTER(rp->res, res);
>  	kref_init(&rp->kref);
>  
> -	return (struct revocable_provider __rcu *)rp;
> +	return (revocable_provider_t)rp;
>  }
>  EXPORT_SYMBOL_GPL(revocable_provider_alloc);
>  
> @@ -120,7 +120,7 @@ static void revocable_provider_release(struct kref *kref)
>   * It enforces the caller to pass a pointer of pointer of resource provider so
>   * that it sets \*rp_ptr to NULL to prevent from keeping a dangling pointer.
>   */
> -void revocable_provider_revoke(struct revocable_provider __rcu **rp_ptr)
> +void revocable_provider_revoke(revocable_provider_t *rp_ptr)
>  {
>  	struct revocable_provider *rp;
>  
> @@ -143,7 +143,7 @@ EXPORT_SYMBOL_GPL(revocable_provider_revoke);
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int revocable_init(struct revocable_provider __rcu *_rp, struct revocable *rev)
> +int revocable_init(revocable_provider_t _rp, struct revocable *rev)
>  {
>  	struct revocable_provider *rp;
>  
> diff --git a/drivers/base/revocable_test.c b/drivers/base/revocable_test.c
> index 27f5d7d96f4b..732197c887ef 100644
> --- a/drivers/base/revocable_test.c
> +++ b/drivers/base/revocable_test.c
> @@ -30,7 +30,7 @@
>  
>  static void revocable_test_basic(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	struct revocable rev;
>  	void *real_res = (void *)0x12345678, *res;
>  	int ret;
> @@ -52,7 +52,7 @@ static void revocable_test_basic(struct kunit *test)
>  
>  static void revocable_test_revocation(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	struct revocable rev;
>  	void *real_res = (void *)0x12345678, *res;
>  	int ret;
> @@ -79,7 +79,7 @@ static void revocable_test_revocation(struct kunit *test)
>  
>  static void revocable_test_try_access_macro(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	void *real_res = (void *)0x12345678, *res;
>  
>  	rp = revocable_provider_alloc(real_res);
> @@ -101,7 +101,7 @@ static void revocable_test_try_access_macro(struct kunit *test)
>  
>  static void revocable_test_try_access_macro2(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	void *real_res = (void *)0x12345678, *res;
>  	bool accessed;
>  
> @@ -128,7 +128,7 @@ static void revocable_test_try_access_macro2(struct kunit *test)
>  
>  static void revocable_test_provider_use_after_free(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	struct revocable_provider *old_rp;
>  	struct revocable rev;
>  	void *real_res = (void *)0x12345678;
> @@ -167,7 +167,7 @@ static void revocable_test_provider_use_after_free(struct kunit *test)
>  
>  struct test_concurrent_access_context {
>  	struct kunit *test;
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	struct revocable rev;
>  	struct completion started, enter, exit;
>  	struct task_struct *thread;
> @@ -206,7 +206,7 @@ static int test_concurrent_access_consumer(void *data)
>  
>  static void revocable_test_concurrent_access(struct kunit *test)
>  {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	void *real_res = (void *)0x12345678;
>  	struct test_concurrent_access_context *ctx;
>  	int ret, i;
> diff --git a/include/linux/revocable.h b/include/linux/revocable.h
> index e3d6d2c953a3..f0aea6f56a25 100644
> --- a/include/linux/revocable.h
> +++ b/include/linux/revocable.h
> @@ -12,6 +12,8 @@
>  struct device;
>  struct revocable_provider;
>  
> +typedef struct revocable_provider __rcu *revocable_provider_t;
> +
>  /**
>   * struct revocable - A handle for resource consumer.
>   * @rp: The pointer of resource provider.
> @@ -22,10 +24,10 @@ struct revocable {
>  	int idx;
>  };
>  
> -struct revocable_provider __rcu *revocable_provider_alloc(void *res);
> -void revocable_provider_revoke(struct revocable_provider __rcu **rp);
> +revocable_provider_t revocable_provider_alloc(void *res);
> +void revocable_provider_revoke(revocable_provider_t *rp);
>  
> -int revocable_init(struct revocable_provider __rcu *rp, struct revocable *rev);
> +int revocable_init(revocable_provider_t rp, struct revocable *rev);
>  void revocable_deinit(struct revocable *rev);
>  void *revocable_try_access(struct revocable *rev) __acquires(&rev->rp->srcu);
>  void revocable_withdraw_access(struct revocable *rev) __releases(&rev->rp->srcu);
> diff --git a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
> index a560ceda7318..30cc9c145725 100644
> --- a/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
> +++ b/tools/testing/selftests/drivers/base/revocable/test_modules/revocable_test.c
> @@ -17,7 +17,7 @@
>  static struct dentry *debugfs_dir;
>  
>  struct revocable_test_provider_priv {
> -	struct revocable_provider __rcu *rp;
> +	revocable_provider_t rp;
>  	struct dentry *dentry;
>  	char res[16];
>  };
> @@ -37,7 +37,7 @@ static ssize_t revocable_test_consumer_read(struct file *filp,
>  	char data[16];
>  	size_t len;
>  	struct revocable rev;
> -	struct revocable_provider __rcu *rp = filp->private_data;
> +	revocable_provider_t rp = filp->private_data;
>  
>  	switch (*offset) {
>  	case 0:
> -- 
> 2.47.3
> 

Re: [PATCH] revocable: hide the implementation details from users

[Bartosz Golaszewski]

On Fri, 6 Feb 2026 23:02:44 +0100, "Paul E. McKenney" <paulmck@kernel.org> said:
> On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
>> Revocable stacks two layers of SRCU on top of each other: one to protect
>> the actual revocable resource and another to synchronize the revoking.
>> While this design itself is questionable, it also forces the user of
>> revokable to think about the implementation details and annotate the
>> pointer holding the address of the revocable_provider struct with __rcu.
>> Hide the real type of struct revokable_provider behind a typedef to free
>> the users from this responsability. While adding new typedefs goes
>> against current guidelines, it's still better than the current
>> requirement.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>> ---
>> I realized that one important person was missing from the whole review
>> process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
>> at the SRCU usage in GPIO and I think he should have also signed off on
>> revocable before it got queued.
>>
>> Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
>> The series that implemented it and made its way into v7.0 is here:
>>
>>   https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/
>>
>> Could you please take a look and say if the design looks sane to you?
>> Especially the double SRCU on the revocable_provider.
>
> The first patch in the above URL adds SRCU, and the other
> two add various tests.  I do not see a double SRCU, just an
> srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
> in revocable_withdraw_access().
>
> You are allowed to nest srcu_read_lock(), if that is what you are asking.
> *However*, nesting revocable_try_access() on the same revocable structure
> is buggy because the second call to revocable_try_access() would overwrite
> the rp->srcu value written by the first call.  This could result in both
> SRCU grace-period hangs and too-short SRCU grace periods, more likely
> the former than the latter.
>
> Or do you mean something else by "double SRCU"?
>

This series didn't have it yet, it appeared as a fix to a race reported after
it was queued, sorry for the confusion. I'm talking about this bit[1] here.
It returns an __rcu-annotated pointer, forcing the user to keep and manage it.

This is because when revoking the resource[2], the pointer storing the address
of the revokable provider is also managed by SRCU - in addition to the
revokable resource itself which seems to me like a weird concept. I understand
the race condition it fixes but I assumed the whole concept of revokable is to
free the user from being bothered by the implementation details behind it which
leak out of the API if you need to keep __rcu around.

Bart

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/include/linux/revocable.h#n25
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/base/revocable.c#n123

Re: [PATCH] revocable: hide the implementation details from users

[Tzung-Bi Shih]

On Mon, Feb 09, 2026 at 05:13:17AM -0600, Bartosz Golaszewski wrote:
> On Fri, 6 Feb 2026 23:02:44 +0100, "Paul E. McKenney" <paulmck@kernel.org> said:
> > On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
> >> Revocable stacks two layers of SRCU on top of each other: one to protect
> >> the actual revocable resource and another to synchronize the revoking.
> >> While this design itself is questionable, it also forces the user of
> >> revokable to think about the implementation details and annotate the
> >> pointer holding the address of the revocable_provider struct with __rcu.
> >> Hide the real type of struct revokable_provider behind a typedef to free
> >> the users from this responsability. While adding new typedefs goes
> >> against current guidelines, it's still better than the current
> >> requirement.
> >>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> >> ---
> >> I realized that one important person was missing from the whole review
> >> process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
> >> at the SRCU usage in GPIO and I think he should have also signed off on
> >> revocable before it got queued.
> >>
> >> Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
> >> The series that implemented it and made its way into v7.0 is here:
> >>
> >>   https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/
> >>
> >> Could you please take a look and say if the design looks sane to you?
> >> Especially the double SRCU on the revocable_provider.
> >
> > The first patch in the above URL adds SRCU, and the other
> > two add various tests.  I do not see a double SRCU, just an
> > srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
> > in revocable_withdraw_access().
> >
> > You are allowed to nest srcu_read_lock(), if that is what you are asking.
> > *However*, nesting revocable_try_access() on the same revocable structure
> > is buggy because the second call to revocable_try_access() would overwrite
> > the rp->srcu value written by the first call.  This could result in both
> > SRCU grace-period hangs and too-short SRCU grace periods, more likely
> > the former than the latter.
> >
> > Or do you mean something else by "double SRCU"?
> >
> 
> This series didn't have it yet, it appeared as a fix to a race reported after
> it was queued, sorry for the confusion. I'm talking about this bit[1] here.
> It returns an __rcu-annotated pointer, forcing the user to keep and manage it.
> 
> This is because when revoking the resource[2], the pointer storing the address
> of the revokable provider is also managed by SRCU - in addition to the
> revokable resource itself which seems to me like a weird concept. I understand
> the race condition it fixes but I assumed the whole concept of revokable is to
> free the user from being bothered by the implementation details behind it which
> leak out of the API if you need to keep __rcu around.

Please hold off on reviewing the patch and the "double SRCU" usage for now.
I'll remove the second RCU in the next version, which should serve as a
better starting point for a clean review.

Side note: It was actually one SRCU and one RCU, not a double SRCU.

Re: [PATCH] revocable: hide the implementation details from users

[Paul E. McKenney]

On Wed, Feb 11, 2026 at 02:05:26AM +0000, Tzung-Bi Shih wrote:
> On Mon, Feb 09, 2026 at 05:13:17AM -0600, Bartosz Golaszewski wrote:
> > On Fri, 6 Feb 2026 23:02:44 +0100, "Paul E. McKenney" <paulmck@kernel.org> said:
> > > On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:
> > >> Revocable stacks two layers of SRCU on top of each other: one to protect
> > >> the actual revocable resource and another to synchronize the revoking.
> > >> While this design itself is questionable, it also forces the user of
> > >> revokable to think about the implementation details and annotate the
> > >> pointer holding the address of the revocable_provider struct with __rcu.
> > >> Hide the real type of struct revokable_provider behind a typedef to free
> > >> the users from this responsability. While adding new typedefs goes
> > >> against current guidelines, it's still better than the current
> > >> requirement.
> > >>
> > >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > >> ---
> > >> I realized that one important person was missing from the whole review
> > >> process: Paul E. McKenney who wrote and maintains SRCU. I had Paul look
> > >> at the SRCU usage in GPIO and I think he should have also signed off on
> > >> revocable before it got queued.
> > >>
> > >> Paul: I'm Cc'ing you on this patch to bring revocable to your attention.
> > >> The series that implemented it and made its way into v7.0 is here:
> > >>
> > >>   https://lore.kernel.org/all/20260116080235.350305-1-tzungbi@kernel.org/
> > >>
> > >> Could you please take a look and say if the design looks sane to you?
> > >> Especially the double SRCU on the revocable_provider.
> > >
> > > The first patch in the above URL adds SRCU, and the other
> > > two add various tests.  I do not see a double SRCU, just an
> > > srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
> > > in revocable_withdraw_access().
> > >
> > > You are allowed to nest srcu_read_lock(), if that is what you are asking.
> > > *However*, nesting revocable_try_access() on the same revocable structure
> > > is buggy because the second call to revocable_try_access() would overwrite
> > > the rp->srcu value written by the first call.  This could result in both
> > > SRCU grace-period hangs and too-short SRCU grace periods, more likely
> > > the former than the latter.
> > >
> > > Or do you mean something else by "double SRCU"?
> > >
> > 
> > This series didn't have it yet, it appeared as a fix to a race reported after
> > it was queued, sorry for the confusion. I'm talking about this bit[1] here.
> > It returns an __rcu-annotated pointer, forcing the user to keep and manage it.
> > 
> > This is because when revoking the resource[2], the pointer storing the address
> > of the revokable provider is also managed by SRCU - in addition to the
> > revokable resource itself which seems to me like a weird concept. I understand
> > the race condition it fixes but I assumed the whole concept of revokable is to
> > free the user from being bothered by the implementation details behind it which
> > leak out of the API if you need to keep __rcu around.
> 
> Please hold off on reviewing the patch and the "double SRCU" usage for now.
> I'll remove the second RCU in the next version, which should serve as a
> better starting point for a clean review.

Will do, thank you!

> Side note: It was actually one SRCU and one RCU, not a double SRCU.

That does sound more likely, though you really could nest SRCU or use
different srcu_struct structures in the same algorithm.  But let's see
what you come up with.

							Thanx, Paul

Re: [PATCH] revocable: hide the implementation details from users

[Johan Hovold]

On Tue, Feb 10, 2026 at 08:59:20PM -0800, Paul E. McKenney wrote:
> On Wed, Feb 11, 2026 at 02:05:26AM +0000, Tzung-Bi Shih wrote:
> > On Mon, Feb 09, 2026 at 05:13:17AM -0600, Bartosz Golaszewski wrote:
> > > On Fri, 6 Feb 2026 23:02:44 +0100, "Paul E. McKenney" <paulmck@kernel.org> said:
> > > > On Fri, Feb 06, 2026 at 11:32:06AM +0100, Bartosz Golaszewski wrote:

> > > >> Could you please take a look and say if the design looks sane to you?
> > > >> Especially the double SRCU on the revocable_provider.
> > > >
> > > > The first patch in the above URL adds SRCU, and the other
> > > > two add various tests.  I do not see a double SRCU, just an
> > > > srcu_read_lock() in revocable_try_access() and an srcu_read_unlock()
> > > > in revocable_withdraw_access().

> > > This series didn't have it yet, it appeared as a fix to a race reported after
> > > it was queued, sorry for the confusion. I'm talking about this bit[1] here.
> > > It returns an __rcu-annotated pointer, forcing the user to keep and manage it.

> > Please hold off on reviewing the patch and the "double SRCU" usage for now.
> > I'll remove the second RCU in the next version, which should serve as a
> > better starting point for a clean review.

In case Paul or anyone else reading this wonders, the "revocable" code
was reverted last Friday so that's why the links Bartosz posted are
broken:

	https://lore.kernel.org/lkml/20260204142849.22055-1-johan@kernel.org/

Johan