Re: [PATCH v5 1/6] iommu/arm-smmu-v3: Add arm_smmu_kdump_adopt_strtab() for kdump

[Nicolin Chen <nicolinc@nvidia.com>]

On Tue, May 19, 2026 at 02:10:03PM -0300, Jason Gunthorpe wrote:
> On Sun, May 10, 2026 at 02:23:00PM -0700, Nicolin Chen wrote:
> 
> > +#include <linux/dma-direct.h>
> 
> Nope, never do this, it is an internal header.

Hmm, I have included it for a wrong reason, yet it does mention
"IOMMU drivers".

/*
 * Internals of the DMA direct mapping implementation.  Only for use by the
 * DMA mapping code and IOMMU drivers.
 */

> > +/*
> > + * Adopting the crashed kernel's stream table has risks: the physical addresses
> > + * read from ARM_SMMU_STRTAB_BASE / L1 descriptors may be corrupted. Reject any
> > + * range that overlaps the kdump kernel's critical regions.
> > + */
> > +static bool arm_smmu_kdump_phys_is_corrupted(phys_addr_t base, size_t size)
[..]
> Something like this should not be in the smmu driver, this is some
> core kdump code. I'd drop it, I don't see other drivers doing this?

OK.

> > +static int arm_smmu_kdump_adopt_l2_strtab(struct arm_smmu_device *smmu, u32 sid,
> > +					  u32 l1_idx, u64 l2_dma, u32 span,
> > +					  struct arm_smmu_strtab_l2 **l2table)
> > +{
> > +	phys_addr_t base = dma_to_phys(smmu->dev, l2_dma);
> 
> The thing stored in the L2PTR is a *phys*, the HW doesn't support any
> kind of translation. When using dma_alloc_coherent we never get a phys
> so it uses the dma_addr_t and assumes it is == phys.
> 
> But on this flow this is *phys* and should remain phys. Never touch
> dma_addr_t.

Fixing that and other places too.
 
> > +static void arm_smmu_kdump_adopt_cleanup(struct arm_smmu_device *smmu, u32 fmt)
> > +{
> > +	struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg;
> > +
> > +	if (fmt == STRTAB_BASE_CFG_FMT_2LVL) {
> > +		if (cfg->l2.l2ptrs)
> > +			devm_kfree(smmu->dev, cfg->l2.l2ptrs);
> > +		if (!IS_ERR_OR_NULL(cfg->l2.l1tab))
> > +			devm_memunmap(smmu->dev, cfg->l2.l1tab);
> > +	} else if (fmt == STRTAB_BASE_CFG_FMT_LINEAR) {
> > +		if (!IS_ERR_OR_NULL(cfg->linear.table))
> > +			devm_memunmap(smmu->dev, cfg->linear.table);
> > +	}
> > +}
> 
> If we have a cleanup function why is it using devm? Call the cleanup
> function during remove too?

Dropping "devm_"s.

Thanks
Nicolin