Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

[Olivier Dautricourt]

Hello all,

I am facing a problem when using dmam_alloc_coherent (the managed
version of dma_alloc_coherent) along with a device-specific reserved memory
region using the CMA.

My observation is on a kernel 5.10.19 (arm), as i'm unable to test the exact
same configuration on a newer kernel. However it seems that the relevent code
did not change too much since, so i think it's still applicable.


....
The issue:

I declare a reserved region on my board such as:

mydevice_reserved: linux,cma {
        compatible = "shared-dma-pool";
        reusable;
        size = <0x2400000>;
};

and start the kernel with cma=0, i want my region to be reserved to my device.

My driver basically does:

probe(dev):
	of_reserved_mem_device_init(dev)
	dmam_alloc_coherent(...)

release(dev):
	of_reserved_mem_device_release(dev)


On driver detach, of_reserved_mem_device_release will call
rmem_cma_device_release which sets dev->cma_area = NULL;
Then the manager will try to free the dma memory allocated in the probe:

__free_from_contiguous -> dma_release_from_contiguous ->
cma_release(dev_get_cma_area(dev), ...);

Except that now dev_get_cma_area will return dma_contiguous_default_area
which is null in my setup:

static inline struct cma *dev_get_cma_area(struct device *dev)
{
	if (dev && dev->cma_area) // dev->cma_area is null
		return dev->cma_area;

	return dma_contiguous_default_area; // null in my setup
}

and so cma_release will do nothing.

bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
{
	unsigned long pfn;

	if (!cma || !pages) // cma is NULL
		return false;

__free_from_contiguous will fail silently because it ignores
dma_release_from_contiguous boolean result.

The driver will be unable to load and allocate memory again because the
area allocated with dmam_alloc_coherent is not freed.
...

So i started to look at drivers using both dmam_alloc_coherent and
of_reserved_mem_device_release and found this driver:
(gpu/drm/ingenic/ingenic-drm-drv.c).
This is why i included the original author, Paul Cercueil, in the loop.

Q:

I noticed that Paul used devm_add_action_or_reset to trigger
of_reserved_mem_device_release on driver detach, is this because of this
problem that we use a devm trigger here ?

I tried to do the same in my driver, but rmem_cma_device_release is still
called before dmam_release, is there a way to force the order ?

Is what i described a bug that needs fixing ?


Thank you,


Olivier

Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

[Paul Cercueil]

Hi Olivier,


Le ven., mai 21 2021 at 20:15:42 +0200, Olivier Dautricourt 
<olivier.dautricourt@orolia.com> a écrit :
> Hello all,
> 
> I am facing a problem when using dmam_alloc_coherent (the managed
> version of dma_alloc_coherent) along with a device-specific reserved 
> memory
> region using the CMA.
> 
> My observation is on a kernel 5.10.19 (arm), as i'm unable to test 
> the exact
> same configuration on a newer kernel. However it seems that the 
> relevent code
> did not change too much since, so i think it's still applicable.
> 
> 
> ....
> The issue:
> 
> I declare a reserved region on my board such as:
> 
> mydevice_reserved: linux,cma {
>         compatible = "shared-dma-pool";
>         reusable;
>         size = <0x2400000>;
> };
> 
> and start the kernel with cma=0, i want my region to be reserved to 
> my device.
> 
> My driver basically does:
> 
> probe(dev):
> 	of_reserved_mem_device_init(dev)
> 	dmam_alloc_coherent(...)
> 
> release(dev):
> 	of_reserved_mem_device_release(dev)

You must make sure that whatever is allocated or initialized is freed 
or deinitialized in the reverse order, which is not what will happen 
here: release(dev) will be called before the dev-managed cleanups.

To fix your issue, either use dma_alloc_coherent() and call 
dma_free_coherent() in release(), or register 
of_reserved_mem_device_release() as a dev-managed cleanup function 
(which is what my driver does).

Cheers,
-Paul

> On driver detach, of_reserved_mem_device_release will call
> rmem_cma_device_release which sets dev->cma_area = NULL;
> Then the manager will try to free the dma memory allocated in the 
> probe:
> 
> __free_from_contiguous -> dma_release_from_contiguous ->
> cma_release(dev_get_cma_area(dev), ...);
> 
> Except that now dev_get_cma_area will return 
> dma_contiguous_default_area
> which is null in my setup:
> 
> static inline struct cma *dev_get_cma_area(struct device *dev)
> {
> 	if (dev && dev->cma_area) // dev->cma_area is null
> 		return dev->cma_area;
> 
> 	return dma_contiguous_default_area; // null in my setup
> }
> 
> and so cma_release will do nothing.
> 
> bool cma_release(struct cma *cma, const struct page *pages, unsigned 
> int count)
> {
> 	unsigned long pfn;
> 
> 	if (!cma || !pages) // cma is NULL
> 		return false;
> 
> __free_from_contiguous will fail silently because it ignores
> dma_release_from_contiguous boolean result.
> 
> The driver will be unable to load and allocate memory again because 
> the
> area allocated with dmam_alloc_coherent is not freed.
> ...
> 
> So i started to look at drivers using both dmam_alloc_coherent and
> of_reserved_mem_device_release and found this driver:
> (gpu/drm/ingenic/ingenic-drm-drv.c).
> This is why i included the original author, Paul Cercueil, in the 
> loop.
> 
> Q:
> 
> I noticed that Paul used devm_add_action_or_reset to trigger
> of_reserved_mem_device_release on driver detach, is this because of 
> this
> problem that we use a devm trigger here ?
> 
> I tried to do the same in my driver, but rmem_cma_device_release is 
> still
> called before dmam_release, is there a way to force the order ?
> 
> Is what i described a bug that needs fixing ?
> 
> 
> Thank you,
> 
> 
> Olivier
> 
> 

Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

[Olivier Dautricourt]

Hello Paul,

The 05/22/2021 11:28, Paul Cercueil wrote:
> Hi Olivier,
>
>
> Le ven., mai 21 2021 at 20:15:42 +0200, Olivier Dautricourt
> <olivier.dautricourt@orolia.com> a écrit :
> > Hello all,
> >
> > I am facing a problem when using dmam_alloc_coherent (the managed
> > version of dma_alloc_coherent) along with a device-specific reserved
> > memory
> > region using the CMA.
> >
> > My observation is on a kernel 5.10.19 (arm), as i'm unable to test
> > the exact
> > same configuration on a newer kernel. However it seems that the
> > relevent code
> > did not change too much since, so i think it's still applicable.
> >
> >
> > ....
> > The issue:
> >
> > I declare a reserved region on my board such as:
> >
> > mydevice_reserved: linux,cma {
> >         compatible = "shared-dma-pool";
> >         reusable;
> >         size = <0x2400000>;
> > };
> >
> > and start the kernel with cma=0, i want my region to be reserved to
> > my device.
> >
> > My driver basically does:
> >
> > probe(dev):
> >       of_reserved_mem_device_init(dev)
> >       dmam_alloc_coherent(...)
> >
> > release(dev):
> >       of_reserved_mem_device_release(dev)
>
> You must make sure that whatever is allocated or initialized is freed
> or deinitialized in the reverse order, which is not what will happen
> here: release(dev) will be called before the dev-managed cleanups.
>
> To fix your issue, either use dma_alloc_coherent() and call
> dma_free_coherent() in release(), or register
> of_reserved_mem_device_release() as a dev-managed cleanup function
> (which is what my driver does).
>
> Cheers,
> -Paul

as i was saying in my previous mail, i tried to register a devm action
to trigger of_reserved_mem_device_release on cleanup but it was still
called before dmam_alloc_coherent_memory's cleanup.

So my question is: How do you make sure that the managed cleanup routines
are executed in the right order ?

Should we have to care about that when using a managed
function that belongs to the core ?


Olivier
>
> > On driver detach, of_reserved_mem_device_release will call
> > rmem_cma_device_release which sets dev->cma_area = NULL;
> > Then the manager will try to free the dma memory allocated in the
> > probe:
> >
> > __free_from_contiguous -> dma_release_from_contiguous ->
> > cma_release(dev_get_cma_area(dev), ...);
> >
> > Except that now dev_get_cma_area will return
> > dma_contiguous_default_area
> > which is null in my setup:
> >
> > static inline struct cma *dev_get_cma_area(struct device *dev)
> > {
> >       if (dev && dev->cma_area) // dev->cma_area is null
> >               return dev->cma_area;
> >
> >       return dma_contiguous_default_area; // null in my setup
> > }
> >
> > and so cma_release will do nothing.
> >
> > bool cma_release(struct cma *cma, const struct page *pages, unsigned
> > int count)
> > {
> >       unsigned long pfn;
> >
> >       if (!cma || !pages) // cma is NULL
> >               return false;
> >
> > __free_from_contiguous will fail silently because it ignores
> > dma_release_from_contiguous boolean result.
> >
> > The driver will be unable to load and allocate memory again because
> > the
> > area allocated with dmam_alloc_coherent is not freed.
> > ...
> >
> > So i started to look at drivers using both dmam_alloc_coherent and
> > of_reserved_mem_device_release and found this driver:
> > (gpu/drm/ingenic/ingenic-drm-drv.c).
> > This is why i included the original author, Paul Cercueil, in the
> > loop.
> >
> > Q:
> >
> > I noticed that Paul used devm_add_action_or_reset to trigger
> > of_reserved_mem_device_release on driver detach, is this because of
> > this
> > problem that we use a devm trigger here ?
> >
> > I tried to do the same in my driver, but rmem_cma_device_release is
> > still
> > called before dmam_release, is there a way to force the order ?
> >
> > Is what i described a bug that needs fixing ?
> >
> >
> > Thank you,
> >
> >
> > Olivier
> >
> >
>
>

Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

[Paul Cercueil]

Le sam., mai 22 2021 at 13:42:54 +0200, Olivier Dautricourt 
<olivier.dautricourt@orolia.com> a écrit :
> Hello Paul,
> 
> The 05/22/2021 11:28, Paul Cercueil wrote:
>>  Hi Olivier,
>> 
>> 
>>  Le ven., mai 21 2021 at 20:15:42 +0200, Olivier Dautricourt
>>  <olivier.dautricourt@orolia.com> a écrit :
>>  > Hello all,
>>  >
>>  > I am facing a problem when using dmam_alloc_coherent (the managed
>>  > version of dma_alloc_coherent) along with a device-specific 
>> reserved
>>  > memory
>>  > region using the CMA.
>>  >
>>  > My observation is on a kernel 5.10.19 (arm), as i'm unable to test
>>  > the exact
>>  > same configuration on a newer kernel. However it seems that the
>>  > relevent code
>>  > did not change too much since, so i think it's still applicable.
>>  >
>>  >
>>  > ....
>>  > The issue:
>>  >
>>  > I declare a reserved region on my board such as:
>>  >
>>  > mydevice_reserved: linux,cma {
>>  >         compatible = "shared-dma-pool";
>>  >         reusable;
>>  >         size = <0x2400000>;
>>  > };
>>  >
>>  > and start the kernel with cma=0, i want my region to be reserved 
>> to
>>  > my device.
>>  >
>>  > My driver basically does:
>>  >
>>  > probe(dev):
>>  >       of_reserved_mem_device_init(dev)
>>  >       dmam_alloc_coherent(...)
>>  >
>>  > release(dev):
>>  >       of_reserved_mem_device_release(dev)
>> 
>>  You must make sure that whatever is allocated or initialized is 
>> freed
>>  or deinitialized in the reverse order, which is not what will happen
>>  here: release(dev) will be called before the dev-managed cleanups.
>> 
>>  To fix your issue, either use dma_alloc_coherent() and call
>>  dma_free_coherent() in release(), or register
>>  of_reserved_mem_device_release() as a dev-managed cleanup function
>>  (which is what my driver does).
>> 
>>  Cheers,
>>  -Paul
> 
> as i was saying in my previous mail, i tried to register a devm action
> to trigger of_reserved_mem_device_release on cleanup but it was still
> called before dmam_alloc_coherent_memory's cleanup.
> 
> So my question is: How do you make sure that the managed cleanup 
> routines
> are executed in the right order ?

And when exactly do you register the devm action?

If you register it right after of_reserved_mem_device_init() and before 
dmam_alloc_coherent(), like in my driver, it should work fine (provided 
your .release doesn't call of_reserved_mem_device_release() itself) and 
you shouldn't have to do anything more than that.

-Paul


> Should we have to care about that when using a managed
> function that belongs to the core ?
> 
> 
> Olivier
>> 
>>  > On driver detach, of_reserved_mem_device_release will call
>>  > rmem_cma_device_release which sets dev->cma_area = NULL;
>>  > Then the manager will try to free the dma memory allocated in the
>>  > probe:
>>  >
>>  > __free_from_contiguous -> dma_release_from_contiguous ->
>>  > cma_release(dev_get_cma_area(dev), ...);
>>  >
>>  > Except that now dev_get_cma_area will return
>>  > dma_contiguous_default_area
>>  > which is null in my setup:
>>  >
>>  > static inline struct cma *dev_get_cma_area(struct device *dev)
>>  > {
>>  >       if (dev && dev->cma_area) // dev->cma_area is null
>>  >               return dev->cma_area;
>>  >
>>  >       return dma_contiguous_default_area; // null in my setup
>>  > }
>>  >
>>  > and so cma_release will do nothing.
>>  >
>>  > bool cma_release(struct cma *cma, const struct page *pages, 
>> unsigned
>>  > int count)
>>  > {
>>  >       unsigned long pfn;
>>  >
>>  >       if (!cma || !pages) // cma is NULL
>>  >               return false;
>>  >
>>  > __free_from_contiguous will fail silently because it ignores
>>  > dma_release_from_contiguous boolean result.
>>  >
>>  > The driver will be unable to load and allocate memory again 
>> because
>>  > the
>>  > area allocated with dmam_alloc_coherent is not freed.
>>  > ...
>>  >
>>  > So i started to look at drivers using both dmam_alloc_coherent and
>>  > of_reserved_mem_device_release and found this driver:
>>  > (gpu/drm/ingenic/ingenic-drm-drv.c).
>>  > This is why i included the original author, Paul Cercueil, in the
>>  > loop.
>>  >
>>  > Q:
>>  >
>>  > I noticed that Paul used devm_add_action_or_reset to trigger
>>  > of_reserved_mem_device_release on driver detach, is this because 
>> of
>>  > this
>>  > problem that we use a devm trigger here ?
>>  >
>>  > I tried to do the same in my driver, but rmem_cma_device_release 
>> is
>>  > still
>>  > called before dmam_release, is there a way to force the order ?
>>  >
>>  > Is what i described a bug that needs fixing ?
>>  >
>>  >
>>  > Thank you,
>>  >
>>  >
>>  > Olivier
>>  >
>>  >
>> 
>> 

Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

[Olivier Dautricourt]

The 05/22/2021 13:31, Paul Cercueil wrote:
> Le sam., mai 22 2021 at 13:42:54 +0200, Olivier Dautricourt
> <olivier.dautricourt@orolia.com> a écrit :
> > Hello Paul,
> >
> > The 05/22/2021 11:28, Paul Cercueil wrote:
> > >  Hi Olivier,
> > >
> > >
> > >  Le ven., mai 21 2021 at 20:15:42 +0200, Olivier Dautricourt
> > >  <olivier.dautricourt@orolia.com> a écrit :
> > >  > Hello all,
> > >  >
> > >  > I am facing a problem when using dmam_alloc_coherent (the managed
> > >  > version of dma_alloc_coherent) along with a device-specific
> > > reserved
> > >  > memory
> > >  > region using the CMA.
> > >  >
> > >  > My observation is on a kernel 5.10.19 (arm), as i'm unable to test
> > >  > the exact
> > >  > same configuration on a newer kernel. However it seems that the
> > >  > relevent code
> > >  > did not change too much since, so i think it's still applicable.
> > >  >
> > >  >
> > >  > ....
> > >  > The issue:
> > >  >
> > >  > I declare a reserved region on my board such as:
> > >  >
> > >  > mydevice_reserved: linux,cma {
> > >  >         compatible = "shared-dma-pool";
> > >  >         reusable;
> > >  >         size = <0x2400000>;
> > >  > };
> > >  >
> > >  > and start the kernel with cma=0, i want my region to be reserved
> > > to
> > >  > my device.
> > >  >
> > >  > My driver basically does:
> > >  >
> > >  > probe(dev):
> > >  >       of_reserved_mem_device_init(dev)
> > >  >       dmam_alloc_coherent(...)
> > >  >
> > >  > release(dev):
> > >  >       of_reserved_mem_device_release(dev)
> > >
> > >  You must make sure that whatever is allocated or initialized is
> > > freed
> > >  or deinitialized in the reverse order, which is not what will happen
> > >  here: release(dev) will be called before the dev-managed cleanups.
> > >
> > >  To fix your issue, either use dma_alloc_coherent() and call
> > >  dma_free_coherent() in release(), or register
> > >  of_reserved_mem_device_release() as a dev-managed cleanup function
> > >  (which is what my driver does).
> > >
> > >  Cheers,
> > >  -Paul
> >
> > as i was saying in my previous mail, i tried to register a devm action
> > to trigger of_reserved_mem_device_release on cleanup but it was still
> > called before dmam_alloc_coherent_memory's cleanup.
> >
> > So my question is: How do you make sure that the managed cleanup
> > routines
> > are executed in the right order ?
>
> And when exactly do you register the devm action?
>
> If you register it right after of_reserved_mem_device_init() and before
> dmam_alloc_coherent(), like in my driver, it should work fine (provided
> your .release doesn't call of_reserved_mem_device_release() itself) and
> you shouldn't have to do anything more than that.

I'm unable to test right now but you must be right, i registered the
devm action after dmam_alloc_coherent, so i suppose that if i change the
order it will work...

However i am still wondering if it is ok for the dmam release
to depend on the reserved mem state to suceed.
I think we should at least issue a warning when  __free_from_contiguous
fails.


Olivier

>
> -Paul
>
>
> > Should we have to care about that when using a managed
> > function that belongs to the core ?
> >
> >
> > Olivier
> > >
> > >  > On driver detach, of_reserved_mem_device_release will call
> > >  > rmem_cma_device_release which sets dev->cma_area = NULL;
> > >  > Then the manager will try to free the dma memory allocated in the
> > >  > probe:
> > >  >
> > >  > __free_from_contiguous -> dma_release_from_contiguous ->
> > >  > cma_release(dev_get_cma_area(dev), ...);
> > >  >
> > >  > Except that now dev_get_cma_area will return
> > >  > dma_contiguous_default_area
> > >  > which is null in my setup:
> > >  >
> > >  > static inline struct cma *dev_get_cma_area(struct device *dev)
> > >  > {
> > >  >       if (dev && dev->cma_area) // dev->cma_area is null
> > >  >               return dev->cma_area;
> > >  >
> > >  >       return dma_contiguous_default_area; // null in my setup
> > >  > }
> > >  >
> > >  > and so cma_release will do nothing.
> > >  >
> > >  > bool cma_release(struct cma *cma, const struct page *pages,
> > > unsigned
> > >  > int count)
> > >  > {
> > >  >       unsigned long pfn;
> > >  >
> > >  >       if (!cma || !pages) // cma is NULL
> > >  >               return false;
> > >  >
> > >  > __free_from_contiguous will fail silently because it ignores
> > >  > dma_release_from_contiguous boolean result.
> > >  >
> > >  > The driver will be unable to load and allocate memory again
> > > because
> > >  > the
> > >  > area allocated with dmam_alloc_coherent is not freed.
> > >  > ...
> > >  >
> > >  > So i started to look at drivers using both dmam_alloc_coherent and
> > >  > of_reserved_mem_device_release and found this driver:
> > >  > (gpu/drm/ingenic/ingenic-drm-drv.c).
> > >  > This is why i included the original author, Paul Cercueil, in the
> > >  > loop.
> > >  >
> > >  > Q:
> > >  >
> > >  > I noticed that Paul used devm_add_action_or_reset to trigger
> > >  > of_reserved_mem_device_release on driver detach, is this because
> > > of
> > >  > this
> > >  > problem that we use a devm trigger here ?
> > >  >
> > >  > I tried to do the same in my driver, but rmem_cma_device_release
> > > is
> > >  > still
> > >  > called before dmam_release, is there a way to force the order ?
> > >  >
> > >  > Is what i described a bug that needs fixing ?
> > >  >
> > >  >
> > >  > Thank you,
> > >  >
> > >  >
> > >  > Olivier
> > >  >
> > >  >
> > >
> > >
>
>