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

[Robin Murphy <robin.murphy@arm.com>]

On 2021-06-18 08:41, Jean-Philippe Brucker wrote:
> 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.

Right, IIRC the multiple assignments to ops were more of a haphazard 
evolution inherited from the DT version, and looking at it now I think 
the multiple-return is indeed a bit nicer.

Similarly, it looks like the factoring out of iort_add_device_replay() 
was originally an attempt to encapsulate the IOMMU_API dependency, but 
things have moved around a lot since then, so that seems like a sensible 
simplification to make too.

Robin.

> 
>> 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
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>