Re: [PATCH rc 1/2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()

[Jason Gunthorpe]

On Mon, Apr 07, 2025 at 01:34:59AM -0700, Nicolin Chen wrote:
> Two WARNINGs are observed when SMMU driver rolls back upon failure:
>  arm-smmu-v3.9.auto: Failed to register iommu
>  arm-smmu-v3.9.auto: probe with driver arm-smmu-v3 failed with error -22
>  ------------[ cut here ]------------
>  WARNING: CPU: 5 PID: 1 at kernel/dma/mapping.c:74 dmam_free_coherent+0xc0/0xd8
>  Call trace:
>   dmam_free_coherent+0xc0/0xd8 (P)
>   tegra241_vintf_free_lvcmdq+0x74/0x188
>   tegra241_cmdqv_remove_vintf+0x60/0x148
>   tegra241_cmdqv_remove+0x48/0xc8
>   arm_smmu_impl_remove+0x28/0x60
>   devm_action_release+0x1c/0x40
>  ------------[ cut here ]------------
>  128 pages are still in use!
>  WARNING: CPU: 16 PID: 1 at mm/page_alloc.c:6902 free_contig_range+0x18c/0x1c8
>  Call trace:
>   free_contig_range+0x18c/0x1c8 (P)
>   cma_release+0x154/0x2f0
>   dma_free_contiguous+0x38/0xa0
>   dma_direct_free+0x10c/0x248
>   dma_free_attrs+0x100/0x290
>   dmam_free_coherent+0x78/0xd8
>   tegra241_vintf_free_lvcmdq+0x74/0x160
>   tegra241_cmdqv_remove+0x98/0x198
>   arm_smmu_impl_remove+0x28/0x60
>   devm_action_release+0x1c/0x40
> 
> For the first warning: when the main SMMU driver cleans up its resources,
> any routine in arm_smmu_impl_remove() should not use any devres function.

Bleck. This is situations where you should not be using devres at all.

It is not that arm_smmu_impl_remove() should not use devres, the
problem is that arm_smmu_impl_probe() has mis-ordered the devres
callbacks if ops->device_remove() is going to be manually freeing
things that probe allocated.

IMHO you should just put the goto unwind back into arm_smmu_device()
probe and not use devm for ops->device_remove(). That will put things
in their proper order and no problem.

Because changing arm_smmu_init_one_queue to avoid devm looks worse..

> -static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
> +static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx,
> +				       bool removing_smmu)
>  {

And this is kind of ugly

Jason