On Mon, Sep 24, 2018 at 08:50:35PM +0300, Dmitry Osipenko wrote:
> On 9/24/18 2:10 PM, Thierry Reding wrote:
> > On Mon, Sep 24, 2018 at 03:41:52AM +0300, Dmitry Osipenko wrote:
> >> GART is a simple IOMMU provider that has single address space. There is
> >> no need to setup global clients list and manage it for tracking of the
> >> active domain, hence lot's of code could be safely removed and replaced
> >> with a simpler alternative.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> >> ---
> >> drivers/iommu/tegra-gart.c | 157 +++++++++----------------------------
> >> 1 file changed, 39 insertions(+), 118 deletions(-)
> >>
> >> diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c
> >> index 306e9644a676..7182445c3b76 100644
> >> --- a/drivers/iommu/tegra-gart.c
> >> +++ b/drivers/iommu/tegra-gart.c
> >> @@ -19,7 +19,6 @@
> >>
> >> #include <linux/io.h>
> >> #include <linux/iommu.h>
> >> -#include <linux/list.h>
> >> #include <linux/module.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/slab.h>
> >> @@ -42,30 +41,20 @@
> >> #define GART_PAGE_MASK \
> >> (~(GART_PAGE_SIZE - 1) & ~GART_ENTRY_PHYS_ADDR_VALID)
> >>
> >> -struct gart_client {
> >> - struct device *dev;
> >> - struct list_head list;
> >> -};
> >> -
> >> struct gart_device {
> >> void __iomem *regs;
> >> u32 *savedata;
> >> u32 page_count; /* total remappable size */
> >> dma_addr_t iovmm_base; /* offset to vmm_area */
> >> spinlock_t pte_lock; /* for pagetable */
> >> - struct list_head client;
> >> - spinlock_t client_lock; /* for client list */
> >> + spinlock_t dom_lock; /* for active domain */
> >> + unsigned int active_devices; /* number of active devices */
> >> struct iommu_domain *active_domain; /* current active domain */
> >> struct device *dev;
> >>
> >> struct iommu_device iommu; /* IOMMU Core handle */
> >> };
> >>
> >> -struct gart_domain {
> >> - struct iommu_domain domain; /* generic domain handle */
> >> - struct gart_device *gart; /* link to gart device */
> >> -};
> >> -
> >> static struct gart_device *gart_handle; /* unique for a system */
> >>
> >> static bool gart_debug;
> >> @@ -73,11 +62,6 @@ static bool gart_debug;
> >> #define GART_PTE(_pfn) \
> >> (GART_ENTRY_PHYS_ADDR_VALID | ((_pfn) << PAGE_SHIFT))
> >>
> >> -static struct gart_domain *to_gart_domain(struct iommu_domain *dom)
> >> -{
> >> - return container_of(dom, struct gart_domain, domain);
> >> -}
> >> -
> >> /*
> >> * Any interaction between any block on PPSB and a block on APB or AHB
> >> * must have these read-back to ensure the APB/AHB bus transaction is
> >> @@ -166,128 +150,69 @@ static inline bool gart_iova_range_valid(struct gart_device *gart,
> >> static int gart_iommu_attach_dev(struct iommu_domain *domain,
> >> struct device *dev)
> >> {
> >> - struct gart_domain *gart_domain = to_gart_domain(domain);
> >> struct gart_device *gart = gart_handle;
> >> - struct gart_client *client, *c;
> >> - int err = 0;
> >> -
> >> - client = kzalloc(sizeof(*c), GFP_KERNEL);
> >> - if (!client)
> >> - return -ENOMEM;
> >> - client->dev = dev;
> >> -
> >> - spin_lock(&gart->client_lock);
> >> - list_for_each_entry(c, &gart->client, list) {
> >> - if (c->dev == dev) {
> >> - dev_err(gart->dev, "GART: %s is already attached\n",
> >> - dev_name(dev));
> >> - err = -EINVAL;
> >> - goto fail;
> >> - }
> >> - }
> >> - if (gart->active_domain && gart->active_domain != domain) {
> >> - dev_err(gart->dev,
> >> - "GART: Only one domain can be active at a time\n");
> >> - err = -EINVAL;
> >> - goto fail;
> >> - }
> >> - gart->active_domain = domain;
> >> - gart_domain->gart = gart;
> >> - list_add(&client->list, &gart->client);
> >> - spin_unlock(&gart->client_lock);
> >> - dev_dbg(gart->dev, "GART: Attached %s\n", dev_name(dev));
> >> - return 0;
> >> + int ret = 0;
> >>
> >> -fail:
> >> - kfree(client);
> >> - spin_unlock(&gart->client_lock);
> >> - return err;
> >> -}
> >> + spin_lock(&gart->dom_lock);
> >>
> >> -static void __gart_iommu_detach_dev(struct iommu_domain *domain,
> >> - struct device *dev)
> >> -{
> >> - struct gart_domain *gart_domain = to_gart_domain(domain);
> >> - struct gart_device *gart = gart_domain->gart;
> >> - struct gart_client *c;
> >> -
> >> - list_for_each_entry(c, &gart->client, list) {
> >> - if (c->dev == dev) {
> >> - list_del(&c->list);
> >> - kfree(c);
> >> - if (list_empty(&gart->client)) {
> >> - gart->active_domain = NULL;
> >> - gart_domain->gart = NULL;
> >> - }
> >> - dev_dbg(gart->dev, "GART: Detached %s\n",
> >> - dev_name(dev));
> >> - return;
> >> - }
> >> + if (gart->active_domain && gart->active_domain != domain) {
> >> + ret = -EBUSY;
> >
> > This omits the error message and returns -EBUSY instead of -EINVAL. Was
> > this intended? For what it's worth, I do agree with the changes, it's
> > just that I think you could've made those in the earlier patch that
> > introduced them.
>
> The message isn't really needed and EBUSY seems fit better than EINVAL here.
>
> > But this is all one series and the end result looks fine, so no need to
> > be that picky.
>
> Good, thanks.
>
> >> + } else if (dev->archdata.iommu != domain) {
> >> + dev->archdata.iommu = domain;
> >> + gart->active_domain = domain;
> >> + gart->active_devices++;
> >> }
> >>
> >> - dev_err(gart->dev, "GART: Couldn't find %s to detach\n",
> >> - dev_name(dev));
> >> + spin_unlock(&gart->dom_lock);
> >> +
> >> + return ret;
> >> }
> >>
> >> static void gart_iommu_detach_dev(struct iommu_domain *domain,
> >> struct device *dev)
> >> {
> >> - struct gart_domain *gart_domain = to_gart_domain(domain);
> >> - struct gart_device *gart = gart_domain->gart;
> >> + struct gart_device *gart = gart_handle;
> >> +
> >> + spin_lock(&gart->dom_lock);
> >>
> >> - spin_lock(&gart->client_lock);
> >> - __gart_iommu_detach_dev(domain, dev);
> >> - spin_unlock(&gart->client_lock);
> >> + if (dev->archdata.iommu == domain) {
> >> + dev->archdata.iommu = NULL;
> >> +
> >> + if (--gart->active_devices == 0)
> >> + gart->active_domain = NULL;
> >> + }
> >> +
> >> + spin_unlock(&gart->dom_lock);
> >> }
> >>
> >> static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
> >> {
> >> - struct gart_domain *gart_domain;
> >> - struct gart_device *gart;
> >> + struct gart_device *gart = gart_handle;
> >> + struct iommu_domain *domain;
> >>
> >> if (type != IOMMU_DOMAIN_UNMANAGED)
> >> return NULL;
> >>
> >> - gart = gart_handle;
> >> - if (!gart)
> >> - return NULL;
> >> -
> >> - gart_domain = kzalloc(sizeof(*gart_domain), GFP_KERNEL);
> >> - if (!gart_domain)
> >> - return NULL;
> >> -
> >> - gart_domain->domain.geometry.aperture_start = gart->iovmm_base;
> >> - gart_domain->domain.geometry.aperture_end = gart->iovmm_base +
> >> + domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> >> + if (domain) {
> >> + domain->geometry.aperture_start = gart->iovmm_base;
> >> + domain->geometry.aperture_end = gart->iovmm_base +
> >> gart->page_count * GART_PAGE_SIZE - 1;
> >> - gart_domain->domain.geometry.force_aperture = true;
> >> + domain->geometry.force_aperture = true;
> >> + }
> >>
> >> - return &gart_domain->domain;
> >> + return domain;
> >> }
> >>
> >> static void gart_iommu_domain_free(struct iommu_domain *domain)
> >> {
> >> - struct gart_domain *gart_domain = to_gart_domain(domain);
> >> - struct gart_device *gart = gart_domain->gart;
> >> -
> >> - if (gart) {
> >> - spin_lock(&gart->client_lock);
> >> - if (!list_empty(&gart->client)) {
> >> - struct gart_client *c, *tmp;
> >> -
> >> - list_for_each_entry_safe(c, tmp, &gart->client, list)
> >> - __gart_iommu_detach_dev(domain, c->dev);
> >> - }
> >> - spin_unlock(&gart->client_lock);
> >> - }
> >> -
> >> - kfree(gart_domain);
> >> + kfree(domain);
> >> }
> >
> > Doesn't this now make it possible to free a potentially active domain?
>
> Yes, don't do it. I can add a WARN_ON() here, though I think IOMMU core
> should be the one taking care about that.
Yeah, might be good to have the WARN_ON() either here or in the IOMMU
core. Force-detaching is probably a good idea, too, otherwise the users
of the freed domain are just going to crash anyway, right? Maybe
something to discuss more generally with Joerg.
I think in the meantime just having the WARN_ON() here is probably good
enough. It should point out the cases where we do free the domain with
devices still attached, which hopefully don't exist, and we can fix
them.
> >> static int gart_iommu_map(struct iommu_domain *domain, unsigned long iova,
> >> phys_addr_t pa, size_t bytes, int prot)
> >> {
> >> - struct gart_domain *gart_domain = to_gart_domain(domain);
> >> - struct gart_device *gart = gart_domain->gart;
> >> + struct gart_device *gart = gart_handle;
> >
> > Hmm... this now introduces more uses of the gart_handle that I hoped we
> > could get rid of. I think we could still keep around struct gart_domain
> > and just make sure it is unique. The small amounts of casting here seem
> > mostly harmless to me, especially since they will be nops, so we end up
> > with just one dereference to get at the struct gart_device. I think the
> > benefits of not having this global variable around are worth the one
> > dereference here.
>
> What are the benefits? I don't see anything other than the pedantic oddity.
>
> I've removed gart_domain in the end because it is an extra code (and
> consumed resources) without any benefit. Let's keep that part as it is
> now. I'll be happy to change that code if you'll explain why it is worth
> it.
I thought I did explain. Anyway, it's always been like this, so no need
to change it as part of this series.
Thierry
Attachment:
signature.asc
Description: PGP signature
