[PATCH v2 2/2] spi: atcspi200: fix mutex initialization order

Pei Xiao posted 2 patches 4 weeks ago
[PATCH v2 2/2] spi: atcspi200: fix mutex initialization order
Posted by Pei Xiao 4 weeks ago
The atcspi_exec_mem_op() function may call mutex_lock() on the
driver's mutex before it is properly initialized if a SPI memory
operation is initiated immediately after devm_spi_register_controller()
is called. The mutex initialization currently occurs after the
controller registration, which leaves a window where the mutex could
be used uninitialized.

Move the mutex initialization to the beginning of the probe function,
before any registration or resource allocation. Also replace the
plain mutex_init() with devm_mutex_init() to benefit from automatic
cleanup on driver unbind, preventing potential resource leaks.

Fixes: 34e3815ea459 ("spi: atcspi200: Add ATCSPI200 SPI controller driver")
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/spi/spi-atcspi200.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-atcspi200.c b/drivers/spi/spi-atcspi200.c
index c0607193be8d..dd1292c5bb98 100644
--- a/drivers/spi/spi-atcspi200.c
+++ b/drivers/spi/spi-atcspi200.c
@@ -551,6 +551,10 @@ static int atcspi_probe(struct platform_device *pdev)
 	spi->dev = &pdev->dev;
 	dev_set_drvdata(&pdev->dev, host);
 
+	ret = devm_mutex_init(spi->dev, &spi->mutex_lock);
+	if (ret)
+		goto free_controller;
+
 	ret = atcspi_init_resources(pdev, spi, &mem_res);
 	if (ret)
 		goto free_controller;
@@ -580,7 +584,6 @@ static int atcspi_probe(struct platform_device *pdev)
 		else
 			spi->use_dma = true;
 	}
-	mutex_init(&spi->mutex_lock);
 
 	return 0;
 
-- 
2.25.1
Re: [PATCH v2 2/2] spi: atcspi200: fix mutex initialization order
Posted by Mark Brown 3 weeks, 6 days ago
On Wed, Mar 11, 2026 at 09:55:14AM +0800, Pei Xiao wrote:

> @@ -551,6 +551,10 @@ static int atcspi_probe(struct platform_device *pdev)
>  	spi->dev = &pdev->dev;
>  	dev_set_drvdata(&pdev->dev, host);
>  
> +	ret = devm_mutex_init(spi->dev, &spi->mutex_lock);
> +	if (ret)
> +		goto free_controller;
> +

This means we're now using devm to free the mutex that is in spi, but
when we pass spi into devm_spi_register_controller() we'll cause spi to
be freed before we free the mutex.  This pattern gets a bit clunky with
devm...
Re: [PATCH v2 2/2] spi: atcspi200: fix mutex initialization order
Posted by Pei Xiao 3 weeks, 6 days ago
在 2026/3/11 20:29, Mark Brown 写道:
> On Wed, Mar 11, 2026 at 09:55:14AM +0800, Pei Xiao wrote:
>
>> @@ -551,6 +551,10 @@ static int atcspi_probe(struct platform_device *pdev)
>>  	spi->dev = &pdev->dev;
>>  	dev_set_drvdata(&pdev->dev, host);
>>  
>> +	ret = devm_mutex_init(spi->dev, &spi->mutex_lock);
>> +	if (ret)
>> +		goto free_controller;
>> +
> This means we're now using devm to free the mutex that is in spi, but
> when we pass spi into devm_spi_register_controller() we'll cause spi to
> be freed before we free the mutex.  This pattern gets a bit clunky with
> devm...
Thanks for review! I will use mutex_init() for V3 version.
Re: [PATCH v2 2/2] spi: atcspi200: fix mutex initialization order
Posted by Mark Brown 3 weeks, 5 days ago
On Thu, Mar 12, 2026 at 10:25:11AM +0800, Pei Xiao wrote:
> 在 2026/3/11 20:29, Mark Brown 写道:
> > On Wed, Mar 11, 2026 at 09:55:14AM +0800, Pei Xiao wrote:

> >> +	ret = devm_mutex_init(spi->dev, &spi->mutex_lock);
> >> +	if (ret)
> >> +		goto free_controller;
> >> +

> > This means we're now using devm to free the mutex that is in spi, but
> > when we pass spi into devm_spi_register_controller() we'll cause spi to
> > be freed before we free the mutex.  This pattern gets a bit clunky with
> > devm...

> Thanks for review! I will use mutex_init() for V3 version.

That's still going to have trouble because if you do the free in the
removal function the lock will go away before the controller which is
it's own issue (this is what I mean about clunky).  It's especially
annoying as it's only an issue for debugging builds.  I can't actually
see a good way of dealing with this, perhaps just a plain mutex_init()
without a free is the least bad option.