| Message ID | 1468321722-16568-1-git-send-email-weiyj_lk@163.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 <yongjun_wei@trendmicro.com.cn> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andi, At 2016-07-13 12:50:30, "Andi Shyti" <andi.shyti@samsung.com> 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 <yongjun_wei@trendmicro.com.cn> > >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
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 <yongjun_wei@trendmicro.com.cn> > > > >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-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 12, 2016 at 11:08:42AM +0000, weiyj_lk@163.com wrote: > From: Wei Yongjun <yongjun_wei@trendmicro.com.cn> > > 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 <yongjun_wei@trendmicro.com.cn> After our discussion, I checked it again and feel free to add Reviewed-by: Andi Shyti <andi.shyti@samsung.com> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);