Re: [PATCH v4 2/6] ACPI: Move IOMMU setup code out of IORT

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

Hi Eric,

On Wed, Jun 16, 2021 at 11:35:13AM +0200, Eric Auger wrote:
> > -const struct iommu_ops *iort_iommu_configure_id(struct device *dev,
> > -						const u32 *id_in)
> > +int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
> >  {
> >  	struct acpi_iort_node *node;
> > -	const struct iommu_ops *ops;
> > +	const struct iommu_ops *ops = NULL;

Oops, I need to remove this (and add -Werror to my tests.)


> > +static const struct iommu_ops *acpi_iommu_configure_id(struct device *dev,
> > +						       const u32 *id_in)
> > +{
> > +	int err;
> > +	const struct iommu_ops *ops;
> > +
> > +	/*
> > +	 * If we already translated the fwspec there is nothing left to do,
> > +	 * return the iommu_ops.
> > +	 */
> > +	ops = acpi_iommu_fwspec_ops(dev);
> > +	if (ops)
> > +		return ops;
> > +
> > +	err = iort_iommu_configure_id(dev, id_in);
> > +
> > +	/*
> > +	 * If we have reason to believe the IOMMU driver missed the initial
> > +	 * add_device callback for dev, replay it to get things in order.
> > +	 */
> > +	if (!err && dev->bus && !device_iommu_mapped(dev))
> > +		err = iommu_probe_device(dev);
> Previously we had:
>     if (!err) {
>         ops = iort_fwspec_iommu_ops(dev);
>         err = iort_add_device_replay(dev);
>     }
> 
> Please can you explain the transform? I see the
> 
> acpi_iommu_fwspec_ops call below but is it not straightforward to me.

I figured that iort_add_device_replay() is only used once and is
sufficiently simple to be inlined manually (saving 10 lines). Then I
replaced the ops assignment with returns, which saves another line and may
be slightly clearer?  I guess it's mostly a matter of taste, the behavior
should be exactly the same.

> Also the comment mentions replay. Unsure if it is still OK.

The "replay" part is, but "add_device" isn't accurate because it has since
been replaced by probe_device. I'll refresh the comment.

Thanks,
Jean