Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 6 Feb 2024 11:01:16 +0000 | Subject | Re: [PATCH v3 1/3] iommu/iova: Tidy up iova_cache_get() failure | From | John Garry <> |
| |
On 05/02/2024 15:32, Robin Murphy wrote: > Failure handling in iova_cache_get() is a little messy, and we'd like > to add some more to it, so let's tidy up a bit first. By leaving the > hotplug handler until last we can take advantage of kmem_cache_destroy() > being NULL-safe to have a single cleanup label. We can also improve the > error reporting, noting that kmem_cache_create() already screams if it > fails, so that one is redundant. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
Regardless of a couple of minor comments, below, FWIW: Reviewed-by: John Garry <john.g.garry@oracle.com>
> --- > drivers/iommu/iova.c | 33 ++++++++++++++++----------------- > 1 file changed, 16 insertions(+), 17 deletions(-) > > diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c > index d30e453d0fb4..cf95001d85c0 100644 > --- a/drivers/iommu/iova.c > +++ b/drivers/iommu/iova.c > @@ -254,26 +254,20 @@ static void free_iova_mem(struct iova *iova) > > int iova_cache_get(void) > { > + int err = -ENOMEM; > + > mutex_lock(&iova_cache_mutex); > if (!iova_cache_users) { > - int ret; > + iova_cache = kmem_cache_create("iommu_iova", sizeof(struct iova), 0, > + SLAB_HWCACHE_ALIGN, NULL);
Maybe can use KMEM_CACHE(), but it would mean that the name would change.
> + if (!iova_cache) > + goto out_err; > > - ret = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD, "iommu/iova:dead", NULL, > - iova_cpuhp_dead); > - if (ret) { > - mutex_unlock(&iova_cache_mutex); > - pr_err("Couldn't register cpuhp handler\n"); > - return ret; > - } > - > - iova_cache = kmem_cache_create( > - "iommu_iova", sizeof(struct iova), 0, > - SLAB_HWCACHE_ALIGN, NULL); > - if (!iova_cache) { > - cpuhp_remove_multi_state(CPUHP_IOMMU_IOVA_DEAD); > - mutex_unlock(&iova_cache_mutex); > - pr_err("Couldn't create iova cache\n"); > - return -ENOMEM; > + err = cpuhp_setup_state_multi(CPUHP_IOMMU_IOVA_DEAD, "iommu/iova:dead", > + NULL, iova_cpuhp_dead); > + if (err) { > + pr_err("IOVA: Couldn't register cpuhp handler: %pe\n", ERR_PTR(err));
Maybe can use pr_fmt with "iova".
> + goto out_err; > } > } > > @@ -281,6 +275,11 @@ int iova_cache_get(void) > mutex_unlock(&iova_cache_mutex); > > return 0; > + > +out_err: > + kmem_cache_destroy(iova_cache); > + mutex_unlock(&iova_cache_mutex); > + return err; > } > EXPORT_SYMBOL_GPL(iova_cache_get); >
| ![\](/images/icornerr.gif) |