2019-07-11 03:29:11

by Fuqian Huang

Subject: [PATCH 1/2] drm/ttm: use the same attributes when freeing d_page->vaddr

In function __ttm_dma_alloc_page(), d_page->addr is allocated
by dma_alloc_attrs() but freed with use dma_free_coherent() in
__ttm_dma_free_page().
Use the correct dma_free_attrs() to free d_page->vaddr.

Signed-off-by: Fuqian Huang <[email protected]>
---
drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
index d594f7520b7b..7d78e6deac89 100644
--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -285,9 +285,13 @@ static int ttm_set_pages_caching(struct dma_pool *pool,

static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page *d_page)
{
+ unsigned long attrs = 0;
dma_addr_t dma = d_page->dma;
d_page->vaddr &= ~VADDR_FLAG_HUGE_POOL;
- dma_free_coherent(pool->dev, pool->size, (void *)d_page->vaddr, dma);
+ if (pool->type & IS_HUGE)
+ attrs = DMA_ATTR_NO_WARN;
+
+ dma_free_attrs(pool->dev, pool->size, (void *)d_page->vaddr, dma, attrs);

kfree(d_page);
d_page = NULL;
--
2.11.0

2019-07-16 13:39:37

by Christian König

Subject: Re: [PATCH 1/2] drm/ttm: use the same attributes when freeing d_page->vaddr

Am 11.07.19 um 05:10 schrieb Fuqian Huang:
> In function __ttm_dma_alloc_page(), d_page->addr is allocated
> by dma_alloc_attrs() but freed with use dma_free_coherent() in
> __ttm_dma_free_page().
> Use the correct dma_free_attrs() to free d_page->vaddr.
>
> Signed-off-by: Fuqian Huang <[email protected]>

Reviewed-by: Christian König <[email protected]>

How do you want to upstream that? Should I pull it into our tree?

Thanks,
Christian.

> ---
> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index d594f7520b7b..7d78e6deac89 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -285,9 +285,13 @@ static int ttm_set_pages_caching(struct dma_pool *pool,
>
> static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page *d_page)
> {
> + unsigned long attrs = 0;
> dma_addr_t dma = d_page->dma;
> d_page->vaddr &= ~VADDR_FLAG_HUGE_POOL;
> - dma_free_coherent(pool->dev, pool->size, (void *)d_page->vaddr, dma);
> + if (pool->type & IS_HUGE)
> + attrs = DMA_ATTR_NO_WARN;
> +
> + dma_free_attrs(pool->dev, pool->size, (void *)d_page->vaddr, dma, attrs);
>
> kfree(d_page);
> d_page = NULL;

2019-07-18 07:22:35

by Fuqian Huang

Subject: Re: [PATCH 1/2] drm/ttm: use the same attributes when freeing d_page->vaddr

Koenig, Christian <[email protected]> 於 2019年7月16日週二 下午9:38寫道：
>
> Am 11.07.19 um 05:10 schrieb Fuqian Huang:
> > In function __ttm_dma_alloc_page(), d_page->addr is allocated
> > by dma_alloc_attrs() but freed with use dma_free_coherent() in
> > __ttm_dma_free_page().
> > Use the correct dma_free_attrs() to free d_page->vaddr.
> >
> > Signed-off-by: Fuqian Huang <[email protected]>
>
> Reviewed-by: Christian König <[email protected]>
>
> How do you want to upstream that? Should I pull it into our tree?

I just came across this misuse case accidentally.
I am not very clear about 'How to upstream that'.
Are there more than one way to upstream the code and fix the problem?

From my side, it is ok that you pull it into your tree and fix it or
fix it in other way.
:) It will be fine if the problem is fixed.

Thanks.

>
> Thanks,
> Christian.
>
> > ---
> > drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > index d594f7520b7b..7d78e6deac89 100644
> > --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> > @@ -285,9 +285,13 @@ static int ttm_set_pages_caching(struct dma_pool *pool,
> >
> > static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page *d_page)
> > {
> > + unsigned long attrs = 0;
> > dma_addr_t dma = d_page->dma;
> > d_page->vaddr &= ~VADDR_FLAG_HUGE_POOL;
> > - dma_free_coherent(pool->dev, pool->size, (void *)d_page->vaddr, dma);
> > + if (pool->type & IS_HUGE)
> > + attrs = DMA_ATTR_NO_WARN;
> > +
> > + dma_free_attrs(pool->dev, pool->size, (void *)d_page->vaddr, dma, attrs);
> >
> > kfree(d_page);
> > d_page = NULL;
>

2019-07-18 07:56:40

by Christian König

Subject: Re: [PATCH 1/2] drm/ttm: use the same attributes when freeing d_page->vaddr

Am 18.07.19 um 09:21 schrieb Fuqian Huang:
> Koenig, Christian <[email protected]> 於 2019年7月16日週二 下午9:38寫道：
>> Am 11.07.19 um 05:10 schrieb Fuqian Huang:
>>> In function __ttm_dma_alloc_page(), d_page->addr is allocated
>>> by dma_alloc_attrs() but freed with use dma_free_coherent() in
>>> __ttm_dma_free_page().
>>> Use the correct dma_free_attrs() to free d_page->vaddr.
>>>
>>> Signed-off-by: Fuqian Huang <[email protected]>
>> Reviewed-by: Christian König <[email protected]>
>>
>> How do you want to upstream that? Should I pull it into our tree?
> I just came across this misuse case accidentally.
> I am not very clear about 'How to upstream that'.
> Are there more than one way to upstream the code and fix the problem?

Well I can add it to the TTM tree which send to David or it could be
pulled through some other way towards Linus.

> From my side, it is ok that you pull it into your tree and fix it or
> fix it in other way.
> :) It will be fine if the problem is fixed.

Ok, fine with me :)

Christian.

>
> Thanks.
>
>> Thanks,
>> Christian.
>>
>>> ---
>>> drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> index d594f7520b7b..7d78e6deac89 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>>> @@ -285,9 +285,13 @@ static int ttm_set_pages_caching(struct dma_pool *pool,
>>>
>>> static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page *d_page)
>>> {
>>> + unsigned long attrs = 0;
>>> dma_addr_t dma = d_page->dma;
>>> d_page->vaddr &= ~VADDR_FLAG_HUGE_POOL;
>>> - dma_free_coherent(pool->dev, pool->size, (void *)d_page->vaddr, dma);
>>> + if (pool->type & IS_HUGE)
>>> + attrs = DMA_ATTR_NO_WARN;
>>> +
>>> + dma_free_attrs(pool->dev, pool->size, (void *)d_page->vaddr, dma, attrs);
>>>
>>> kfree(d_page);
>>> d_page = NULL;