| Message ID | ea775b351a3dbe4cef4056ea89da25084f73df22.1716650050.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: tegra - Remove an incorrect iommu_fwspec_free() call in tegra_se_remove() | expand |
> > The only iommu function call in this driver is a > tegra_dev_iommu_get_stream_id() which does not allocate anything and does > not take any reference. > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the > probe. > > So there is no point in calling iommu_fwspec_free() in the remove function. > > Remove this incorrect function call. > > Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only > > This patch is completely speculative. *Review with care*. > --- > drivers/crypto/tegra/tegra-se-main.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/crypto/tegra/tegra-se-main.c b/drivers/crypto/tegra/tegra-se- > main.c > index 9955874b3dc3..f94c0331b148 100644 > --- a/drivers/crypto/tegra/tegra-se-main.c > +++ b/drivers/crypto/tegra/tegra-se-main.c > @@ -326,7 +326,6 @@ static void tegra_se_remove(struct platform_device > *pdev) > > crypto_engine_stop(se->engine); > crypto_engine_exit(se->engine); > - iommu_fwspec_free(se->dev); > host1x_client_unregister(&se->client); > } > This was a futile attempt to fix a kmemleak warning in host1x_client_unregister() in a very old kernel. I don't see it anymore, either with or without this change. So, Tested-by: Akhil R <akhilrajeev@nvidia.com> Acked-by: Akhil R <akhilrajeev@nvidia.com> Regards, Akhil
> The only iommu function call in this driver is a > tegra_dev_iommu_get_stream_id() which does not allocate anything and does > not take any reference. > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the > probe. I did not completely understand what is being tried to convey here. If I understand it right, iommu_fwspec_free() does not do anything with the "devm_kzalloc"ed variable. It would probably be a good idea to remove this line from the commit message. > > So there is no point in calling iommu_fwspec_free() in the remove function. > > Remove this incorrect function call. > > Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
On Wed May 29, 2024 at 8:53 AM CEST, Akhil R wrote: > > The only iommu function call in this driver is a > > tegra_dev_iommu_get_stream_id() which does not allocate anything and does > > not take any reference. > > > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the > > probe. > > I did not completely understand what is being tried to convey here. > If I understand it right, iommu_fwspec_free() does not do anything > with the "devm_kzalloc"ed variable. > > It would probably be a good idea to remove this line from the commit message. Yeah, I think that's a bit misleading. What iommu_fwspec_free() does is get the iommu_fwspec from the passed-in device and then frees that iommu_fwspec. That said, as I was looking around I didn't spot anything that was calling iommu_fwspec_free() in any of the cleanup paths, so either I'm missing something or it's a real memory leak (though perhaps one that we are ignoring on purpose because these are usually attached to devices that don't just go away). Thierry
On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote: > > The only iommu function call in this driver is a > > tegra_dev_iommu_get_stream_id() which does not allocate anything and does > > not take any reference. > > > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the > > probe. > > I did not completely understand what is being tried to convey here. > If I understand it right, iommu_fwspec_free() does not do anything > with the "devm_kzalloc"ed variable. > > It would probably be a good idea to remove this line from the commit message. I think he means that as the memory was allocated via devm, it will be automatically freed by the kernel and the driver does not need to (and should not) free the memory by hand. Cheers,
> -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: Friday, May 31, 2024 10:53 AM > To: Akhil R <akhilrajeev@nvidia.com> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; David S. Miller > <davem@davemloft.net>; Thierry Reding <thierry.reding@gmail.com>; Jon > Hunter <jonathanh@nvidia.com>; linux-kernel@vger.kernel.org; kernel- > janitors@vger.kernel.org; linux-crypto@vger.kernel.org; linux- > tegra@vger.kernel.org > Subject: Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() > call in tegra_se_remove() > > External email: Use caution opening links or attachments > > > On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote: > > > The only iommu function call in this driver is a > > > tegra_dev_iommu_get_stream_id() which does not allocate anything and > does > > > not take any reference. > > > > > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the > > > probe. > > > > I did not completely understand what is being tried to convey here. > > If I understand it right, iommu_fwspec_free() does not do anything > > with the "devm_kzalloc"ed variable. > > > > It would probably be a good idea to remove this line from the commit message. > > I think he means that as the memory was allocated via devm, it will > be automatically freed by the kernel and the driver does not need > to (and should not) free the memory by hand. Ya. But iommu_fwspec_free() does not free the memory allocated via devm. I think iommu_fwspec_free() is expected to be called in symmetry with iommu_fwspec_init(). So, I do agree that the SE driver does not allocate what is freed by iommu_fwspec_free(), but I feel this line is a bit misleading.
On Sat, May 25, 2024 at 05:14:35PM +0200, Christophe JAILLET wrote: > The only iommu function call in this driver is a > tegra_dev_iommu_get_stream_id() which does not allocate anything and does > not take any reference. > > More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in > the probe. > > So there is no point in calling iommu_fwspec_free() in the remove function. > > Remove this incorrect function call. > > Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested only > > This patch is completely speculative. *Review with care*. > --- > drivers/crypto/tegra/tegra-se-main.c | 1 - > 1 file changed, 1 deletion(-) Patch applied. Thanks.
Le 31/05/2024 à 07:36, Akhil R a écrit : >> -----Original Message----- >> From: Herbert Xu <herbert@gondor.apana.org.au> >> Sent: Friday, May 31, 2024 10:53 AM >> To: Akhil R <akhilrajeev@nvidia.com> >> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>; David S. Miller >> <davem@davemloft.net>; Thierry Reding <thierry.reding@gmail.com>; Jon >> Hunter <jonathanh@nvidia.com>; linux-kernel@vger.kernel.org; kernel- >> janitors@vger.kernel.org; linux-crypto@vger.kernel.org; linux- >> tegra@vger.kernel.org >> Subject: Re: [PATCH] crypto: tegra - Remove an incorrect iommu_fwspec_free() >> call in tegra_se_remove() >> >> External email: Use caution opening links or attachments >> >> >> On Wed, May 29, 2024 at 06:53:42AM +0000, Akhil R wrote: >>>> The only iommu function call in this driver is a >>>> tegra_dev_iommu_get_stream_id() which does not allocate anything and >> does >>>> not take any reference. >>>> >>>> More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the >>>> probe. >>> I did not completely understand what is being tried to convey here. >>> If I understand it right, iommu_fwspec_free() does not do anything >>> with the "devm_kzalloc"ed variable. >>> >>> It would probably be a good idea to remove this line from the commit message. >> I think he means that as the memory was allocated via devm, it will >> be automatically freed by the kernel and the driver does not need >> to (and should not) free the memory by hand. Yes, that is my point. > Ya. But iommu_fwspec_free() does not free the memory allocated via devm. > > I think iommu_fwspec_free() is expected to be called in symmetry with > iommu_fwspec_init(). So, I do agree that the SE driver does not allocate > what is freed by iommu_fwspec_free(), but I feel this line is a bit misleading. > Yes, I spoke too fast. What is freed is dev_iommu_fwspec_get(dev);, not dev. So the sentence I wrote makes no sense and should be removed :( CJ
diff --git a/drivers/crypto/tegra/tegra-se-main.c b/drivers/crypto/tegra/tegra-se-main.c index 9955874b3dc3..f94c0331b148 100644 --- a/drivers/crypto/tegra/tegra-se-main.c +++ b/drivers/crypto/tegra/tegra-se-main.c @@ -326,7 +326,6 @@ static void tegra_se_remove(struct platform_device *pdev) crypto_engine_stop(se->engine); crypto_engine_exit(se->engine); - iommu_fwspec_free(se->dev); host1x_client_unregister(&se->client); }
The only iommu function call in this driver is a tegra_dev_iommu_get_stream_id() which does not allocate anything and does not take any reference. More-over, what is freed is "se->dev" which has been devm_kzalloc()'ed in the probe. So there is no point in calling iommu_fwspec_free() in the remove function. Remove this incorrect function call. Fixes: 0880bb3b00c8 ("crypto: tegra - Add Tegra Security Engine driver") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested only This patch is completely speculative. *Review with care*. --- drivers/crypto/tegra/tegra-se-main.c | 1 - 1 file changed, 1 deletion(-)