Re: [PATCH v3 9/9] iommu/arm-smmu-v3: Reorganize struct arm_smmu_ctx_desc_cfg

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Sep 06, 2024 at 02:22:22PM +0100, Will Deacon wrote:
> On Tue, Aug 06, 2024 at 08:31:23PM -0300, Jason Gunthorpe wrote:
> > @@ -1373,8 +1368,6 @@ void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid)
> >  
> >  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master)
> >  {
> > -	int ret;
> > -	size_t l1size;
> >  	size_t max_contexts;
> >  	struct arm_smmu_device *smmu = master->smmu;
> >  	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
> > @@ -1385,71 +1378,67 @@ static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master)
> >  	if (!(smmu->features & ARM_SMMU_FEAT_2_LVL_CDTAB) ||
> >  	    max_contexts <= CTXDESC_L2_ENTRIES) {
> >  		cd_table->s1fmt = STRTAB_STE_0_S1FMT_LINEAR;
> > -		cd_table->num_l1_ents = max_contexts;
> > +		cd_table->linear.num_ents = max_contexts;
> >  
> > -		l1size = max_contexts * sizeof(struct arm_smmu_cd);
> > +		cd_table->linear.table = dma_alloc_coherent(
> > +			smmu->dev, max_contexts * sizeof(struct arm_smmu_cd),
> > +			&cd_table->cdtab_dma, GFP_KERNEL);
> > +		if (!cd_table->linear.table)
> > +			return -ENOMEM;
> >  	} else {
> >  		cd_table->s1fmt = STRTAB_STE_0_S1FMT_64K_L2;
> > -		cd_table->num_l1_ents = DIV_ROUND_UP(max_contexts,
> > -						  CTXDESC_L2_ENTRIES);
> > +		cd_table->l2.num_l1_ents =
> > +			DIV_ROUND_UP(max_contexts, CTXDESC_L2_ENTRIES);
> >  
> > -		cd_table->l1_desc = kcalloc(cd_table->num_l1_ents,
> > -					    sizeof(*cd_table->l1_desc),
> > -					    GFP_KERNEL);
> > -		if (!cd_table->l1_desc)
> > +		cd_table->l2.l2ptrs = kcalloc(cd_table->l2.num_l1_ents,
> > +					     sizeof(*cd_table->l2.l2ptrs),
> > +					     GFP_KERNEL);
> > +		if (!cd_table->l2.l2ptrs)
> >  			return -ENOMEM;
> >  
> > -		l1size = cd_table->num_l1_ents *
> > -			 sizeof(struct arm_smmu_cdtab_l1);
> > +		cd_table->l2.l1tab = dma_alloc_coherent(
> > +			smmu->dev,
> > +			cd_table->l2.num_l1_ents *
> > +				sizeof(struct arm_smmu_cdtab_l1),
> > +			&cd_table->cdtab_dma, GFP_KERNEL);
> > +		if (!cd_table->l2.l1tab) {
> > +			kfree(cd_table->l2.l2ptrs);
> > +			cd_table->l2.l2ptrs = NULL;
> > +			return -ENOMEM;
> > +		}
> >  	}
> > -
> > -	cd_table->cdtab = dma_alloc_coherent(smmu->dev, l1size,
> > -					     &cd_table->cdtab_dma, GFP_KERNEL);
> > -	if (!cd_table->cdtab) {
> > -		dev_warn(smmu->dev, "failed to allocate context descriptor\n");
> > -		ret = -ENOMEM;
> > -		goto err_free_l1;
> > -	}
> > -
> >  	return 0;
> > -
> > -err_free_l1:
> > -	if (cd_table->l1_desc) {
> > -		kfree(cd_table->l1_desc);
> > -		cd_table->l1_desc = NULL;
> > -	}
> > -	return ret;
> 
> Why inline the error path here? Sure, I get that it works both ways, but
> it seems a little gratuitous to me.

With the change there is only one goto.

But sure, can leave it:

[..]
		if (!cd_table->l2.l2ptrs) {
			ret = -ENOMEM;
			goto err_free_l2ptrs;
		}
	}
	return 0;

err_free_l2ptrs:
	kfree(cd_table->l2.l2ptrs);
	cd_table->l2.l2ptrs = NULL;
	return ret;
}

Jason