All of lore.kernel.org
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: "Isaac J. Manjarres" <isaacm@codeaurora.org>,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org
Cc: will@kernel.org, joro@8bytes.org, pdaly@codeaurora.org,
	pratikp@codeaurora.org, kernel-team@android.com
Subject: Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration
Date: Tue, 22 Dec 2020 19:27:41 +0000	[thread overview]
Message-ID: <4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com> (raw)
In-Reply-To: <1608597876-32367-2-git-send-email-isaacm@codeaurora.org>

On 2020-12-22 00:44, Isaac J. Manjarres wrote:
> The io-pgtable code constructs an array of init functions for each
> page table format at compile time. This is not ideal, as this
> increases the footprint of the io-pgtable code, as well as prevents
> io-pgtable formats from being built as kernel modules.
> 
> In preparation for modularizing the io-pgtable formats, switch to a
> dynamic registration scheme, where each io-pgtable format can register
> their init functions with the io-pgtable code at boot or module
> insertion time.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>   drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++-
>   drivers/iommu/io-pgtable-arm.c     | 90 ++++++++++++++++++++++++++----------
>   drivers/iommu/io-pgtable.c         | 94 ++++++++++++++++++++++++++++++++------
>   include/linux/io-pgtable.h         | 51 +++++++++++++--------
>   4 files changed, 209 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 1d92ac9..89aad2f 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -28,6 +28,7 @@
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/kmemleak.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +	.fmt	= ARM_V7S,
>   	.alloc	= arm_v7s_alloc_pgtable,
>   	.free	= arm_v7s_free_pgtable,
>   };
> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void)
>   	pr_info("self test ok\n");
>   	return 0;
>   }
> -subsys_initcall(arm_v7s_do_selftests);
> +#else
> +static int arm_v7s_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_v7s_init(void)
> +{
> +	int ret;
> +
> +	ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns);
> +	if (ret < 0) {
> +		pr_err("Failed to register ARM V7S format\n");

Super-nit: I think "v7s" should probably be lowercase there. Also 
general consistency WRT to showing the error code and whether or not to 
abbreviate "format" would be nice.

> +		return ret;
> +	}
> +
> +	ret = arm_v7s_do_selftests();
> +	if (ret < 0)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +
> +	return ret;
> +}
> +core_initcall(arm_v7s_init);
> +
> +static void __exit arm_v7s_exit(void)
> +{
> +	io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +}
> +module_exit(arm_v7s_exit);
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58..ff0ea2f 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -13,6 +13,7 @@
>   #include <linux/bitops.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> -	.alloc	= arm_mali_lpae_alloc_pgtable,
> -	.free	= arm_lpae_free_pgtable,
> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = {
> +	{
> +		.fmt	= ARM_32_LPAE_S1,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_32_LPAE_S2,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S1,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S2,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_MALI_LPAE,
> +		.alloc	= arm_mali_lpae_alloc_pgtable,
> +		.free	= arm_lpae_free_pgtable,
> +	},
>   };
>   
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void)
>   	pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail);
>   	return fail ? -EFAULT : 0;
>   }
> -subsys_initcall(arm_lpae_do_selftests);
> +#else
> +static int __init arm_lpae_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_lpae_init(void)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) {
> +		ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]);
> +		if (ret < 0) {
> +			pr_err("Failed to register ARM LPAE fmt: %d\n");
> +			goto err_io_pgtable_register;
> +		}
> +	}
> +
> +	ret = arm_lpae_do_selftests();
> +	if (ret < 0)
> +		goto err_io_pgtable_register;
> +
> +	return 0;
> +
> +err_io_pgtable_register:
> +	for (i = i - 1; i >= 0; i--)

Personally I find "while (i--)" a bit clearer for this kind of 
unwinding, but maybe post-decrement isn't to everyone's taste.

> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +	return ret;
> +}
> +core_initcall(arm_lpae_init);
> +
> +static void __exit arm_lpae_exit(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +}
> +module_exit(arm_lpae_exit);
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 94394c8..2c6eb2e 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -10,33 +10,45 @@
>   #include <linux/bug.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/rwlock.h>
> +#include <linux/slab.h>
>   #include <linux/types.h>
>   
> -static const struct io_pgtable_init_fns *
> -io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE
> -	[ARM_32_LPAE_S1] = &io_pgtable_arm_32_lpae_s1_init_fns,
> -	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
> -	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
> -	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
> -	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
> -#endif
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
> -	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
> -#endif
> +struct io_pgtable_init_fns_node {
> +	struct io_pgtable_init_fns *fns;
> +	struct list_head list;
>   };
>   
> +static LIST_HEAD(io_pgtable_init_fns_list);
> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock);
> +
> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt)
> +{
> +	struct io_pgtable_init_fns_node *iter;
> +	struct io_pgtable_init_fns *fns = NULL;
> +
> +	read_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == fmt) {
> +			fns = iter->fns;
> +			break;
> +		}
> +	read_unlock(&io_pgtable_init_fns_list_lock);
> +
> +	return fns;
> +}

I think it would be a lot easier to stick with a simple array indexed by 
enum - that way you can just set/clear/test entries without needing to 
worry about locking. Basically just remove the const and the 
initialisers from the existing one ;)

(and if you think you're concerned about memory, consider that just the 
list head plus lock is already half the size of the table)

Other than that, I think this all looks pretty promising - I'd suggest 
sending a non-RFC after rc1 so that it gets everyone's proper attention.

