Re: [PATCHv5 2/9] driver/core: populate devices in order for IOMMUs

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 11/21/2013 06:15 AM, Grant Likely wrote:
> On Tue, 19 Nov 2013 11:33:06 +0200, Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> IOMMU devices on the bus need to be poplulated first, then iommu
>> master devices are done later.
>>
>> With CONFIG_OF_IOMMU, "iommus=" DT binding would be used to identify
>> whether a device can be an iommu msater or not. If a device can, we'll
>> defer to populate that device till an iommu device is populated. Once
>> an iommu device is populated, "dev->bus->iommu_ops" is set in the
>> bus. Then, those defered iommu master devices are populated and
>> configured for IOMMU with help of the already populated iommu device
>> via iommu_ops->add_device(). Multiple IOMMUs can be listed on this
>> "iommus" binding so that a device can have multiple IOMMUs attached.
>>
>> Signed-off-by: Hiroshi Doyu <hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> v5:
>> Use "iommus=" binding instread of arm,smmu's "#stream-id-cells".
>>
>> v4:
>> This is newly added, and the successor of the following RFC:
>>   [RFC][PATCHv3+ 1/2] driver/core: Add of_iommu_attach()
>>   http://lists.linuxfoundation.org/pipermail/iommu/2013-November/006914.html
>> ---
>>  drivers/base/dd.c        |  5 +++++
>>  drivers/iommu/of_iommu.c | 22 ++++++++++++++++++++++
>>  include/linux/of_iommu.h |  7 +++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 35fa368..6e892d4 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/async.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/pinctrl/devinfo.h>
>> +#include <linux/of_iommu.h>
>>  
>>  #include "base.h"
>>  #include "power/power.h"
>> @@ -273,6 +274,10 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>>  
>>  	dev->driver = drv;
>>  
>> +	ret = of_iommu_attach(dev);
>> +	if (ret)
>> +		goto probe_failed;
>> +
>>  	/* If using pinctrl, bind pins now before probing */
>>  	ret = pinctrl_bind_pins(dev);
>>  	if (ret)
> 
> I'm very concerned about this approach. Hooking into the core probe
> path for things like this is not going to scale well. I'm not thrilled
> with the pinctrl hook being here either, but that is already merged. :-(
> Also, hooking in here is going affect every single device device driver
> probe path, and a large number of them are never, ever, going to have
> iommu interactions.
> 
> There needs to be a less invasive way of doing what you want. I still
> feel like the individual device drivers themselves need to be aware that
> they might be hooking through an IOMMU. Or, if they are hooking through
> a bus like PCIe, then have the PCIe bus defer creating child devices
> until the IOMMU is configured and in place.

I general though, couldn't any MMIO on-SoC device potentially be
affected by an IOMMU? The point of putting the call to of_iommu_attach()
here rather than inside individual driver's probe() is to avoid
requiring "every" driver having to paste more boiler-plate into probe().

Perhaps what we need is a flag in struct device to indicate that the
driver/device is MMIO, and hence potentially affected by an IOMMU. would
the following work better for you?

+	if (drv->is_mmio) { // let's bikeshed on the field name:-)
+		ret = of_iommu_attach(dev);
+		if (ret)
+			goto probe_failed;
+	}

I've often thought that struct device_driver (or similar) should declare
the set of resources that a device needs (e.g. a list of GPIO names,
regulator names, etc.), so that the driver core can parse all these
standard properties from DT/... before calling the custom probe()
function for all the non-standard stuff. This "is_mmio" flag would fit
into that model well.