| Message ID | 5480B924.2010205@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 04, 2014 at 07:42:28PM +0000, Robin Murphy wrote: > On 04/12/14 17:58, Grant Likely wrote: > [...] > >>>> +struct of_iommu_node { > >>>> + struct list_head list; > >>>> + struct device_node *np; > >>>> + struct iommu_ops *ops; > >>> > >>> > >>> Why can't this be const? Why would anyone ever need to modify it? Also > >>> drivers do define their iommu_ops structures const these days, so you'll > >>> either still have to cast away at some point or the compiler will > >>> complain once you start calling this from drivers. > >>> > >> > >> ...whereas if we make everything const then the drivers that _do_ want to > >> use the private data introduced by this series and thus pass mutable > >> per-instance iommu_ops will be the ones having to cast. We have no users > >> either way until this series is merged, and the need to stash the > >> per-instance data somewhere other than np->data is in the way of getting it > >> merged - this is just a quick hack to address that. I think we've already > >> agreed that mutable per-instance iommu_ops holding private data aren't > >> particularly pleasant and will (hopefully) go away in the next iteration[1], > >> at which point this all changes anyway. > > > > Do you expect drivers to modify that *priv pointer after the ops > > structure is registered? I'd be very surprised if that was the use > > case. It's fine for the driver to register a non-const version, but > > once it is registered, the infrastructure can treat it as const from > > then on. > > Possibly not - certainly my current port of the ARM SMMU which makes use > of *priv is only ever reading it - although we did also wave around > reasons for mutable ops like dynamically changing the pgsize_bitmap and > possibly even swizzling individual ops for runtime reconfiguration. On > consideration though, I'd agree that things like that are mad enough to > stay well within individual drivers if they did ever happen, and > certainly shouldn't apply to this bit of the infrastructure at any rate. I certainly need to update the pgsize_bitmap at runtime because I don't know the supported page sizes until I've both (a) probed the hardware and (b) allocated page tables for a domain. We've already discussed moving the pgsize_bitmap out of the ops, but moving it somewhere where it remains const doesn't really help. Can I just take the patch that Grant acked, in the interest of getting something merged? As you say, there's plenty of planned changes in this area anyway. I plan to send Olof a pull request this afternoon. Will
On Friday 05 December 2014 12:10:37 Will Deacon wrote: > On Thu, Dec 04, 2014 at 07:42:28PM +0000, Robin Murphy wrote: > > On 04/12/14 17:58, Grant Likely wrote: > > [...] > > >>>> +struct of_iommu_node { > > >>>> + struct list_head list; > > >>>> + struct device_node *np; > > >>>> + struct iommu_ops *ops; > > >>> > > >>> > > >>> Why can't this be const? Why would anyone ever need to modify it? Also > > >>> drivers do define their iommu_ops structures const these days, so you'll > > >>> either still have to cast away at some point or the compiler will > > >>> complain once you start calling this from drivers. > > >>> > > >> > > >> ...whereas if we make everything const then the drivers that _do_ want to > > >> use the private data introduced by this series and thus pass mutable > > >> per-instance iommu_ops will be the ones having to cast. We have no users > > >> either way until this series is merged, and the need to stash the > > >> per-instance data somewhere other than np->data is in the way of getting it > > >> merged - this is just a quick hack to address that. I think we've already > > >> agreed that mutable per-instance iommu_ops holding private data aren't > > >> particularly pleasant and will (hopefully) go away in the next iteration[1], > > >> at which point this all changes anyway. > > > > > > Do you expect drivers to modify that *priv pointer after the ops > > > structure is registered? I'd be very surprised if that was the use > > > case. It's fine for the driver to register a non-const version, but > > > once it is registered, the infrastructure can treat it as const from > > > then on. > > > > Possibly not - certainly my current port of the ARM SMMU which makes use > > of *priv is only ever reading it - although we did also wave around > > reasons for mutable ops like dynamically changing the pgsize_bitmap and > > possibly even swizzling individual ops for runtime reconfiguration. On > > consideration though, I'd agree that things like that are mad enough to > > stay well within individual drivers if they did ever happen, and > > certainly shouldn't apply to this bit of the infrastructure at any rate. > > I certainly need to update the pgsize_bitmap at runtime because I don't > know the supported page sizes until I've both (a) probed the hardware > and (b) allocated page tables for a domain. We've already discussed > moving the pgsize_bitmap out of the ops, but moving it somewhere where > it remains const doesn't really help. > > Can I just take the patch that Grant acked, in the interest of getting > something merged? As you say, there's plenty of planned changes in this > area anyway. I plan to send Olof a pull request this afternoon. > I think that would be ok. The fix later should be to move the private data pointer into of_iommu_node, but I think that will require a larger set of changes, so let's defer that. Arnd
Hi Will, On 05/12/14 12:10, Will Deacon wrote: [...] >>> Do you expect drivers to modify that *priv pointer after the ops >>> structure is registered? I'd be very surprised if that was the use >>> case. It's fine for the driver to register a non-const version, but >>> once it is registered, the infrastructure can treat it as const from >>> then on. >> >> Possibly not - certainly my current port of the ARM SMMU which makes use >> of *priv is only ever reading it - although we did also wave around >> reasons for mutable ops like dynamically changing the pgsize_bitmap and >> possibly even swizzling individual ops for runtime reconfiguration. On >> consideration though, I'd agree that things like that are mad enough to >> stay well within individual drivers if they did ever happen, and >> certainly shouldn't apply to this bit of the infrastructure at any rate. > > I certainly need to update the pgsize_bitmap at runtime because I don't > know the supported page sizes until I've both (a) probed the hardware > and (b) allocated page tables for a domain. We've already discussed > moving the pgsize_bitmap out of the ops, but moving it somewhere where > it remains const doesn't really help. We can safely cast the call to get_ops in the SMMU driver though, since we'll know that we put a mutable per-instance ops in there in the first place. At least that way drivers that aren't taking advantage and just pass their static const ops around shouldn't provoke warnings. I deliberately didn't touch anything beyond get_ops as that would be too disruptive. > Can I just take the patch that Grant acked, in the interest of getting > something merged? As you say, there's plenty of planned changes in this > area anyway. I plan to send Olof a pull request this afternoon. Grant, Thierry? Personally I'm not fussed either way - the sooner something goes in, the sooner I can carry on working at replacing it :D Robin.
On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy <robin.murphy@arm.com> wrote: > Hi Will, > > On 05/12/14 12:10, Will Deacon wrote: > [...] >>>> >>>> Do you expect drivers to modify that *priv pointer after the ops >>>> structure is registered? I'd be very surprised if that was the use >>>> case. It's fine for the driver to register a non-const version, but >>>> once it is registered, the infrastructure can treat it as const from >>>> then on. >>> >>> >>> Possibly not - certainly my current port of the ARM SMMU which makes use >>> of *priv is only ever reading it - although we did also wave around >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and >>> possibly even swizzling individual ops for runtime reconfiguration. On >>> consideration though, I'd agree that things like that are mad enough to >>> stay well within individual drivers if they did ever happen, and >>> certainly shouldn't apply to this bit of the infrastructure at any rate. >> >> >> I certainly need to update the pgsize_bitmap at runtime because I don't >> know the supported page sizes until I've both (a) probed the hardware >> and (b) allocated page tables for a domain. We've already discussed >> moving the pgsize_bitmap out of the ops, but moving it somewhere where >> it remains const doesn't really help. > > > We can safely cast the call to get_ops in the SMMU driver though, since > we'll know that we put a mutable per-instance ops in there in the first > place. At least that way drivers that aren't taking advantage and just pass > their static const ops around shouldn't provoke warnings. I deliberately > didn't touch anything beyond get_ops as that would be too disruptive. > >> Can I just take the patch that Grant acked, in the interest of getting >> something merged? As you say, there's plenty of planned changes in this >> area anyway. I plan to send Olof a pull request this afternoon. > > > Grant, Thierry? Personally I'm not fussed either way - the sooner something > goes in, the sooner I can carry on working at replacing it :D I've already acked it. Why are we still talking about it? :-D g.
On Fri, Dec 05, 2014 at 01:06:52PM +0000, Grant Likely wrote: > On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy <robin.murphy@arm.com> wrote: > > Hi Will, > > > > On 05/12/14 12:10, Will Deacon wrote: > > [...] > >>>> > >>>> Do you expect drivers to modify that *priv pointer after the ops > >>>> structure is registered? I'd be very surprised if that was the use > >>>> case. It's fine for the driver to register a non-const version, but > >>>> once it is registered, the infrastructure can treat it as const from > >>>> then on. > >>> > >>> > >>> Possibly not - certainly my current port of the ARM SMMU which makes use > >>> of *priv is only ever reading it - although we did also wave around > >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and > >>> possibly even swizzling individual ops for runtime reconfiguration. On > >>> consideration though, I'd agree that things like that are mad enough to > >>> stay well within individual drivers if they did ever happen, and > >>> certainly shouldn't apply to this bit of the infrastructure at any rate. > >> > >> > >> I certainly need to update the pgsize_bitmap at runtime because I don't > >> know the supported page sizes until I've both (a) probed the hardware > >> and (b) allocated page tables for a domain. We've already discussed > >> moving the pgsize_bitmap out of the ops, but moving it somewhere where > >> it remains const doesn't really help. > > > > > > We can safely cast the call to get_ops in the SMMU driver though, since > > we'll know that we put a mutable per-instance ops in there in the first > > place. At least that way drivers that aren't taking advantage and just pass > > their static const ops around shouldn't provoke warnings. I deliberately > > didn't touch anything beyond get_ops as that would be too disruptive. > > > >> Can I just take the patch that Grant acked, in the interest of getting > >> something merged? As you say, there's plenty of planned changes in this > >> area anyway. I plan to send Olof a pull request this afternoon. > > > > > > Grant, Thierry? Personally I'm not fussed either way - the sooner something > > goes in, the sooner I can carry on working at replacing it :D > > I've already acked it. Why are we still talking about it? :-D Am I missing something? Why is there a need to rush things? Are there actually drivers that depend on this that will be merged during the 3.19 merge window? It seems like that'd be cutting it really close given where we are in the release cycle. If that's not the case, why even bother getting this hack into 3.19 if nobody uses it and we're going to change it in 3.20 anyway? Thierry
On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Dec 05, 2014 at 01:06:52PM +0000, Grant Likely wrote: >> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> > Hi Will, >> > >> > On 05/12/14 12:10, Will Deacon wrote: >> > [...] >> >>>> >> >>>> Do you expect drivers to modify that *priv pointer after the ops >> >>>> structure is registered? I'd be very surprised if that was the use >> >>>> case. It's fine for the driver to register a non-const version, but >> >>>> once it is registered, the infrastructure can treat it as const from >> >>>> then on. >> >>> >> >>> >> >>> Possibly not - certainly my current port of the ARM SMMU which makes use >> >>> of *priv is only ever reading it - although we did also wave around >> >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and >> >>> possibly even swizzling individual ops for runtime reconfiguration. On >> >>> consideration though, I'd agree that things like that are mad enough to >> >>> stay well within individual drivers if they did ever happen, and >> >>> certainly shouldn't apply to this bit of the infrastructure at any rate. >> >> >> >> >> >> I certainly need to update the pgsize_bitmap at runtime because I don't >> >> know the supported page sizes until I've both (a) probed the hardware >> >> and (b) allocated page tables for a domain. We've already discussed >> >> moving the pgsize_bitmap out of the ops, but moving it somewhere where >> >> it remains const doesn't really help. >> > >> > >> > We can safely cast the call to get_ops in the SMMU driver though, since >> > we'll know that we put a mutable per-instance ops in there in the first >> > place. At least that way drivers that aren't taking advantage and just pass >> > their static const ops around shouldn't provoke warnings. I deliberately >> > didn't touch anything beyond get_ops as that would be too disruptive. >> > >> >> Can I just take the patch that Grant acked, in the interest of getting >> >> something merged? As you say, there's plenty of planned changes in this >> >> area anyway. I plan to send Olof a pull request this afternoon. >> > >> > >> > Grant, Thierry? Personally I'm not fussed either way - the sooner something >> > goes in, the sooner I can carry on working at replacing it :D >> >> I've already acked it. Why are we still talking about it? :-D > > Am I missing something? Why is there a need to rush things? Are there > actually drivers that depend on this that will be merged during the 3.19 > merge window? It seems like that'd be cutting it really close given > where we are in the release cycle. > > If that's not the case, why even bother getting this hack into 3.19 if > nobody uses it and we're going to change it in 3.20 anyway? I also acked the non-hack version, the patch that doesn't try to make everything const. I assumed that was the one that we are talking about merging. g.
On Fri, Dec 05, 2014 at 01:21:31PM +0000, Grant Likely wrote: > On Fri, Dec 5, 2014 at 1:18 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Fri, Dec 05, 2014 at 01:06:52PM +0000, Grant Likely wrote: > >> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy <robin.murphy@arm.com> wrote: > >> > Hi Will, > >> > > >> > On 05/12/14 12:10, Will Deacon wrote: > >> > [...] > >> >>>> > >> >>>> Do you expect drivers to modify that *priv pointer after the ops > >> >>>> structure is registered? I'd be very surprised if that was the use > >> >>>> case. It's fine for the driver to register a non-const version, but > >> >>>> once it is registered, the infrastructure can treat it as const from > >> >>>> then on. > >> >>> > >> >>> > >> >>> Possibly not - certainly my current port of the ARM SMMU which makes use > >> >>> of *priv is only ever reading it - although we did also wave around > >> >>> reasons for mutable ops like dynamically changing the pgsize_bitmap and > >> >>> possibly even swizzling individual ops for runtime reconfiguration. On > >> >>> consideration though, I'd agree that things like that are mad enough to > >> >>> stay well within individual drivers if they did ever happen, and > >> >>> certainly shouldn't apply to this bit of the infrastructure at any rate. > >> >> > >> >> > >> >> I certainly need to update the pgsize_bitmap at runtime because I don't > >> >> know the supported page sizes until I've both (a) probed the hardware > >> >> and (b) allocated page tables for a domain. We've already discussed > >> >> moving the pgsize_bitmap out of the ops, but moving it somewhere where > >> >> it remains const doesn't really help. > >> > > >> > > >> > We can safely cast the call to get_ops in the SMMU driver though, since > >> > we'll know that we put a mutable per-instance ops in there in the first > >> > place. At least that way drivers that aren't taking advantage and just pass > >> > their static const ops around shouldn't provoke warnings. I deliberately > >> > didn't touch anything beyond get_ops as that would be too disruptive. > >> > > >> >> Can I just take the patch that Grant acked, in the interest of getting > >> >> something merged? As you say, there's plenty of planned changes in this > >> >> area anyway. I plan to send Olof a pull request this afternoon. > >> > > >> > > >> > Grant, Thierry? Personally I'm not fussed either way - the sooner something > >> > goes in, the sooner I can carry on working at replacing it :D > >> > >> I've already acked it. Why are we still talking about it? :-D > > > > Am I missing something? Why is there a need to rush things? Are there > > actually drivers that depend on this that will be merged during the 3.19 > > merge window? It seems like that'd be cutting it really close given > > where we are in the release cycle. > > > > If that's not the case, why even bother getting this hack into 3.19 if > > nobody uses it and we're going to change it in 3.20 anyway? > > I also acked the non-hack version, the patch that doesn't try to make > everything const. I assumed that was the one that we are talking about > merging. Actually not making everything const would be a hack. Drivers already mark their struct iommu_ops as const. But I'm more referring to the series as a whole. It seems like there are various issues that still need to be ironed out, and there's committment to do that before 3.20, so unless there are drivers that need any of the unfinished patches for 3.19 I don't see why we should be merging them in the first place. If getting them into 3.19 is merely to resolve dependencies then it's not going to work well anyway. Since this is all going to change in 3.20 anyway we'd likely have new dependencies that need to be handled, so might just as well do it properly at that time. Thierry
Hello, On 2014-12-05 14:18, Thierry Reding wrote: > On Fri, Dec 05, 2014 at 01:06:52PM +0000, Grant Likely wrote: >> On Fri, Dec 5, 2014 at 12:35 PM, Robin Murphy <robin.murphy@arm.com> wrote: >>> Hi Will, >>> >>> On 05/12/14 12:10, Will Deacon wrote: >>> [...] >>>>>> Do you expect drivers to modify that *priv pointer after the ops >>>>>> structure is registered? I'd be very surprised if that was the use >>>>>> case. It's fine for the driver to register a non-const version, but >>>>>> once it is registered, the infrastructure can treat it as const from >>>>>> then on. >>>>> >>>>> Possibly not - certainly my current port of the ARM SMMU which makes use >>>>> of *priv is only ever reading it - although we did also wave around >>>>> reasons for mutable ops like dynamically changing the pgsize_bitmap and >>>>> possibly even swizzling individual ops for runtime reconfiguration. On >>>>> consideration though, I'd agree that things like that are mad enough to >>>>> stay well within individual drivers if they did ever happen, and >>>>> certainly shouldn't apply to this bit of the infrastructure at any rate. >>>> >>>> I certainly need to update the pgsize_bitmap at runtime because I don't >>>> know the supported page sizes until I've both (a) probed the hardware >>>> and (b) allocated page tables for a domain. We've already discussed >>>> moving the pgsize_bitmap out of the ops, but moving it somewhere where >>>> it remains const doesn't really help. >>> >>> We can safely cast the call to get_ops in the SMMU driver though, since >>> we'll know that we put a mutable per-instance ops in there in the first >>> place. At least that way drivers that aren't taking advantage and just pass >>> their static const ops around shouldn't provoke warnings. I deliberately >>> didn't touch anything beyond get_ops as that would be too disruptive. >>> >>>> Can I just take the patch that Grant acked, in the interest of getting >>>> something merged? As you say, there's plenty of planned changes in this >>>> area anyway. I plan to send Olof a pull request this afternoon. >>> >>> Grant, Thierry? Personally I'm not fussed either way - the sooner something >>> goes in, the sooner I can carry on working at replacing it :D >> I've already acked it. Why are we still talking about it? :-D > Am I missing something? Why is there a need to rush things? Are there > actually drivers that depend on this that will be merged during the 3.19 > merge window? It seems like that'd be cutting it really close given > where we are in the release cycle. > > If that's not the case, why even bother getting this hack into 3.19 if > nobody uses it and we're going to change it in 3.20 anyway? There are Exynos SYSMMU patches ready & waiting for this gets merged... Best regards
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 73236d3..39f581f 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -94,6 +94,44 @@ int of_get_dma_window(struct device_node *dn, const char *prefix, int index, } EXPORT_SYMBOL_GPL(of_get_dma_window); +struct of_iommu_node { + struct list_head list; + struct device_node *np; + const struct iommu_ops *ops; +}; +static LIST_HEAD(of_iommu_list); +static DEFINE_SPINLOCK(of_iommu_lock); + +void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops) +{ + struct of_iommu_node *iommu = kzalloc(sizeof(*iommu), GFP_KERNEL); + + if (WARN_ON(!iommu)) + return; + + INIT_LIST_HEAD(&iommu->list); + iommu->np = np; + iommu->ops = ops; + spin_lock(&of_iommu_lock); + list_add_tail(&iommu->list, &of_iommu_list); + spin_unlock(&of_iommu_lock); +} + +const struct iommu_ops *of_iommu_get_ops(struct device_node *np) +{ + struct of_iommu_node *node; + const struct iommu_ops *ops = NULL; + + spin_lock(&of_iommu_lock); + list_for_each_entry(node, &of_iommu_list, list) + if (node->np == np) { + ops = node->ops; + break; + } + spin_unlock(&of_iommu_lock); + return ops; +} + struct iommu_ops *of_iommu_configure(struct device *dev) { struct of_phandle_args iommu_spec; @@ -110,7 +148,7 @@ struct iommu_ops *of_iommu_configure(struct device *dev) "#iommu-cells", idx, &iommu_spec)) { np = iommu_spec.np; - ops = of_iommu_get_ops(np); + ops = (struct iommu_ops *)of_iommu_get_ops(np); if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec)) goto err_put_node; diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index d03abbb..83f6d0b 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -31,16 +31,8 @@ static inline struct iommu_ops *of_iommu_configure(struct device *dev) #endif /* CONFIG_OF_IOMMU */ -static inline void of_iommu_set_ops(struct device_node *np, - const struct iommu_ops *ops) -{ - np->data = (struct iommu_ops *)ops; -} - -static inline struct iommu_ops *of_iommu_get_ops(struct device_node *np) -{ - return np->data; -} +void of_iommu_set_ops(struct device_node *np, const struct iommu_ops *ops); +const struct iommu_ops *of_iommu_get_ops(struct device_node *np); extern struct of_device_id __iommu_of_table;