Thanks,
Robin.

> +
>   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>   					    struct io_pgtable_cfg *cfg,
>   					    void *cookie)
>   {
>   	struct io_pgtable *iop;
> -	const struct io_pgtable_init_fns *fns;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (fmt >= IO_PGTABLE_NUM_FMTS)
>   		return NULL;
>   
> -	fns = io_pgtable_init_table[fmt];
> +	fns = io_pgtable_get_init_fns(fmt);
>   	if (!fns)
>   		return NULL;
>   
> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops);
>   void free_io_pgtable_ops(struct io_pgtable_ops *ops)
>   {
>   	struct io_pgtable *iop;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (!ops)
>   		return;
>   
>   	iop = io_pgtable_ops_to_pgtable(ops);
>   	io_pgtable_tlb_flush_all(iop);
> -	io_pgtable_init_table[iop->fmt]->free(iop);
> +	fns = io_pgtable_get_init_fns(iop->fmt);
> +	if (fns)
> +		fns->free(iop);
>   }
>   EXPORT_SYMBOL_GPL(free_io_pgtable_ops);
> +
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *fns_node;
> +	int ret = 0;
> +
> +	if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS ||
> +	    !init_fns->alloc || !init_fns->free)
> +		return -EINVAL;
> +
> +	fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL);
> +	if (!fns_node)
> +		return -ENOMEM;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == init_fns->fmt) {
> +			ret = -EEXIST;
> +			kfree(fns_node);
> +			break;
> +		}
> +
> +	if (!ret) {
> +		fns_node->fns = init_fns;
> +		INIT_LIST_HEAD(&fns_node->list);
> +		list_add_tail(&fns_node->list, &io_pgtable_init_fns_list);
> +	}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register);
> +
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *tmp;
> +
> +	if (!init_fns)
> +		return;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list)
> +		if (iter->fns == init_fns) {
> +			list_del(&iter->list);
> +			kfree(iter);
> +			break;
> +		}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister);
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index ea727eb..45b367ce 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -163,6 +163,38 @@ struct io_pgtable_ops {
>   };
>   
>   /**
> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> + *                              particular format.
> + *
> + * @fmt:   The page table format.
> + * @alloc: Allocate a set of page tables described by cfg.
> + * @free:  Free the page tables associated with iop.
> + */
> +struct io_pgtable_init_fns {
> +	enum io_pgtable_fmt fmt;
> +	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> +	void (*free)(struct io_pgtable *iop);
> +};
> +
> +/**
> + * io_pgtable_ops_register() - Register the page table routines for a page table
> + *                             format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns);
> +
> +/**
> + * io_pgtable_ops_unregister() - Unregister the page table routines for a page
> + *                               table format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns);
> +
> +/**
>    * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU.
>    *
>    * @fmt:    The page table format.
> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop,
>   		iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie);
>   }
>   
> -/**
> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> - *                              particular format.
> - *
> - * @alloc: Allocate a set of page tables described by cfg.
> - * @free:  Free the page tables associated with iop.
> - */
> -struct io_pgtable_init_fns {
> -	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> -	void (*free)(struct io_pgtable *iop);
> -};
> -
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
> -
>   #endif /* __IO_PGTABLE_H */
> 

WARNING: multiple messages have this Message-ID
From: Robin Murphy <robin.murphy@arm.com>
To: "Isaac J. Manjarres" <isaacm@codeaurora.org>,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org
Cc: pratikp@codeaurora.org, kernel-team@android.com, will@kernel.org,
	pdaly@codeaurora.org
Subject: Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration
Date: Tue, 22 Dec 2020 19:27:41 +0000	[thread overview]
Message-ID: <4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com> (raw)
In-Reply-To: <1608597876-32367-2-git-send-email-isaacm@codeaurora.org>

On 2020-12-22 00:44, Isaac J. Manjarres wrote:
> The io-pgtable code constructs an array of init functions for each
> page table format at compile time. This is not ideal, as this
> increases the footprint of the io-pgtable code, as well as prevents
> io-pgtable formats from being built as kernel modules.
> 
> In preparation for modularizing the io-pgtable formats, switch to a
> dynamic registration scheme, where each io-pgtable format can register
> their init functions with the io-pgtable code at boot or module
> insertion time.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>   drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++-
>   drivers/iommu/io-pgtable-arm.c     | 90 ++++++++++++++++++++++++++----------
>   drivers/iommu/io-pgtable.c         | 94 ++++++++++++++++++++++++++++++++------
>   include/linux/io-pgtable.h         | 51 +++++++++++++--------
>   4 files changed, 209 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 1d92ac9..89aad2f 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -28,6 +28,7 @@
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/kmemleak.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +	.fmt	= ARM_V7S,
>   	.alloc	= arm_v7s_alloc_pgtable,
>   	.free	= arm_v7s_free_pgtable,
>   };
> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void)
>   	pr_info("self test ok\n");
>   	return 0;
>   }
> -subsys_initcall(arm_v7s_do_selftests);
> +#else
> +static int arm_v7s_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_v7s_init(void)
> +{
> +	int ret;
> +
> +	ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns);
> +	if (ret < 0) {
> +		pr_err("Failed to register ARM V7S format\n");

Super-nit: I think "v7s" should probably be lowercase there. Also 
general consistency WRT to showing the error code and whether or not to 
abbreviate "format" would be nice.

> +		return ret;
> +	}
> +
> +	ret = arm_v7s_do_selftests();
> +	if (ret < 0)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +
> +	return ret;
> +}
> +core_initcall(arm_v7s_init);
> +
> +static void __exit arm_v7s_exit(void)
> +{
> +	io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +}
> +module_exit(arm_v7s_exit);
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58..ff0ea2f 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -13,6 +13,7 @@
>   #include <linux/bitops.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> -	.alloc	= arm_mali_lpae_alloc_pgtable,
> -	.free	= arm_lpae_free_pgtable,
> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = {
> +	{
> +		.fmt	= ARM_32_LPAE_S1,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_32_LPAE_S2,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S1,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S2,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_MALI_LPAE,
> +		.alloc	= arm_mali_lpae_alloc_pgtable,
> +		.free	= arm_lpae_free_pgtable,
> +	},
>   };
>   
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void)
>   	pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail);
>   	return fail ? -EFAULT : 0;
>   }
> -subsys_initcall(arm_lpae_do_selftests);
> +#else
> +static int __init arm_lpae_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_lpae_init(void)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) {
> +		ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]);
> +		if (ret < 0) {
> +			pr_err("Failed to register ARM LPAE fmt: %d\n");
> +			goto err_io_pgtable_register;
> +		}
> +	}
> +
> +	ret = arm_lpae_do_selftests();
> +	if (ret < 0)
> +		goto err_io_pgtable_register;
> +
> +	return 0;
> +
> +err_io_pgtable_register:
> +	for (i = i - 1; i >= 0; i--)

