Messages in this thread | | | Subject | Re: [PATCH] iommu/iova: Separate out rcache init | From | John Garry <> | Date | Wed, 26 Jan 2022 17:58:04 +0000 |
| |
Hi Robin,
>> >> Signed-off-by: John Garry<john.garry@huawei.com> > Mangled patch? (no "---" separator here)
hmm... not sure. As an experiment, I just downloaded this patch from lore.kernel.org and it applies ok.
> > Overall this looks great, just a few comments further down... >
...
>> +} >> +EXPORT_SYMBOL_GPL(iova_domain_init_rcaches); >> + >> +void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> + cpuhp_state_remove_instance_nocalls(CPUHP_IOMMU_IOVA_DEAD, >> + &iovad->cpuhp_dead); >> + free_iova_rcaches(iovad); >> } >> +EXPORT_SYMBOL_GPL(iova_domain_free_rcaches); > I think we should continue to expect external callers to clean up with > put_iova_domain().
ok, fine, makes sense
> If they aren't doing that already they have a bug > (albeit minor), and we don't want to give the impression that it's OK to > free the caches at any point*other* than tearing down the whole > iova_domain, since the implementation really wouldn't expect that. > >> /* >> * Try inserting IOVA range starting with 'iova_pfn' into 'rcache', and >> @@ -831,7 +872,7 @@ static unsigned long iova_rcache_get(struct iova_domain *iovad, >> { >> unsigned int log_size = order_base_2(size); >> >> - if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE) >> + if (log_size >= IOVA_RANGE_CACHE_MAX_SIZE || !iovad->rcaches) >> return 0; >>
..
>> @@ -102,6 +92,8 @@ struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo, >> unsigned long pfn_hi); >> void init_iova_domain(struct iova_domain *iovad, unsigned long granule, >> unsigned long start_pfn); >> +int iova_domain_init_rcaches(struct iova_domain *iovad); >> +void iova_domain_free_rcaches(struct iova_domain *iovad); > As above, I vote for just forward-declaring the free routine in iova.c > and keeping it entirely private.
ok
> >> struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn); >> void put_iova_domain(struct iova_domain *iovad); >> #else >> @@ -157,6 +149,15 @@ static inline void init_iova_domain(struct iova_domain *iovad, >> { >> } >> >> +static inline int iova_domain_init_rcaches(struct iova_domain *iovad) >> +{ >> + return -ENOTSUPP; >> +} >> + >> +static inline void iova_domain_free_rcaches(struct iova_domain *iovad) >> +{ >> +} >> + > I'd be inclined not to add stubs at all - I think it's a reasonable > assumption that anyone involved enough to care about rcaches has a hard > dependency on IOMMU_IOVA already.
So iova_domain_free_rcaches() would disappear from here as a result of the changes discussed above.
As for iova_domain_init_rcaches(), I was just following the IOMMU/IOVA coding practice - that is, always stub.
> It's certainly the case today, and I'd > hardly want to encourage more users anyway.
I think that stronger deterrents would be needed :)
Anyway, I can remove it.
Thanks, John
| |