| Message ID | 20200320091414.3941-11-joro@8bytes.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: Move iommu_fwspec out of 'struct device' | expand |
Hi Joerg, Thanks for tackling this! On 2020-03-20 9:14 am, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > Make use of dev_iommu_priv_set/get() functions and simplify the code > where possible with this change. > > Tested-by: Will Deacon <will@kernel.org> # arm-smmu > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/iommu/arm-smmu.c | 57 +++++++++++++++++++++------------------- > 1 file changed, 30 insertions(+), 27 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 980aae73b45b..7aa36e6c19c0 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -98,12 +98,15 @@ struct arm_smmu_master_cfg { > s16 smendx[]; > }; > #define INVALID_SMENDX -1 > -#define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv) > -#define fwspec_smmu(fw) (__fwspec_cfg(fw)->smmu) > -#define fwspec_smendx(fw, i) \ > - (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i]) > -#define for_each_cfg_sme(fw, i, idx) \ > - for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i) > +#define __fwspec_cfg(dev) ((struct arm_smmu_master_cfg *)dev_iommu_priv_get(dev)) > +#define fwspec_smmu(dev) (__fwspec_cfg(dev)->smmu) > +#define fwspec_smendx(dev, i) \ > + (i >= dev_iommu_fwspec_get(dev)->num_ids ? \ > + INVALID_SMENDX : \ > + __fwspec_cfg(dev)->smendx[i]) > +#define for_each_cfg_sme(dev, i, idx) \ > + for (i = 0; idx = fwspec_smendx(dev, i), \ > + i < dev_iommu_fwspec_get(dev)->num_ids; ++i) Yikes, this ends up pretty ugly, and I'd prefer not have this much complexity hidden in macros that were intended just to be convenient shorthand. Would you mind pulling in the patch below as a precursor? Other than that, the rest of the series looks OK at a glance. We should also move fwspec->ops to dev_iommu, as those are "IOMMU API" data rather than "firmware" data, but let's consider that separately as this series is already long enough. Thanks, Robin. ----->8----- Subject: [PATCH] iommu/arm-smmu: Refactor master_cfg/fwspec usage In preparation for restructuring iommu_fwspec, refactor the way we access the arm_smmu_master_cfg private data to be less dependent on the current layout. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/arm-smmu.c | 42 +++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 16c4b87af42b..b4978f45a7f2 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -98,12 +98,10 @@ struct arm_smmu_master_cfg { s16 smendx[]; }; #define INVALID_SMENDX -1 -#define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv) -#define fwspec_smmu(fw) (__fwspec_cfg(fw)->smmu) -#define fwspec_smendx(fw, i) \ - (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i]) -#define for_each_cfg_sme(fw, i, idx) \ - for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i) +#define cfg_smendx(cfg, fw, i) \ + (i >= fw->num_ids ? INVALID_SMENDX : cfg->smendx[i]) +#define for_each_cfg_sme(cfg, fw, i, idx) \ + for (i = 0; idx = cfg_smendx(cfg, fw, i), i < fw->num_ids; ++i) static bool using_legacy_binding, using_generic_binding; @@ -1069,7 +1067,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev) mutex_lock(&smmu->stream_map_mutex); /* Figure out a viable stream map entry allocation */ - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(cfg, fwspec, i, idx) { u16 sid = FIELD_GET(ARM_SMMU_SMR_ID, fwspec->ids[i]); u16 mask = FIELD_GET(ARM_SMMU_SMR_MASK, fwspec->ids[i]); @@ -1100,7 +1098,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev) iommu_group_put(group); /* It worked! Now, poke the actual hardware */ - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(cfg, fwspec, i, idx) { arm_smmu_write_sme(smmu, idx); smmu->s2crs[idx].group = group; } @@ -1117,14 +1115,14 @@ static int arm_smmu_master_alloc_smes(struct device *dev) return ret; } -static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec) +static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg, + struct iommu_fwspec *fwspec) { - struct arm_smmu_device *smmu = fwspec_smmu(fwspec); - struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv; + struct arm_smmu_device *smmu = cfg->smmu; int i, idx; mutex_lock(&smmu->stream_map_mutex); - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(cfg, fwspec, i, idx) { if (arm_smmu_free_sme(smmu, idx)) arm_smmu_write_sme(smmu, idx); cfg->smendx[i] = INVALID_SMENDX; @@ -1133,6 +1131,7 @@ static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec) } static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, + struct arm_smmu_master_cfg *cfg, struct iommu_fwspec *fwspec) { struct arm_smmu_device *smmu = smmu_domain->smmu; @@ -1146,7 +1145,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, else type = S2CR_TYPE_TRANS; - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(cfg, fwspec, i, idx) { if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx) continue; @@ -1162,8 +1161,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) { int ret; struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct arm_smmu_device *smmu; struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + struct arm_smmu_master_cfg *cfg; + struct arm_smmu_device *smmu; if (!fwspec || fwspec->ops != &arm_smmu_ops) { dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n"); @@ -1177,10 +1177,11 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) * domains, just say no (but more politely than by dereferencing NULL). * This should be at least a WARN_ON once that's sorted. */ - if (!fwspec->iommu_priv) + cfg = fwspec->iommu_priv; + if (!cfg) return -ENODEV; - smmu = fwspec_smmu(fwspec); + smmu = cfg->smmu; ret = arm_smmu_rpm_get(smmu); if (ret < 0) @@ -1204,7 +1205,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } /* Looks ok, so add the device to the domain */ - ret = arm_smmu_domain_add_master(smmu_domain, fwspec); + ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec); /* * Setup an autosuspend delay to avoid bouncing runpm state. @@ -1475,7 +1476,7 @@ static void arm_smmu_remove_device(struct device *dev) return; iommu_device_unlink(&smmu->iommu, dev); - arm_smmu_master_free_smes(fwspec); + arm_smmu_master_free_smes(cfg, fwspec); arm_smmu_rpm_put(smmu); @@ -1487,11 +1488,12 @@ static void arm_smmu_remove_device(struct device *dev) static struct iommu_group *arm_smmu_device_group(struct device *dev) { struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct arm_smmu_device *smmu = fwspec_smmu(fwspec); + struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv; + struct arm_smmu_device *smmu = cfg->smmu; struct iommu_group *group = NULL; int i, idx; - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(cfg, fwspec, i, idx) { if (group && smmu->s2crs[idx].group && group != smmu->s2crs[idx].group) return ERR_PTR(-EINVAL);
Hey Robin, On Mon, Mar 23, 2020 at 04:02:33PM +0000, Robin Murphy wrote: > Yikes, this ends up pretty ugly, and I'd prefer not have this much > complexity hidden in macros that were intended just to be convenient > shorthand. Would you mind pulling in the patch below as a precursor? Sure thing, but your mail-client seemed to have fiddled with the patch so that is is unusable to me. I tried to fix it up, but it still doesn't apply. Can you please re-send it to me either via git-send-email or just as a mime-attachement? > Other than that, the rest of the series looks OK at a glance. We should also > move fwspec->ops to dev_iommu, as those are "IOMMU API" data rather than > "firmware" data, but let's consider that separately as this series is > already long enough. Yes, moving ops out of fwspec is next on the list, and moving the iommu_group pointer into dev_iommu. Regards, Joerg
On 2020-03-24 10:08 am, Joerg Roedel wrote: > Hey Robin, > > On Mon, Mar 23, 2020 at 04:02:33PM +0000, Robin Murphy wrote: >> Yikes, this ends up pretty ugly, and I'd prefer not have this much >> complexity hidden in macros that were intended just to be convenient >> shorthand. Would you mind pulling in the patch below as a precursor? > > Sure thing, but your mail-client seemed to have fiddled with the patch > so that is is unusable to me. I tried to fix it up, but it still doesn't > apply. Can you please re-send it to me either via git-send-email or just > as a mime-attachement? Oops, sorry - as you might imagine I'm not in my normal workflow :) Let me rebase that onto something actually in your tree (rather than whatever detached HEAD this is checked out out on my laptop...) and try resending it properly. >> Other than that, the rest of the series looks OK at a glance. We should also >> move fwspec->ops to dev_iommu, as those are "IOMMU API" data rather than >> "firmware" data, but let's consider that separately as this series is >> already long enough. > > Yes, moving ops out of fwspec is next on the list, and moving the > iommu_group pointer into dev_iommu. Cool, let me know if you need a hand with all the *_iommu_configure() stuff - I still have plans for overhauling that lot anyway, but not imminently, so it probably is worthwhile to do the straightforward housekeeping first. Thanks, Robin.
Hi Robin, On Wed, Mar 25, 2020 at 12:31:46PM +0000, Robin Murphy wrote: > Oops, sorry - as you might imagine I'm not in my normal workflow :) No problem, nobody is right now :) > Let me rebase that onto something actually in your tree (rather than > whatever detached HEAD this is checked out out on my laptop...) and try > resending it properly. Got it, thanks. Added to the next version of the patch-set which I will send out shortly. > Cool, let me know if you need a hand with all the *_iommu_configure() stuff > - I still have plans for overhauling that lot anyway, but not imminently, so > it probably is worthwhile to do the straightforward housekeeping first. Okay, I'll get back to you if I need help with the conversion. Thanks, Joerg
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 980aae73b45b..7aa36e6c19c0 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -98,12 +98,15 @@ struct arm_smmu_master_cfg { s16 smendx[]; }; #define INVALID_SMENDX -1 -#define __fwspec_cfg(fw) ((struct arm_smmu_master_cfg *)fw->iommu_priv) -#define fwspec_smmu(fw) (__fwspec_cfg(fw)->smmu) -#define fwspec_smendx(fw, i) \ - (i >= fw->num_ids ? INVALID_SMENDX : __fwspec_cfg(fw)->smendx[i]) -#define for_each_cfg_sme(fw, i, idx) \ - for (i = 0; idx = fwspec_smendx(fw, i), i < fw->num_ids; ++i) +#define __fwspec_cfg(dev) ((struct arm_smmu_master_cfg *)dev_iommu_priv_get(dev)) +#define fwspec_smmu(dev) (__fwspec_cfg(dev)->smmu) +#define fwspec_smendx(dev, i) \ + (i >= dev_iommu_fwspec_get(dev)->num_ids ? \ + INVALID_SMENDX : \ + __fwspec_cfg(dev)->smendx[i]) +#define for_each_cfg_sme(dev, i, idx) \ + for (i = 0; idx = fwspec_smendx(dev, i), \ + i < dev_iommu_fwspec_get(dev)->num_ids; ++i) static bool using_legacy_binding, using_generic_binding; @@ -1061,7 +1064,7 @@ static bool arm_smmu_free_sme(struct arm_smmu_device *smmu, int idx) static int arm_smmu_master_alloc_smes(struct device *dev) { struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv; + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); struct arm_smmu_device *smmu = cfg->smmu; struct arm_smmu_smr *smrs = smmu->smrs; struct iommu_group *group; @@ -1069,7 +1072,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev) mutex_lock(&smmu->stream_map_mutex); /* Figure out a viable stream map entry allocation */ - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(dev, i, idx) { u16 sid = FIELD_GET(ARM_SMMU_SMR_ID, fwspec->ids[i]); u16 mask = FIELD_GET(ARM_SMMU_SMR_MASK, fwspec->ids[i]); @@ -1100,7 +1103,7 @@ static int arm_smmu_master_alloc_smes(struct device *dev) iommu_group_put(group); /* It worked! Now, poke the actual hardware */ - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(dev, i, idx) { arm_smmu_write_sme(smmu, idx); smmu->s2crs[idx].group = group; } @@ -1117,14 +1120,14 @@ static int arm_smmu_master_alloc_smes(struct device *dev) return ret; } -static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec) +static void arm_smmu_master_free_smes(struct device *dev) { - struct arm_smmu_device *smmu = fwspec_smmu(fwspec); - struct arm_smmu_master_cfg *cfg = fwspec->iommu_priv; + struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev); + struct arm_smmu_device *smmu = fwspec_smmu(dev); int i, idx; mutex_lock(&smmu->stream_map_mutex); - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(dev, i, idx) { if (arm_smmu_free_sme(smmu, idx)) arm_smmu_write_sme(smmu, idx); cfg->smendx[i] = INVALID_SMENDX; @@ -1133,7 +1136,7 @@ static void arm_smmu_master_free_smes(struct iommu_fwspec *fwspec) } static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, - struct iommu_fwspec *fwspec) + struct device *dev) { struct arm_smmu_device *smmu = smmu_domain->smmu; struct arm_smmu_s2cr *s2cr = smmu->s2crs; @@ -1146,7 +1149,7 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, else type = S2CR_TYPE_TRANS; - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(dev, i, idx) { if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx) continue; @@ -1160,10 +1163,10 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain, static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) { - int ret; + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); struct arm_smmu_device *smmu; - struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); + int ret; if (!fwspec || fwspec->ops != &arm_smmu_ops) { dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n"); @@ -1177,10 +1180,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) * domains, just say no (but more politely than by dereferencing NULL). * This should be at least a WARN_ON once that's sorted. */ - if (!fwspec->iommu_priv) + if (!dev_iommu_priv_get(dev)) return -ENODEV; - smmu = fwspec_smmu(fwspec); + smmu = fwspec_smmu(dev); ret = arm_smmu_rpm_get(smmu); if (ret < 0) @@ -1204,7 +1207,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } /* Looks ok, so add the device to the domain */ - ret = arm_smmu_domain_add_master(smmu_domain, fwspec); + ret = arm_smmu_domain_add_master(smmu_domain, dev); /* * Setup an autosuspend delay to avoid bouncing runpm state. @@ -1429,7 +1432,7 @@ static int arm_smmu_add_device(struct device *dev) goto out_free; cfg->smmu = smmu; - fwspec->iommu_priv = cfg; + dev_iommu_priv_set(dev, cfg); while (i--) cfg->smendx[i] = INVALID_SMENDX; @@ -1467,7 +1470,7 @@ static void arm_smmu_remove_device(struct device *dev) if (!fwspec || fwspec->ops != &arm_smmu_ops) return; - cfg = fwspec->iommu_priv; + cfg = dev_iommu_priv_get(dev); smmu = cfg->smmu; ret = arm_smmu_rpm_get(smmu); @@ -1475,23 +1478,23 @@ static void arm_smmu_remove_device(struct device *dev) return; iommu_device_unlink(&smmu->iommu, dev); - arm_smmu_master_free_smes(fwspec); + arm_smmu_master_free_smes(dev); arm_smmu_rpm_put(smmu); + dev_iommu_priv_set(dev, NULL); iommu_group_remove_device(dev); - kfree(fwspec->iommu_priv); + kfree(cfg); iommu_fwspec_free(dev); } static struct iommu_group *arm_smmu_device_group(struct device *dev) { - struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - struct arm_smmu_device *smmu = fwspec_smmu(fwspec); + struct arm_smmu_device *smmu = fwspec_smmu(dev); struct iommu_group *group = NULL; int i, idx; - for_each_cfg_sme(fwspec, i, idx) { + for_each_cfg_sme(dev, i, idx) { if (group && smmu->s2crs[idx].group && group != smmu->s2crs[idx].group) return ERR_PTR(-EINVAL);