Personally I find "while (i--)" a bit clearer for this kind of 
unwinding, but maybe post-decrement isn't to everyone's taste.

> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +	return ret;
> +}
> +core_initcall(arm_lpae_init);
> +
> +static void __exit arm_lpae_exit(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +}
> +module_exit(arm_lpae_exit);
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 94394c8..2c6eb2e 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -10,33 +10,45 @@
>   #include <linux/bug.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/rwlock.h>
> +#include <linux/slab.h>
>   #include <linux/types.h>
>   
> -static const struct io_pgtable_init_fns *
> -io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE
> -	[ARM_32_LPAE_S1] = &io_pgtable_arm_32_lpae_s1_init_fns,
> -	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
> -	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
> -	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
> -	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
> -#endif
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
> -	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
> -#endif
> +struct io_pgtable_init_fns_node {
> +	struct io_pgtable_init_fns *fns;
> +	struct list_head list;
>   };
>   
> +static LIST_HEAD(io_pgtable_init_fns_list);
> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock);
> +
> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt)
> +{
> +	struct io_pgtable_init_fns_node *iter;
> +	struct io_pgtable_init_fns *fns = NULL;
> +
> +	read_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == fmt) {
> +			fns = iter->fns;
> +			break;
> +		}
> +	read_unlock(&io_pgtable_init_fns_list_lock);
> +
> +	return fns;
> +}

I think it would be a lot easier to stick with a simple array indexed by 
enum - that way you can just set/clear/test entries without needing to 
worry about locking. Basically just remove the const and the 
initialisers from the existing one ;)

(and if you think you're concerned about memory, consider that just the 
list head plus lock is already half the size of the table)

Other than that, I think this all looks pretty promising - I'd suggest 
sending a non-RFC after rc1 so that it gets everyone's proper attention.

Thanks,
Robin.

> +
>   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>   					    struct io_pgtable_cfg *cfg,
>   					    void *cookie)
>   {
>   	struct io_pgtable *iop;
> -	const struct io_pgtable_init_fns *fns;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (fmt >= IO_PGTABLE_NUM_FMTS)
>   		return NULL;
>   
> -	fns = io_pgtable_init_table[fmt];
> +	fns = io_pgtable_get_init_fns(fmt);
>   	if (!fns)
>   		return NULL;
>   
> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops);
>   void free_io_pgtable_ops(struct io_pgtable_ops *ops)
>   {
>   	struct io_pgtable *iop;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (!ops)
>   		return;
>   
>   	iop = io_pgtable_ops_to_pgtable(ops);
>   	io_pgtable_tlb_flush_all(iop);
> -	io_pgtable_init_table[iop->fmt]->free(iop);
> +	fns = io_pgtable_get_init_fns(iop->fmt);
> +	if (fns)
> +		fns->free(iop);
>   }
>   EXPORT_SYMBOL_GPL(free_io_pgtable_ops);
> +
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *fns_node;
> +	int ret = 0;
> +
> +	if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS ||
> +	    !init_fns->alloc || !init_fns->free)
> +		return -EINVAL;
> +
> +	fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL);
> +	if (!fns_node)
> +		return -ENOMEM;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == init_fns->fmt) {
> +			ret = -EEXIST;
> +			kfree(fns_node);
> +			break;
> +		}
> +
> +	if (!ret) {
> +		fns_node->fns = init_fns;
> +		INIT_LIST_HEAD(&fns_node->list);
> +		list_add_tail(&fns_node->list, &io_pgtable_init_fns_list);
> +	}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register);
> +
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *tmp;
> +
> +	if (!init_fns)
> +		return;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list)
> +		if (iter->fns == init_fns) {
> +			list_del(&iter->list);
> +			kfree(iter);
> +			break;
> +		}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister);
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index ea727eb..45b367ce 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -163,6 +163,38 @@ struct io_pgtable_ops {
>   };
>   
>   /**
> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> + *                              particular format.
> + *
> + * @fmt:   The page table format.
> + * @alloc: Allocate a set of page tables described by cfg.
> + * @free:  Free the page tables associated with iop.
> + */
> +struct io_pgtable_init_fns {
> +	enum io_pgtable_fmt fmt;
> +	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> +	void (*free)(struct io_pgtable *iop);
> +};
> +
> +/**
> + * io_pgtable_ops_register() - Register the page table routines for a page table
> + *                             format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns);
> +
> +/**
> + * io_pgtable_ops_unregister() - Unregister the page table routines for a page
> + *                               table format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns);
> +
> +/**
>    * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU.
>    *
>    * @fmt:    The page table format.
> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop,
>   		iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie);
>   }
>   
> -/**
> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> - *                              particular format.
> - *
> - * @alloc: Allocate a set of page tables described by cfg.
> - * @free:  Free the page tables associated with iop.
> - */
> -struct io_pgtable_init_fns {
> -	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> -	void (*free)(struct io_pgtable *iop);
> -};
> -
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
> -
>   #endif /* __IO_PGTABLE_H */
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

