On Mon, Nov 11, 2019 at 03:50:07PM +0000, Jonathan Cameron wrote: > > + cfg->l1ptr = dmam_alloc_coherent(smmu->dev, size, > > + &cfg->l1ptr_dma, > > + GFP_KERNEL | __GFP_ZERO); > > As before. Fairly sure __GFP_ZERO doesn't give you anything extra. Indeed > > + if (!cfg->l1ptr) { > > + dev_warn(smmu->dev, "failed to allocate L1 context table\n"); > > + return -ENOMEM; > > + } > > + } > > + > > + cfg->tables = devm_kzalloc(smmu->dev, sizeof(struct arm_smmu_cd_table) * > > + cfg->num_tables, GFP_KERNEL); > > + if (!cfg->tables) { > > + ret = -ENOMEM; > > + goto err_free_l1; > > + } > > + > > + /* With two levels, leaf tables are allocated lazily */ > This comment is a kind of odd one. It is actually talking about what > 'doesn't' happen here I think.. > > Perhaps /* > * Only allocate a leaf table for linear case. > * With two levels, the leaf tables are allocated lazily. > */ Yes, that's clearer > > + if (!cfg->l1ptr) { > > + ret = arm_smmu_alloc_cd_leaf_table(smmu, &cfg->tables[0], > > + max_contexts); > > + if (ret) > > + goto err_free_tables; > > + } > > + > > + return 0; > > + > > +err_free_tables: > > + devm_kfree(smmu->dev, cfg->tables); > > +err_free_l1: > > + if (cfg->l1ptr) > > + dmam_free_coherent(smmu->dev, size, cfg->l1ptr, cfg->l1ptr_dma); > > This cleanup only occurs if we have had an error. > Is there potential for this to rerun at some point later? If so we should > be careful to also reset relevant pointers - e.g. cfg->l1ptr = NULL as > they are used to control the flow above. Yes we should definitely clear l1ptr. The domain may be managed by a device driver, and if attach_dev() fails they will call domain_free(), which checks this pointer. Plus nothing prevents them from calling attach_dev() again with the same domain. > If there is no chance of a rerun why bother cleaning them up at all? Something > has gone horribly wrong so let the eventual smmu cleanup deal with them. The domain is much shorter-lived than the SMMU device, so we need this cleanup. > > + return ret; > > } > > > > static void arm_smmu_free_cd_tables(struct arm_smmu_domain *smmu_domain) > > { > > + int i; > > struct arm_smmu_device *smmu = smmu_domain->smmu; > > struct arm_smmu_s1_cfg *cfg = &smmu_domain->s1_cfg; > > + size_t num_leaf_entries = 1 << cfg->s1cdmax; > > + struct arm_smmu_cd_table *table = cfg->tables; > > > > - arm_smmu_free_cd_leaf_table(smmu, &cfg->table, 1 << cfg->s1cdmax); > > + if (cfg->l1ptr) { > > + size_t size = cfg->num_tables * (CTXDESC_L1_DESC_DWORDS << 3); > > + > > + dmam_free_coherent(smmu->dev, size, cfg->l1ptr, cfg->l1ptr_dma); > > As above, if we can call this in a fashion that makes sense > other than in eventual smmu tear down, then we need to be > careful to reset the pointers. If not, then why are we clearing > managed resourced by hand anyway? Yes, we call this on the error cleanup path (not only domain_free()), so it needs to leave the domain in a usable state. Thanks, Jean