On 29/07/16 15:46, Jean-Philippe Brucker wrote:
> Hi Robin,
>
> Very sorry about the delay, I forgot about this minor comment, below
>
> On Fri, Jul 01, 2016 at 05:50:15PM +0100, Robin Murphy wrote:
>> Now that we can properly describe the mapping between PCI RIDs and
>> stream IDs via "iommu-map", and have it fed it to the driver
>> automatically via of_xlate(), rework the SMMUv3 driver to benefit from
>> that, and get rid of the current misuse of the "iommus" binding.
>>
>> Since having of_xlate wired up means that masters will now be given the
>> appropriate DMA ops, we also need to make sure that default domains work
>> properly. This necessitates dispensing with the "whole group at a time"
>> notion for attaching to a domain, as devices which share a group get
>> attached to the group's default domain one by one as they are initially
>> probed.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
>> ---
>>
>> v4: Add explicit device probe in of_init callback.
>>
>> drivers/iommu/arm-smmu-v3.c | 278 +++++++++++++++++++-------------------------
>> 1 file changed, 121 insertions(+), 157 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 94b68213c50d..fb438664b9f0 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
> [...]
>> @@ -1800,94 +1752,74 @@ static bool arm_smmu_sid_in_range(struct arm_smmu_device *smmu, u32 sid)
>> return sid < limit;
>> }
>>
>> +static struct iommu_ops arm_smmu_ops;
>> +
>> static int arm_smmu_add_device(struct device *dev)
>> {
>> int i, ret;
>> - u32 sid, *sids;
>> - struct pci_dev *pdev;
>> - struct iommu_group *group;
>> - struct arm_smmu_group *smmu_group;
>> struct arm_smmu_device *smmu;
>> + struct arm_smmu_master_data *master;
>> + struct iommu_fwspec *fwspec = dev_iommu_fwspec(dev);
>> + struct iommu_group *group;
>>
>> - /* We only support PCI, for now */
>> - if (!dev_is_pci(dev))
>> + if (!fwspec || fwspec->iommu_ops != &arm_smmu_ops)
>> return -ENODEV;
>> -
>> - pdev = to_pci_dev(dev);
>> - group = iommu_group_get_for_dev(dev);
>> - if (IS_ERR(group))
>> - return PTR_ERR(group);
>> -
>> - smmu_group = iommu_group_get_iommudata(group);
>> - if (!smmu_group) {
>> - smmu = arm_smmu_get_for_pci_dev(pdev);
>> - if (!smmu) {
>> - ret = -ENOENT;
>> - goto out_remove_dev;
>> - }
>> -
>> - smmu_group = kzalloc(sizeof(*smmu_group), GFP_KERNEL);
>> - if (!smmu_group) {
>> - ret = -ENOMEM;
>> - goto out_remove_dev;
>> - }
>> -
>> - smmu_group->ste.valid = true;
>> - smmu_group->smmu = smmu;
>> - iommu_group_set_iommudata(group, smmu_group,
>> - __arm_smmu_release_pci_iommudata);
>> + /*
>> + * We _can_ actually withstand dodgy bus code re-calling add_device()
>> + * without an intervening remove_device()/of_xlate() sequence, but
>> + * we're not going to do so quietly...
>> + */
>> + if (WARN_ON_ONCE(fwspec->iommu_priv)) {
>> + master = fwspec->iommu_priv;
>> + smmu = master->smmu;
>> } else {
>> - smmu = smmu_group->smmu;
>> + smmu = arm_smmu_get_by_node(fwspec->iommu_np);
>> + if (!smmu)
>> + return -ENODEV;
>> + master = kzalloc(sizeof(*master), GFP_KERNEL);
>> + if (!master)
>> + return -ENOMEM;
>> +
>> + master->smmu = smmu;
>> + fwspec->iommu_priv = master;
>
> It's probably best to initialise master->ste.bypass = true here, to
> reflect the initial state of STEs. Otherwise attach_dev always calls
> detach_dev first for nothing.
I'm actually now thinking that that check in attach_dev() should be for
ste->valid, rather than ste->bypass. That matches the similar check in
remove_device(), and looking at old local branches I apparently did have
it that way at some point, and I now can't quite remember why it ended
up differently. I'll have a proper dig into it next week.
> Apart from that, this version works fine with my twisted setup. Note
> that we might have to add a master->fwspec reference in the future, to
> access those SIDs from various places. If I understood correctly, it
> should be fine as those objects have the same lifetime after the
> add_device call.
That sounds reasonable, although I can't think offhand where we might
have a master_data without having got it via the containing device
(unless we also add some other means of keeping track of them). Since
this series doesn't need any kind of reverse-lookup capabilities I'm
just keeping things as simple as possible for the time being.
Robin.
>
> Thanks,
> Jean-Philippe
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
