[PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Andy Shevchenko]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 47 ++++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e25df9990f82..f077ea74e299 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3079,9 +3079,9 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__spi_alloc_controller);
 
-static void devm_spi_release_controller(struct device *dev, void *ctlr)
+static void devm_spi_release_controller(void *ctlr)
 {
-	spi_controller_put(*(struct spi_controller **)ctlr);
+	spi_controller_put(ctlr);
 }
 
 /**
@@ -3103,21 +3103,18 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev,
 						   unsigned int size,
 						   bool target)
 {
-	struct spi_controller **ptr, *ctlr;
-
-	ptr = devres_alloc(devm_spi_release_controller, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return NULL;
+	struct spi_controller *ctlr;
+	int ret;
 
 	ctlr = __spi_alloc_controller(dev, size, target);
-	if (ctlr) {
-		ctlr->devm_allocated = true;
-		*ptr = ctlr;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (!ctlr)
+		return NULL;
+
+	ret = devm_add_action_or_reset(dev, devm_spi_release_controller, ctlr);
+	if (ret)
+		return NULL;
+
+	ctlr->devm_allocated = true;
 
 	return ctlr;
 }
@@ -3378,9 +3375,9 @@ int spi_register_controller(struct spi_controller *ctlr)
 }
 EXPORT_SYMBOL_GPL(spi_register_controller);
 
-static void devm_spi_unregister(struct device *dev, void *res)
+static void devm_spi_unregister_controller(void *ctlr)
 {
-	spi_unregister_controller(*(struct spi_controller **)res);
+	spi_unregister_controller(ctlr);
 }
 
 /**
@@ -3398,22 +3395,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
 int devm_spi_register_controller(struct device *dev,
 				 struct spi_controller *ctlr)
 {
-	struct spi_controller **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
 	ret = spi_register_controller(ctlr);
-	if (!ret) {
-		*ptr = ctlr;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
 
-	return ret;
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_controller);
 
-- 
2.50.1

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Mark Brown]

On Thu, 08 Jan 2026 18:51:45 +0100, Andy Shevchenko wrote:
> Use devm_add_action_or_reset() instead of devres_alloc() and
> devres_add(), which works the same. This will simplify the
> code. There is no functional changes.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: Simplify devm_spi_*_controller()
      commit: b6376dbed8e173f9571583b5d358b08ff394e864

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Felix Gu]

>  /**
> @@ -3398,22 +3395,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
>  int devm_spi_register_controller(struct device *dev,
>  				 struct spi_controller *ctlr)
>  {
> -	struct spi_controller **ptr;
>  	int ret;
>  
> -	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> -		return -ENOMEM;
> -
>  	ret = spi_register_controller(ctlr);
> -	if (!ret) {
> -		*ptr = ctlr;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> -	}
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
>  
> -	return ret;
>  }
>  EXPORT_SYMBOL_GPL(devm_spi_register_controller);
Hi Andy and Mark,
It seems has a potential corner case in this commit.

The issue is that devm_add_action_or_reset() triggers its callback 
immediately if the internal devres allocation fails. This creates the 
following sequence:
1. spi_register_controller(ctlr) succeeds.
2. devm_add_action_or_reset() is called but fails.
3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
4. This calls spi_unregister_controller(ctlr).
5. For controllers allocated via spi_alloc_host() (where 
   ctlr->devm_allocated is false), spi_unregister_controller() calls 
   put_device(&ctlr->dev).
6. This drops the reference count to zero and frees the ctlr structure.

However, the driver still holds the ctlr pointer and will typically 
attempt its own cleanup, leading to a use-after-free or double-free.

What are your thoughts on this? Does this analysis seem correct, or am
I missing a detail in how the refcounting is handled here?

Best regards,
Felix

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Andy Shevchenko]

On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:

...

> >  int devm_spi_register_controller(struct device *dev,
> >  				 struct spi_controller *ctlr)
> >  {
> > -	struct spi_controller **ptr;
> >  	int ret;
> >  
> > -	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> > -	if (!ptr)
> > -		return -ENOMEM;
> > -
> >  	ret = spi_register_controller(ctlr);
> > -	if (!ret) {
> > -		*ptr = ctlr;
> > -		devres_add(dev, ptr);
> > -	} else {
> > -		devres_free(ptr);
> > -	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
> >  
> > -	return ret;
> >  }

> It seems has a potential corner case in this commit.
> 
> The issue is that devm_add_action_or_reset() triggers its callback 
> immediately if the internal devres allocation fails.

Yes, it does.

> This creates the following sequence:
> 1. spi_register_controller(ctlr) succeeds.
> 2. devm_add_action_or_reset() is called but fails.
> 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> 4. This calls spi_unregister_controller(ctlr).
> 5. For controllers allocated via spi_alloc_host() (where 
>    ctlr->devm_allocated is false), spi_unregister_controller() calls 
>    put_device(&ctlr->dev).
> 6. This drops the reference count to zero and frees the ctlr structure.

I am not sure I see how the sequence is different from the say this

	ret = spi_register_controller(...)
	if (ret)
		goto err;
	// ...if success do something else...
	ret = foo();
	if (ret) {
		spi_unregister_controller(...);
		goto err;
	}

So, each driver should be prepared for an error handling in which it should
release resources in the reversed order.

> However, the driver still holds the ctlr pointer and will typically 
> attempt its own cleanup, leading to a use-after-free or double-free.

You mean buggy driver? Any real example of the use case?

In the 5. you implied something like

	ret = spi_alloc_host();
	...
	ret = devm_spi_register_controller()

?

But this is against the rule how devm must be used.

> What are your thoughts on this? Does this analysis seem correct, or am
> I missing a detail in how the refcounting is handled here?

Is this analysis AI assisted?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Felix Gu]

On Wed, Mar 25, 2026 at 5:54 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> > It seems has a potential corner case in this commit.
> >
> > The issue is that devm_add_action_or_reset() triggers its callback
> > immediately if the internal devres allocation fails.
>
> Yes, it does.
>
> > This creates the following sequence:
> > 1. spi_register_controller(ctlr) succeeds.
> > 2. devm_add_action_or_reset() is called but fails.
> > 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> > 4. This calls spi_unregister_controller(ctlr).
> > 5. For controllers allocated via spi_alloc_host() (where
> >    ctlr->devm_allocated is false), spi_unregister_controller() calls
> >    put_device(&ctlr->dev).
> > 6. This drops the reference count to zero and frees the ctlr structure.
>
> I am not sure I see how the sequence is different from the say this
>
>         ret = spi_register_controller(...)
>         if (ret)
>                 goto err;
>         // ...if success do something else...
>         ret = foo();
>         if (ret) {
>                 spi_unregister_controller(...);
>                 goto err;
>         }
>
> So, each driver should be prepared for an error handling in which it should
> release resources in the reversed order.
>
> > However, the driver still holds the ctlr pointer and will typically
> > attempt its own cleanup, leading to a use-after-free or double-free.
>
> You mean buggy driver? Any real example of the use case?
>
> In the 5. you implied something like
>
>         ret = spi_alloc_host();
>         ...
>         ret = devm_spi_register_controller()
>
> ?
>
> But this is against the rule how devm must be used.
>
> > What are your thoughts on this? Does this analysis seem correct, or am
> > I missing a detail in how the refcounting is handled here?
>
> Is this analysis AI assisted?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Hi Andy,
I got the thoughts from an AI review from sashiko and  verified it on my own.
https://sashiko.dev/#/patchset/20260322-rockchip-v1-1-fac3f0c6dad8%40gmail.com

Best Regards,
Felix

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Andy Shevchenko]

On Wed, Mar 25, 2026 at 06:22:33PM +0800, Felix Gu wrote:
> On Wed, Mar 25, 2026 at 5:54 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > It seems has a potential corner case in this commit.
> > >
> > > The issue is that devm_add_action_or_reset() triggers its callback
> > > immediately if the internal devres allocation fails.
> >
> > Yes, it does.
> >
> > > This creates the following sequence:
> > > 1. spi_register_controller(ctlr) succeeds.
> > > 2. devm_add_action_or_reset() is called but fails.
> > > 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> > > 4. This calls spi_unregister_controller(ctlr).
> > > 5. For controllers allocated via spi_alloc_host() (where
> > >    ctlr->devm_allocated is false), spi_unregister_controller() calls
> > >    put_device(&ctlr->dev).
> > > 6. This drops the reference count to zero and frees the ctlr structure.
> >
> > I am not sure I see how the sequence is different from the say this
> >
> >         ret = spi_register_controller(...)
> >         if (ret)
> >                 goto err;
> >         // ...if success do something else...
> >         ret = foo();
> >         if (ret) {
> >                 spi_unregister_controller(...);
> >                 goto err;
> >         }
> >
> > So, each driver should be prepared for an error handling in which it should
> > release resources in the reversed order.
> >
> > > However, the driver still holds the ctlr pointer and will typically
> > > attempt its own cleanup, leading to a use-after-free or double-free.
> >
> > You mean buggy driver? Any real example of the use case?
> >
> > In the 5. you implied something like
> >
> >         ret = spi_alloc_host();
> >         ...
> >         ret = devm_spi_register_controller()
> >
> > ?
> >
> > But this is against the rule how devm must be used.
> >
> > > What are your thoughts on this? Does this analysis seem correct, or am
> > > I missing a detail in how the refcounting is handled here?
> >
> > Is this analysis AI assisted?
> 
> I got the thoughts from an AI review from sashiko and  verified it on my own.
> https://sashiko.dev/#/patchset/20260322-rockchip-v1-1-fac3f0c6dad8%40gmail.com

You must have said that in the first place. And have you found a real bug
when verified on your own? Or is it just theoretical possibility? If 'yes'
is the answer to the last question, then theoretically we have thousands
of ways in the kernel to shoot ourselves in the foot.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Johan Hovold]

On Wed, Mar 25, 2026 at 11:54:34AM +0200, Andy Shevchenko wrote:
> On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:

> You mean buggy driver? Any real example of the use case?
> 
> In the 5. you implied something like
> 
> 	ret = spi_alloc_host();
> 	...
> 	ret = devm_spi_register_controller()
> 
> ?

We have some 50 spi driver using a non-managed controller allocation
with managed registration. This was the only way to use these interfaces
before managed allocation was added much later.

> But this is against the rule how devm must be used.

SPI is "special"... But yeah, we should eventually fix the API to avoid
inconsistencies like this.

Johan

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Andy Shevchenko]

On Wed, Mar 25, 2026 at 04:03:43PM +0100, Johan Hovold wrote:
> On Wed, Mar 25, 2026 at 11:54:34AM +0200, Andy Shevchenko wrote:
> > On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:
> 
> > You mean buggy driver? Any real example of the use case?
> > 
> > In the 5. you implied something like
> > 
> > 	ret = spi_alloc_host();
> > 	...
> > 	ret = devm_spi_register_controller()
> > 
> > ?
> 
> We have some 50 spi driver using a non-managed controller allocation
> with managed registration.

Ouch!

> This was the only way to use these interfaces before managed allocation was
> added much later.
> 
> > But this is against the rule how devm must be used.
> 
> SPI is "special"... But yeah, we should eventually fix the API to avoid
> inconsistencies like this.

I see, yeah, it's better to do so.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Johan Hovold]

On Tue, Mar 24, 2026 at 10:55:48PM +0800, Felix Gu wrote:
> >  /**
> > @@ -3398,22 +3395,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
> >  int devm_spi_register_controller(struct device *dev,
> >  				 struct spi_controller *ctlr)
> >  {
> > -	struct spi_controller **ptr;
> >  	int ret;
> >  
> > -	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
> > -	if (!ptr)
> > -		return -ENOMEM;
> > -
> >  	ret = spi_register_controller(ctlr);
> > -	if (!ret) {
> > -		*ptr = ctlr;
> > -		devres_add(dev, ptr);
> > -	} else {
> > -		devres_free(ptr);
> > -	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
> >  
> > -	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(devm_spi_register_controller);

> It seems has a potential corner case in this commit.
> 
> The issue is that devm_add_action_or_reset() triggers its callback 
> immediately if the internal devres allocation fails. This creates the 
> following sequence:
> 1. spi_register_controller(ctlr) succeeds.
> 2. devm_add_action_or_reset() is called but fails.
> 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> 4. This calls spi_unregister_controller(ctlr).
> 5. For controllers allocated via spi_alloc_host() (where 
>    ctlr->devm_allocated is false), spi_unregister_controller() calls 
>    put_device(&ctlr->dev).
> 6. This drops the reference count to zero and frees the ctlr structure.
> 
> However, the driver still holds the ctlr pointer and will typically 
> attempt its own cleanup, leading to a use-after-free or double-free.

Indeed, you're right. I stumbled over this change as well recently an
flagged it as something that's likely broken, but when I had another
quick look at it I somehow convinced myself that my instinct had been
wrong.

I just sent a patch to fix this here:

	https://lore.kernel.org/all/20260325145319.1132072-1-johan@kernel.org/

I think handling this explicitly is preferred over reverting as
otherwise someone will just send the same conversion again later.

Johan

[PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Andy Shevchenko]

Use devm_add_action_or_reset() instead of devres_alloc() and
devres_add(), which works the same. This will simplify the
code. There is no functional changes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/spi/spi.c | 47 ++++++++++++++++++-----------------------------
 1 file changed, 18 insertions(+), 29 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 071f7d754931..0850734e6d50 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3058,9 +3058,9 @@ struct spi_controller *__spi_alloc_controller(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__spi_alloc_controller);
 
-static void devm_spi_release_controller(struct device *dev, void *ctlr)
+static void devm_spi_release_controller(void *ctlr)
 {
-	spi_controller_put(*(struct spi_controller **)ctlr);
+	spi_controller_put(ctlr);
 }
 
 /**
@@ -3082,21 +3082,18 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev,
 						   unsigned int size,
 						   bool target)
 {
-	struct spi_controller **ptr, *ctlr;
-
-	ptr = devres_alloc(devm_spi_release_controller, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return NULL;
+	struct spi_controller *ctlr;
+	int ret;
 
 	ctlr = __spi_alloc_controller(dev, size, target);
-	if (ctlr) {
-		ctlr->devm_allocated = true;
-		*ptr = ctlr;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (!ctlr)
+		return NULL;
+
+	ret = devm_add_action_or_reset(dev, devm_spi_release_controller, ctlr);
+	if (ret)
+		return NULL;
+
+	ctlr->devm_allocated = true;
 
 	return ctlr;
 }
@@ -3357,9 +3354,9 @@ int spi_register_controller(struct spi_controller *ctlr)
 }
 EXPORT_SYMBOL_GPL(spi_register_controller);
 
-static void devm_spi_unregister(struct device *dev, void *res)
+static void devm_spi_unregister_controller(void *ctlr)
 {
-	spi_unregister_controller(*(struct spi_controller **)res);
+	spi_unregister_controller(ctlr);
 }
 
 /**
@@ -3377,22 +3374,14 @@ static void devm_spi_unregister(struct device *dev, void *res)
 int devm_spi_register_controller(struct device *dev,
 				 struct spi_controller *ctlr)
 {
-	struct spi_controller **ptr;
 	int ret;
 
-	ptr = devres_alloc(devm_spi_unregister, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return -ENOMEM;
-
 	ret = spi_register_controller(ctlr);
-	if (!ret) {
-		*ptr = ctlr;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_spi_unregister_controller, ctlr);
 
-	return ret;
 }
 EXPORT_SYMBOL_GPL(devm_spi_register_controller);
 
-- 
2.50.1