Hi Robin, Thanks for your feedback!! On Tue, Sep 20, 2016 at 10:18 PM, Robin Murphy <robin.murphy@xxxxxxx> wrote: > Hi Magnus, > > On 20/09/16 13:41, Magnus Damm wrote: >> From: Magnus Damm <damm+renesas@xxxxxxxxxxxxx> >> >> Update the IPMMU driver to return -ENODEV when adding devices >> not hooked up a particular IPMMU instance. >> >> Currently the ->add_device() callback implementation in the IPMMU >> driver returns -ENODEV for devices with no "iommus" property, >> however the function ipmmu_find_utlbs() may return -EINVAL. > > If there were no "iommus" property at all, of_parse_phandle_with_args() > should return -ENOENT - that probably does want to be caught and passed > back as -ENODEV to imply an untranslated device. On the other hand, > -EINVAL would stem from the existence of the property, but in a somehow > erroneous manner - other than the "args.np != mmu->dev->of_node" check > (which could legitimately fail and be safely ignored if there are > multiple IOMMUs in the system), any other reason implies a DT error > which probably shouldn't be papered over. Regarding -ENOENT to -ENODEV, I agree but I believe this case is already handled. The ->add_device() callback seems to be using of_count_phandle_with_args() to check for the presence of "iommus" property before calling ipmmu_find_utlbs(). So for the case with no "iommus" property at all it is returned as -ENODEV as you suggest. The ->add_device() callback has the ret variable initialised to -ENODEV by default. In case there is only one IPMMU device on the ipmmu_device list and it matches the struct device passed to the ipmmu_add_device() function then all is fine. However when there are more than one IPMMU device on the ipmmu_device list then ipmmu_find_utlbs() may return -EINVAL. Judging by the code this seems to happen when the order of the IPMMU devices on the "iommus" property does not match the IPMMU order on the ipmmu_device list. Hm, I wonder if all this should be replaced with ->xlate() somehow? Thanks, / magnus
