* [PATCH v7 3/9] iommu/exynos: fix page table maintenance @ 2013-07-05 12:29 Cho KyongHo 2013-07-11 17:37 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 5+ messages in thread From: Cho KyongHo @ 2013-07-05 12:29 UTC (permalink / raw) To: 'Linux ARM Kernel', 'Linux IOMMU', 'Linux Kernel', 'Linux Samsung SOC' Cc: 'Hyunwoong Kim', 'Joerg Roedel', 'Kukjin Kim', 'Prathyush', 'Rahul Sharma', 'Subash Patel', 'Keyyoung Park', 'Grant Grundler' This prevents allocating lv2 page table for the lv1 page table entry that already has 1MB page mapping. In addition some BUG_ON() is changed to WARN_ON(). Signed-off-by: Cho KyongHo <pullip.cho@samsung.com> --- drivers/iommu/exynos-iommu.c | 34 ++++++++++++++++++++++++++-------- 1 files changed, 26 insertions(+), 8 deletions(-) diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c index e3be3e5..2bfe9fa 100644 --- a/drivers/iommu/exynos-iommu.c +++ b/drivers/iommu/exynos-iommu.c @@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova, pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC); BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1)); if (!pent) - return NULL; + return ERR_PTR(-ENOMEM); *sent = mk_lv1ent_page(__pa(pent)); *pgcounter = NUM_LV2ENTRIES; pgtable_flush(pent, pent + NUM_LV2ENTRIES); pgtable_flush(sent, sent + 1); + } else if (lv1ent_section(sent)) { + return ERR_PTR(-EADDRINUSE); } return page_entry(sent, iova); @@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova, pent = alloc_lv2entry(entry, iova, &priv->lv2entcnt[lv1ent_offset(iova)]); - if (!pent) - ret = -ENOMEM; + if (IS_ERR(pent)) + ret = PTR_ERR(pent); else ret = lv2set_page(pent, paddr, size, &priv->lv2entcnt[lv1ent_offset(iova)]); } if (ret) { - pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n", - __func__, iova, size); + pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n", + __func__, ret, size, iova); } spin_unlock_irqrestore(&priv->pgtablelock, flags); @@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, struct sysmmu_drvdata *data; unsigned long flags; unsigned long *ent; + size_t err_page; BUG_ON(priv->pgtable == NULL); @@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, ent = section_entry(priv->pgtable, iova); if (lv1ent_section(ent)) { - BUG_ON(size < SECT_SIZE); + if (WARN_ON(size < SECT_SIZE)) + goto err; *ent = 0; pgtable_flush(ent, ent + 1); @@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, } /* lv1ent_large(ent) == true here */ - BUG_ON(size < LPAGE_SIZE); + if (WARN_ON(size < LPAGE_SIZE)) + goto err; memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE); pgtable_flush(ent, ent + SPAGES_PER_LPAGE); @@ -1023,8 +1028,21 @@ done: sysmmu_tlb_invalidate_entry(data->dev, iova); spin_unlock_irqrestore(&priv->lock, flags); - return size; +err: + spin_unlock_irqrestore(&priv->pgtablelock, flags); + + err_page = ( + ((unsigned long)ent - (unsigned long)priv->pgtable) + < (NUM_LV1ENTRIES * sizeof(long)) + ) ? SECT_SIZE : LPAGE_SIZE; + + pr_err("%s: Failed due to size(%#lx) @ %#x is"\ + " smaller than page size %#x\n", + __func__, iova, size, err_page); + + return 0; + } static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, -- 1.7.2.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v7 3/9] iommu/exynos: fix page table maintenance 2013-07-05 12:29 [PATCH v7 3/9] iommu/exynos: fix page table maintenance Cho KyongHo @ 2013-07-11 17:37 ` Bartlomiej Zolnierkiewicz 2013-07-15 11:21 ` Cho KyongHo 0 siblings, 1 reply; 5+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2013-07-11 17:37 UTC (permalink / raw) To: Cho KyongHo Cc: 'Linux ARM Kernel', 'Linux IOMMU', 'Linux Kernel', 'Linux Samsung SOC', 'Hyunwoong Kim', 'Joerg Roedel', 'Kukjin Kim', 'Prathyush', 'Rahul Sharma', 'Subash Patel', 'Keyyoung Park', 'Grant Grundler' Hi, Some minor nitpicks below. On Friday, July 05, 2013 09:29:18 PM Cho KyongHo wrote: > This prevents allocating lv2 page table for the lv1 page table entry > that already has 1MB page mapping. In addition some BUG_ON() is > changed to WARN_ON(). > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 34 ++++++++++++++++++++++++++-------- > 1 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index e3be3e5..2bfe9fa 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova, > pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC); > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1)); > if (!pent) > - return NULL; > + return ERR_PTR(-ENOMEM); > > *sent = mk_lv1ent_page(__pa(pent)); > *pgcounter = NUM_LV2ENTRIES; > pgtable_flush(pent, pent + NUM_LV2ENTRIES); > pgtable_flush(sent, sent + 1); > + } else if (lv1ent_section(sent)) { > + return ERR_PTR(-EADDRINUSE); > } > > return page_entry(sent, iova); > @@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova, > pent = alloc_lv2entry(entry, iova, > &priv->lv2entcnt[lv1ent_offset(iova)]); > > - if (!pent) > - ret = -ENOMEM; > + if (IS_ERR(pent)) > + ret = PTR_ERR(pent); > else > ret = lv2set_page(pent, paddr, size, > &priv->lv2entcnt[lv1ent_offset(iova)]); > } > > if (ret) { > - pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n", > - __func__, iova, size); > + pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n", > + __func__, ret, size, iova); > } Intendation is a bit weird, it should be more like: pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n", __func__, ret, size, iova); to be consistent with the rest of the driver. You could have also removed superfluous braces while at it. > spin_unlock_irqrestore(&priv->pgtablelock, flags); > @@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > struct sysmmu_drvdata *data; > unsigned long flags; > unsigned long *ent; > + size_t err_page; > > BUG_ON(priv->pgtable == NULL); > > @@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > ent = section_entry(priv->pgtable, iova); > > if (lv1ent_section(ent)) { > - BUG_ON(size < SECT_SIZE); > + if (WARN_ON(size < SECT_SIZE)) > + goto err; > > *ent = 0; > pgtable_flush(ent, ent + 1); > @@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > } > > /* lv1ent_large(ent) == true here */ > - BUG_ON(size < LPAGE_SIZE); > + if (WARN_ON(size < LPAGE_SIZE)) > + goto err; > > memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE); > pgtable_flush(ent, ent + SPAGES_PER_LPAGE); > @@ -1023,8 +1028,21 @@ done: > sysmmu_tlb_invalidate_entry(data->dev, iova); > spin_unlock_irqrestore(&priv->lock, flags); > > - > return size; > +err: > + spin_unlock_irqrestore(&priv->pgtablelock, flags); > + > + err_page = ( > + ((unsigned long)ent - (unsigned long)priv->pgtable) > + < (NUM_LV1ENTRIES * sizeof(long)) Maybe you could add LV1TABLE_SIZE define and use it here (there is already a LV2TABLE_SIZE define)? > + ) ? SECT_SIZE : LPAGE_SIZE; It also seems that err_page should be of unsigned long type, no need to make it size_t one. The above code is quite ugly currently, it could be rewritten into something prettier, i.e.: err_page = (unsigned long)ent - (unsigned long)priv->pgtable; err_page = (err_page < LV1TABLE_SIZE) ? SECT_SIZE : LPAGE_SIZE; > + pr_err("%s: Failed due to size(%#lx) @ %#x is"\ > + " smaller than page size %#x\n", > + __func__, iova, size, err_page); Aren't iova and size arguments interchanged here? > + > + return 0; There is an intendation issue here (extra whitespaces). > + > } > > static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v7 3/9] iommu/exynos: fix page table maintenance 2013-07-11 17:37 ` Bartlomiej Zolnierkiewicz @ 2013-07-15 11:21 ` Cho KyongHo 2013-07-15 16:18 ` Grant Grundler 0 siblings, 1 reply; 5+ messages in thread From: Cho KyongHo @ 2013-07-15 11:21 UTC (permalink / raw) To: 'Bartlomiej Zolnierkiewicz' Cc: 'Linux ARM Kernel', 'Linux IOMMU', 'Linux Kernel', 'Linux Samsung SOC', 'Hyunwoong Kim', 'Joerg Roedel', 'Kukjin Kim', 'Prathyush', 'Rahul Sharma', 'Subash Patel', 'Keyyoung Park', 'Grant Grundler' > -----Original Message----- > From: Bartlomiej Zolnierkiewicz [mailto:b.zolnierkie@samsung.com] > Sent: Friday, July 12, 2013 2:38 AM > > Hi, > > Some minor nitpicks below. > > On Friday, July 05, 2013 09:29:18 PM Cho KyongHo wrote: > > This prevents allocating lv2 page table for the lv1 page table entry > > that already has 1MB page mapping. In addition some BUG_ON() is > > changed to WARN_ON(). > > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com> > > --- > > drivers/iommu/exynos-iommu.c | 34 ++++++++++++++++++++++++++-------- > > 1 files changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > > index e3be3e5..2bfe9fa 100644 > > --- a/drivers/iommu/exynos-iommu.c > > +++ b/drivers/iommu/exynos-iommu.c > > @@ -862,12 +862,14 @@ static unsigned long *alloc_lv2entry(unsigned long *sent, unsigned long iova, > > pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC); > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1)); > > if (!pent) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > > > *sent = mk_lv1ent_page(__pa(pent)); > > *pgcounter = NUM_LV2ENTRIES; > > pgtable_flush(pent, pent + NUM_LV2ENTRIES); > > pgtable_flush(sent, sent + 1); > > + } else if (lv1ent_section(sent)) { > > + return ERR_PTR(-EADDRINUSE); > > } > > > > return page_entry(sent, iova); > > @@ -944,16 +946,16 @@ static int exynos_iommu_map(struct iommu_domain *domain, unsigned long iova, > > pent = alloc_lv2entry(entry, iova, > > &priv->lv2entcnt[lv1ent_offset(iova)]); > > > > - if (!pent) > > - ret = -ENOMEM; > > + if (IS_ERR(pent)) > > + ret = PTR_ERR(pent); > > else > > ret = lv2set_page(pent, paddr, size, > > &priv->lv2entcnt[lv1ent_offset(iova)]); > > } > > > > if (ret) { > > - pr_debug("%s: Failed to map iova 0x%lx/0x%x bytes\n", > > - __func__, iova, size); > > + pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n", > > + __func__, ret, size, iova); > > } > > Intendation is a bit weird, it should be more like: > > pr_err("%s: Failed(%d) to map iova 0x%#x bytes @ %#lx\n", > __func__, ret, size, iova); > > to be consistent with the rest of the driver. > > You could have also removed superfluous braces while at it. > Do you think it is better to read the code? Hmm, I think it is better too. Thanks. > > spin_unlock_irqrestore(&priv->pgtablelock, flags); > > @@ -968,6 +970,7 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > > struct sysmmu_drvdata *data; > > unsigned long flags; > > unsigned long *ent; > > + size_t err_page; > > > > BUG_ON(priv->pgtable == NULL); > > > > @@ -976,7 +979,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > > ent = section_entry(priv->pgtable, iova); > > > > if (lv1ent_section(ent)) { > > - BUG_ON(size < SECT_SIZE); > > + if (WARN_ON(size < SECT_SIZE)) > > + goto err; > > > > *ent = 0; > > pgtable_flush(ent, ent + 1); > > @@ -1008,7 +1012,8 @@ static size_t exynos_iommu_unmap(struct iommu_domain *domain, > > } > > > > /* lv1ent_large(ent) == true here */ > > - BUG_ON(size < LPAGE_SIZE); > > + if (WARN_ON(size < LPAGE_SIZE)) > > + goto err; > > > > memset(ent, 0, sizeof(*ent) * SPAGES_PER_LPAGE); > > pgtable_flush(ent, ent + SPAGES_PER_LPAGE); > > @@ -1023,8 +1028,21 @@ done: > > sysmmu_tlb_invalidate_entry(data->dev, iova); > > spin_unlock_irqrestore(&priv->lock, flags); > > > > - > > return size; > > +err: > > + spin_unlock_irqrestore(&priv->pgtablelock, flags); > > + > > + err_page = ( > > + ((unsigned long)ent - (unsigned long)priv->pgtable) > > + < (NUM_LV1ENTRIES * sizeof(long)) > > Maybe you could add LV1TABLE_SIZE define and use it here (there is > already a LV2TABLE_SIZE define)? Yes. But, LV2TABLE_SIZE is used in more places than one. I do not feel that it is needed to define LV1TABLE_SIZE for the single line. > > > + ) ? SECT_SIZE : LPAGE_SIZE; > > It also seems that err_page should be of unsigned long type, no need > to make it size_t one. > err_page is the page size. I agree that the name of the variable is not proper to indicate size. > The above code is quite ugly currently, it could be rewritten into > something prettier, i.e.: > > err_page = (unsigned long)ent - (unsigned long)priv->pgtable; > err_page = (err_page < LV1TABLE_SIZE) ? SECT_SIZE : LPAGE_SIZE; > Ah, this is the reason that you addresses err_page could be unsigned long. I agree that it is prettier. > > + pr_err("%s: Failed due to size(%#lx) @ %#x is"\ > > + " smaller than page size %#x\n", > > + __func__, iova, size, err_page); > > Aren't iova and size arguments interchanged here? Oh, it is my mistake. It will should be fixed in the next patchset with the change in calculation of err_page. > > > + > > + return 0; > > There is an intendation issue here (extra whitespaces). Yes. Thanks. > > > + > > } > > > > static phys_addr_t exynos_iommu_iova_to_phys(struct iommu_domain *domain, > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics Thank you. Cho KyongHo. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 3/9] iommu/exynos: fix page table maintenance 2013-07-15 11:21 ` Cho KyongHo @ 2013-07-15 16:18 ` Grant Grundler 2013-07-16 13:07 ` Cho KyongHo 0 siblings, 1 reply; 5+ messages in thread From: Grant Grundler @ 2013-07-15 16:18 UTC (permalink / raw) To: Cho KyongHo Cc: Bartlomiej Zolnierkiewicz, Linux ARM Kernel, Linux IOMMU, Linux Kernel, Linux Samsung SOC, Hyunwoong Kim, Joerg Roedel, Kukjin Kim, Prathyush, Rahul Sharma, Subash Patel, Keyyoung Park, Grant Grundler On Mon, Jul 15, 2013 at 4:21 AM, Cho KyongHo <pullip.cho@samsung.com> wrote: ... >> Maybe you could add LV1TABLE_SIZE define and use it here (there is >> already a LV2TABLE_SIZE define)? > > Yes. But, LV2TABLE_SIZE is used in more places than one. > I do not feel that it is needed to define LV1TABLE_SIZE for the single line. Cho, #define's are part of the code "documentation". Doesn't really matter how often it's used. I think Bartlomiej's suggestion is a good one. Key is to use "consistent" names so they makes sense to someone not familiar with the code. In this case, we want to show this use is the same way LV2TABLE_SIZE is used and serves the same purpose. cheers. grant ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH v7 3/9] iommu/exynos: fix page table maintenance 2013-07-15 16:18 ` Grant Grundler @ 2013-07-16 13:07 ` Cho KyongHo 0 siblings, 0 replies; 5+ messages in thread From: Cho KyongHo @ 2013-07-16 13:07 UTC (permalink / raw) To: 'Grant Grundler' Cc: 'Bartlomiej Zolnierkiewicz', 'Linux ARM Kernel', 'Linux IOMMU', 'Linux Kernel', 'Linux Samsung SOC', 'Hyunwoong Kim', 'Joerg Roedel', 'Kukjin Kim', 'Prathyush', 'Rahul Sharma', 'Subash Patel', 'Keyyoung Park' > From: grundler@google.com [mailto:grundler@google.com] On Behalf Of Grant Grundler > Sent: Tuesday, July 16, 2013 1:19 AM > > On Mon, Jul 15, 2013 at 4:21 AM, Cho KyongHo <pullip.cho@samsung.com> wrote: > ... > >> Maybe you could add LV1TABLE_SIZE define and use it here (there is > >> already a LV2TABLE_SIZE define)? > > > > Yes. But, LV2TABLE_SIZE is used in more places than one. > > I do not feel that it is needed to define LV1TABLE_SIZE for the single line. > > Cho, > #define's are part of the code "documentation". Doesn't really matter > how often it's used. I think Bartlomiej's suggestion is a good one. > > Key is to use "consistent" names so they makes sense to someone not > familiar with the code. In this case, we want to show this use is the > same way LV2TABLE_SIZE is used and serves the same purpose. > Ok. I agree the error handling code is not pretty to understand. I will change it in the next patch. > cheers. > grant Thank you. Cho KyongHo. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-16 13:07 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-07-05 12:29 [PATCH v7 3/9] iommu/exynos: fix page table maintenance Cho KyongHo 2013-07-11 17:37 ` Bartlomiej Zolnierkiewicz 2013-07-15 11:21 ` Cho KyongHo 2013-07-15 16:18 ` Grant Grundler 2013-07-16 13:07 ` Cho KyongHo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox