Re: [PATCH v11 7/9] iommu/arm-smmu-v3: Add struct arm_smmu_impl

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Aug 06, 2024 at 07:11:52PM -0700, Nicolin Chen wrote:
>  
> -static int arm_smmu_device_acpi_probe(struct platform_device *pdev,
> -				      struct arm_smmu_device *smmu)
> +static struct arm_smmu_device *
> +arm_smmu_impl_acpi_probe(struct arm_smmu_device *smmu,
> +			 struct acpi_iort_node *node)
> +{
> +	/*
> +	 * DSDT might hold some SMMU extension, so we have no option but to go
> +	 * through the ACPI tables unconditionally. On success, this returns a
> +	 * copy of smmu struct holding an impl pointer. Otherwise, an impl may
> +	 * choose to return an ERR_PTR as an error out, or to return the pass-
> +	 * in smmu pointer as a fallback to the standard SMMU.
> +	 */
> +	return arm_smmu_impl_acpi_dsdt_probe(smmu, node);
> +}

Lets generalize this a bit more and have the impl mechanism work for
DT too.. Keep the main probe the same and add a new function after the
dt/acpi steps:

	smmu = arm_smmu_probe_impl(smmu);
	if (IS_ERR(smmu))
		return PTR_ERR(smmu);

Which is more like:

/*
 * Probe all the compiled in implementations. Each one checks to see if it
 * matches this HW and if so returns a devm_krealloc'd arm_smmu_device which
 * replaces the callers. Otherwise the original is returned or ERR_PTR.
 *
 */
static struct arm_smmu_device *arm_smmu_probe_impl(struct arm_smmu_device *orig_smmu)
{
	struct arm_smmu_device *new_smmu;
	int ret;

	new_smmu = tegra241_cmdqv_acpi_dsdt_probe(orig_smmu);
	if (new_smmu != ERR_PTR(-ENODEV))
		goto out_new_impl;
	return orig_smmu;

out_new_impl:
	if (IS_ERR(new_smmu))
		return new_smmu;

	/* FIXME: check is this ordering OK during remove? */
	ret = devm_add_action_or_reset(new_smmu->dev, arm_smmu_impl_remove,
				       new_smmu);
	if (ret)
		return ERR_PTR(ret);
	return new_smmu;
}

Easy to add new sub implementations. Provide an inline ENODEV sub in
the header file for tegra241_cmdqv_acpi_dsdt_probe

Add something like this to the header to get the ACPI node:

static inline struct acpi_iort_node *
arm_smmu_get_iort_node(struct arm_smmu_device *smmu)
{
	return *(struct acpi_iort_node **)dev_get_platdata(smmu->dev);
}

Since it isn't passed down

> @@ -4560,6 +4602,7 @@ static void arm_smmu_device_remove(struct platform_device *pdev)
>  {
>  	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
>  
> +	arm_smmu_impl_remove(smmu);

Can't call this if devm has been used to set it up, and this would be
in the wrong order anyhow. Just remove it.. I guess the devm was put
for this to avoid adding goto error unwind to probe?


> +struct arm_smmu_impl {
> +	int (*device_reset)(struct arm_smmu_device *smmu);
> +	void (*device_remove)(struct arm_smmu_device *smmu);
> +	struct arm_smmu_cmdq *(*get_secondary_cmdq)(struct arm_smmu_device *smmu);
> +};

Can we put the word "ops" into this struct somehow? That would be a
more typically kernely name.

arm_smmu_impl_ops perhaps?

>  struct arm_smmu_device {
>  	struct device			*dev;
> +	/* An SMMUv3 implementation */

The comment is self explanatory

Jason