| Message ID | 1458669509-7178-7-git-send-email-joro@8bytes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 22, 2016 at 12:58 PM, Joerg Roedel <joro@8bytes.org> wrote: > From: Joerg Roedel <jroedel@suse.de> > > Remove the usage of of_parse_phandle_with_args() and replace > it by the phandle-iterator implementation so that we can > parse out all of the potentially present 128 stream-ids. > > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 59ee4b8..413bd64 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -48,7 +48,7 @@ > #include "io-pgtable.h" > > /* Maximum number of stream IDs assigned to a single device */ > -#define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS > +#define MAX_MASTER_STREAMIDS 128 > > /* Maximum number of context banks per SMMU */ > #define ARM_SMMU_MAX_CBS 128 > @@ -349,6 +349,12 @@ struct arm_smmu_domain { > struct iommu_domain domain; > }; > > +struct arm_smmu_phandle_args { > + struct device_node *np; > + int args_count; > + uint32_t args[MAX_MASTER_STREAMIDS]; > +}; > + > static struct iommu_ops arm_smmu_ops; > > static DEFINE_SPINLOCK(arm_smmu_devices_lock); > @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, > > static int register_smmu_master(struct arm_smmu_device *smmu, > struct device *dev, > - struct of_phandle_args *masterspec) > + struct arm_smmu_phandle_args *masterspec) > { > int i; > struct arm_smmu_master *master; > @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > struct arm_smmu_device *smmu; > struct device *dev = &pdev->dev; > struct rb_node *node; > - struct of_phandle_args masterspec; > + struct of_phandle_iterator it; > + struct arm_smmu_phandle_args masterspec; Isn't this a bit big to put on the stack being ~512 bytes? > int num_irqs, i, err; > > smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
Hi Joerg, On 22/03/16 17:58, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > Remove the usage of of_parse_phandle_with_args() and replace > it by the phandle-iterator implementation so that we can > parse out all of the potentially present 128 stream-ids. In a stream-matching implementation, a device may quite legitimately own anything up to _all_ of the stream IDs (32768, or 65536 if we ever implement support for the SMMUv2 EXID extension), so this is only a genuine limit for stream indexing (and if anyone ever actually made one of those, I don't think they're running mainline on it). Alternatively, how straightforward is it to change the DT on your machine? I'll be getting a v2 of [1] out in a couple of weeks (after imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS altogether, and might also have grown proper SMR support by then. Robin. [1]:http://thread.gmane.org/gmane.linux.kernel.iommu/12454 > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 59ee4b8..413bd64 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -48,7 +48,7 @@ > #include "io-pgtable.h" > > /* Maximum number of stream IDs assigned to a single device */ > -#define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS > +#define MAX_MASTER_STREAMIDS 128 > > /* Maximum number of context banks per SMMU */ > #define ARM_SMMU_MAX_CBS 128 > @@ -349,6 +349,12 @@ struct arm_smmu_domain { > struct iommu_domain domain; > }; > > +struct arm_smmu_phandle_args { > + struct device_node *np; > + int args_count; > + uint32_t args[MAX_MASTER_STREAMIDS]; > +}; > + > static struct iommu_ops arm_smmu_ops; > > static DEFINE_SPINLOCK(arm_smmu_devices_lock); > @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, > > static int register_smmu_master(struct arm_smmu_device *smmu, > struct device *dev, > - struct of_phandle_args *masterspec) > + struct arm_smmu_phandle_args *masterspec) > { > int i; > struct arm_smmu_master *master; > @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > struct arm_smmu_device *smmu; > struct device *dev = &pdev->dev; > struct rb_node *node; > - struct of_phandle_args masterspec; > + struct of_phandle_iterator it; > + struct arm_smmu_phandle_args masterspec; > int num_irqs, i, err; > > smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL); > @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > > i = 0; > smmu->masters = RB_ROOT; > - while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters", > - "#stream-id-cells", i, > - &masterspec)) { > + > + of_for_each_phandle(&it, err, dev->of_node, > + "mmu-masters", "#stream-id-cells", 0) { > + int count = of_phandle_iterator_args(&it, masterspec.args, > + MAX_MASTER_STREAMIDS); > + masterspec.np = of_node_get(it.node); > + masterspec.args_count = count; > + > err = register_smmu_master(smmu, dev, &masterspec); > if (err) { > dev_err(dev, "failed to add master %s\n", > @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > > i++; > } > + > + if (i == 0) > + goto out_put_masters; > + > dev_notice(dev, "registered %d master devices\n", i); > > parse_driver_options(smmu); >
Hi Robin, On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote: > In a stream-matching implementation, a device may quite legitimately > own anything up to _all_ of the stream IDs (32768, or 65536 if we > ever implement support for the SMMUv2 EXID extension), so this is > only a genuine limit for stream indexing (and if anyone ever > actually made one of those, I don't think they're running mainline > on it). Do you mean we might see a lot more than the currently 128 supported stream-ids for an smmu? > Alternatively, how straightforward is it to change the DT on your > machine? I'll be getting a v2 of [1] out in a couple of weeks (after > imminent holidays), which already gets rid of MAX_MASTER_STREAMIDS > altogether, and might also have grown proper SMR support by then. Hmm, I don't know how to change the DT of this Seattle machine. I think it is provided by the ACPI BIOS. At least it boots with grub2 and not u-boot and there is no DT in /boot. Joerg
On Wed, Mar 23, 2016 at 12:51:28PM +0100, Joerg Roedel wrote: > On Tue, Mar 22, 2016 at 06:53:48PM +0000, Robin Murphy wrote: > > In a stream-matching implementation, a device may quite legitimately > > own anything up to _all_ of the stream IDs (32768, or 65536 if we > > ever implement support for the SMMUv2 EXID extension), so this is > > only a genuine limit for stream indexing (and if anyone ever > > actually made one of those, I don't think they're running mainline > > on it). > > Do you mean we might see a lot more than the currently 128 supported > stream-ids for an smmu? We might, but this patch is still an improvement for now. Will
Hi Joerg, On Tue, Mar 22, 2016 at 06:58:29PM +0100, Joerg Roedel wrote: > From: Joerg Roedel <jroedel@suse.de> > > Remove the usage of of_parse_phandle_with_args() and replace > it by the phandle-iterator implementation so that we can > parse out all of the potentially present 128 stream-ids. > > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > drivers/iommu/arm-smmu.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 59ee4b8..413bd64 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -48,7 +48,7 @@ > #include "io-pgtable.h" > > /* Maximum number of stream IDs assigned to a single device */ > -#define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS > +#define MAX_MASTER_STREAMIDS 128 > > /* Maximum number of context banks per SMMU */ > #define ARM_SMMU_MAX_CBS 128 > @@ -349,6 +349,12 @@ struct arm_smmu_domain { > struct iommu_domain domain; > }; > > +struct arm_smmu_phandle_args { > + struct device_node *np; > + int args_count; > + uint32_t args[MAX_MASTER_STREAMIDS]; > +}; > + > static struct iommu_ops arm_smmu_ops; > > static DEFINE_SPINLOCK(arm_smmu_devices_lock); > @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, > > static int register_smmu_master(struct arm_smmu_device *smmu, > struct device *dev, > - struct of_phandle_args *masterspec) > + struct arm_smmu_phandle_args *masterspec) > { > int i; > struct arm_smmu_master *master; > @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > struct arm_smmu_device *smmu; > struct device *dev = &pdev->dev; > struct rb_node *node; > - struct of_phandle_args masterspec; > + struct of_phandle_iterator it; > + struct arm_smmu_phandle_args masterspec; > int num_irqs, i, err; > > smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL); > @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > > i = 0; > smmu->masters = RB_ROOT; > - while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters", > - "#stream-id-cells", i, > - &masterspec)) { > + > + of_for_each_phandle(&it, err, dev->of_node, > + "mmu-masters", "#stream-id-cells", 0) { > + int count = of_phandle_iterator_args(&it, masterspec.args, > + MAX_MASTER_STREAMIDS); > + masterspec.np = of_node_get(it.node); > + masterspec.args_count = count; > + > err = register_smmu_master(smmu, dev, &masterspec); > if (err) { > dev_err(dev, "failed to add master %s\n", > @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) > > i++; > } > + > + if (i == 0) > + goto out_put_masters; I'm confused by this hunk. If i == 0, then we shouldn't have registered any masters and therefore out_put_masters won't have anything to do. In fact, I'm not completely clear on how the of_node refcounting interacts with your iterators. Does the iterator put the node after you call the "next" function, or does it increment each thing exactly once? Will
Hi Will, On Tue, Mar 29, 2016 at 06:22:16PM +0100, Will Deacon wrote: > > + > > + if (i == 0) > > + goto out_put_masters; > > I'm confused by this hunk. If i == 0, then we shouldn't have registered > any masters and therefore out_put_masters won't have anything to do. The idea was that there is nothing more to do in the function when it didn't find any masters and so it can safely skip the rest of the function. But the original code doesn't do this either, so it certainly doesn't belong into this patch. I remove it for the next post. > In fact, I'm not completely clear on how the of_node refcounting interacts > with your iterators. Does the iterator put the node after you call the > "next" function, or does it increment each thing exactly once? The iterator will put the current node at the following _next call, so when you want to use each node, you need your own reference. It works like the pci_dev iterators, so if you break out of the loop you have to manually put the last node it returned. Joerg
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 59ee4b8..413bd64 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -48,7 +48,7 @@ #include "io-pgtable.h" /* Maximum number of stream IDs assigned to a single device */ -#define MAX_MASTER_STREAMIDS MAX_PHANDLE_ARGS +#define MAX_MASTER_STREAMIDS 128 /* Maximum number of context banks per SMMU */ #define ARM_SMMU_MAX_CBS 128 @@ -349,6 +349,12 @@ struct arm_smmu_domain { struct iommu_domain domain; }; +struct arm_smmu_phandle_args { + struct device_node *np; + int args_count; + uint32_t args[MAX_MASTER_STREAMIDS]; +}; + static struct iommu_ops arm_smmu_ops; static DEFINE_SPINLOCK(arm_smmu_devices_lock); @@ -458,7 +464,7 @@ static int insert_smmu_master(struct arm_smmu_device *smmu, static int register_smmu_master(struct arm_smmu_device *smmu, struct device *dev, - struct of_phandle_args *masterspec) + struct arm_smmu_phandle_args *masterspec) { int i; struct arm_smmu_master *master; @@ -1716,7 +1722,8 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) struct arm_smmu_device *smmu; struct device *dev = &pdev->dev; struct rb_node *node; - struct of_phandle_args masterspec; + struct of_phandle_iterator it; + struct arm_smmu_phandle_args masterspec; int num_irqs, i, err; smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL); @@ -1777,9 +1784,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) i = 0; smmu->masters = RB_ROOT; - while (!of_parse_phandle_with_args(dev->of_node, "mmu-masters", - "#stream-id-cells", i, - &masterspec)) { + + of_for_each_phandle(&it, err, dev->of_node, + "mmu-masters", "#stream-id-cells", 0) { + int count = of_phandle_iterator_args(&it, masterspec.args, + MAX_MASTER_STREAMIDS); + masterspec.np = of_node_get(it.node); + masterspec.args_count = count; + err = register_smmu_master(smmu, dev, &masterspec); if (err) { dev_err(dev, "failed to add master %s\n", @@ -1789,6 +1801,10 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev) i++; } + + if (i == 0) + goto out_put_masters; + dev_notice(dev, "registered %d master devices\n", i); parse_driver_options(smmu);