[-next] spi: s3c64xx: fix reference leak to master in s3c64xx_spi_remove()

Commit Message

[e-mail] July 12, 2016, 11:08 a.m. UTC

From: Wei Yongjun <[e-mail]>

Once a spi_master_get() call succeeds, we need an additional
spi_master_put() call to free the memory, otherwise we will
leak a reference to master. Fix by removing the unnecessary
spi_master_get() call.

Signed-off-by: Wei Yongjun <[e-mail]>
---
 drivers/spi/spi-s3c64xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andi Shyti July 13, 2016, 4:50 a.m. UTC | #1

Hi Wei,

> Once a spi_master_get() call succeeds, we need an additional
> spi_master_put() call to free the memory, otherwise we will
> leak a reference to master. Fix by removing the unnecessary
> spi_master_get() call.
> 
> Signed-off-by: Wei Yongjun <[e-mail]>

The patch doesn't have anything wrong, but what you write in the
description is not what you are doing in the patch.

There is no memory allocated in spi_master_get and nothing leaks.
Besides master's resources are managed, so that there would
not be any need to call spi_master_put().

spi_master_put() has been, indeed, removed in this commit:
91800f0e90050a4db4c77e940796f501e02af8be.

But even if you correct the commit log, I don't see much
advantage for this patch on its own.

Thanks,
Andi

> ---
>  drivers/spi/spi-s3c64xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index df88fa1..001c9eb 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1211,7 +1211,7 @@ err0:
>  
>  static int s3c64xx_spi_remove(struct platform_device *pdev)
>  {
> -	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
> +	struct spi_master *master = platform_get_drvdata(pdev);
>  	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
>  
>  	pm_runtime_get_sync(&pdev->dev);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] July 13, 2016, 5:36 a.m. UTC | #2

Hi Andi,

At 2016-07-13 12:50:30, "Andi Shyti" <[e-mail]> wrote:
>Hi Wei,
>
>> Once a spi_master_get() call succeeds, we need an additional
>> spi_master_put() call to free the memory, otherwise we will
>> leak a reference to master. Fix by removing the unnecessary
>> spi_master_get() call.
>> 
>> Signed-off-by: Wei Yongjun <[e-mail]>
>
>The patch doesn't have anything wrong, but what you write in the
>description is not what you are doing in the patch.
>
>There is no memory allocated in spi_master_get and nothing leaks.
>Besides master's resources are managed, so that there would
>not be any need to call spi_master_put().
>
>spi_master_put() has been, indeed, removed in this commit:
>91800f0e90050a4db4c77e940796f501e02af8be.
>

master allocated in spi_alloc_master() need device reference count to zero
to be freed.

The call sequence spi_alloc_master/spi_register_master/spi_unregister_master
is complete; it reduces the device reference count to zero, which results in
device memory being freed.

In commit 91800f0e90050a4db4c77e940796f501e02af8be, 
devm_spi_register_master() equal to spi_register_master() and auto call
spi_unregister_master() when remove. But  remove spi_master_put()
will cause the device reference count left no zero, since spi_master_get()
is callled at the begin of remove function already.

So I think we should either remove the spi_master_get() or add spi_master_put()
before return from remove function.

Regards,
Yongjun Wei

Andi Shyti July 13, 2016, 8:34 a.m. UTC | #3

Hi Wei,

> >> Once a spi_master_get() call succeeds, we need an additional
> >> spi_master_put() call to free the memory, otherwise we will
> >> leak a reference to master. Fix by removing the unnecessary
> >> spi_master_get() call.
> >> 
> >> Signed-off-by: Wei Yongjun <[e-mail]>
> >
> >The patch doesn't have anything wrong, but what you write in the
> >description is not what you are doing in the patch.
> >
> >There is no memory allocated in spi_master_get and nothing leaks.
> >Besides master's resources are managed, so that there would
> >not be any need to call spi_master_put().
> >
> >spi_master_put() has been, indeed, removed in this commit:
> >91800f0e90050a4db4c77e940796f501e02af8be.
> >
> 
> master allocated in spi_alloc_master() need device reference count to zero
> to be freed.
> 
> The call sequence spi_alloc_master/spi_register_master/spi_unregister_master
> is complete; it reduces the device reference count to zero, which results in
> device memory being freed.
> 
> In commit 91800f0e90050a4db4c77e940796f501e02af8be, 
> devm_spi_register_master() equal to spi_register_master() and auto call
> spi_unregister_master() when remove. But  remove spi_master_put()
> will cause the device reference count left no zero, since spi_master_get()
> is callled at the begin of remove function already.
> 
> So I think we should either remove the spi_master_get() or add spi_master_put()
> before return from remove function.

Yes, you are right, I missed this part :)

Thanks,
Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andi Shyti July 13, 2016, 9:17 a.m. UTC | #4

On Tue, Jul 12, 2016 at 11:08:42AM +0000, [e-mail] wrote:
> From: Wei Yongjun <[e-mail]>
> 
> Once a spi_master_get() call succeeds, we need an additional
> spi_master_put() call to free the memory, otherwise we will
> leak a reference to master. Fix by removing the unnecessary
> spi_master_get() call.
> 
> Signed-off-by: Wei Yongjun <[e-mail]>

After our discussion, I checked it again and feel free to add

Reviewed-by: Andi Shyti <[e-mail]>

Thanks,
Andi

> ---
>  drivers/spi/spi-s3c64xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index df88fa1..001c9eb 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1211,7 +1211,7 @@ err0:
>  
>  static int s3c64xx_spi_remove(struct platform_device *pdev)
>  {
> -	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
> +	struct spi_master *master = platform_get_drvdata(pdev);
>  	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
>  
>  	pm_runtime_get_sync(&pdev->dev);
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index df88fa1..001c9eb 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1211,7 +1211,7 @@  err0:
 
 static int s3c64xx_spi_remove(struct platform_device *pdev)
 {
-	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
+	struct spi_master *master = platform_get_drvdata(pdev);
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 
 	pm_runtime_get_sync(&pdev->dev);