Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release

DISCUSSION Olivier Dautricourt 2 participants 5 messages May 21, 2021 – May 22, 2021 dmaengine

Share & tools

Olivier Dautricourt

author

2021-05-21 18:15 UTC #0 lore raw

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.

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

Paul Cercueil

2021-05-22 10:29 UTC #1 lore raw

Hi Olivier,

Quoted 24 lines · 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.

Quoted 8 lines

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

Quoted 57 lines

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

Olivier Dautricourt

author

2021-05-22 11:43 UTC #2 lore raw

Hello Paul,

Quoted 46 lines · Paul Cercueil

Hi Olivier,
Quoted 28 lines · 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.
Quoted 8 lines
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.

Quoted 7 lines

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

Quoted 67 lines

Quoted 65 lines
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

Paul Cercueil

2021-05-22 12:31 UTC #3 lore raw

Quoted 60 lines · Olivier Dautricourt

Hello Paul,
Quoted 49 lines · Paul Cercueil
Hi Olivier,
Quoted 4 lines · 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
Quoted 15 lines
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:
Quoted 7 lines
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
Quoted 10 lines
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.
Quoted 7 lines
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

Quoted 6 lines

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

Quoted 76 lines

Should we have to care about that when using a managed function that belongs to the core ?
Olivier
Quoted 71 lines
Quoted 21 lines
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
}
Quoted 3 lines
and so cma_release will do nothing.
bool cma_release(struct cma *cma, const struct page *pages,
unsigned
Quoted 11 lines
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
Quoted 10 lines
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.
Quoted 4 lines
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
Quoted 4 lines
this
problem that we use a devm trigger here ?
I tried to do the same in my driver, but rmem_cma_device_release
is
Quoted 12 lines
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

Olivier Dautricourt

author

2021-05-22 13:09 UTC #4 lore raw

Quoted 70 lines · Paul Cercueil

Quoted 60 lines · Olivier Dautricourt
Hello Paul,
Quoted 49 lines · Paul Cercueil
Hi Olivier,
Quoted 4 lines · 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
Quoted 15 lines
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:
Quoted 7 lines
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
Quoted 10 lines
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.
Quoted 7 lines
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
Quoted 7 lines
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 ?

Quoted 6 lines

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

Quoted 82 lines

-Paul
Quoted 76 lines
Should we have to care about that when using a managed function that belongs to the core ?
Olivier
Quoted 71 lines
Quoted 21 lines
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
}
Quoted 3 lines
and so cma_release will do nothing.
bool cma_release(struct cma *cma, const struct page *pages,
unsigned
Quoted 11 lines
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
Quoted 10 lines
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.
Quoted 4 lines
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
Quoted 4 lines
this
problem that we use a devm trigger here ?
I tried to do the same in my driver, but rmem_cma_device_release
is
Quoted 12 lines
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