Re: [PATCH v6 1/8] iommu: provide early initialisation hook for IOMMU drivers

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

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