| Message ID | 2-v1-1b720dce51d1+4f44-smmuv3_tidy_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tidy some minor things in the stream table/cd table area | expand |
Hi Jason, On Mon, Jun 03, 2024 at 07:31:28PM -0300, Jason Gunthorpe wrote: > dmam_alloc_coherent() already returns zero'd memory so cfg->strtab.l1_desc > (the list of DMA addresses for the L2 entries) is already zero'd. > > arm_smmu_init_l1_strtab() goes through and calls > arm_smmu_write_strtab_l1_desc() on the newly allocated (and zero'd) struct > arm_smmu_strtab_l1_desc, which ends up computing 'val = 0' and zeroing it > again. > > Remove arm_smmu_init_l1_strtab() and just call devm_kcalloc() from > arm_smmu_init_strtab_2lvl to allocate the companion struct. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Looking at the code for dmam_alloc_coherent(basically dma_alloc_coherent) I see that the memory is zeroed for both DMA direct and IOMMU, however I don’t see that documented (in DMA-API.txt). Assuming that’s guaranteed to be zeroed (maybe we should update the docs if I am not missing something) Reviewed-by: Mostafa Saleh <smostafa@google.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 27 +++++++-------------- > 1 file changed, 9 insertions(+), 18 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 6b4f1a664288db..d27dd0600bf1df 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -3220,23 +3220,6 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu) > PRIQ_ENT_DWORDS, "priq"); > } > > -static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu) > -{ > - unsigned int i; > - struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; > - > - cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, > - sizeof(*cfg->l1_desc), GFP_KERNEL); > - if (!cfg->l1_desc) > - return -ENOMEM; > - > - for (i = 0; i < cfg->num_l1_ents; ++i) > - arm_smmu_write_strtab_l1_desc( > - &smmu->strtab_cfg.strtab.l1_desc[i], &cfg->l1_desc[i]); > - > - return 0; > -} > - > static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) > { > void *strtab; > @@ -3272,7 +3255,15 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) > reg |= FIELD_PREP(STRTAB_BASE_CFG_SPLIT, STRTAB_SPLIT); > cfg->strtab_base_cfg = reg; > > - return arm_smmu_init_l1_strtab(smmu); > + cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, > + sizeof(*cfg->l1_desc), GFP_KERNEL); > + if (!cfg->l1_desc) { > + dev_err(smmu->dev, > + "failed to allocate l1 stream table (%zu bytes)\n", > + cfg->num_l1_ents * sizeof(*cfg->l1_desc)); > + return -ENOMEM; > + } > + return 0; > } > > static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu) > -- > 2.45.2 >
On Tue, Jun 04, 2024 at 03:56:04PM +0000, Mostafa Saleh wrote: > > arm_smmu_init_l1_strtab() goes through and calls > > arm_smmu_write_strtab_l1_desc() on the newly allocated (and zero'd) struct > > arm_smmu_strtab_l1_desc, which ends up computing 'val = 0' and zeroing it > > again. > > > > Remove arm_smmu_init_l1_strtab() and just call devm_kcalloc() from > > arm_smmu_init_strtab_2lvl to allocate the companion struct. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > Looking at the code for dmam_alloc_coherent(basically dma_alloc_coherent) > I see that the memory is zeroed for both DMA direct and IOMMU, however > I don’t see that documented (in DMA-API.txt). > Assuming that’s guaranteed to be zeroed (maybe we should update the docs > if I am not missing something) Yeah, people have been sending clean ups removing zeroing and GFP_ZERO's in this space for a bit now. There are many places that rely on it. Thanks, Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 6b4f1a664288db..d27dd0600bf1df 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3220,23 +3220,6 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu) PRIQ_ENT_DWORDS, "priq"); } -static int arm_smmu_init_l1_strtab(struct arm_smmu_device *smmu) -{ - unsigned int i; - struct arm_smmu_strtab_cfg *cfg = &smmu->strtab_cfg; - - cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, - sizeof(*cfg->l1_desc), GFP_KERNEL); - if (!cfg->l1_desc) - return -ENOMEM; - - for (i = 0; i < cfg->num_l1_ents; ++i) - arm_smmu_write_strtab_l1_desc( - &smmu->strtab_cfg.strtab.l1_desc[i], &cfg->l1_desc[i]); - - return 0; -} - static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) { void *strtab; @@ -3272,7 +3255,15 @@ static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) reg |= FIELD_PREP(STRTAB_BASE_CFG_SPLIT, STRTAB_SPLIT); cfg->strtab_base_cfg = reg; - return arm_smmu_init_l1_strtab(smmu); + cfg->l1_desc = devm_kcalloc(smmu->dev, cfg->num_l1_ents, + sizeof(*cfg->l1_desc), GFP_KERNEL); + if (!cfg->l1_desc) { + dev_err(smmu->dev, + "failed to allocate l1 stream table (%zu bytes)\n", + cfg->num_l1_ents * sizeof(*cfg->l1_desc)); + return -ENOMEM; + } + return 0; } static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
dmam_alloc_coherent() already returns zero'd memory so cfg->strtab.l1_desc (the list of DMA addresses for the L2 entries) is already zero'd. arm_smmu_init_l1_strtab() goes through and calls arm_smmu_write_strtab_l1_desc() on the newly allocated (and zero'd) struct arm_smmu_strtab_l1_desc, which ends up computing 'val = 0' and zeroing it again. Remove arm_smmu_init_l1_strtab() and just call devm_kcalloc() from arm_smmu_init_strtab_2lvl to allocate the companion struct. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 27 +++++++-------------- 1 file changed, 9 insertions(+), 18 deletions(-)