WARNING: multiple messages have this Message-ID
From: Robin Murphy <robin.murphy@arm.com>
To: "Isaac J. Manjarres" <isaacm@codeaurora.org>,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org
Cc: pratikp@codeaurora.org, joro@8bytes.org, kernel-team@android.com,
	will@kernel.org, pdaly@codeaurora.org
Subject: Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration
Date: Tue, 22 Dec 2020 19:27:41 +0000	[thread overview]
Message-ID: <4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com> (raw)
In-Reply-To: <1608597876-32367-2-git-send-email-isaacm@codeaurora.org>

On 2020-12-22 00:44, Isaac J. Manjarres wrote:
> The io-pgtable code constructs an array of init functions for each
> page table format at compile time. This is not ideal, as this
> increases the footprint of the io-pgtable code, as well as prevents
> io-pgtable formats from being built as kernel modules.
> 
> In preparation for modularizing the io-pgtable formats, switch to a
> dynamic registration scheme, where each io-pgtable format can register
> their init functions with the io-pgtable code at boot or module
> insertion time.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>   drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++-
>   drivers/iommu/io-pgtable-arm.c     | 90 ++++++++++++++++++++++++++----------
>   drivers/iommu/io-pgtable.c         | 94 ++++++++++++++++++++++++++++++++------
>   include/linux/io-pgtable.h         | 51 +++++++++++++--------
>   4 files changed, 209 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 1d92ac9..89aad2f 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -28,6 +28,7 @@
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/kmemleak.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +	.fmt	= ARM_V7S,
>   	.alloc	= arm_v7s_alloc_pgtable,
>   	.free	= arm_v7s_free_pgtable,
>   };
> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void)
>   	pr_info("self test ok\n");
>   	return 0;
>   }
> -subsys_initcall(arm_v7s_do_selftests);
> +#else
> +static int arm_v7s_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_v7s_init(void)
> +{
> +	int ret;
> +
> +	ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns);
> +	if (ret < 0) {
> +		pr_err("Failed to register ARM V7S format\n");

Super-nit: I think "v7s" should probably be lowercase there. Also 
general consistency WRT to showing the error code and whether or not to 
abbreviate "format" would be nice.

> +		return ret;
> +	}
> +
> +	ret = arm_v7s_do_selftests();
> +	if (ret < 0)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +
> +	return ret;
> +}
> +core_initcall(arm_v7s_init);
> +
> +static void __exit arm_v7s_exit(void)
> +{
> +	io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +}
> +module_exit(arm_v7s_exit);
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58..ff0ea2f 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -13,6 +13,7 @@
>   #include <linux/bitops.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> -	.alloc	= arm_mali_lpae_alloc_pgtable,
> -	.free	= arm_lpae_free_pgtable,
> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = {
> +	{
> +		.fmt	= ARM_32_LPAE_S1,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_32_LPAE_S2,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S1,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S2,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_MALI_LPAE,
> +		.alloc	= arm_mali_lpae_alloc_pgtable,
> +		.free	= arm_lpae_free_pgtable,
> +	},
>   };
>   
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void)
>   	pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail);
>   	return fail ? -EFAULT : 0;
>   }
> -subsys_initcall(arm_lpae_do_selftests);
> +#else
> +static int __init arm_lpae_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_lpae_init(void)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) {
> +		ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]);
> +		if (ret < 0) {
> +			pr_err("Failed to register ARM LPAE fmt: %d\n");
> +			goto err_io_pgtable_register;
> +		}
> +	}
> +
> +	ret = arm_lpae_do_selftests();
> +	if (ret < 0)
> +		goto err_io_pgtable_register;
> +
> +	return 0;
> +
> +err_io_pgtable_register:
> +	for (i = i - 1; i >= 0; i--)

Personally I find "while (i--)" a bit clearer for this kind of 
unwinding, but maybe post-decrement isn't to everyone's taste.

> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +	return ret;
> +}
> +core_initcall(arm_lpae_init);
> +
> +static void __exit arm_lpae_exit(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +}
> +module_exit(arm_lpae_exit);
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 94394c8..2c6eb2e 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -10,33 +10,45 @@
>   #include <linux/bug.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/rwlock.h>
> +#include <linux/slab.h>
>   #include <linux/types.h>
>   
> -static const struct io_pgtable_init_fns *
> -io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE
> -	[ARM_32_LPAE_S1] = &io_pgtable_arm_32_lpae_s1_init_fns,
> -	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
> -	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
> -	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
> -	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
> -#endif
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
> -	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
> -#endif
> +struct io_pgtable_init_fns_node {
> +	struct io_pgtable_init_fns *fns;
> +	struct list_head list;
>   };
>   
> +static LIST_HEAD(io_pgtable_init_fns_list);
> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock);
> +
> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt)
> +{
> +	struct io_pgtable_init_fns_node *iter;
> +	struct io_pgtable_init_fns *fns = NULL;
> +
> +	read_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == fmt) {
> +			fns = iter->fns;
> +			break;
> +		}
> +	read_unlock(&io_pgtable_init_fns_list_lock);
> +
> +	return fns;
> +}

I think it would be a lot easier to stick with a simple array indexed by 
enum - that way you can just set/clear/test entries without needing to 
worry about locking. Basically just remove the const and the 
initialisers from the existing one ;)

(and if you think you're concerned about memory, consider that just the 
list head plus lock is already half the size of the table)

Other than that, I think this all looks pretty promising - I'd suggest 
sending a non-RFC after rc1 so that it gets everyone's proper attention.

Thanks,
Robin.

> +
>   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>   					    struct io_pgtable_cfg *cfg,
>   					    void *cookie)
>   {
>   	struct io_pgtable *iop;
> -	const struct io_pgtable_init_fns *fns;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (fmt >= IO_PGTABLE_NUM_FMTS)
>   		return NULL;
>   
> -	fns = io_pgtable_init_table[fmt];
> +	fns = io_pgtable_get_init_fns(fmt);
>   	if (!fns)
>   		return NULL;
>   
> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops);
>   void free_io_pgtable_ops(struct io_pgtable_ops *ops)
>   {
>   	struct io_pgtable *iop;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (!ops)
>   		return;
>   
>   	iop = io_pgtable_ops_to_pgtable(ops);
>   	io_pgtable_tlb_flush_all(iop);
> -	io_pgtable_init_table[iop->fmt]->free(iop);
> +	fns = io_pgtable_get_init_fns(iop->fmt);
> +	if (fns)
> +		fns->free(iop);
>   }
>   EXPORT_SYMBOL_GPL(free_io_pgtable_ops);
> +
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *fns_node;
> +	int ret = 0;
> +
> +	if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS ||
> +	    !init_fns->alloc || !init_fns->free)
> +		return -EINVAL;
> +
> +	fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL);
> +	if (!fns_node)
> +		return -ENOMEM;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == init_fns->fmt) {
> +			ret = -EEXIST;
> +			kfree(fns_node);
> +			break;
> +		}
> +
> +	if (!ret) {
> +		fns_node->fns = init_fns;
> +		INIT_LIST_HEAD(&fns_node->list);
> +		list_add_tail(&fns_node->list, &io_pgtable_init_fns_list);
> +	}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register);
> +
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *tmp;
> +
> +	if (!init_fns)
> +		return;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list)
> +		if (iter->fns == init_fns) {
> +			list_del(&iter->list);
> +			kfree(iter);
> +			break;
> +		}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister);
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index ea727eb..45b367ce 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -163,6 +163,38 @@ struct io_pgtable_ops {
>   };
>   
>   /**
> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> + *                              particular format.
> + *
> + * @fmt:   The page table format.
> + * @alloc: Allocate a set of page tables described by cfg.
> + * @free:  Free the page tables associated with iop.
> + */
> +struct io_pgtable_init_fns {
> +	enum io_pgtable_fmt fmt;
> +	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> +	void (*free)(struct io_pgtable *iop);
> +};
> +
> +/**
> + * io_pgtable_ops_register() - Register the page table routines for a page table
> + *                             format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns);
> +
> +/**
> + * io_pgtable_ops_unregister() - Unregister the page table routines for a page
> + *                               table format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns);
> +
> +/**
>    * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU.
>    *
>    * @fmt:    The page table format.
> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop,
>   		iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie);
>   }
>   
> -/**
> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> - *                              particular format.
> - *
> - * @alloc: Allocate a set of page tables described by cfg.
> - * @free:  Free the page tables associated with iop.
> - */
> -struct io_pgtable_init_fns {
> -	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> -	void (*free)(struct io_pgtable *iop);
> -};
> -
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
> -
>   #endif /* __IO_PGTABLE_H */
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID
From: Robin Murphy <robin.murphy@arm.com>
To: "Isaac J. Manjarres" <isaacm@codeaurora.org>,
	iommu@lists.linux-foundation.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org
Cc: pratikp@codeaurora.org, joro@8bytes.org, kernel-team@android.com,
	will@kernel.org, pdaly@codeaurora.org
Subject: Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration
Date: Tue, 22 Dec 2020 19:27:41 +0000	[thread overview]
Message-ID: <4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com> (raw)
In-Reply-To: <1608597876-32367-2-git-send-email-isaacm@codeaurora.org>

