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

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
>

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