* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-09 7:55 ` Tomasz Figa
0 siblings, 0 replies; 25+ messages in thread
From: Tomasz Figa @ 2013-08-09 7:55 UTC (permalink / raw)
To: Cho KyongHo
Cc: Tomasz Figa, 'Linux ARM Kernel', 'Linux IOMMU',
'Linux Kernel', 'Linux Samsung SOC', devicetree,
'Joerg Roedel', 'Kukjin Kim', 'Prathyush',
'Rahul Sharma', 'Subash Patel',
'Grant Grundler', 'Antonios Motakis', kvmarm,
'Sachin Kamat'
Hi KyongHo,
On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > Since kmalloc() does not guarantee that the allignment of 1KiB when
> > > it
> > > allocates 1KiB, it is required to allocate lv2 page table from own
> > > slab that guarantees alignment of 1KiB
> > >
> > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > ---
> > >
> > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iommu/exynos-iommu.c
> > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > --- a/drivers/iommu/exynos-iommu.c
> > > +++ b/drivers/iommu/exynos-iommu.c
> > > @@ -100,6 +100,8 @@
> > >
> > > #define REG_PB1_SADDR 0x054
> > > #define REG_PB1_EADDR 0x058
> > >
> > > +static struct kmem_cache *lv2table_kmem_cache;
> > > +
> > >
> > > static unsigned long *section_entry(unsigned long *pgtable,
> > > unsigned
> > >
> > > long iova) {
> > >
> > > return pgtable + lv1ent_offset(iova);
> > >
> > > @@ -765,7 +767,8 @@ static void exynos_iommu_domain_destroy(struct
> > > iommu_domain *domain)
> > >
> > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > >
> > > if (lv1ent_page(priv->pgtable + i))
> > >
> > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > + kmem_cache_free(lv2table_kmem_cache,
> > > + __va(lv2table_base(priv->pgtable +
i)));
> > >
> > > free_pages((unsigned long)priv->pgtable, 2);
> > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > >
> > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > long
> > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > >
> > > unsigned long *pent;
> > >
> > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > >
> > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > if (!pent)
> > >
> > > return ERR_PTR(-ENOMEM);
> > >
> > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long *sent,
> > > phys_addr_t paddr, short *pgcnt)
> > >
> > > if (lv1ent_page(sent)) {
> > >
> > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > >
> > > - kfree(page_entry(sent, 0));
> > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > >
> > > *pgcnt = 0;
> > >
> > > }
> > >
> > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > >
> > > {
> > >
> > > int ret;
> > >
> > > + lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > + if (!lv2table_kmem_cache) {
> > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > + return -ENOMEM;
> > > + }
> > > +
> > >
> > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > >
> > > if (ret == 0)
> > >
> > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > + ret = bus_set_iommu(&platform_bus_type,
&exynos_iommu_ops);
> > > +
> > > + if (ret) {
> > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > + __func__);
> > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > + }
> >
> > What about making the return value handling here cleaner? For example:
> > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> >
> > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> >
> > if (!lv2table_kmem_cache) {
> >
> > ...
> > return -ENOMEM;
> >
> > }
> >
> > ret = platform_driver_register(&exynos_sysmmu_driver);
> > if (ret) {
> >
> > ...
> > goto err_destroy_kmem_cache;
> >
> > }
> >
> > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > if (ret) {
> >
> > ...
> > goto err_platform_unregister;
> >
> > }
> >
> > return 0;
> >
> > err_platform_unregister:
> > ...
> >
> > err_destroy_kmem_cache:
> > ...
> > return ret;
> >
> > }
>
> Thank you for suggestion.
> I think you are worrying about missing the information who makes 'ret'
> non-zero.
Oh, this is a valid point, but it was more a nitpick about the coding
style. Single path error handling (with goto) is widely used in the kernel
in cases when more than one thing has to be undone and so I suggested this
method of error handling here as well.
> Ok. I will process it separately.
Since this patch adds most of the error handling to this function, I think
it should be fine to do it as a part of this patch.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-09 7:55 ` Tomasz Figa
0 siblings, 0 replies; 25+ messages in thread
From: Tomasz Figa @ 2013-08-09 7:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi KyongHo,
On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > Since kmalloc() does not guarantee that the allignment of 1KiB when
> > > it
> > > allocates 1KiB, it is required to allocate lv2 page table from own
> > > slab that guarantees alignment of 1KiB
> > >
> > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > ---
> > >
> > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iommu/exynos-iommu.c
> > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > --- a/drivers/iommu/exynos-iommu.c
> > > +++ b/drivers/iommu/exynos-iommu.c
> > > @@ -100,6 +100,8 @@
> > >
> > > #define REG_PB1_SADDR 0x054
> > > #define REG_PB1_EADDR 0x058
> > >
> > > +static struct kmem_cache *lv2table_kmem_cache;
> > > +
> > >
> > > static unsigned long *section_entry(unsigned long *pgtable,
> > > unsigned
> > >
> > > long iova) {
> > >
> > > return pgtable + lv1ent_offset(iova);
> > >
> > > @@ -765,7 +767,8 @@ static void exynos_iommu_domain_destroy(struct
> > > iommu_domain *domain)
> > >
> > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > >
> > > if (lv1ent_page(priv->pgtable + i))
> > >
> > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > + kmem_cache_free(lv2table_kmem_cache,
> > > + __va(lv2table_base(priv->pgtable +
i)));
> > >
> > > free_pages((unsigned long)priv->pgtable, 2);
> > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > >
> > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > long
> > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > >
> > > unsigned long *pent;
> > >
> > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > >
> > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > if (!pent)
> > >
> > > return ERR_PTR(-ENOMEM);
> > >
> > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long *sent,
> > > phys_addr_t paddr, short *pgcnt)
> > >
> > > if (lv1ent_page(sent)) {
> > >
> > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > >
> > > - kfree(page_entry(sent, 0));
> > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > >
> > > *pgcnt = 0;
> > >
> > > }
> > >
> > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > >
> > > {
> > >
> > > int ret;
> > >
> > > + lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > + if (!lv2table_kmem_cache) {
> > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > + return -ENOMEM;
> > > + }
> > > +
> > >
> > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > >
> > > if (ret == 0)
> > >
> > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > + ret = bus_set_iommu(&platform_bus_type,
&exynos_iommu_ops);
> > > +
> > > + if (ret) {
> > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > + __func__);
> > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > + }
> >
> > What about making the return value handling here cleaner? For example:
> > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> >
> > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> >
> > if (!lv2table_kmem_cache) {
> >
> > ...
> > return -ENOMEM;
> >
> > }
> >
> > ret = platform_driver_register(&exynos_sysmmu_driver);
> > if (ret) {
> >
> > ...
> > goto err_destroy_kmem_cache;
> >
> > }
> >
> > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > if (ret) {
> >
> > ...
> > goto err_platform_unregister;
> >
> > }
> >
> > return 0;
> >
> > err_platform_unregister:
> > ...
> >
> > err_destroy_kmem_cache:
> > ...
> > return ret;
> >
> > }
>
> Thank you for suggestion.
> I think you are worrying about missing the information who makes 'ret'
> non-zero.
Oh, this is a valid point, but it was more a nitpick about the coding
style. Single path error handling (with goto) is widely used in the kernel
in cases when more than one thing has to be undone and so I suggested this
method of error handling here as well.
> Ok. I will process it separately.
Since this patch adds most of the error handling to this function, I think
it should be fine to do it as a part of this patch.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
2013-08-09 7:55 ` Tomasz Figa
(?)
@ 2013-08-09 8:51 ` Cho KyongHo
-1 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-09 8:51 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, 'Linux Samsung SOC',
'Prathyush', 'Grant Grundler', Tomasz Figa,
'Subash Patel', 'Linux Kernel',
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg, 'Sachin Kamat',
'Linux IOMMU', 'Kukjin Kim',
'Antonios Motakis', 'Linux ARM Kernel',
'Rahul Sharma'
On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> Hi KyongHo,
>
> On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > Since kmalloc() does not guarantee that the allignment of 1KiB when
> > > > it
> > > > allocates 1KiB, it is required to allocate lv2 page table from own
> > > > slab that guarantees alignment of 1KiB
> > > >
> > > > Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > ---
> > > >
> > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -100,6 +100,8 @@
> > > >
> > > > #define REG_PB1_SADDR 0x054
> > > > #define REG_PB1_EADDR 0x058
> > > >
> > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > +
> > > >
> > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > unsigned
> > > >
> > > > long iova) {
> > > >
> > > > return pgtable + lv1ent_offset(iova);
> > > >
> > > > @@ -765,7 +767,8 @@ static void exynos_iommu_domain_destroy(struct
> > > > iommu_domain *domain)
> > > >
> > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > >
> > > > if (lv1ent_page(priv->pgtable + i))
> > > >
> > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > + __va(lv2table_base(priv->pgtable +
> i)));
> > > >
> > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > >
> > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > long
> > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > >
> > > > unsigned long *pent;
> > > >
> > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > >
> > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > if (!pent)
> > > >
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long *sent,
> > > > phys_addr_t paddr, short *pgcnt)
> > > >
> > > > if (lv1ent_page(sent)) {
> > > >
> > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > >
> > > > - kfree(page_entry(sent, 0));
> > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > >
> > > > *pgcnt = 0;
> > > >
> > > > }
> > > >
> > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > >
> > > > {
> > > >
> > > > int ret;
> > > >
> > > > + lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > + if (!lv2table_kmem_cache) {
> > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > >
> > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > >
> > > > if (ret == 0)
> > > >
> > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > + ret = bus_set_iommu(&platform_bus_type,
> &exynos_iommu_ops);
> > > > +
> > > > + if (ret) {
> > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > + __func__);
> > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > + }
> > >
> > > What about making the return value handling here cleaner? For example:
> > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > >
> > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > >
> > > if (!lv2table_kmem_cache) {
> > >
> > > ...
> > > return -ENOMEM;
> > >
> > > }
> > >
> > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > if (ret) {
> > >
> > > ...
> > > goto err_destroy_kmem_cache;
> > >
> > > }
> > >
> > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > if (ret) {
> > >
> > > ...
> > > goto err_platform_unregister;
> > >
> > > }
> > >
> > > return 0;
> > >
> > > err_platform_unregister:
> > > ...
> > >
> > > err_destroy_kmem_cache:
> > > ...
> > > return ret;
> > >
> > > }
> >
> > Thank you for suggestion.
> > I think you are worrying about missing the information who makes 'ret'
> > non-zero.
>
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
>
> > Ok. I will process it separately.
>
> Since this patch adds most of the error handling to this function, I think
> it should be fine to do it as a part of this patch.
>
I meant 'separately' that checking all return values each.
I think it can be simpler without goto.
There are just 2 cases to rollback previous changes in that function.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-09 8:51 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-09 8:51 UTC (permalink / raw)
To: Tomasz Figa
Cc: Tomasz Figa, 'Linux ARM Kernel', 'Linux IOMMU',
'Linux Kernel', 'Linux Samsung SOC', devicetree,
'Joerg Roedel', 'Kukjin Kim', 'Prathyush',
'Rahul Sharma', 'Subash Patel',
'Grant Grundler', 'Antonios Motakis', kvmarm,
'Sachin Kamat'
On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> Hi KyongHo,
>
> On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > Since kmalloc() does not guarantee that the allignment of 1KiB when
> > > > it
> > > > allocates 1KiB, it is required to allocate lv2 page table from own
> > > > slab that guarantees alignment of 1KiB
> > > >
> > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > ---
> > > >
> > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -100,6 +100,8 @@
> > > >
> > > > #define REG_PB1_SADDR 0x054
> > > > #define REG_PB1_EADDR 0x058
> > > >
> > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > +
> > > >
> > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > unsigned
> > > >
> > > > long iova) {
> > > >
> > > > return pgtable + lv1ent_offset(iova);
> > > >
> > > > @@ -765,7 +767,8 @@ static void exynos_iommu_domain_destroy(struct
> > > > iommu_domain *domain)
> > > >
> > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > >
> > > > if (lv1ent_page(priv->pgtable + i))
> > > >
> > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > + __va(lv2table_base(priv->pgtable +
> i)));
> > > >
> > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > >
> > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > long
> > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > >
> > > > unsigned long *pent;
> > > >
> > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > >
> > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > if (!pent)
> > > >
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long *sent,
> > > > phys_addr_t paddr, short *pgcnt)
> > > >
> > > > if (lv1ent_page(sent)) {
> > > >
> > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > >
> > > > - kfree(page_entry(sent, 0));
> > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > >
> > > > *pgcnt = 0;
> > > >
> > > > }
> > > >
> > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > >
> > > > {
> > > >
> > > > int ret;
> > > >
> > > > + lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > + if (!lv2table_kmem_cache) {
> > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > >
> > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > >
> > > > if (ret == 0)
> > > >
> > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > + ret = bus_set_iommu(&platform_bus_type,
> &exynos_iommu_ops);
> > > > +
> > > > + if (ret) {
> > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > + __func__);
> > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > + }
> > >
> > > What about making the return value handling here cleaner? For example:
> > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > >
> > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > >
> > > if (!lv2table_kmem_cache) {
> > >
> > > ...
> > > return -ENOMEM;
> > >
> > > }
> > >
> > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > if (ret) {
> > >
> > > ...
> > > goto err_destroy_kmem_cache;
> > >
> > > }
> > >
> > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > if (ret) {
> > >
> > > ...
> > > goto err_platform_unregister;
> > >
> > > }
> > >
> > > return 0;
> > >
> > > err_platform_unregister:
> > > ...
> > >
> > > err_destroy_kmem_cache:
> > > ...
> > > return ret;
> > >
> > > }
> >
> > Thank you for suggestion.
> > I think you are worrying about missing the information who makes 'ret'
> > non-zero.
>
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
>
> > Ok. I will process it separately.
>
> Since this patch adds most of the error handling to this function, I think
> it should be fine to do it as a part of this patch.
>
I meant 'separately' that checking all return values each.
I think it can be simpler without goto.
There are just 2 cases to rollback previous changes in that function.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-09 8:51 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-09 8:51 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> Hi KyongHo,
>
> On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > Since kmalloc() does not guarantee that the allignment of 1KiB when
> > > > it
> > > > allocates 1KiB, it is required to allocate lv2 page table from own
> > > > slab that guarantees alignment of 1KiB
> > > >
> > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > ---
> > > >
> > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -100,6 +100,8 @@
> > > >
> > > > #define REG_PB1_SADDR 0x054
> > > > #define REG_PB1_EADDR 0x058
> > > >
> > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > +
> > > >
> > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > unsigned
> > > >
> > > > long iova) {
> > > >
> > > > return pgtable + lv1ent_offset(iova);
> > > >
> > > > @@ -765,7 +767,8 @@ static void exynos_iommu_domain_destroy(struct
> > > > iommu_domain *domain)
> > > >
> > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > >
> > > > if (lv1ent_page(priv->pgtable + i))
> > > >
> > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > + __va(lv2table_base(priv->pgtable +
> i)));
> > > >
> > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > >
> > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > long
> > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > >
> > > > unsigned long *pent;
> > > >
> > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > >
> > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > if (!pent)
> > > >
> > > > return ERR_PTR(-ENOMEM);
> > > >
> > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long *sent,
> > > > phys_addr_t paddr, short *pgcnt)
> > > >
> > > > if (lv1ent_page(sent)) {
> > > >
> > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > >
> > > > - kfree(page_entry(sent, 0));
> > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > >
> > > > *pgcnt = 0;
> > > >
> > > > }
> > > >
> > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > >
> > > > {
> > > >
> > > > int ret;
> > > >
> > > > + lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > + if (!lv2table_kmem_cache) {
> > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > >
> > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > >
> > > > if (ret == 0)
> > > >
> > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > + ret = bus_set_iommu(&platform_bus_type,
> &exynos_iommu_ops);
> > > > +
> > > > + if (ret) {
> > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > + __func__);
> > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > + }
> > >
> > > What about making the return value handling here cleaner? For example:
> > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > >
> > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > >
> > > if (!lv2table_kmem_cache) {
> > >
> > > ...
> > > return -ENOMEM;
> > >
> > > }
> > >
> > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > if (ret) {
> > >
> > > ...
> > > goto err_destroy_kmem_cache;
> > >
> > > }
> > >
> > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > if (ret) {
> > >
> > > ...
> > > goto err_platform_unregister;
> > >
> > > }
> > >
> > > return 0;
> > >
> > > err_platform_unregister:
> > > ...
> > >
> > > err_destroy_kmem_cache:
> > > ...
> > > return ret;
> > >
> > > }
> >
> > Thank you for suggestion.
> > I think you are worrying about missing the information who makes 'ret'
> > non-zero.
>
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
>
> > Ok. I will process it separately.
>
> Since this patch adds most of the error handling to this function, I think
> it should be fine to do it as a part of this patch.
>
I meant 'separately' that checking all return values each.
I think it can be simpler without goto.
There are just 2 cases to rollback previous changes in that function.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
2013-08-09 8:51 ` Cho KyongHo
@ 2013-08-09 9:35 ` Tomasz Figa
-1 siblings, 0 replies; 25+ messages in thread
From: Tomasz Figa @ 2013-08-09 9:35 UTC (permalink / raw)
To: Cho KyongHo
Cc: Tomasz Figa, 'Linux ARM Kernel', 'Linux IOMMU',
'Linux Kernel', 'Linux Samsung SOC', devicetree,
'Joerg Roedel', 'Kukjin Kim', 'Prathyush',
'Rahul Sharma', 'Subash Patel',
'Grant Grundler', 'Antonios Motakis', kvmarm,
'Sachin Kamat'
On Friday 09 of August 2013 17:51:56 Cho KyongHo wrote:
> On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> > Hi KyongHo,
> >
> > On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > > Since kmalloc() does not guarantee that the allignment of 1KiB
> > > > > when
> > > > > it
> > > > > allocates 1KiB, it is required to allocate lv2 page table from
> > > > > own
> > > > > slab that guarantees alignment of 1KiB
> > > > >
> > > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > > ---
> > > > >
> > > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > > --- a/drivers/iommu/exynos-iommu.c
> > > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > > @@ -100,6 +100,8 @@
> > > > >
> > > > > #define REG_PB1_SADDR 0x054
> > > > > #define REG_PB1_EADDR 0x058
> > > > >
> > > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > > +
> > > > >
> > > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > > unsigned
> > > > >
> > > > > long iova) {
> > > > >
> > > > > return pgtable + lv1ent_offset(iova);
> > > > >
> > > > > @@ -765,7 +767,8 @@ static void
> > > > > exynos_iommu_domain_destroy(struct
> > > > > iommu_domain *domain)
> > > > >
> > > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > > >
> > > > > if (lv1ent_page(priv->pgtable + i))
> > > > >
> > > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > > + __va(lv2table_base(priv->pgtable +
> >
> > i)));
> >
> > > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > > >
> > > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > > long
> > > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > > >
> > > > > unsigned long *pent;
> > > > >
> > > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > > >
> > > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > > if (!pent)
> > > > >
> > > > > return ERR_PTR(-ENOMEM);
> > > > >
> > > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long
> > > > > *sent,
> > > > > phys_addr_t paddr, short *pgcnt)
> > > > >
> > > > > if (lv1ent_page(sent)) {
> > > > >
> > > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > > >
> > > > > - kfree(page_entry(sent, 0));
> > > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > > >
> > > > > *pgcnt = 0;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > > >
> > > > > {
> > > > >
> > > > > int ret;
> > > > >
> > > > > + lv2table_kmem_cache =
> > > > > kmem_cache_create("exynos-iommu-lv2table",
> > > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > > + if (!lv2table_kmem_cache) {
> > > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > > + return -ENOMEM;
> > > > > + }
> > > > > +
> > > > >
> > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > >
> > > > > if (ret == 0)
> > > > >
> > > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > + ret = bus_set_iommu(&platform_bus_type,
> >
> > &exynos_iommu_ops);
> >
> > > > > +
> > > > > + if (ret) {
> > > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > > + __func__);
> > > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > > + }
> > > >
> > > > What about making the return value handling here cleaner? For
example:
> > > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > >
> > > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > >
> > > > if (!lv2table_kmem_cache) {
> > > >
> > > > ...
> > > > return -ENOMEM;
> > > >
> > > > }
> > > >
> > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > if (ret) {
> > > >
> > > > ...
> > > > goto err_destroy_kmem_cache;
> > > >
> > > > }
> > > >
> > > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > if (ret) {
> > > >
> > > > ...
> > > > goto err_platform_unregister;
> > > >
> > > > }
> > > >
> > > > return 0;
> > > >
> > > > err_platform_unregister:
> > > > ...
> > > >
> > > > err_destroy_kmem_cache:
> > > > ...
> > > > return ret;
> > > >
> > > > }
> > >
> > > Thank you for suggestion.
> > > I think you are worrying about missing the information who makes
> > > 'ret'
> > > non-zero.
> >
> > Oh, this is a valid point, but it was more a nitpick about the coding
> > style. Single path error handling (with goto) is widely used in the
> > kernel in cases when more than one thing has to be undone and so I
> > suggested this method of error handling here as well.
> >
> > > Ok. I will process it separately.
> >
> > Since this patch adds most of the error handling to this function, I
> > think it should be fine to do it as a part of this patch.
>
> I meant 'separately' that checking all return values each.
> I think it can be simpler without goto.
>
> There are just 2 cases to rollback previous changes in that function.
It might end up with less lines, but having roll back in single place is
more readable. That's why it's the preferred error handling way in Linux
kernel, if you have more than one action to undo on your error path.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-09 9:35 ` Tomasz Figa
0 siblings, 0 replies; 25+ messages in thread
From: Tomasz Figa @ 2013-08-09 9:35 UTC (permalink / raw)
To: linux-arm-kernel
On Friday 09 of August 2013 17:51:56 Cho KyongHo wrote:
> On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> > Hi KyongHo,
> >
> > On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > > Since kmalloc() does not guarantee that the allignment of 1KiB
> > > > > when
> > > > > it
> > > > > allocates 1KiB, it is required to allocate lv2 page table from
> > > > > own
> > > > > slab that guarantees alignment of 1KiB
> > > > >
> > > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > > ---
> > > > >
> > > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > > --- a/drivers/iommu/exynos-iommu.c
> > > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > > @@ -100,6 +100,8 @@
> > > > >
> > > > > #define REG_PB1_SADDR 0x054
> > > > > #define REG_PB1_EADDR 0x058
> > > > >
> > > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > > +
> > > > >
> > > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > > unsigned
> > > > >
> > > > > long iova) {
> > > > >
> > > > > return pgtable + lv1ent_offset(iova);
> > > > >
> > > > > @@ -765,7 +767,8 @@ static void
> > > > > exynos_iommu_domain_destroy(struct
> > > > > iommu_domain *domain)
> > > > >
> > > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > > >
> > > > > if (lv1ent_page(priv->pgtable + i))
> > > > >
> > > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > > + __va(lv2table_base(priv->pgtable +
> >
> > i)));
> >
> > > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > > >
> > > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > > long
> > > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > > >
> > > > > unsigned long *pent;
> > > > >
> > > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > > >
> > > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > > if (!pent)
> > > > >
> > > > > return ERR_PTR(-ENOMEM);
> > > > >
> > > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long
> > > > > *sent,
> > > > > phys_addr_t paddr, short *pgcnt)
> > > > >
> > > > > if (lv1ent_page(sent)) {
> > > > >
> > > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > > >
> > > > > - kfree(page_entry(sent, 0));
> > > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > > >
> > > > > *pgcnt = 0;
> > > > >
> > > > > }
> > > > >
> > > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > > >
> > > > > {
> > > > >
> > > > > int ret;
> > > > >
> > > > > + lv2table_kmem_cache =
> > > > > kmem_cache_create("exynos-iommu-lv2table",
> > > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > > + if (!lv2table_kmem_cache) {
> > > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > > + return -ENOMEM;
> > > > > + }
> > > > > +
> > > > >
> > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > >
> > > > > if (ret == 0)
> > > > >
> > > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > + ret = bus_set_iommu(&platform_bus_type,
> >
> > &exynos_iommu_ops);
> >
> > > > > +
> > > > > + if (ret) {
> > > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > > + __func__);
> > > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > > + }
> > > >
> > > > What about making the return value handling here cleaner? For
example:
> > > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > >
> > > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > >
> > > > if (!lv2table_kmem_cache) {
> > > >
> > > > ...
> > > > return -ENOMEM;
> > > >
> > > > }
> > > >
> > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > if (ret) {
> > > >
> > > > ...
> > > > goto err_destroy_kmem_cache;
> > > >
> > > > }
> > > >
> > > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > if (ret) {
> > > >
> > > > ...
> > > > goto err_platform_unregister;
> > > >
> > > > }
> > > >
> > > > return 0;
> > > >
> > > > err_platform_unregister:
> > > > ...
> > > >
> > > > err_destroy_kmem_cache:
> > > > ...
> > > > return ret;
> > > >
> > > > }
> > >
> > > Thank you for suggestion.
> > > I think you are worrying about missing the information who makes
> > > 'ret'
> > > non-zero.
> >
> > Oh, this is a valid point, but it was more a nitpick about the coding
> > style. Single path error handling (with goto) is widely used in the
> > kernel in cases when more than one thing has to be undone and so I
> > suggested this method of error handling here as well.
> >
> > > Ok. I will process it separately.
> >
> > Since this patch adds most of the error handling to this function, I
> > think it should be fine to do it as a part of this patch.
>
> I meant 'separately' that checking all return values each.
> I think it can be simpler without goto.
>
> There are just 2 cases to rollback previous changes in that function.
It might end up with less lines, but having roll back in single place is
more readable. That's why it's the preferred error handling way in Linux
kernel, if you have more than one action to undo on your error path.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
2013-08-09 9:35 ` Tomasz Figa
(?)
@ 2013-08-12 1:59 ` Cho KyongHo
-1 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-12 1:59 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, 'Linux Samsung SOC',
'Prathyush', 'Grant Grundler',
'Subash Patel', Tomasz Figa, 'Linux Kernel',
'Sachin Kamat', 'Linux IOMMU',
'Kukjin Kim', 'Antonios Motakis',
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
'Linux ARM Kernel', 'Rahul Sharma'
On Fri, 09 Aug 2013 11:35:20 +0200, Tomasz Figa wrote:
> On Friday 09 of August 2013 17:51:56 Cho KyongHo wrote:
> > On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> > > Hi KyongHo,
> > >
> > > On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > > > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > > > Since kmalloc() does not guarantee that the allignment of 1KiB
> > > > > > when
> > > > > > it
> > > > > > allocates 1KiB, it is required to allocate lv2 page table from
> > > > > > own
> > > > > > slab that guarantees alignment of 1KiB
> > > > > >
> > > > > > Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > > > > > ---
> > > > > >
> > > > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > > > --- a/drivers/iommu/exynos-iommu.c
> > > > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > > > @@ -100,6 +100,8 @@
> > > > > >
> > > > > > #define REG_PB1_SADDR 0x054
> > > > > > #define REG_PB1_EADDR 0x058
> > > > > >
> > > > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > > > +
> > > > > >
> > > > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > > > unsigned
> > > > > >
> > > > > > long iova) {
> > > > > >
> > > > > > return pgtable + lv1ent_offset(iova);
> > > > > >
> > > > > > @@ -765,7 +767,8 @@ static void
> > > > > > exynos_iommu_domain_destroy(struct
> > > > > > iommu_domain *domain)
> > > > > >
> > > > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > > > >
> > > > > > if (lv1ent_page(priv->pgtable + i))
> > > > > >
> > > > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > > > + __va(lv2table_base(priv->pgtable +
> > >
> > > i)));
> > >
> > > > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > > > >
> > > > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > > > long
> > > > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > > > >
> > > > > > unsigned long *pent;
> > > > > >
> > > > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > > > >
> > > > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > > > if (!pent)
> > > > > >
> > > > > > return ERR_PTR(-ENOMEM);
> > > > > >
> > > > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long
> > > > > > *sent,
> > > > > > phys_addr_t paddr, short *pgcnt)
> > > > > >
> > > > > > if (lv1ent_page(sent)) {
> > > > > >
> > > > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > > > >
> > > > > > - kfree(page_entry(sent, 0));
> > > > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > > > >
> > > > > > *pgcnt = 0;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > > > >
> > > > > > {
> > > > > >
> > > > > > int ret;
> > > > > >
> > > > > > + lv2table_kmem_cache =
> > > > > > kmem_cache_create("exynos-iommu-lv2table",
> > > > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > > > + if (!lv2table_kmem_cache) {
> > > > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > > > + return -ENOMEM;
> > > > > > + }
> > > > > > +
> > > > > >
> > > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > >
> > > > > > if (ret == 0)
> > > > > >
> > > > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > > + ret = bus_set_iommu(&platform_bus_type,
> > >
> > > &exynos_iommu_ops);
> > >
> > > > > > +
> > > > > > + if (ret) {
> > > > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > > > + __func__);
> > > > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > > > + }
> > > > >
> > > > > What about making the return value handling here cleaner? For
> example:
> > > > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > >
> > > > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > >
> > > > > if (!lv2table_kmem_cache) {
> > > > >
> > > > > ...
> > > > > return -ENOMEM;
> > > > >
> > > > > }
> > > > >
> > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_destroy_kmem_cache;
> > > > >
> > > > > }
> > > > >
> > > > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_platform_unregister;
> > > > >
> > > > > }
> > > > >
> > > > > return 0;
> > > > >
> > > > > err_platform_unregister:
> > > > > ...
> > > > >
> > > > > err_destroy_kmem_cache:
> > > > > ...
> > > > > return ret;
> > > > >
> > > > > }
> > > >
> > > > Thank you for suggestion.
> > > > I think you are worrying about missing the information who makes
> > > > 'ret'
> > > > non-zero.
> > >
> > > Oh, this is a valid point, but it was more a nitpick about the coding
> > > style. Single path error handling (with goto) is widely used in the
> > > kernel in cases when more than one thing has to be undone and so I
> > > suggested this method of error handling here as well.
> > >
> > > > Ok. I will process it separately.
> > >
> > > Since this patch adds most of the error handling to this function, I
> > > think it should be fine to do it as a part of this patch.
> >
> > I meant 'separately' that checking all return values each.
> > I think it can be simpler without goto.
> >
> > There are just 2 cases to rollback previous changes in that function.
>
> It might end up with less lines, but having roll back in single place is
> more readable. That's why it's the preferred error handling way in Linux
> kernel, if you have more than one action to undo on your error path.
>
I fully understand what you are concerning about and I also agree.
Actually it is simple choice without any side effect.
It is just a preference.
I will change it into 'goto'.
Then we can determine what looks better :)
Thank you.
KyongHo.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-12 1:59 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-12 1:59 UTC (permalink / raw)
To: Tomasz Figa
Cc: Tomasz Figa, 'Linux ARM Kernel', 'Linux IOMMU',
'Linux Kernel', 'Linux Samsung SOC', devicetree,
'Joerg Roedel', 'Kukjin Kim', 'Prathyush',
'Rahul Sharma', 'Subash Patel',
'Grant Grundler', 'Antonios Motakis', kvmarm,
'Sachin Kamat'
On Fri, 09 Aug 2013 11:35:20 +0200, Tomasz Figa wrote:
> On Friday 09 of August 2013 17:51:56 Cho KyongHo wrote:
> > On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> > > Hi KyongHo,
> > >
> > > On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > > > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > > > Since kmalloc() does not guarantee that the allignment of 1KiB
> > > > > > when
> > > > > > it
> > > > > > allocates 1KiB, it is required to allocate lv2 page table from
> > > > > > own
> > > > > > slab that guarantees alignment of 1KiB
> > > > > >
> > > > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > > > ---
> > > > > >
> > > > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > > > --- a/drivers/iommu/exynos-iommu.c
> > > > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > > > @@ -100,6 +100,8 @@
> > > > > >
> > > > > > #define REG_PB1_SADDR 0x054
> > > > > > #define REG_PB1_EADDR 0x058
> > > > > >
> > > > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > > > +
> > > > > >
> > > > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > > > unsigned
> > > > > >
> > > > > > long iova) {
> > > > > >
> > > > > > return pgtable + lv1ent_offset(iova);
> > > > > >
> > > > > > @@ -765,7 +767,8 @@ static void
> > > > > > exynos_iommu_domain_destroy(struct
> > > > > > iommu_domain *domain)
> > > > > >
> > > > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > > > >
> > > > > > if (lv1ent_page(priv->pgtable + i))
> > > > > >
> > > > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > > > + __va(lv2table_base(priv->pgtable +
> > >
> > > i)));
> > >
> > > > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > > > >
> > > > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > > > long
> > > > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > > > >
> > > > > > unsigned long *pent;
> > > > > >
> > > > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > > > >
> > > > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > > > if (!pent)
> > > > > >
> > > > > > return ERR_PTR(-ENOMEM);
> > > > > >
> > > > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long
> > > > > > *sent,
> > > > > > phys_addr_t paddr, short *pgcnt)
> > > > > >
> > > > > > if (lv1ent_page(sent)) {
> > > > > >
> > > > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > > > >
> > > > > > - kfree(page_entry(sent, 0));
> > > > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > > > >
> > > > > > *pgcnt = 0;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > > > >
> > > > > > {
> > > > > >
> > > > > > int ret;
> > > > > >
> > > > > > + lv2table_kmem_cache =
> > > > > > kmem_cache_create("exynos-iommu-lv2table",
> > > > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > > > + if (!lv2table_kmem_cache) {
> > > > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > > > + return -ENOMEM;
> > > > > > + }
> > > > > > +
> > > > > >
> > > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > >
> > > > > > if (ret == 0)
> > > > > >
> > > > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > > + ret = bus_set_iommu(&platform_bus_type,
> > >
> > > &exynos_iommu_ops);
> > >
> > > > > > +
> > > > > > + if (ret) {
> > > > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > > > + __func__);
> > > > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > > > + }
> > > > >
> > > > > What about making the return value handling here cleaner? For
> example:
> > > > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > >
> > > > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > >
> > > > > if (!lv2table_kmem_cache) {
> > > > >
> > > > > ...
> > > > > return -ENOMEM;
> > > > >
> > > > > }
> > > > >
> > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_destroy_kmem_cache;
> > > > >
> > > > > }
> > > > >
> > > > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_platform_unregister;
> > > > >
> > > > > }
> > > > >
> > > > > return 0;
> > > > >
> > > > > err_platform_unregister:
> > > > > ...
> > > > >
> > > > > err_destroy_kmem_cache:
> > > > > ...
> > > > > return ret;
> > > > >
> > > > > }
> > > >
> > > > Thank you for suggestion.
> > > > I think you are worrying about missing the information who makes
> > > > 'ret'
> > > > non-zero.
> > >
> > > Oh, this is a valid point, but it was more a nitpick about the coding
> > > style. Single path error handling (with goto) is widely used in the
> > > kernel in cases when more than one thing has to be undone and so I
> > > suggested this method of error handling here as well.
> > >
> > > > Ok. I will process it separately.
> > >
> > > Since this patch adds most of the error handling to this function, I
> > > think it should be fine to do it as a part of this patch.
> >
> > I meant 'separately' that checking all return values each.
> > I think it can be simpler without goto.
> >
> > There are just 2 cases to rollback previous changes in that function.
>
> It might end up with less lines, but having roll back in single place is
> more readable. That's why it's the preferred error handling way in Linux
> kernel, if you have more than one action to undo on your error path.
>
I fully understand what you are concerning about and I also agree.
Actually it is simple choice without any side effect.
It is just a preference.
I will change it into 'goto'.
Then we can determine what looks better :)
Thank you.
KyongHo.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-12 1:59 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-12 1:59 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, 09 Aug 2013 11:35:20 +0200, Tomasz Figa wrote:
> On Friday 09 of August 2013 17:51:56 Cho KyongHo wrote:
> > On Fri, 09 Aug 2013 09:55:30 +0200, Tomasz Figa wrote:
> > > Hi KyongHo,
> > >
> > > On Friday 09 of August 2013 14:58:49 Cho KyongHo wrote:
> > > > On Thu, 08 Aug 2013 16:00:18 +0200, Tomasz Figa wrote:
> > > > > On Thursday 08 of August 2013 18:38:04 Cho KyongHo wrote:
> > > > > > Since kmalloc() does not guarantee that the allignment of 1KiB
> > > > > > when
> > > > > > it
> > > > > > allocates 1KiB, it is required to allocate lv2 page table from
> > > > > > own
> > > > > > slab that guarantees alignment of 1KiB
> > > > > >
> > > > > > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > > > > > ---
> > > > > >
> > > > > > drivers/iommu/exynos-iommu.c | 24 ++++++++++++++++++++----
> > > > > > 1 files changed, 20 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > > > b/drivers/iommu/exynos-iommu.c index d90e6fa..a318049 100644
> > > > > > --- a/drivers/iommu/exynos-iommu.c
> > > > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > > > @@ -100,6 +100,8 @@
> > > > > >
> > > > > > #define REG_PB1_SADDR 0x054
> > > > > > #define REG_PB1_EADDR 0x058
> > > > > >
> > > > > > +static struct kmem_cache *lv2table_kmem_cache;
> > > > > > +
> > > > > >
> > > > > > static unsigned long *section_entry(unsigned long *pgtable,
> > > > > > unsigned
> > > > > >
> > > > > > long iova) {
> > > > > >
> > > > > > return pgtable + lv1ent_offset(iova);
> > > > > >
> > > > > > @@ -765,7 +767,8 @@ static void
> > > > > > exynos_iommu_domain_destroy(struct
> > > > > > iommu_domain *domain)
> > > > > >
> > > > > > for (i = 0; i < NUM_LV1ENTRIES; i++)
> > > > > >
> > > > > > if (lv1ent_page(priv->pgtable + i))
> > > > > >
> > > > > > - kfree(__va(lv2table_base(priv->pgtable + i)));
> > > > > > + kmem_cache_free(lv2table_kmem_cache,
> > > > > > + __va(lv2table_base(priv->pgtable +
> > >
> > > i)));
> > >
> > > > > > free_pages((unsigned long)priv->pgtable, 2);
> > > > > > free_pages((unsigned long)priv->lv2entcnt, 1);
> > > > > >
> > > > > > @@ -861,7 +864,7 @@ static unsigned long *alloc_lv2entry(unsigned
> > > > > > long
> > > > > > *sent, unsigned long iova, if (lv1ent_fault(sent)) {
> > > > > >
> > > > > > unsigned long *pent;
> > > > > >
> > > > > > - pent = kzalloc(LV2TABLE_SIZE, GFP_ATOMIC);
> > > > > > + pent = kmem_cache_zalloc(lv2table_kmem_cache, GFP_ATOMIC);
> > > > > >
> > > > > > BUG_ON((unsigned long)pent & (LV2TABLE_SIZE - 1));
> > > > > > if (!pent)
> > > > > >
> > > > > > return ERR_PTR(-ENOMEM);
> > > > > >
> > > > > > @@ -881,7 +884,7 @@ static int lv1set_section(unsigned long
> > > > > > *sent,
> > > > > > phys_addr_t paddr, short *pgcnt)
> > > > > >
> > > > > > if (lv1ent_page(sent)) {
> > > > > >
> > > > > > BUG_ON(*pgcnt != NUM_LV2ENTRIES);
> > > > > >
> > > > > > - kfree(page_entry(sent, 0));
> > > > > > + kmem_cache_free(lv2table_kmem_cache, page_entry(sent, 0));
> > > > > >
> > > > > > *pgcnt = 0;
> > > > > >
> > > > > > }
> > > > > >
> > > > > > @@ -1082,10 +1085,23 @@ static int __init exynos_iommu_init(void)
> > > > > >
> > > > > > {
> > > > > >
> > > > > > int ret;
> > > > > >
> > > > > > + lv2table_kmem_cache =
> > > > > > kmem_cache_create("exynos-iommu-lv2table",
> > > > > > + LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > > > + if (!lv2table_kmem_cache) {
> > > > > > + pr_err("%s: Failed to create kmem cache\n", __func__);
> > > > > > + return -ENOMEM;
> > > > > > + }
> > > > > > +
> > > > > >
> > > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > >
> > > > > > if (ret == 0)
> > > > > >
> > > > > > - bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > > + ret = bus_set_iommu(&platform_bus_type,
> > >
> > > &exynos_iommu_ops);
> > >
> > > > > > +
> > > > > > + if (ret) {
> > > > > > + pr_err("%s: Failed to register exynos-iommu driver.\n",
> > > > > > + __func__);
> > > > > > + kmem_cache_destroy(lv2table_kmem_cache);
> > > > > > + }
> > > > >
> > > > > What about making the return value handling here cleaner? For
> example:
> > > > > lv2table_kmem_cache = kmem_cache_create("exynos-iommu-lv2table",
> > > > >
> > > > > LV2TABLE_SIZE, LV2TABLE_SIZE, 0, NULL);
> > > > >
> > > > > if (!lv2table_kmem_cache) {
> > > > >
> > > > > ...
> > > > > return -ENOMEM;
> > > > >
> > > > > }
> > > > >
> > > > > ret = platform_driver_register(&exynos_sysmmu_driver);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_destroy_kmem_cache;
> > > > >
> > > > > }
> > > > >
> > > > > ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
> > > > > if (ret) {
> > > > >
> > > > > ...
> > > > > goto err_platform_unregister;
> > > > >
> > > > > }
> > > > >
> > > > > return 0;
> > > > >
> > > > > err_platform_unregister:
> > > > > ...
> > > > >
> > > > > err_destroy_kmem_cache:
> > > > > ...
> > > > > return ret;
> > > > >
> > > > > }
> > > >
> > > > Thank you for suggestion.
> > > > I think you are worrying about missing the information who makes
> > > > 'ret'
> > > > non-zero.
> > >
> > > Oh, this is a valid point, but it was more a nitpick about the coding
> > > style. Single path error handling (with goto) is widely used in the
> > > kernel in cases when more than one thing has to be undone and so I
> > > suggested this method of error handling here as well.
> > >
> > > > Ok. I will process it separately.
> > >
> > > Since this patch adds most of the error handling to this function, I
> > > think it should be fine to do it as a part of this patch.
> >
> > I meant 'separately' that checking all return values each.
> > I think it can be simpler without goto.
> >
> > There are just 2 cases to rollback previous changes in that function.
>
> It might end up with less lines, but having roll back in single place is
> more readable. That's why it's the preferred error handling way in Linux
> kernel, if you have more than one action to undo on your error path.
>
I fully understand what you are concerning about and I also agree.
Actually it is simple choice without any side effect.
It is just a preference.
I will change it into 'goto'.
Then we can determine what looks better :)
Thank you.
KyongHo.
> Best regards,
> Tomasz
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
2013-08-09 7:55 ` Tomasz Figa
(?)
@ 2013-08-14 10:56 ` 'Joerg Roedel'
-1 siblings, 0 replies; 25+ messages in thread
From: 'Joerg Roedel' @ 2013-08-14 10:56 UTC (permalink / raw)
To: Tomasz Figa
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, 'Linux Samsung SOC',
'Prathyush', 'Grant Grundler', Tomasz Figa,
'Subash Patel', 'Linux Kernel',
'Sachin Kamat', 'Linux IOMMU',
'Kukjin Kim', 'Antonios Motakis', Cho KyongHo,
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
'Linux ARM Kernel', 'Rahul Sharma'
On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
I agree with that review from Tomasz, the error-handling should look
similar to all the other places in the kernel and use goto.
Joerg
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-14 10:56 ` 'Joerg Roedel'
0 siblings, 0 replies; 25+ messages in thread
From: 'Joerg Roedel' @ 2013-08-14 10:56 UTC (permalink / raw)
To: Tomasz Figa
Cc: Cho KyongHo, Tomasz Figa, 'Linux ARM Kernel',
'Linux IOMMU', 'Linux Kernel',
'Linux Samsung SOC', devicetree, 'Kukjin Kim',
'Prathyush', 'Rahul Sharma',
'Subash Patel', 'Grant Grundler',
'Antonios Motakis', kvmarm, 'Sachin Kamat'
On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
I agree with that review from Tomasz, the error-handling should look
similar to all the other places in the kernel and use goto.
Joerg
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-14 10:56 ` 'Joerg Roedel'
0 siblings, 0 replies; 25+ messages in thread
From: 'Joerg Roedel' @ 2013-08-14 10:56 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> Oh, this is a valid point, but it was more a nitpick about the coding
> style. Single path error handling (with goto) is widely used in the kernel
> in cases when more than one thing has to be undone and so I suggested this
> method of error handling here as well.
I agree with that review from Tomasz, the error-handling should look
similar to all the other places in the kernel and use goto.
Joerg
^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <20130814105615.GG4491-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]
* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
2013-08-14 10:56 ` 'Joerg Roedel'
(?)
@ 2013-08-16 11:17 ` Cho KyongHo
-1 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-16 11:17 UTC (permalink / raw)
To: 'Joerg Roedel'
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, 'Linux Samsung SOC',
'Prathyush', 'Grant Grundler', Tomasz Figa,
'Subash Patel', Tomasz Figa, 'Linux Kernel',
'Sachin Kamat', 'Linux IOMMU',
'Kukjin Kim', 'Antonios Motakis',
kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
'Linux ARM Kernel', 'Rahul Sharma'
On Wed, 14 Aug 2013 12:56:16 +0200, 'Joerg Roedel' wrote:
> On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> > Oh, this is a valid point, but it was more a nitpick about the coding
> > style. Single path error handling (with goto) is widely used in the kernel
> > in cases when more than one thing has to be undone and so I suggested this
> > method of error handling here as well.
>
> I agree with that review from Tomasz, the error-handling should look
> similar to all the other places in the kernel and use goto.
>
Ok.
Thanks.
KyongHo.
>
> Joerg
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-16 11:17 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-16 11:17 UTC (permalink / raw)
To: 'Joerg Roedel'
Cc: Tomasz Figa, Tomasz Figa, 'Linux ARM Kernel',
'Linux IOMMU', 'Linux Kernel',
'Linux Samsung SOC', devicetree, 'Kukjin Kim',
'Prathyush', 'Rahul Sharma',
'Subash Patel', 'Grant Grundler',
'Antonios Motakis', kvmarm, 'Sachin Kamat'
On Wed, 14 Aug 2013 12:56:16 +0200, 'Joerg Roedel' wrote:
> On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> > Oh, this is a valid point, but it was more a nitpick about the coding
> > style. Single path error handling (with goto) is widely used in the kernel
> > in cases when more than one thing has to be undone and so I suggested this
> > method of error handling here as well.
>
> I agree with that review from Tomasz, the error-handling should look
> similar to all the other places in the kernel and use goto.
>
Ok.
Thanks.
KyongHo.
>
> Joerg
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v9 04/16] iommu/exynos: allocate lv2 page table from own slab
@ 2013-08-16 11:17 ` Cho KyongHo
0 siblings, 0 replies; 25+ messages in thread
From: Cho KyongHo @ 2013-08-16 11:17 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 14 Aug 2013 12:56:16 +0200, 'Joerg Roedel' wrote:
> On Fri, Aug 09, 2013 at 09:55:30AM +0200, Tomasz Figa wrote:
> > Oh, this is a valid point, but it was more a nitpick about the coding
> > style. Single path error handling (with goto) is widely used in the kernel
> > in cases when more than one thing has to be undone and so I suggested this
> > method of error handling here as well.
>
> I agree with that review from Tomasz, the error-handling should look
> similar to all the other places in the kernel and use goto.
>
Ok.
Thanks.
KyongHo.
>
> Joerg
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread