| Message ID | 1-v2-318ed5f6983b+198f-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 |
On Mon, Jun 10, 2024 at 09:31:10PM -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. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Reviewed-by: Mostafa Saleh <smostafa@google.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
On Mon, Jun 10, 2024 at 5:31 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > + 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)); The error message "failed to allocate l1 stream table (%zu bytes)\n" is identical to the one a few lines above that's printed if dmam_alloc_coherent fails. This might make it difficult to determine the origin of this message if someone sees it in a log file. Also, it's not technically an SMMUv3 l1 stream table but rather some internal data structure. I thought there was a guideline around not printing error messages on certain allocation failures because there'll be a generic OOM message that'll get printed. Also, this driver never prints an error message when devm_kcalloc or devm_kzalloc fails, only on dmam_alloc_coherent failures. Just nitpicking here. Looks good to me, otherwise.
On Tue, Jun 11, 2024 at 04:52:57PM -0700, Daniel Mentz wrote: > I thought there was a guideline around not printing error messages on > certain allocation failures because there'll be a generic OOM message > that'll get printed. Oops, right, C&P mistake. I removed it. > Also, this driver never prints an error message > when devm_kcalloc or devm_kzalloc fails, only on dmam_alloc_coherent > failures. dma_alloc_coherent generates the same dump as kzalloc on ENOMEM, I guessed the intention was to add extra logging on the high order allocations, but this allocation is larger than the dma one and doesn't get a print so <shrug> I was tempted to remove those prints too.. 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 ec8c7b7f4cb9c1..d56e19ca6ef6c5 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3215,25 +3215,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; - void *strtab = smmu->strtab_cfg.strtab; - - 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(strtab, &cfg->l1_desc[i]); - strtab += STRTAB_L1_DESC_DWORDS << 3; - } - - return 0; -} - static int arm_smmu_init_strtab_2lvl(struct arm_smmu_device *smmu) { void *strtab; @@ -3269,7 +3250,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)