* [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()
@ 2025-04-07 20:19 Nicolin Chen
2025-04-08 16:14 ` Jason Gunthorpe
2025-04-11 10:45 ` Joerg Roedel
0 siblings, 2 replies; 3+ messages in thread
From: Nicolin Chen @ 2025-04-07 20:19 UTC (permalink / raw)
To: will, jgg
Cc: thierry.reding, vdumpa, robin.murphy, joro, jonathanh,
linux-tegra, linux-arm-kernel, iommu, linux-kernel
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
This is because the LVCMDQ queue memory are managed by devres, while that
dmam_free_coherent() is called in the context of devm_action_release().
Jason pointed out 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":
https://lore.kernel.org/linux-iommu/20250407174408.GB1722458@nvidia.com/
In fact, tegra241_cmdqv_init_structures() only allocates memory resources
which means any failure that it generates would be similar to -ENOMEM, so
there is no point in having that "falling back to standard SMMU" routine,
as the standard SMMU would likely fail to allocate memory too.
Remove the unwind part in tegra241_cmdqv_init_structures(), and return a
proper error code to ask SMMU driver to call tegra241_cmdqv_remove() via
impl_ops->device_remove(). Then, drop tegra241_vintf_free_lvcmdq() since
devres will take care of that.
Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
Cc: stable@vger.kernel.org
Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
Changelog
v2
* Fail tegra241_cmdqv_init_structures() and let devres take care of the
lvcmdq queue memory space
v1
https://lore.kernel.org/all/cover.1744014481.git.nicolinc@nvidia.com/
.../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 32 +++----------------
1 file changed, 5 insertions(+), 27 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
index d525ab43a4ae..dd7d030d2e89 100644
--- a/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
+++ b/drivers/iommu/arm/arm-smmu-v3/tegra241-cmdqv.c
@@ -487,17 +487,6 @@ static int tegra241_cmdqv_hw_reset(struct arm_smmu_device *smmu)
/* VCMDQ Resource Helpers */
-static void tegra241_vcmdq_free_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
-{
- struct arm_smmu_queue *q = &vcmdq->cmdq.q;
- size_t nents = 1 << q->llq.max_n_shift;
- size_t qsz = nents << CMDQ_ENT_SZ_SHIFT;
-
- if (!q->base)
- return;
- dmam_free_coherent(vcmdq->cmdqv->smmu.dev, qsz, q->base, q->base_dma);
-}
-
static int tegra241_vcmdq_alloc_smmu_cmdq(struct tegra241_vcmdq *vcmdq)
{
struct arm_smmu_device *smmu = &vcmdq->cmdqv->smmu;
@@ -560,7 +549,8 @@ static void tegra241_vintf_free_lvcmdq(struct tegra241_vintf *vintf, u16 lidx)
struct tegra241_vcmdq *vcmdq = vintf->lvcmdqs[lidx];
char header[64];
- tegra241_vcmdq_free_smmu_cmdq(vcmdq);
+ /* Note that the lvcmdq queue memory space is managed by devres */
+
tegra241_vintf_deinit_lvcmdq(vintf, lidx);
dev_dbg(vintf->cmdqv->dev,
@@ -768,13 +758,13 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
vintf = kzalloc(sizeof(*vintf), GFP_KERNEL);
if (!vintf)
- goto out_fallback;
+ return -ENOMEM;
/* Init VINTF0 for in-kernel use */
ret = tegra241_cmdqv_init_vintf(cmdqv, 0, vintf);
if (ret) {
dev_err(cmdqv->dev, "failed to init vintf0: %d\n", ret);
- goto free_vintf;
+ return ret;
}
/* Preallocate logical VCMDQs to VINTF0 */
@@ -783,24 +773,12 @@ static int tegra241_cmdqv_init_structures(struct arm_smmu_device *smmu)
vcmdq = tegra241_vintf_alloc_lvcmdq(vintf, lidx);
if (IS_ERR(vcmdq))
- goto free_lvcmdq;
+ return PTR_ERR(vcmdq);
}
/* Now, we are ready to run all the impl ops */
smmu->impl_ops = &tegra241_cmdqv_impl_ops;
return 0;
-
-free_lvcmdq:
- for (lidx--; lidx >= 0; lidx--)
- tegra241_vintf_free_lvcmdq(vintf, lidx);
- tegra241_cmdqv_deinit_vintf(cmdqv, vintf->idx);
-free_vintf:
- kfree(vintf);
-out_fallback:
- dev_info(smmu->impl_dev, "Falling back to standard SMMU CMDQ\n");
- smmu->options &= ~ARM_SMMU_OPT_TEGRA241_CMDQV;
- tegra241_cmdqv_remove(smmu);
- return 0;
}
#ifdef CONFIG_IOMMU_DEBUGFS
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()
2025-04-07 20:19 [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent() Nicolin Chen
@ 2025-04-08 16:14 ` Jason Gunthorpe
2025-04-11 10:45 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Jason Gunthorpe @ 2025-04-08 16:14 UTC (permalink / raw)
To: Nicolin Chen
Cc: will, thierry.reding, vdumpa, robin.murphy, joro, jonathanh,
linux-tegra, linux-arm-kernel, iommu, linux-kernel
On Mon, Apr 07, 2025 at 01:19:08PM -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
>
> This is because the LVCMDQ queue memory are managed by devres, while that
> dmam_free_coherent() is called in the context of devm_action_release().
> Jason pointed out 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":
> https://lore.kernel.org/linux-iommu/20250407174408.GB1722458@nvidia.com/
>
> In fact, tegra241_cmdqv_init_structures() only allocates memory resources
> which means any failure that it generates would be similar to -ENOMEM, so
> there is no point in having that "falling back to standard SMMU" routine,
> as the standard SMMU would likely fail to allocate memory too.
>
> Remove the unwind part in tegra241_cmdqv_init_structures(), and return a
> proper error code to ask SMMU driver to call tegra241_cmdqv_remove() via
> impl_ops->device_remove(). Then, drop tegra241_vintf_free_lvcmdq() since
> devres will take care of that.
>
> Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
> Cc: stable@vger.kernel.org
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v2
> * Fail tegra241_cmdqv_init_structures() and let devres take care of the
> lvcmdq queue memory space
> v1
> https://lore.kernel.org/all/cover.1744014481.git.nicolinc@nvidia.com/
>
> .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 32 +++----------------
> 1 file changed, 5 insertions(+), 27 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent()
2025-04-07 20:19 [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent() Nicolin Chen
2025-04-08 16:14 ` Jason Gunthorpe
@ 2025-04-11 10:45 ` Joerg Roedel
1 sibling, 0 replies; 3+ messages in thread
From: Joerg Roedel @ 2025-04-11 10:45 UTC (permalink / raw)
To: Nicolin Chen
Cc: will, jgg, thierry.reding, vdumpa, robin.murphy, jonathanh,
linux-tegra, linux-arm-kernel, iommu, linux-kernel
On Mon, Apr 07, 2025 at 01:19:08PM -0700, Nicolin Chen wrote:
> Fixes: 483e0bd8883a ("iommu/tegra241-cmdqv: Do not allocate vcmdq until dma_set_mask_and_coherent")
> Cc: stable@vger.kernel.org
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> Changelog
> v2
> * Fail tegra241_cmdqv_init_structures() and let devres take care of the
> lvcmdq queue memory space
> v1
> https://lore.kernel.org/all/cover.1744014481.git.nicolinc@nvidia.com/
>
> .../iommu/arm/arm-smmu-v3/tegra241-cmdqv.c | 32 +++----------------
> 1 file changed, 5 insertions(+), 27 deletions(-)
Applied, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-11 10:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-07 20:19 [PATCH rc v2] iommu/tegra241-cmdqv: Fix warnings due to dmam_free_coherent() Nicolin Chen
2025-04-08 16:14 ` Jason Gunthorpe
2025-04-11 10:45 ` Joerg Roedel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).