On 06/06/16 16:47, Jean-Philippe Brucker wrote:
Hi Robin,
I quite like this change, the result looks pretty clean. I rebased my
current work and didn't notice any major issue. Some nits below.
Thanks for looking!
On Fri, Jun 03, 2016 at 06:15:41PM +0100, Robin Murphy wrote:
The driver's current reliance on attaching/detaching an entire group
for the first device it sees is at odds with the IOMMU core's initial
construction of a group by adding each device and attaching it to the
default domain in turn. As it turns out, we can happily do away with the
whole palaver by simply letting each device be in charge of its own
stream ID/stream table entry, and reducing the problem of tracking
duplicate IDs and domains down to "Is the STE already associated with
the appropriate context?", which is easily done by just looking at the
stream table itself.
With an of_xlate() callback in place, devices attached to default
domains will now get appropriate DMA ops installed, so make sure we
enable translation again to stop them getting horribly confused - this
reverts the SMMUv3 portion of cbf8277ef456 ("iommu/arm-smmu: Treat
IOMMU_DOMAIN_DMA as bypass for now")
Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
---
...
-static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
+static void arm_smmu_install_ste(struct arm_smmu_master_data *master,
+ struct arm_smmu_domain *smmu_domain)
(Second line is misaligned here)
Fixed.
{
- int i;
- struct arm_smmu_domain *smmu_domain = smmu_group->domain;
- struct arm_smmu_strtab_ent *ste = &smmu_group->ste;
- struct arm_smmu_device *smmu = smmu_group->smmu;
+ struct arm_smmu_device *smmu = master->smmu;
+ struct arm_smmu_strtab_ent *ste = &master->ste;
+ __le64 *step = arm_smmu_get_step_for_sid(smmu, master->sid);
if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) {
ste->s1_cfg = &smmu_domain->s1_cfg;
@@ -1634,42 +1628,16 @@ static int arm_smmu_install_ste_for_group(struct arm_smmu_group *smmu_group)
ste->s2_cfg = &smmu_domain->s2_cfg;
}
- for (i = 0; i < smmu_group->num_sids; ++i) {
- u32 sid = smmu_group->sids[i];
- __le64 *step = arm_smmu_get_step_for_sid(smmu, sid);
-
- arm_smmu_write_strtab_ent(smmu, sid, step, ste);
- }
-
- return 0;
-}
-
-static void arm_smmu_detach_dev(struct device *dev)
-{
- struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
-
- smmu_group->ste.bypass = true;
- if (arm_smmu_install_ste_for_group(smmu_group) < 0)
- dev_warn(dev, "failed to install bypass STE\n");
-
- smmu_group->domain = NULL;
+ arm_smmu_write_strtab_ent(smmu, master->sid, step, ste);
}
static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
{
int ret = 0;
- struct arm_smmu_device *smmu;
struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
- struct arm_smmu_group *smmu_group = arm_smmu_group_get(dev);
+ struct arm_smmu_master_data *master = dev->archdata.iommu;
(calling this member 'master' or 'smmu_master' consistently, instead of
'data', would make the driver easier to grep)
Good point; it's now called "master" everywhere.
+ struct arm_smmu_device *smmu = master->smmu;
- if (!smmu_group)
- return -ENOENT;
-
- /* Already attached to a different domain? */
- if (smmu_group->domain && smmu_group->domain != smmu_domain)
- arm_smmu_detach_dev(dev);
-
- smmu = smmu_group->smmu;
mutex_lock(&smmu_domain->init_mutex);
if (!smmu_domain->smmu) {
@@ -1688,21 +1656,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
goto out_unlock;
}
- /* Group already attached to this domain? */
- if (smmu_group->domain)
- goto out_unlock;
+ master->ste.bypass = false;
Should also set master->ste.valid = true. It worked out of the box
during my first test, because master is allocated with kmalloc and
initialised with garbage. Could we also use kzalloc, in the of_xlate
patch?
Yikes! I think this is the of_xlate directly cherry-picked from a more
aggressive attempt to remove arm_smmu_strtab_ent along with
arm_smmu_group, and those bits of initialisation never got added back;
both fixed.
Thanks for catching all those - I've recreated the unversioned
iommu/generic branch with a fixup patch on top for the time being.
Robin.
Thanks,
Jean-Philippe
--
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