On 2020-12-22 00:44, Isaac J. Manjarres wrote:
> The io-pgtable code constructs an array of init functions for each
> page table format at compile time. This is not ideal, as this
> increases the footprint of the io-pgtable code, as well as prevents
> io-pgtable formats from being built as kernel modules.
> 
> In preparation for modularizing the io-pgtable formats, switch to a
> dynamic registration scheme, where each io-pgtable format can register
> their init functions with the io-pgtable code at boot or module
> insertion time.
> 
> Signed-off-by: Isaac J. Manjarres <isaacm@codeaurora.org>
> ---
>   drivers/iommu/io-pgtable-arm-v7s.c | 34 +++++++++++++-
>   drivers/iommu/io-pgtable-arm.c     | 90 ++++++++++++++++++++++++++----------
>   drivers/iommu/io-pgtable.c         | 94 ++++++++++++++++++++++++++++++++------
>   include/linux/io-pgtable.h         | 51 +++++++++++++--------
>   4 files changed, 209 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> index 1d92ac9..89aad2f 100644
> --- a/drivers/iommu/io-pgtable-arm-v7s.c
> +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> @@ -28,6 +28,7 @@
>   #include <linux/iommu.h>
>   #include <linux/kernel.h>
>   #include <linux/kmemleak.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
> @@ -835,7 +836,8 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +static struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns = {
> +	.fmt	= ARM_V7S,
>   	.alloc	= arm_v7s_alloc_pgtable,
>   	.free	= arm_v7s_free_pgtable,
>   };
> @@ -982,5 +984,33 @@ static int __init arm_v7s_do_selftests(void)
>   	pr_info("self test ok\n");
>   	return 0;
>   }
> -subsys_initcall(arm_v7s_do_selftests);
> +#else
> +static int arm_v7s_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_v7s_init(void)
> +{
> +	int ret;
> +
> +	ret = io_pgtable_ops_register(&io_pgtable_arm_v7s_init_fns);
> +	if (ret < 0) {
> +		pr_err("Failed to register ARM V7S format\n");

Super-nit: I think "v7s" should probably be lowercase there. Also 
general consistency WRT to showing the error code and whether or not to 
abbreviate "format" would be nice.

> +		return ret;
> +	}
> +
> +	ret = arm_v7s_do_selftests();
> +	if (ret < 0)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +
> +	return ret;
> +}
> +core_initcall(arm_v7s_init);
> +
> +static void __exit arm_v7s_exit(void)
> +{
> +	io_pgtable_ops_unregister(&io_pgtable_arm_v7s_init_fns);
> +}
> +module_exit(arm_v7s_exit);
> diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
> index 87def58..ff0ea2f 100644
> --- a/drivers/iommu/io-pgtable-arm.c
> +++ b/drivers/iommu/io-pgtable-arm.c
> @@ -13,6 +13,7 @@
>   #include <linux/bitops.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/module.h>
>   #include <linux/sizes.h>
>   #include <linux/slab.h>
>   #include <linux/types.h>
> @@ -1043,29 +1044,32 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
>   	return NULL;
>   }
>   
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns = {
> -	.alloc	= arm_64_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s1,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns = {
> -	.alloc	= arm_32_lpae_alloc_pgtable_s2,
> -	.free	= arm_lpae_free_pgtable,
> -};
> -
> -struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns = {
> -	.alloc	= arm_mali_lpae_alloc_pgtable,
> -	.free	= arm_lpae_free_pgtable,
> +static struct io_pgtable_init_fns io_pgtable_arm_lpae_init_fns[] = {
> +	{
> +		.fmt	= ARM_32_LPAE_S1,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_32_LPAE_S2,
> +		.alloc	= arm_32_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S1,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s1,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_64_LPAE_S2,
> +		.alloc	= arm_64_lpae_alloc_pgtable_s2,
> +		.free	= arm_lpae_free_pgtable,
> +	},
> +	{
> +		.fmt	= ARM_MALI_LPAE,
> +		.alloc	= arm_mali_lpae_alloc_pgtable,
> +		.free	= arm_lpae_free_pgtable,
> +	},
>   };
>   
>   #ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE_SELFTEST
> @@ -1250,5 +1254,43 @@ static int __init arm_lpae_do_selftests(void)
>   	pr_info("selftest: completed with %d PASS %d FAIL\n", pass, fail);
>   	return fail ? -EFAULT : 0;
>   }
> -subsys_initcall(arm_lpae_do_selftests);
> +#else
> +static int __init arm_lpae_do_selftests(void)
> +{
> +	return 0;
> +}
>   #endif
> +
> +static int __init arm_lpae_init(void)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++) {
> +		ret = io_pgtable_ops_register(&io_pgtable_arm_lpae_init_fns[i]);
> +		if (ret < 0) {
> +			pr_err("Failed to register ARM LPAE fmt: %d\n");
> +			goto err_io_pgtable_register;
> +		}
> +	}
> +
> +	ret = arm_lpae_do_selftests();
> +	if (ret < 0)
> +		goto err_io_pgtable_register;
> +
> +	return 0;
> +
> +err_io_pgtable_register:
> +	for (i = i - 1; i >= 0; i--)

Personally I find "while (i--)" a bit clearer for this kind of 
unwinding, but maybe post-decrement isn't to everyone's taste.

> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +	return ret;
> +}
> +core_initcall(arm_lpae_init);
> +
> +static void __exit arm_lpae_exit(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(io_pgtable_arm_lpae_init_fns); i++)
> +		io_pgtable_ops_unregister(&io_pgtable_arm_lpae_init_fns[i]);
> +}
> +module_exit(arm_lpae_exit);
> diff --git a/drivers/iommu/io-pgtable.c b/drivers/iommu/io-pgtable.c
> index 94394c8..2c6eb2e 100644
> --- a/drivers/iommu/io-pgtable.c
> +++ b/drivers/iommu/io-pgtable.c
> @@ -10,33 +10,45 @@
>   #include <linux/bug.h>
>   #include <linux/io-pgtable.h>
>   #include <linux/kernel.h>
> +#include <linux/rwlock.h>
> +#include <linux/slab.h>
>   #include <linux/types.h>
>   
> -static const struct io_pgtable_init_fns *
> -io_pgtable_init_table[IO_PGTABLE_NUM_FMTS] = {
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_LPAE
> -	[ARM_32_LPAE_S1] = &io_pgtable_arm_32_lpae_s1_init_fns,
> -	[ARM_32_LPAE_S2] = &io_pgtable_arm_32_lpae_s2_init_fns,
> -	[ARM_64_LPAE_S1] = &io_pgtable_arm_64_lpae_s1_init_fns,
> -	[ARM_64_LPAE_S2] = &io_pgtable_arm_64_lpae_s2_init_fns,
> -	[ARM_MALI_LPAE] = &io_pgtable_arm_mali_lpae_init_fns,
> -#endif
> -#ifdef CONFIG_IOMMU_IO_PGTABLE_ARMV7S
> -	[ARM_V7S] = &io_pgtable_arm_v7s_init_fns,
> -#endif
> +struct io_pgtable_init_fns_node {
> +	struct io_pgtable_init_fns *fns;
> +	struct list_head list;
>   };
>   
> +static LIST_HEAD(io_pgtable_init_fns_list);
> +static DEFINE_RWLOCK(io_pgtable_init_fns_list_lock);
> +
> +static struct io_pgtable_init_fns *io_pgtable_get_init_fns(enum io_pgtable_fmt fmt)
> +{
> +	struct io_pgtable_init_fns_node *iter;
> +	struct io_pgtable_init_fns *fns = NULL;
> +
> +	read_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == fmt) {
> +			fns = iter->fns;
> +			break;
> +		}
> +	read_unlock(&io_pgtable_init_fns_list_lock);
> +
> +	return fns;
> +}

I think it would be a lot easier to stick with a simple array indexed by 
enum - that way you can just set/clear/test entries without needing to 
worry about locking. Basically just remove the const and the 
initialisers from the existing one ;)

(and if you think you're concerned about memory, consider that just the 
list head plus lock is already half the size of the table)

Other than that, I think this all looks pretty promising - I'd suggest 
sending a non-RFC after rc1 so that it gets everyone's proper attention.

Thanks,
Robin.

