[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4817218.RlfHpgC8v6@avalon>
Date: Tue, 29 Dec 2015 02:04:04 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Magnus Damm <magnus.damm@...il.com>
Cc: iommu@...ts.linux-foundation.org,
laurent.pinchart+renesas@...asonboard.com, geert+renesas@...der.be,
linux-sh@...r.kernel.org, joro@...tes.org,
linux-kernel@...r.kernel.org, horms+renesas@...ge.net.au
Subject: Re: [PATCH 02/06] iommu/ipmmu-vmsa: Convert to dev_data
Hi Magnus,
Thank you for the patch.
On Tuesday 15 December 2015 21:02:30 Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@...nsource.se>
>
> Rename ipmmu_vmsa_archdata to ipmmu_vmsa_dev_data to avoid
> confusion when using the driver on multiple architectures.
I'm sorry but this patches makes the driver more confusing to me. archdata is
a well established concept for IOMMU on ARM, I don't see how hiding help would
help.
> The data now stored in ipmmu_vmsa_dev_data is used to point
> various on-chip devices to the actual IPMMU instances.
>
> Signed-off-by: Magnus Damm <damm+renesas@...nsource.se>
> ---
>
> drivers/iommu/ipmmu-vmsa.c | 58 +++++++++++++++++++++++++----------------
> 1 file changed, 36 insertions(+), 22 deletions(-)
>
> --- 0003/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2015-12-15 13:16:27.160513000 +0900
> @@ -47,7 +47,7 @@ struct ipmmu_vmsa_domain {
> spinlock_t lock; /* Protects mappings */
> };
>
> -struct ipmmu_vmsa_archdata {
> +struct ipmmu_vmsa_dev_data {
> struct ipmmu_vmsa_device *mmu;
> unsigned int *utlbs;
> unsigned int num_utlbs;
> @@ -182,6 +182,20 @@ static struct ipmmu_vmsa_domain *to_vmsa
> #define IMUASID_ASID0_SHIFT 0
>
> /* ------------------------------------------------------------------------
> + * Consumer device side private data handling
> + */
> +
> +static struct ipmmu_vmsa_dev_data *get_dev_data(struct device *dev)
> +{
> + return dev->archdata.iommu;
> +}
> +
> +static void set_dev_data(struct device *dev, struct ipmmu_vmsa_dev_data
> *data)
> +{
> + dev->archdata.iommu = data;
> +}
> +
> +/* ------------------------------------------------------------------------
> * Read/Write Access
> */
>
> @@ -485,8 +499,8 @@ static void ipmmu_domain_free(struct iom
> static int ipmmu_attach_device(struct iommu_domain *io_domain,
> struct device *dev)
> {
> - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> - struct ipmmu_vmsa_device *mmu = archdata->mmu;
> + struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> + struct ipmmu_vmsa_device *mmu = dev_data->mmu;
> struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> unsigned long flags;
> unsigned int i;
> @@ -518,8 +532,8 @@ static int ipmmu_attach_device(struct io
> if (ret < 0)
> return ret;
>
> - for (i = 0; i < archdata->num_utlbs; ++i)
> - ipmmu_utlb_enable(domain, archdata->utlbs[i]);
> + for (i = 0; i < dev_data->num_utlbs; ++i)
> + ipmmu_utlb_enable(domain, dev_data->utlbs[i]);
>
> return 0;
> }
> @@ -527,12 +541,12 @@ static int ipmmu_attach_device(struct io
> static void ipmmu_detach_device(struct iommu_domain *io_domain,
> struct device *dev)
> {
> - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> + struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
> unsigned int i;
>
> - for (i = 0; i < archdata->num_utlbs; ++i)
> - ipmmu_utlb_disable(domain, archdata->utlbs[i]);
> + for (i = 0; i < dev_data->num_utlbs; ++i)
> + ipmmu_utlb_disable(domain, dev_data->utlbs[i]);
>
> /*
> * TODO: Optimize by disabling the context when no device is attached.
> @@ -595,7 +609,7 @@ static int ipmmu_find_utlbs(struct ipmmu
>
> static int ipmmu_add_device(struct device *dev)
> {
> - struct ipmmu_vmsa_archdata *archdata;
> + struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
> struct ipmmu_vmsa_device *mmu;
> struct iommu_group *group = NULL;
> unsigned int *utlbs;
> @@ -603,7 +617,7 @@ static int ipmmu_add_device(struct devic
> int num_utlbs;
> int ret = -ENODEV;
>
> - if (dev->archdata.iommu) {
> + if (dev_data) {
> dev_warn(dev, "IOMMU driver already assigned to device %s\n",
> dev_name(dev));
> return -EINVAL;
> @@ -662,16 +676,16 @@ static int ipmmu_add_device(struct devic
> goto error;
> }
>
> - archdata = kzalloc(sizeof(*archdata), GFP_KERNEL);
> - if (!archdata) {
> + dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
> + if (!dev_data) {
> ret = -ENOMEM;
> goto error;
> }
>
> - archdata->mmu = mmu;
> - archdata->utlbs = utlbs;
> - archdata->num_utlbs = num_utlbs;
> - dev->archdata.iommu = archdata;
> + dev_data->mmu = mmu;
> + dev_data->utlbs = utlbs;
> + dev_data->num_utlbs = num_utlbs;
> + set_dev_data(dev, dev_data);
>
> /*
> * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> @@ -708,10 +722,10 @@ static int ipmmu_add_device(struct devic
> error:
> arm_iommu_release_mapping(mmu->mapping);
>
> - kfree(dev->archdata.iommu);
> + kfree(dev_data);
> kfree(utlbs);
>
> - dev->archdata.iommu = NULL;
> + set_dev_data(dev, NULL);
>
> if (!IS_ERR_OR_NULL(group))
> iommu_group_remove_device(dev);
> @@ -721,15 +735,15 @@ error:
>
> static void ipmmu_remove_device(struct device *dev)
> {
> - struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
> + struct ipmmu_vmsa_dev_data *dev_data = get_dev_data(dev);
>
> arm_iommu_detach_device(dev);
> iommu_group_remove_device(dev);
>
> - kfree(archdata->utlbs);
> - kfree(archdata);
> + kfree(dev_data->utlbs);
> + kfree(dev_data);
>
> - dev->archdata.iommu = NULL;
> + set_dev_data(dev, NULL);
> }
>
> static const struct iommu_ops ipmmu_ops = {
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists