* Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release @ 2021-05-21 18:15 Olivier Dautricourt 2021-05-22 10:28 ` Paul Cercueil 0 siblings, 1 reply; 5+ messages in thread From: Olivier Dautricourt @ 2021-05-21 18:15 UTC (permalink / raw) To: Vinod Koul, dmaengine; +Cc: Paul Cercueil, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release 2021-05-21 18:15 Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release Olivier Dautricourt @ 2021-05-22 10:28 ` Paul Cercueil 2021-05-22 11:42 ` Olivier Dautricourt 0 siblings, 1 reply; 5+ messages in thread From: Paul Cercueil @ 2021-05-22 10:28 UTC (permalink / raw) To: Olivier Dautricourt; +Cc: Vinod Koul, dmaengine 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release 2021-05-22 10:28 ` Paul Cercueil @ 2021-05-22 11:42 ` Olivier Dautricourt 2021-05-22 12:31 ` Paul Cercueil 0 siblings, 1 reply; 5+ messages in thread From: Olivier Dautricourt @ 2021-05-22 11:42 UTC (permalink / raw) To: Paul Cercueil; +Cc: Vinod Koul, dmaengine 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 > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release 2021-05-22 11:42 ` Olivier Dautricourt @ 2021-05-22 12:31 ` Paul Cercueil 2021-05-22 13:09 ` Olivier Dautricourt 0 siblings, 1 reply; 5+ messages in thread From: Paul Cercueil @ 2021-05-22 12:31 UTC (permalink / raw) To: Olivier Dautricourt; +Cc: Vinod Koul, dmaengine 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 >> > >> > >> >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release 2021-05-22 12:31 ` Paul Cercueil @ 2021-05-22 13:09 ` Olivier Dautricourt 0 siblings, 0 replies; 5+ messages in thread From: Olivier Dautricourt @ 2021-05-22 13:09 UTC (permalink / raw) To: Paul Cercueil; +Cc: Vinod Koul, dmaengine 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 > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-05-22 13:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-21 18:15 Possible bug when using dmam_alloc_coherent in conjonction with of_reserved_mem_device_release Olivier Dautricourt 2021-05-22 10:28 ` Paul Cercueil 2021-05-22 11:42 ` Olivier Dautricourt 2021-05-22 12:31 ` Paul Cercueil 2021-05-22 13:09 ` Olivier Dautricourt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).