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 <[email protected]>
---
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);
}
--
2.45.1
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Acked-by: Akhil R <[email protected]>
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 <[email protected]>
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
> -----Original Message-----
> From: Herbert Xu <[email protected]>
> Sent: Friday, May 31, 2024 10:53 AM
> To: Akhil R <[email protected]>
> Cc: Christophe JAILLET <[email protected]>; David S. Miller
> <[email protected]>; Thierry Reding <[email protected]>; Jon
> Hunter <[email protected]>; [email protected]; kernel-
> [email protected]; [email protected]; linux-
> [email protected]
> 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.
Le 31/05/2024 à 07:36, Akhil R a écrit :
>> -----Original Message-----
>> From: Herbert Xu <[email protected]>
>> Sent: Friday, May 31, 2024 10:53 AM
>> To: Akhil R <[email protected]>
>> Cc: Christophe JAILLET <[email protected]>; David S. Miller
>> <[email protected]>; Thierry Reding <[email protected]>; Jon
>> Hunter <[email protected]>; [email protected]; kernel-
>> [email protected]; [email protected]; linux-
>> [email protected]
>> 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