> +
>   struct io_pgtable_ops *alloc_io_pgtable_ops(enum io_pgtable_fmt fmt,
>   					    struct io_pgtable_cfg *cfg,
>   					    void *cookie)
>   {
>   	struct io_pgtable *iop;
> -	const struct io_pgtable_init_fns *fns;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (fmt >= IO_PGTABLE_NUM_FMTS)
>   		return NULL;
>   
> -	fns = io_pgtable_init_table[fmt];
> +	fns = io_pgtable_get_init_fns(fmt);
>   	if (!fns)
>   		return NULL;
>   
> @@ -59,12 +71,64 @@ EXPORT_SYMBOL_GPL(alloc_io_pgtable_ops);
>   void free_io_pgtable_ops(struct io_pgtable_ops *ops)
>   {
>   	struct io_pgtable *iop;
> +	struct io_pgtable_init_fns *fns;
>   
>   	if (!ops)
>   		return;
>   
>   	iop = io_pgtable_ops_to_pgtable(ops);
>   	io_pgtable_tlb_flush_all(iop);
> -	io_pgtable_init_table[iop->fmt]->free(iop);
> +	fns = io_pgtable_get_init_fns(iop->fmt);
> +	if (fns)
> +		fns->free(iop);
>   }
>   EXPORT_SYMBOL_GPL(free_io_pgtable_ops);
> +
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *fns_node;
> +	int ret = 0;
> +
> +	if (!init_fns || init_fns->fmt >= IO_PGTABLE_NUM_FMTS ||
> +	    !init_fns->alloc || !init_fns->free)
> +		return -EINVAL;
> +
> +	fns_node = kzalloc(sizeof(*fns_node), GFP_KERNEL);
> +	if (!fns_node)
> +		return -ENOMEM;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry(iter, &io_pgtable_init_fns_list, list)
> +		if (iter->fns->fmt == init_fns->fmt) {
> +			ret = -EEXIST;
> +			kfree(fns_node);
> +			break;
> +		}
> +
> +	if (!ret) {
> +		fns_node->fns = init_fns;
> +		INIT_LIST_HEAD(&fns_node->list);
> +		list_add_tail(&fns_node->list, &io_pgtable_init_fns_list);
> +	}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_register);
> +
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns)
> +{
> +	struct io_pgtable_init_fns_node *iter, *tmp;
> +
> +	if (!init_fns)
> +		return;
> +
> +	write_lock(&io_pgtable_init_fns_list_lock);
> +	list_for_each_entry_safe(iter, tmp, &io_pgtable_init_fns_list, list)
> +		if (iter->fns == init_fns) {
> +			list_del(&iter->list);
> +			kfree(iter);
> +			break;
> +		}
> +	write_unlock(&io_pgtable_init_fns_list_lock);
> +}
> +EXPORT_SYMBOL_GPL(io_pgtable_ops_unregister);
> diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> index ea727eb..45b367ce 100644
> --- a/include/linux/io-pgtable.h
> +++ b/include/linux/io-pgtable.h
> @@ -163,6 +163,38 @@ struct io_pgtable_ops {
>   };
>   
>   /**
> + * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> + *                              particular format.
> + *
> + * @fmt:   The page table format.
> + * @alloc: Allocate a set of page tables described by cfg.
> + * @free:  Free the page tables associated with iop.
> + */
> +struct io_pgtable_init_fns {
> +	enum io_pgtable_fmt fmt;
> +	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> +	void (*free)(struct io_pgtable *iop);
> +};
> +
> +/**
> + * io_pgtable_ops_register() - Register the page table routines for a page table
> + *                             format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +int io_pgtable_ops_register(struct io_pgtable_init_fns *init_fns);
> +
> +/**
> + * io_pgtable_ops_unregister() - Unregister the page table routines for a page
> + *                               table format.
> + *
> + * @init_fns: The functions for allocating and freeing the page tables of
> + *            a particular format.
> + */
> +void io_pgtable_ops_unregister(struct io_pgtable_init_fns *init_fns);
> +
> +/**
>    * alloc_io_pgtable_ops() - Allocate a page table allocator for use by an IOMMU.
>    *
>    * @fmt:    The page table format.
> @@ -233,23 +265,4 @@ io_pgtable_tlb_add_page(struct io_pgtable *iop,
>   		iop->cfg.tlb->tlb_add_page(gather, iova, granule, iop->cookie);
>   }
>   
> -/**
> - * struct io_pgtable_init_fns - Alloc/free a set of page tables for a
> - *                              particular format.
> - *
> - * @alloc: Allocate a set of page tables described by cfg.
> - * @free:  Free the page tables associated with iop.
> - */
> -struct io_pgtable_init_fns {
> -	struct io_pgtable *(*alloc)(struct io_pgtable_cfg *cfg, void *cookie);
> -	void (*free)(struct io_pgtable *iop);
> -};
> -
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_32_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s1_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_64_lpae_s2_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_v7s_init_fns;
> -extern struct io_pgtable_init_fns io_pgtable_arm_mali_lpae_init_fns;
> -
>   #endif /* __IO_PGTABLE_H */
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2020-12-22 19:28 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-22  0:44 [RFC PATCH v2 0/7] iommu: Permit modular builds of io-pgtable drivers Isaac J. Manjarres
2020-12-22  0:44 ` Isaac J. Manjarres
2020-12-22  0:44 ` Isaac J. Manjarres
2020-12-22  0:44 ` [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22 19:27   ` Robin Murphy [this message]
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:54     ` isaacm
2020-12-22 19:54       ` isaacm
2020-12-22 19:54       ` isaacm
2020-12-23 13:44       ` Robin Murphy
2020-12-23 13:44         ` Robin Murphy
2020-12-23 13:44         ` Robin Murphy
2020-12-23 13:44         ` Robin Murphy
2020-12-23 15:14         ` isaacm
2020-12-23 15:14           ` isaacm
2020-12-23 15:14           ` isaacm
2020-12-23 15:38   ` Georgi Djakov
2020-12-23 15:38     ` Georgi Djakov
2020-12-23 15:38     ` Georgi Djakov
2020-12-23 15:38     ` Georgi Djakov
2020-12-23 19:17   ` kernel test robot
2020-12-23 19:17     ` kernel test robot
2020-12-23 19:17     ` kernel test robot
2020-12-23 19:17     ` kernel test robot
2020-12-22  0:44 ` [PATCH v2 2/7] iommu/io-pgtable: Add refcounting for io-pgtable format modules Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44 ` [PATCH v2 3/7] iommu/arm-smmu: Add dependency on " Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22 19:27   ` Robin Murphy
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:27     ` Robin Murphy
2020-12-22 19:49     ` isaacm
2020-12-22 19:49       ` isaacm
2020-12-22 19:49       ` isaacm
2020-12-23 13:05       ` Robin Murphy
2020-12-23 13:05         ` Robin Murphy
2020-12-23 13:05         ` Robin Murphy
2020-12-23 13:05         ` Robin Murphy
2020-12-23 15:10         ` isaacm
2020-12-23 15:10           ` isaacm
2020-12-23 15:10           ` isaacm
2020-12-22  0:44 ` [PATCH v2 4/7] iommu/arm-smmu-v3: Add dependency on io-pgtable-arm format module Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44 ` [PATCH v2 5/7] drm/msm: " Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2021-01-18 21:16   ` Rob Clark
2021-01-18 21:16     ` Rob Clark
2021-01-18 21:16     ` Rob Clark
2021-01-18 21:16     ` Rob Clark
2021-01-18 21:39     ` Will Deacon
2021-01-18 21:39       ` Will Deacon
2021-01-18 21:39       ` Will Deacon
2021-01-18 21:39       ` Will Deacon
2021-01-19 17:19       ` Rob Clark
2021-01-19 17:19         ` Rob Clark
2021-01-19 17:19         ` Rob Clark
2021-01-19 17:19         ` Rob Clark
2020-12-22  0:44 ` [PATCH v2 6/7] drm/panfrost: " Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44 ` [PATCH v2 7/7] iommu/io-pgtable-arm: Allow building modular io-pgtable fmts Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres
2020-12-22  0:44   ` Isaac J. Manjarres

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4f388512-c3df-e9fc-ebd4-624ab36a6858@arm.com \
    --to=robin.murphy@arm.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=freedreno@lists.freedesktop.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=isaacm@codeaurora.org \
    --cc=joro@8bytes.org \
    --cc=kernel-team@android.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pdaly@codeaurora.org \
    --cc=pratikp@codeaurora.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v2 1/7] iommu/io-pgtable: Introduce dynamic io-pgtable fmt registration' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.