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

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

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