* Re: [Xenomai-help] Xenomai on ARMadeus @ 2009-11-12 14:34 Gwenhaël Goavec-Merou 0 siblings, 0 replies; 9+ messages in thread From: Gwenhaël Goavec-Merou @ 2009-11-12 14:34 UTC (permalink / raw) To: xenomai Sorry for the late > gwenhael.goavec wrote: >> On Mon, 02 Nov 2009 23:29:31 +0100 >> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >> wrote: >> >>> gwenhael.goavec wrote: >>>> On Mon, 02 Nov 2009 19:38:51 +0100 >>>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >>>> wrote: >>>> >>>>> By the way, did you make any change to the I-pipe patch to get it running on this board? >>>> The only change realized, is made because Armadeus and >>>> I-pipe does the same change on >>>> mach-imx/include/mach/imxfb.h : >>>> >>>> --- a/arch/arm/mach-imx/include/mach/imxfb.h >>>> +++ b/arch/arm/mach-imx/include/mach/imxfb.h >>>> @@ -14,7 +14,6 @@ >>>> #define PCR_BPIX_8 (3 << 25) >>>> #define PCR_BPIX_12 (4 << 25) >>>> #define PCR_BPIX_16 (4 << 25) >>>> +#define PCR_BPIX_MASK (7 << 25) >>>> #define PCR_PIXPOL (1 << 24) >>>> #define PCR_FLMPOL (1 << 23) >>>> #define PCR_LPPOL (1 << 22) >>>> >>>> I take this opportunity to ask a question: >>>> on APF27 with imx27, I found that the system freeze >>>> when an interrupt handler is in place and an >>>> interruption happens. >>>> if interrupt handler is in place without interruption no >>>> problems and if interruption happens without interrupt >>>> handler it's ok too. >>>> >>>> If someone could help or advise me. >>>> thank you very much > > The reason I knew for this problem was if handle_edge_irq was used for some interrupts. But problems of this kind for IMX GPIOs are fixed in 1.3-03. > > Do you have this problem with in-kernel or out-of-tree drivers? With real-time or non real-time interrupts? Could you show us > /proc/interrupts before the system freezes? > I use an RTDM real-time driver for testing GPIO interrupt don't appears in /proc/interrupts ~ # cat /proc/interrupts CPU0 20: 69 - IMX-uart 26: 1664 - i.MX Timer Tick 29: 84439 - mxc_nd 50: 75 - fec 53: 0 - VPU_CODEC_IRQ Err: 0 Gwenhael Signed-off-by: gwenhael.goavec-merou@domain.hid ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Xenomai-core] [PATCH v3 0/9] heap setup/cleanup fixes, refactorings & more @ 2009-10-20 11:37 Jan Kiszka 2009-10-20 11:37 ` [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2009-10-20 11:37 UTC (permalink / raw) To: Philippe Gerum; +Cc: xenomai Third round of this series. This time I dug deeper into the heap management, trying to simplify its use which should remove a few race conditions of the existing code. The central change is patch 5: xnheap_destroy_mapped will no longer return an error, it will always start deferred deletion in case the heap's memory is still in use. This simplifies the use a lot, allowing among other things to drop -EBUSY from the list of possible return codes of rt_queue/heap_delete. The series furthermore contains an attempt to fix RTAI's shm code, fixes the know leakages of rt_mutex/queue/heap auto-deletion and introduces complete heap statistics (this time without using a Linux spin lock). Please pull the series (or cherry-pick individual patches) from git://xenomai.org/xenomai-jki.git for-upstream if there are no concerns. Jan Kiszka (9): native: Release fastlock to the proper heap nucleus: Use Linux spin lock for heap list management nucleus: Fix race window in heap mapping procedure nucleus: xnheap_destroy does not fail nucleus: Avoid returning errors from xnheap_destroy_mapped rtai: Try to fix _shm_free native: Do not requeue on auto-cleanup errors native: Fix memory leak on heap/queue auto-deletion nucleus: Include all heaps in statistics include/asm-generic/bits/heap.h | 2 +- include/asm-generic/system.h | 2 +- include/native/ppd.h | 16 +-- include/nucleus/heap.h | 32 +++-- ksrc/drivers/ipc/iddp.c | 3 +- ksrc/drivers/ipc/xddp.c | 6 +- ksrc/nucleus/heap.c | 258 +++++++++++++++++++++++++++------------ ksrc/nucleus/module.c | 2 +- ksrc/nucleus/pod.c | 5 +- ksrc/nucleus/shadow.c | 5 +- ksrc/skins/native/heap.c | 41 +++--- ksrc/skins/native/mutex.c | 14 ++- ksrc/skins/native/pipe.c | 4 +- ksrc/skins/native/queue.c | 34 +++--- ksrc/skins/native/syscall.c | 25 +--- ksrc/skins/posix/shm.c | 4 +- ksrc/skins/psos+/rn.c | 6 +- ksrc/skins/rtai/shm.c | 47 ++++--- ksrc/skins/vrtx/heap.c | 6 +- ksrc/skins/vrtx/syscall.c | 3 +- 20 files changed, 317 insertions(+), 198 deletions(-) [1] http://thread.gmane.org/gmane.linux.real-time.xenomai.devel/6559 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-10-20 11:37 [Xenomai-core] [PATCH v3 0/9] heap setup/cleanup fixes, refactorings & more Jan Kiszka @ 2009-10-20 11:37 ` Jan Kiszka 2009-10-24 17:22 ` Philippe Gerum 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2009-10-20 11:37 UTC (permalink / raw) To: Philippe Gerum; +Cc: xenomai Allowing xnheap_delete_mapped to return an error and then attempting to recover from it does not work out very well: Corner cases are racy, intransparent to the user, and proper error handling imposes a lot of complexity on the caller - if it actually bothers to check the return value... Fortunately, there is no reason for this function to fail: If the heap is still mapped, just install the provide cleanup handler and switch to deferred removal. If the unmapping fails, we either raced with some other caller of unmap or user space provided a bogus address, or something else is wrong. In any case, leaving the cleanup callback behind is the best we can do anyway. Removing the return value immediately allows to simplify the callers, namemly rt_queue_delete and rt_heap_delete. Note: This is still not 100% waterproof. If we issue xnheap_destroy_mapped from module cleanup passing a release handler that belongs to the module text, deferred release will cause a crash. But this corner case is no new regression, so let's keep the head in the sand. Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid> --- include/nucleus/heap.h | 6 +++--- ksrc/nucleus/heap.c | 45 +++++++++++++++++++++++++++------------------ ksrc/skins/native/heap.c | 21 ++++++--------------- ksrc/skins/native/queue.c | 21 ++++++--------------- ksrc/skins/rtai/shm.c | 6 +----- 5 files changed, 43 insertions(+), 56 deletions(-) diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h index ca691bf..44db738 100644 --- a/include/nucleus/heap.h +++ b/include/nucleus/heap.h @@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags); -int xnheap_destroy_mapped(xnheap_t *heap, - void (*release)(struct xnheap *heap), - void __user *mapaddr); +void xnheap_destroy_mapped(xnheap_t *heap, + void (*release)(struct xnheap *heap), + void __user *mapaddr); #define xnheap_mapped_offset(heap,ptr) \ (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase)) diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c index 4958daa..96c46f8 100644 --- a/ksrc/nucleus/heap.c +++ b/ksrc/nucleus/heap.c @@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) return 0; } -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), - void __user *mapaddr) +void xnheap_destroy_mapped(xnheap_t *heap, + void (*release)(struct xnheap *heap), + void __user *mapaddr) { - int ret = 0, ccheck; unsigned long len; - ccheck = mapaddr ? 1 : 0; + /* + * Trying to unmap user memory without providing a release handler for + * deferred cleanup is a bug. + */ + XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */); spin_lock(&kheapq_lock); - if (heap->archdep.numaps > ccheck) { - heap->archdep.release = release; - spin_unlock(&kheapq_lock); - return -EBUSY; - } - removeq(&kheapq, &heap->link); /* Prevent further mapping. */ + + heap->archdep.release = release; + + if (heap->archdep.numaps == 0) + mapaddr = NULL; /* nothing left to unmap */ + else + release = NULL; /* will be called by Linux on unmap */ + spin_unlock(&kheapq_lock); len = xnheap_extentsize(heap); if (mapaddr) { down_write(¤t->mm->mmap_sem); - heap->archdep.release = NULL; - ret = do_munmap(current->mm, (unsigned long)mapaddr, len); + do_munmap(current->mm, (unsigned long)mapaddr, len); up_write(¤t->mm->mmap_sem); } - if (ret == 0) { + if (heap->archdep.numaps > 0) { + /* The release handler is supposed to clean up the rest. */ + XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */); + return; + } + + if (!mapaddr) { __unreserve_and_free_heap(heap->archdep.heapbase, len, heap->archdep.kmflags); if (release) release(heap); } - - return ret; } static struct file_operations xnheap_fops = { @@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) return -ENOMEM; } -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), - void __user *mapaddr) +void xnheap_destroy_mapped(xnheap_t *heap, + void (*release)(struct xnheap *heap), + void __user *mapaddr) { xnheap_destroy(heap, &xnheap_free_extent, NULL); - return 0; } #endif /* !CONFIG_XENO_OPT_PERVASIVE */ diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c index 0d7ad83..f7411e8 100644 --- a/ksrc/skins/native/heap.c +++ b/ksrc/skins/native/heap.c @@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h) * * @return 0 is returned upon success. Otherwise: * - * - -EBUSY is returned if @a heap is in use by another process and the - * descriptor is not destroyed. - * * - -EINVAL is returned if @a heap is not a heap descriptor. * * - -EIDRM is returned if @a heap is a deleted heap descriptor. @@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h) int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) { - int err = 0; + int err; spl_t s; if (!xnpod_root_p()) @@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) #ifdef CONFIG_XENO_OPT_PERVASIVE if (heap->mode & H_MAPPABLE) - err = xnheap_destroy_mapped(&heap->heap_base, - __heap_post_release, mapaddr); + xnheap_destroy_mapped(&heap->heap_base, + __heap_post_release, mapaddr); else #endif /* CONFIG_XENO_OPT_PERVASIVE */ + { xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL); - - xnlock_get_irqsave(&nklock, s); - - if (err) - heap->magic = XENO_HEAP_MAGIC; - else if (!(heap->mode & H_MAPPABLE)) __heap_post_release(&heap->heap_base); + } - xnlock_put_irqrestore(&nklock, s); - - return err; + return 0; } int rt_heap_delete(RT_HEAP *heap) diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c index f913675..3592a4a 100644 --- a/ksrc/skins/native/queue.c +++ b/ksrc/skins/native/queue.c @@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap) * - -EPERM is returned if this service was called from an * asynchronous context. * - * - -EBUSY is returned if an attempt is made to delete a shared queue - * which is still bound to a process. - * * Environments: * * This service can be called from: @@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap) int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) { - int err = 0; + int err; spl_t s; if (xnpod_asynch_p()) @@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) #ifdef CONFIG_XENO_OPT_PERVASIVE if (q->mode & Q_SHARED) - err = xnheap_destroy_mapped(&q->bufpool, - __queue_post_release, mapaddr); + xnheap_destroy_mapped(&q->bufpool, + __queue_post_release, mapaddr); else #endif /* CONFIG_XENO_OPT_PERVASIVE */ + { xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL); - - xnlock_get_irqsave(&nklock, s); - - if (err) - q->magic = XENO_QUEUE_MAGIC; - else if (!(q->mode & Q_SHARED)) __queue_post_release(&q->bufpool); + } - xnlock_put_irqrestore(&nklock, s); - - return err; + return 0; } int rt_queue_delete(RT_QUEUE *q) diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c index fddf455..4c56495 100644 --- a/ksrc/skins/rtai/shm.c +++ b/ksrc/skins/rtai/shm.c @@ -293,13 +293,11 @@ static int _shm_free(unsigned long name) * [YES!] */ #ifdef CONFIG_XENO_OPT_PERVASIVE - ret = xnheap_destroy_mapped(p->heap, NULL, NULL); + xnheap_destroy_mapped(p->heap, NULL, NULL); #else /* !CONFIG_XENO_OPT_PERVASIVE */ xnheap_destroy(p->heap, &__heap_flush_private, NULL); #endif /* !CONFIG_XENO_OPT_PERVASIVE */ - if (ret) - goto unlock_and_exit; xnheap_free(&kheap, p->heap); } removeq(&xnshm_allocq, &p->link); @@ -311,8 +309,6 @@ static int _shm_free(unsigned long name) holder = nextq(&xnshm_allocq, holder); } - unlock_and_exit: - xnlock_put_irqrestore(&nklock, s); return ret; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-10-20 11:37 ` [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped Jan Kiszka @ 2009-10-24 17:22 ` Philippe Gerum 2009-11-02 16:04 ` Philippe Gerum 0 siblings, 1 reply; 9+ messages in thread From: Philippe Gerum @ 2009-10-24 17:22 UTC (permalink / raw) To: Jan Kiszka; +Cc: xenomai On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: > Allowing xnheap_delete_mapped to return an error and then attempting to > recover from it does not work out very well: Corner cases are racy, > intransparent to the user, and proper error handling imposes a lot of > complexity on the caller - if it actually bothers to check the return > value... > > Fortunately, there is no reason for this function to fail: If the heap > is still mapped, just install the provide cleanup handler and switch to > deferred removal. If the unmapping fails, we either raced with some > other caller of unmap or user space provided a bogus address, or > something else is wrong. In any case, leaving the cleanup callback > behind is the best we can do anyway. > > Removing the return value immediately allows to simplify the callers, > namemly rt_queue_delete and rt_heap_delete. > > Note: This is still not 100% waterproof. If we issue > xnheap_destroy_mapped from module cleanup passing a release handler > that belongs to the module text, deferred release will cause a crash. > But this corner case is no new regression, so let's keep the head in the > sand. I agree with this one, eventually. This does make things clearer, and removes some opportunities for the upper interfaces to shot themselves in the foot. Merged, thanks. > > Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid> > --- > > include/nucleus/heap.h | 6 +++--- > ksrc/nucleus/heap.c | 45 +++++++++++++++++++++++++++------------------ > ksrc/skins/native/heap.c | 21 ++++++--------------- > ksrc/skins/native/queue.c | 21 ++++++--------------- > ksrc/skins/rtai/shm.c | 6 +----- > 5 files changed, 43 insertions(+), 56 deletions(-) > > diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h > index ca691bf..44db738 100644 > --- a/include/nucleus/heap.h > +++ b/include/nucleus/heap.h > @@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap, > u_long heapsize, > int memflags); > > -int xnheap_destroy_mapped(xnheap_t *heap, > - void (*release)(struct xnheap *heap), > - void __user *mapaddr); > +void xnheap_destroy_mapped(xnheap_t *heap, > + void (*release)(struct xnheap *heap), > + void __user *mapaddr); > > #define xnheap_mapped_offset(heap,ptr) \ > (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase)) > diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c > index 4958daa..96c46f8 100644 > --- a/ksrc/nucleus/heap.c > +++ b/ksrc/nucleus/heap.c > @@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) > return 0; > } > > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), > - void __user *mapaddr) > +void xnheap_destroy_mapped(xnheap_t *heap, > + void (*release)(struct xnheap *heap), > + void __user *mapaddr) > { > - int ret = 0, ccheck; > unsigned long len; > > - ccheck = mapaddr ? 1 : 0; > + /* > + * Trying to unmap user memory without providing a release handler for > + * deferred cleanup is a bug. > + */ > + XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */); > > spin_lock(&kheapq_lock); > > - if (heap->archdep.numaps > ccheck) { > - heap->archdep.release = release; > - spin_unlock(&kheapq_lock); > - return -EBUSY; > - } > - > removeq(&kheapq, &heap->link); /* Prevent further mapping. */ > + > + heap->archdep.release = release; > + > + if (heap->archdep.numaps == 0) > + mapaddr = NULL; /* nothing left to unmap */ > + else > + release = NULL; /* will be called by Linux on unmap */ > + > spin_unlock(&kheapq_lock); > > len = xnheap_extentsize(heap); > > if (mapaddr) { > down_write(¤t->mm->mmap_sem); > - heap->archdep.release = NULL; > - ret = do_munmap(current->mm, (unsigned long)mapaddr, len); > + do_munmap(current->mm, (unsigned long)mapaddr, len); > up_write(¤t->mm->mmap_sem); > } > > - if (ret == 0) { > + if (heap->archdep.numaps > 0) { > + /* The release handler is supposed to clean up the rest. */ > + XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */); > + return; > + } > + > + if (!mapaddr) { > __unreserve_and_free_heap(heap->archdep.heapbase, len, > heap->archdep.kmflags); > if (release) > release(heap); > } > - > - return ret; > } > > static struct file_operations xnheap_fops = { > @@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) > return -ENOMEM; > } > > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), > - void __user *mapaddr) > +void xnheap_destroy_mapped(xnheap_t *heap, > + void (*release)(struct xnheap *heap), > + void __user *mapaddr) > { > xnheap_destroy(heap, &xnheap_free_extent, NULL); > - return 0; > } > #endif /* !CONFIG_XENO_OPT_PERVASIVE */ > > diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c > index 0d7ad83..f7411e8 100644 > --- a/ksrc/skins/native/heap.c > +++ b/ksrc/skins/native/heap.c > @@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h) > * > * @return 0 is returned upon success. Otherwise: > * > - * - -EBUSY is returned if @a heap is in use by another process and the > - * descriptor is not destroyed. > - * > * - -EINVAL is returned if @a heap is not a heap descriptor. > * > * - -EIDRM is returned if @a heap is a deleted heap descriptor. > @@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h) > > int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) > { > - int err = 0; > + int err; > spl_t s; > > if (!xnpod_root_p()) > @@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) > > #ifdef CONFIG_XENO_OPT_PERVASIVE > if (heap->mode & H_MAPPABLE) > - err = xnheap_destroy_mapped(&heap->heap_base, > - __heap_post_release, mapaddr); > + xnheap_destroy_mapped(&heap->heap_base, > + __heap_post_release, mapaddr); > else > #endif /* CONFIG_XENO_OPT_PERVASIVE */ > + { > xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL); > - > - xnlock_get_irqsave(&nklock, s); > - > - if (err) > - heap->magic = XENO_HEAP_MAGIC; > - else if (!(heap->mode & H_MAPPABLE)) > __heap_post_release(&heap->heap_base); > + } > > - xnlock_put_irqrestore(&nklock, s); > - > - return err; > + return 0; > } > > int rt_heap_delete(RT_HEAP *heap) > diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c > index f913675..3592a4a 100644 > --- a/ksrc/skins/native/queue.c > +++ b/ksrc/skins/native/queue.c > @@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap) > * - -EPERM is returned if this service was called from an > * asynchronous context. > * > - * - -EBUSY is returned if an attempt is made to delete a shared queue > - * which is still bound to a process. > - * > * Environments: > * > * This service can be called from: > @@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap) > > int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) > { > - int err = 0; > + int err; > spl_t s; > > if (xnpod_asynch_p()) > @@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) > > #ifdef CONFIG_XENO_OPT_PERVASIVE > if (q->mode & Q_SHARED) > - err = xnheap_destroy_mapped(&q->bufpool, > - __queue_post_release, mapaddr); > + xnheap_destroy_mapped(&q->bufpool, > + __queue_post_release, mapaddr); > else > #endif /* CONFIG_XENO_OPT_PERVASIVE */ > + { > xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL); > - > - xnlock_get_irqsave(&nklock, s); > - > - if (err) > - q->magic = XENO_QUEUE_MAGIC; > - else if (!(q->mode & Q_SHARED)) > __queue_post_release(&q->bufpool); > + } > > - xnlock_put_irqrestore(&nklock, s); > - > - return err; > + return 0; > } > > int rt_queue_delete(RT_QUEUE *q) > diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c > index fddf455..4c56495 100644 > --- a/ksrc/skins/rtai/shm.c > +++ b/ksrc/skins/rtai/shm.c > @@ -293,13 +293,11 @@ static int _shm_free(unsigned long name) > * [YES!] > */ > #ifdef CONFIG_XENO_OPT_PERVASIVE > - ret = xnheap_destroy_mapped(p->heap, NULL, NULL); > + xnheap_destroy_mapped(p->heap, NULL, NULL); > #else /* !CONFIG_XENO_OPT_PERVASIVE */ > xnheap_destroy(p->heap, > &__heap_flush_private, NULL); > #endif /* !CONFIG_XENO_OPT_PERVASIVE */ > - if (ret) > - goto unlock_and_exit; > xnheap_free(&kheap, p->heap); > } > removeq(&xnshm_allocq, &p->link); > @@ -311,8 +309,6 @@ static int _shm_free(unsigned long name) > holder = nextq(&xnshm_allocq, holder); > } > > - unlock_and_exit: > - > xnlock_put_irqrestore(&nklock, s); > > return ret; > -- Philippe. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-10-24 17:22 ` Philippe Gerum @ 2009-11-02 16:04 ` Philippe Gerum 2009-11-02 16:41 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Philippe Gerum @ 2009-11-02 16:04 UTC (permalink / raw) To: Jan Kiszka; +Cc: xenomai On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: > On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: > > Allowing xnheap_delete_mapped to return an error and then attempting to > > recover from it does not work out very well: Corner cases are racy, > > intransparent to the user, and proper error handling imposes a lot of > > complexity on the caller - if it actually bothers to check the return > > value... > > > > Fortunately, there is no reason for this function to fail: If the heap > > is still mapped, just install the provide cleanup handler and switch to > > deferred removal. If the unmapping fails, we either raced with some > > other caller of unmap or user space provided a bogus address, or > > something else is wrong. In any case, leaving the cleanup callback > > behind is the best we can do anyway. > > > > Removing the return value immediately allows to simplify the callers, > > namemly rt_queue_delete and rt_heap_delete. > > > > Note: This is still not 100% waterproof. If we issue > > xnheap_destroy_mapped from module cleanup passing a release handler > > that belongs to the module text, deferred release will cause a crash. > > But this corner case is no new regression, so let's keep the head in the > > sand. > > I agree with this one, eventually. This does make things clearer, and > removes some opportunities for the upper interfaces to shot themselves > in the foot. Merged, thanks. Well, actually, it does make things clearer, but it is broken. Enabling list debugging makes the nucleus pull the break after a double unlink in vmclose(). Basically, the issue is that calling rt_queue/heap_delete() explicitly from userland will break, due to the vmclose() handler being indirectly called by do_munmap() for the last mapping. The nasty thing is that without debugs on, kheapq is just silently trashed. Fix is on its way, along with nommu support for shared heaps as well. > > > > Signed-off-by: Jan Kiszka <jan.kiszka@domain.hid> > > --- > > > > include/nucleus/heap.h | 6 +++--- > > ksrc/nucleus/heap.c | 45 +++++++++++++++++++++++++++------------------ > > ksrc/skins/native/heap.c | 21 ++++++--------------- > > ksrc/skins/native/queue.c | 21 ++++++--------------- > > ksrc/skins/rtai/shm.c | 6 +----- > > 5 files changed, 43 insertions(+), 56 deletions(-) > > > > diff --git a/include/nucleus/heap.h b/include/nucleus/heap.h > > index ca691bf..44db738 100644 > > --- a/include/nucleus/heap.h > > +++ b/include/nucleus/heap.h > > @@ -204,9 +204,9 @@ int xnheap_init_mapped(xnheap_t *heap, > > u_long heapsize, > > int memflags); > > > > -int xnheap_destroy_mapped(xnheap_t *heap, > > - void (*release)(struct xnheap *heap), > > - void __user *mapaddr); > > +void xnheap_destroy_mapped(xnheap_t *heap, > > + void (*release)(struct xnheap *heap), > > + void __user *mapaddr); > > > > #define xnheap_mapped_offset(heap,ptr) \ > > (((caddr_t)(ptr)) - ((caddr_t)(heap)->archdep.heapbase)) > > diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c > > index 4958daa..96c46f8 100644 > > --- a/ksrc/nucleus/heap.c > > +++ b/ksrc/nucleus/heap.c > > @@ -1173,42 +1173,51 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) > > return 0; > > } > > > > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), > > - void __user *mapaddr) > > +void xnheap_destroy_mapped(xnheap_t *heap, > > + void (*release)(struct xnheap *heap), > > + void __user *mapaddr) > > { > > - int ret = 0, ccheck; > > unsigned long len; > > > > - ccheck = mapaddr ? 1 : 0; > > + /* > > + * Trying to unmap user memory without providing a release handler for > > + * deferred cleanup is a bug. > > + */ > > + XENO_ASSERT(NUCLEUS, !mapaddr || release, /* nop */); > > > > spin_lock(&kheapq_lock); > > > > - if (heap->archdep.numaps > ccheck) { > > - heap->archdep.release = release; > > - spin_unlock(&kheapq_lock); > > - return -EBUSY; > > - } > > - > > removeq(&kheapq, &heap->link); /* Prevent further mapping. */ > > + > > + heap->archdep.release = release; > > + > > + if (heap->archdep.numaps == 0) > > + mapaddr = NULL; /* nothing left to unmap */ > > + else > > + release = NULL; /* will be called by Linux on unmap */ > > + > > spin_unlock(&kheapq_lock); > > > > len = xnheap_extentsize(heap); > > > > if (mapaddr) { > > down_write(¤t->mm->mmap_sem); > > - heap->archdep.release = NULL; > > - ret = do_munmap(current->mm, (unsigned long)mapaddr, len); > > + do_munmap(current->mm, (unsigned long)mapaddr, len); > > up_write(¤t->mm->mmap_sem); > > } > > > > - if (ret == 0) { > > + if (heap->archdep.numaps > 0) { > > + /* The release handler is supposed to clean up the rest. */ > > + XENO_ASSERT(NUCLEUS, heap->archdep.release, /* nop */); > > + return; > > + } > > + > > + if (!mapaddr) { > > __unreserve_and_free_heap(heap->archdep.heapbase, len, > > heap->archdep.kmflags); > > if (release) > > release(heap); > > } > > - > > - return ret; > > } > > > > static struct file_operations xnheap_fops = { > > @@ -1260,11 +1269,11 @@ int xnheap_init_mapped(xnheap_t *heap, u_long heapsize, int memflags) > > return -ENOMEM; > > } > > > > -int xnheap_destroy_mapped(xnheap_t *heap, void (*release)(struct xnheap *heap), > > - void __user *mapaddr) > > +void xnheap_destroy_mapped(xnheap_t *heap, > > + void (*release)(struct xnheap *heap), > > + void __user *mapaddr) > > { > > xnheap_destroy(heap, &xnheap_free_extent, NULL); > > - return 0; > > } > > #endif /* !CONFIG_XENO_OPT_PERVASIVE */ > > > > diff --git a/ksrc/skins/native/heap.c b/ksrc/skins/native/heap.c > > index 0d7ad83..f7411e8 100644 > > --- a/ksrc/skins/native/heap.c > > +++ b/ksrc/skins/native/heap.c > > @@ -357,9 +357,6 @@ static void __heap_post_release(struct xnheap *h) > > * > > * @return 0 is returned upon success. Otherwise: > > * > > - * - -EBUSY is returned if @a heap is in use by another process and the > > - * descriptor is not destroyed. > > - * > > * - -EINVAL is returned if @a heap is not a heap descriptor. > > * > > * - -EIDRM is returned if @a heap is a deleted heap descriptor. > > @@ -379,7 +376,7 @@ static void __heap_post_release(struct xnheap *h) > > > > int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) > > { > > - int err = 0; > > + int err; > > spl_t s; > > > > if (!xnpod_root_p()) > > @@ -412,22 +409,16 @@ int rt_heap_delete_inner(RT_HEAP *heap, void __user *mapaddr) > > > > #ifdef CONFIG_XENO_OPT_PERVASIVE > > if (heap->mode & H_MAPPABLE) > > - err = xnheap_destroy_mapped(&heap->heap_base, > > - __heap_post_release, mapaddr); > > + xnheap_destroy_mapped(&heap->heap_base, > > + __heap_post_release, mapaddr); > > else > > #endif /* CONFIG_XENO_OPT_PERVASIVE */ > > + { > > xnheap_destroy(&heap->heap_base, &__heap_flush_private, NULL); > > - > > - xnlock_get_irqsave(&nklock, s); > > - > > - if (err) > > - heap->magic = XENO_HEAP_MAGIC; > > - else if (!(heap->mode & H_MAPPABLE)) > > __heap_post_release(&heap->heap_base); > > + } > > > > - xnlock_put_irqrestore(&nklock, s); > > - > > - return err; > > + return 0; > > } > > > > int rt_heap_delete(RT_HEAP *heap) > > diff --git a/ksrc/skins/native/queue.c b/ksrc/skins/native/queue.c > > index f913675..3592a4a 100644 > > --- a/ksrc/skins/native/queue.c > > +++ b/ksrc/skins/native/queue.c > > @@ -326,9 +326,6 @@ static void __queue_post_release(struct xnheap *heap) > > * - -EPERM is returned if this service was called from an > > * asynchronous context. > > * > > - * - -EBUSY is returned if an attempt is made to delete a shared queue > > - * which is still bound to a process. > > - * > > * Environments: > > * > > * This service can be called from: > > @@ -341,7 +338,7 @@ static void __queue_post_release(struct xnheap *heap) > > > > int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) > > { > > - int err = 0; > > + int err; > > spl_t s; > > > > if (xnpod_asynch_p()) > > @@ -373,22 +370,16 @@ int rt_queue_delete_inner(RT_QUEUE *q, void __user *mapaddr) > > > > #ifdef CONFIG_XENO_OPT_PERVASIVE > > if (q->mode & Q_SHARED) > > - err = xnheap_destroy_mapped(&q->bufpool, > > - __queue_post_release, mapaddr); > > + xnheap_destroy_mapped(&q->bufpool, > > + __queue_post_release, mapaddr); > > else > > #endif /* CONFIG_XENO_OPT_PERVASIVE */ > > + { > > xnheap_destroy(&q->bufpool, &__queue_flush_private, NULL); > > - > > - xnlock_get_irqsave(&nklock, s); > > - > > - if (err) > > - q->magic = XENO_QUEUE_MAGIC; > > - else if (!(q->mode & Q_SHARED)) > > __queue_post_release(&q->bufpool); > > + } > > > > - xnlock_put_irqrestore(&nklock, s); > > - > > - return err; > > + return 0; > > } > > > > int rt_queue_delete(RT_QUEUE *q) > > diff --git a/ksrc/skins/rtai/shm.c b/ksrc/skins/rtai/shm.c > > index fddf455..4c56495 100644 > > --- a/ksrc/skins/rtai/shm.c > > +++ b/ksrc/skins/rtai/shm.c > > @@ -293,13 +293,11 @@ static int _shm_free(unsigned long name) > > * [YES!] > > */ > > #ifdef CONFIG_XENO_OPT_PERVASIVE > > - ret = xnheap_destroy_mapped(p->heap, NULL, NULL); > > + xnheap_destroy_mapped(p->heap, NULL, NULL); > > #else /* !CONFIG_XENO_OPT_PERVASIVE */ > > xnheap_destroy(p->heap, > > &__heap_flush_private, NULL); > > #endif /* !CONFIG_XENO_OPT_PERVASIVE */ > > - if (ret) > > - goto unlock_and_exit; > > xnheap_free(&kheap, p->heap); > > } > > removeq(&xnshm_allocq, &p->link); > > @@ -311,8 +309,6 @@ static int _shm_free(unsigned long name) > > holder = nextq(&xnshm_allocq, holder); > > } > > > > - unlock_and_exit: > > - > > xnlock_put_irqrestore(&nklock, s); > > > > return ret; > > -- Philippe. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-11-02 16:04 ` Philippe Gerum @ 2009-11-02 16:41 ` Jan Kiszka 2009-11-02 16:51 ` Philippe Gerum 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2009-11-02 16:41 UTC (permalink / raw) To: Philippe Gerum; +Cc: xenomai Philippe Gerum wrote: > On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: >> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: >>> Allowing xnheap_delete_mapped to return an error and then attempting to >>> recover from it does not work out very well: Corner cases are racy, >>> intransparent to the user, and proper error handling imposes a lot of >>> complexity on the caller - if it actually bothers to check the return >>> value... >>> >>> Fortunately, there is no reason for this function to fail: If the heap >>> is still mapped, just install the provide cleanup handler and switch to >>> deferred removal. If the unmapping fails, we either raced with some >>> other caller of unmap or user space provided a bogus address, or >>> something else is wrong. In any case, leaving the cleanup callback >>> behind is the best we can do anyway. >>> >>> Removing the return value immediately allows to simplify the callers, >>> namemly rt_queue_delete and rt_heap_delete. >>> >>> Note: This is still not 100% waterproof. If we issue >>> xnheap_destroy_mapped from module cleanup passing a release handler >>> that belongs to the module text, deferred release will cause a crash. >>> But this corner case is no new regression, so let's keep the head in the >>> sand. >> I agree with this one, eventually. This does make things clearer, and >> removes some opportunities for the upper interfaces to shot themselves >> in the foot. Merged, thanks. > > Well, actually, it does make things clearer, but it is broken. Enabling > list debugging makes the nucleus pull the break after a double unlink in > vmclose(). > > Basically, the issue is that calling rt_queue/heap_delete() explicitly > from userland will break, due to the vmclose() handler being indirectly > called by do_munmap() for the last mapping. The nasty thing is that > without debugs on, kheapq is just silently trashed. > > Fix is on its way, along with nommu support for shared heaps as well. OK, I see. Just on minor add-on to your fix: diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c index ec14f73..1ae6af6 100644 --- a/ksrc/nucleus/heap.c +++ b/ksrc/nucleus/heap.c @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap, down_write(¤t->mm->mmap_sem); heap->archdep.release = NULL; do_munmap(current->mm, (unsigned long)mapaddr, len); + heap->archdep.release = release; up_write(¤t->mm->mmap_sem); } @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap, if (heap->archdep.numaps > 0) { /* The release handler is supposed to clean up the rest. */ XENO_ASSERT(NUCLEUS, release != NULL, /* nop */); - heap->archdep.release = release; return; } This is safer than leaving a potential race window open between dropping mmap_sem and fixing up archdep.release again. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-11-02 16:41 ` Jan Kiszka @ 2009-11-02 16:51 ` Philippe Gerum 2009-11-02 16:57 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Philippe Gerum @ 2009-11-02 16:51 UTC (permalink / raw) To: Jan Kiszka; +Cc: xenomai On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote: > Philippe Gerum wrote: > > On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: > >> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: > >>> Allowing xnheap_delete_mapped to return an error and then attempting to > >>> recover from it does not work out very well: Corner cases are racy, > >>> intransparent to the user, and proper error handling imposes a lot of > >>> complexity on the caller - if it actually bothers to check the return > >>> value... > >>> > >>> Fortunately, there is no reason for this function to fail: If the heap > >>> is still mapped, just install the provide cleanup handler and switch to > >>> deferred removal. If the unmapping fails, we either raced with some > >>> other caller of unmap or user space provided a bogus address, or > >>> something else is wrong. In any case, leaving the cleanup callback > >>> behind is the best we can do anyway. > >>> > >>> Removing the return value immediately allows to simplify the callers, > >>> namemly rt_queue_delete and rt_heap_delete. > >>> > >>> Note: This is still not 100% waterproof. If we issue > >>> xnheap_destroy_mapped from module cleanup passing a release handler > >>> that belongs to the module text, deferred release will cause a crash. > >>> But this corner case is no new regression, so let's keep the head in the > >>> sand. > >> I agree with this one, eventually. This does make things clearer, and > >> removes some opportunities for the upper interfaces to shot themselves > >> in the foot. Merged, thanks. > > > > Well, actually, it does make things clearer, but it is broken. Enabling > > list debugging makes the nucleus pull the break after a double unlink in > > vmclose(). > > > > Basically, the issue is that calling rt_queue/heap_delete() explicitly > > from userland will break, due to the vmclose() handler being indirectly > > called by do_munmap() for the last mapping. The nasty thing is that > > without debugs on, kheapq is just silently trashed. > > > > Fix is on its way, along with nommu support for shared heaps as well. > > OK, I see. Just on minor add-on to your fix: > > diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c > index ec14f73..1ae6af6 100644 > --- a/ksrc/nucleus/heap.c > +++ b/ksrc/nucleus/heap.c > @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap, > down_write(¤t->mm->mmap_sem); > heap->archdep.release = NULL; > do_munmap(current->mm, (unsigned long)mapaddr, len); > + heap->archdep.release = release; > up_write(¤t->mm->mmap_sem); > } > > @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap, > if (heap->archdep.numaps > 0) { > /* The release handler is supposed to clean up the rest. */ > XENO_ASSERT(NUCLEUS, release != NULL, /* nop */); > - heap->archdep.release = release; > return; > } > > > This is safer than leaving a potential race window open between dropping > mmap_sem and fixing up archdep.release again. > Actually, we have to hold the kheap lock, in case weird code starts mapping randomly from userland without getting a valid descriptor through a skin call. > Jan > -- Philippe. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-11-02 16:51 ` Philippe Gerum @ 2009-11-02 16:57 ` Jan Kiszka 2009-11-02 18:01 ` Jan Kiszka 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2009-11-02 16:57 UTC (permalink / raw) To: Philippe Gerum; +Cc: xenomai Philippe Gerum wrote: > On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote: >> Philippe Gerum wrote: >>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: >>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: >>>>> Allowing xnheap_delete_mapped to return an error and then attempting to >>>>> recover from it does not work out very well: Corner cases are racy, >>>>> intransparent to the user, and proper error handling imposes a lot of >>>>> complexity on the caller - if it actually bothers to check the return >>>>> value... >>>>> >>>>> Fortunately, there is no reason for this function to fail: If the heap >>>>> is still mapped, just install the provide cleanup handler and switch to >>>>> deferred removal. If the unmapping fails, we either raced with some >>>>> other caller of unmap or user space provided a bogus address, or >>>>> something else is wrong. In any case, leaving the cleanup callback >>>>> behind is the best we can do anyway. >>>>> >>>>> Removing the return value immediately allows to simplify the callers, >>>>> namemly rt_queue_delete and rt_heap_delete. >>>>> >>>>> Note: This is still not 100% waterproof. If we issue >>>>> xnheap_destroy_mapped from module cleanup passing a release handler >>>>> that belongs to the module text, deferred release will cause a crash. >>>>> But this corner case is no new regression, so let's keep the head in the >>>>> sand. >>>> I agree with this one, eventually. This does make things clearer, and >>>> removes some opportunities for the upper interfaces to shot themselves >>>> in the foot. Merged, thanks. >>> Well, actually, it does make things clearer, but it is broken. Enabling >>> list debugging makes the nucleus pull the break after a double unlink in >>> vmclose(). >>> >>> Basically, the issue is that calling rt_queue/heap_delete() explicitly >>> from userland will break, due to the vmclose() handler being indirectly >>> called by do_munmap() for the last mapping. The nasty thing is that >>> without debugs on, kheapq is just silently trashed. >>> >>> Fix is on its way, along with nommu support for shared heaps as well. >> OK, I see. Just on minor add-on to your fix: >> >> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c >> index ec14f73..1ae6af6 100644 >> --- a/ksrc/nucleus/heap.c >> +++ b/ksrc/nucleus/heap.c >> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap, >> down_write(¤t->mm->mmap_sem); >> heap->archdep.release = NULL; >> do_munmap(current->mm, (unsigned long)mapaddr, len); >> + heap->archdep.release = release; >> up_write(¤t->mm->mmap_sem); >> } >> >> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap, >> if (heap->archdep.numaps > 0) { >> /* The release handler is supposed to clean up the rest. */ >> XENO_ASSERT(NUCLEUS, release != NULL, /* nop */); >> - heap->archdep.release = release; >> return; >> } >> >> >> This is safer than leaving a potential race window open between dropping >> mmap_sem and fixing up archdep.release again. >> > > Actually, we have to hold the kheap lock, in case weird code starts > mapping randomly from userland without getting a valid descriptor > through a skin call. Yep, that as well. Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped 2009-11-02 16:57 ` Jan Kiszka @ 2009-11-02 18:01 ` Jan Kiszka 2009-11-02 18:19 ` [Xenomai-help] Xenomai on ARMadeus Pierre Ficheux 0 siblings, 1 reply; 9+ messages in thread From: Jan Kiszka @ 2009-11-02 18:01 UTC (permalink / raw) To: Philippe Gerum; +Cc: xenomai Jan Kiszka wrote: > Philippe Gerum wrote: >> On Mon, 2009-11-02 at 17:41 +0100, Jan Kiszka wrote: >>> Philippe Gerum wrote: >>>> On Sat, 2009-10-24 at 19:22 +0200, Philippe Gerum wrote: >>>>> On Tue, 2009-10-20 at 13:37 +0200, Jan Kiszka wrote: >>>>>> Allowing xnheap_delete_mapped to return an error and then attempting to >>>>>> recover from it does not work out very well: Corner cases are racy, >>>>>> intransparent to the user, and proper error handling imposes a lot of >>>>>> complexity on the caller - if it actually bothers to check the return >>>>>> value... >>>>>> >>>>>> Fortunately, there is no reason for this function to fail: If the heap >>>>>> is still mapped, just install the provide cleanup handler and switch to >>>>>> deferred removal. If the unmapping fails, we either raced with some >>>>>> other caller of unmap or user space provided a bogus address, or >>>>>> something else is wrong. In any case, leaving the cleanup callback >>>>>> behind is the best we can do anyway. >>>>>> >>>>>> Removing the return value immediately allows to simplify the callers, >>>>>> namemly rt_queue_delete and rt_heap_delete. >>>>>> >>>>>> Note: This is still not 100% waterproof. If we issue >>>>>> xnheap_destroy_mapped from module cleanup passing a release handler >>>>>> that belongs to the module text, deferred release will cause a crash. >>>>>> But this corner case is no new regression, so let's keep the head in the >>>>>> sand. >>>>> I agree with this one, eventually. This does make things clearer, and >>>>> removes some opportunities for the upper interfaces to shot themselves >>>>> in the foot. Merged, thanks. >>>> Well, actually, it does make things clearer, but it is broken. Enabling >>>> list debugging makes the nucleus pull the break after a double unlink in >>>> vmclose(). >>>> >>>> Basically, the issue is that calling rt_queue/heap_delete() explicitly >>>> from userland will break, due to the vmclose() handler being indirectly >>>> called by do_munmap() for the last mapping. The nasty thing is that >>>> without debugs on, kheapq is just silently trashed. >>>> >>>> Fix is on its way, along with nommu support for shared heaps as well. >>> OK, I see. Just on minor add-on to your fix: >>> >>> diff --git a/ksrc/nucleus/heap.c b/ksrc/nucleus/heap.c >>> index ec14f73..1ae6af6 100644 >>> --- a/ksrc/nucleus/heap.c >>> +++ b/ksrc/nucleus/heap.c >>> @@ -1241,6 +1241,7 @@ void xnheap_destroy_mapped(xnheap_t *heap, >>> down_write(¤t->mm->mmap_sem); >>> heap->archdep.release = NULL; >>> do_munmap(current->mm, (unsigned long)mapaddr, len); >>> + heap->archdep.release = release; >>> up_write(¤t->mm->mmap_sem); >>> } >>> >>> @@ -1252,7 +1253,6 @@ void xnheap_destroy_mapped(xnheap_t *heap, >>> if (heap->archdep.numaps > 0) { >>> /* The release handler is supposed to clean up the rest. */ >>> XENO_ASSERT(NUCLEUS, release != NULL, /* nop */); >>> - heap->archdep.release = release; >>> return; >>> } >>> >>> >>> This is safer than leaving a potential race window open between dropping >>> mmap_sem and fixing up archdep.release again. >>> >> Actually, we have to hold the kheap lock, in case weird code starts >> mapping randomly from userland without getting a valid descriptor >> through a skin call. > > Yep, that as well. > Note that 6b1a185b46 doesn't obsolete my patch (pull it from my tree if you like). Jan -- Siemens AG, Corporate Technology, CT SE 2 Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Xenomai-help] Xenomai on ARMadeus 2009-11-02 18:01 ` Jan Kiszka @ 2009-11-02 18:19 ` Pierre Ficheux 2009-11-02 18:22 ` Gilles Chanteperdrix 2009-11-02 18:38 ` Gilles Chanteperdrix 0 siblings, 2 replies; 9+ messages in thread From: Pierre Ficheux @ 2009-11-02 18:19 UTC (permalink / raw) To: Xenomai-help Hi, I've just tried to use Xenomai on ARMadeus board (APF9328, kernel 2.6.29). I got the following message when starting "latency" : Any idea? # /usr/xenomai/bin/latency == Sampling period: 100 us == Test mode: periodic user-mode task == All results in microseconds warming up... Unhandled fault: external abort on non-linefetch (0x01a) at 0x40006010 # uname -a Linux armadeus 2.6.29.6 #1 PREEMPT Fri Oct 30 13:11:26 CET 2009 armv4tl unknown # cat /proc/ipipe/version 1.13-00 Looks like there was a discussion about a similar problem some days ago with Gilles (Freescale CPU too). http://www.mail-archive.com/xenomai@xenomai.org Thx, regards -- Pierre FICHEUX -/- CTO OW/OS4I, France -\- pierre.ficheux@domain.hid http://www.os4i.com http://www.ficheux.org I would love to change the world, but they won't give me the source code ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-help] Xenomai on ARMadeus 2009-11-02 18:19 ` [Xenomai-help] Xenomai on ARMadeus Pierre Ficheux @ 2009-11-02 18:22 ` Gilles Chanteperdrix 2009-11-02 18:38 ` Gilles Chanteperdrix 1 sibling, 0 replies; 9+ messages in thread From: Gilles Chanteperdrix @ 2009-11-02 18:22 UTC (permalink / raw) To: Pierre Ficheux; +Cc: Xenomai-help Pierre Ficheux wrote: > Hi, > > I've just tried to use Xenomai on ARMadeus board (APF9328, kernel > 2.6.29). I got the following message when starting "latency" : > > Any idea? > > # /usr/xenomai/bin/latency > == Sampling period: 100 us > == Test mode: periodic user-mode task > == All results in microseconds > warming up... > Unhandled fault: external abort on non-linefetch (0x01a) at 0x40006010 > > > # uname -a > Linux armadeus 2.6.29.6 #1 PREEMPT Fri Oct 30 13:11:26 CET 2009 armv4tl > unknown > > # cat /proc/ipipe/version > 1.13-00 > > > Looks like there was a discussion about a similar problem some days ago > with Gilles (Freescale CPU too). > > http://www.mail-archive.com/xenomai@xenomai.org Yes, the result of which is that you need some code to allow code not running in supervisor mode to access I/O addresses. You can also configure xenomai with --enable-arm-mach=generic, which will avoid access to I/Os from user-space. -- Gilles. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-help] Xenomai on ARMadeus 2009-11-02 18:19 ` [Xenomai-help] Xenomai on ARMadeus Pierre Ficheux 2009-11-02 18:22 ` Gilles Chanteperdrix @ 2009-11-02 18:38 ` Gilles Chanteperdrix 2009-11-02 19:19 ` gwenhael.goavec 1 sibling, 1 reply; 9+ messages in thread From: Gilles Chanteperdrix @ 2009-11-02 18:38 UTC (permalink / raw) To: Pierre Ficheux; +Cc: Xenomai-help Pierre Ficheux wrote: > Hi, > > I've just tried to use Xenomai on ARMadeus board (APF9328, kernel > 2.6.29). By the way, did you make any change to the I-pipe patch to get it running on this board? -- Gilles. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-help] Xenomai on ARMadeus 2009-11-02 18:38 ` Gilles Chanteperdrix @ 2009-11-02 19:19 ` gwenhael.goavec 2009-11-02 22:29 ` Gilles Chanteperdrix 0 siblings, 1 reply; 9+ messages in thread From: gwenhael.goavec @ 2009-11-02 19:19 UTC (permalink / raw) To: xenomai On Mon, 02 Nov 2009 19:38:51 +0100 Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> wrote: > By the way, did you make any change to the I-pipe patch to get it > running on this board? The only change realized, is made because Armadeus and I-pipe does the same change on mach-imx/include/mach/imxfb.h : --- a/arch/arm/mach-imx/include/mach/imxfb.h +++ b/arch/arm/mach-imx/include/mach/imxfb.h @@ -14,7 +14,6 @@ #define PCR_BPIX_8 (3 << 25) #define PCR_BPIX_12 (4 << 25) #define PCR_BPIX_16 (4 << 25) +#define PCR_BPIX_MASK (7 << 25) #define PCR_PIXPOL (1 << 24) #define PCR_FLMPOL (1 << 23) #define PCR_LPPOL (1 << 22) I take this opportunity to ask a question: on APF27 with imx27, I found that the system freeze when an interrupt handler is in place and an interruption happens. if interrupt handler is in place without interruption no problems and if interruption happens without interrupt handler it's ok too. If someone could help or advise me. thank you very much Gwenhael Signed-off-by: gwenhael.goavec-merou@domain.hid ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-help] Xenomai on ARMadeus 2009-11-02 19:19 ` gwenhael.goavec @ 2009-11-02 22:29 ` Gilles Chanteperdrix 2009-11-03 7:36 ` gwenhael.goavec [not found] ` <20091103082204.248eed59@domain.hid> 0 siblings, 2 replies; 9+ messages in thread From: Gilles Chanteperdrix @ 2009-11-02 22:29 UTC (permalink / raw) To: gwenhael.goavec; +Cc: xenomai gwenhael.goavec wrote: > On Mon, 02 Nov 2009 19:38:51 +0100 > Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> > wrote: > >> By the way, did you make any change to the I-pipe patch to get it >> running on this board? > > The only change realized, is made because Armadeus and > I-pipe does the same change on > mach-imx/include/mach/imxfb.h : > > --- a/arch/arm/mach-imx/include/mach/imxfb.h > +++ b/arch/arm/mach-imx/include/mach/imxfb.h > @@ -14,7 +14,6 @@ > #define PCR_BPIX_8 (3 << 25) > #define PCR_BPIX_12 (4 << 25) > #define PCR_BPIX_16 (4 << 25) > +#define PCR_BPIX_MASK (7 << 25) > #define PCR_PIXPOL (1 << 24) > #define PCR_FLMPOL (1 << 23) > #define PCR_LPPOL (1 << 22) > > I take this opportunity to ask a question: > on APF27 with imx27, I found that the system freeze > when an interrupt handler is in place and an > interruption happens. > if interrupt handler is in place without interruption no > problems and if interruption happens without interrupt > handler it's ok too. > > If someone could help or advise me. > thank you very much > > Gwenhael > > Signed-off-by: gwenhael.goavec-merou@domain.hid What version of the I-pipe patch? -- Gilles. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Xenomai-help] Xenomai on ARMadeus 2009-11-02 22:29 ` Gilles Chanteperdrix @ 2009-11-03 7:36 ` gwenhael.goavec [not found] ` <20091103082204.248eed59@domain.hid> 1 sibling, 0 replies; 9+ messages in thread From: gwenhael.goavec @ 2009-11-03 7:36 UTC (permalink / raw) To: xenomai On Mon, 02 Nov 2009 23:29:31 +0100 Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> wrote: > gwenhael.goavec wrote: > > On Mon, 02 Nov 2009 19:38:51 +0100 > > Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> > > wrote: > > > >> By the way, did you make any change to the I-pipe patch to get it > >> running on this board? > > > > The only change realized, is made because Armadeus and > > I-pipe does the same change on > > mach-imx/include/mach/imxfb.h : > > > > --- a/arch/arm/mach-imx/include/mach/imxfb.h > > +++ b/arch/arm/mach-imx/include/mach/imxfb.h > > @@ -14,7 +14,6 @@ > > #define PCR_BPIX_8 (3 << 25) > > #define PCR_BPIX_12 (4 << 25) > > #define PCR_BPIX_16 (4 << 25) > > +#define PCR_BPIX_MASK (7 << 25) > > #define PCR_PIXPOL (1 << 24) > > #define PCR_FLMPOL (1 << 23) > > #define PCR_LPPOL (1 << 22) > > > > I take this opportunity to ask a question: > > on APF27 with imx27, I found that the system freeze > > when an interrupt handler is in place and an > > interruption happens. > > if interrupt handler is in place without interruption no > > problems and if interruption happens without interrupt > > handler it's ok too. > > > > If someone could help or advise me. > > thank you very much > > > > Gwenhael > > > > Signed-off-by: gwenhael.goavec-merou@domain.hid > > What version of the I-pipe patch? > > -- > Gilles. Version 1.13-03 for 2.6.29 Gwenhael Signed-off-by: gwenhael.goavec-merou@domain.hid ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <20091103082204.248eed59@domain.hid>]
* Re: [Xenomai-help] Xenomai on ARMadeus [not found] ` <20091103082204.248eed59@domain.hid> @ 2009-11-04 13:14 ` Gilles Chanteperdrix [not found] ` <fbc4f538a6f4d84cfe514aba0985a525.squirrel@domain.hid> 0 siblings, 1 reply; 9+ messages in thread From: Gilles Chanteperdrix @ 2009-11-04 13:14 UTC (permalink / raw) To: Xenomai help gwenhael.goavec wrote: > On Mon, 02 Nov 2009 23:29:31 +0100 > Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> > wrote: > >> gwenhael.goavec wrote: >>> On Mon, 02 Nov 2009 19:38:51 +0100 >>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >>> wrote: >>> >>>> By the way, did you make any change to the I-pipe patch to get it >>>> running on this board? >>> The only change realized, is made because Armadeus and >>> I-pipe does the same change on >>> mach-imx/include/mach/imxfb.h : >>> >>> --- a/arch/arm/mach-imx/include/mach/imxfb.h >>> +++ b/arch/arm/mach-imx/include/mach/imxfb.h >>> @@ -14,7 +14,6 @@ >>> #define PCR_BPIX_8 (3 << 25) >>> #define PCR_BPIX_12 (4 << 25) >>> #define PCR_BPIX_16 (4 << 25) >>> +#define PCR_BPIX_MASK (7 << 25) >>> #define PCR_PIXPOL (1 << 24) >>> #define PCR_FLMPOL (1 << 23) >>> #define PCR_LPPOL (1 << 22) >>> >>> I take this opportunity to ask a question: >>> on APF27 with imx27, I found that the system freeze >>> when an interrupt handler is in place and an >>> interruption happens. >>> if interrupt handler is in place without interruption no >>> problems and if interruption happens without interrupt >>> handler it's ok too. >>> >>> If someone could help or advise me. >>> thank you very much The reason I knew for this problem was if handle_edge_irq was used for some interrupts. But problems of this kind for IMX GPIOs are fixed in 1.3-03. Do you have this problem with in-kernel or out-of-tree drivers? With real-time or non real-time interrupts? Could you show us /proc/interrupts before the system freezes? -- Gilles ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <fbc4f538a6f4d84cfe514aba0985a525.squirrel@domain.hid>]
* Re: [Xenomai-help] Xenomai on ARMadeus [not found] ` <fbc4f538a6f4d84cfe514aba0985a525.squirrel@domain.hid> @ 2009-11-12 14:59 ` Gilles Chanteperdrix 0 siblings, 0 replies; 9+ messages in thread From: Gilles Chanteperdrix @ 2009-11-12 14:59 UTC (permalink / raw) To: Gwenhaël Goavec-Merou; +Cc: Xenomai help Gwenhaël Goavec-Merou wrote: > Sorry for the late >> gwenhael.goavec wrote: >>> On Mon, 02 Nov 2009 23:29:31 +0100 >>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >>> wrote: >>> >>>> gwenhael.goavec wrote: >>>>> On Mon, 02 Nov 2009 19:38:51 +0100 >>>>> Gilles Chanteperdrix <gilles.chanteperdrix@xenomai.org> >>>>> wrote: >>>>> >>>>>> By the way, did you make any change to the I-pipe patch to get it >>>>>> running on this board? >>>>> The only change realized, is made because Armadeus and >>>>> I-pipe does the same change on >>>>> mach-imx/include/mach/imxfb.h : >>>>> >>>>> --- a/arch/arm/mach-imx/include/mach/imxfb.h >>>>> +++ b/arch/arm/mach-imx/include/mach/imxfb.h >>>>> @@ -14,7 +14,6 @@ >>>>> #define PCR_BPIX_8 (3 << 25) >>>>> #define PCR_BPIX_12 (4 << 25) >>>>> #define PCR_BPIX_16 (4 << 25) >>>>> +#define PCR_BPIX_MASK (7 << 25) >>>>> #define PCR_PIXPOL (1 << 24) >>>>> #define PCR_FLMPOL (1 << 23) >>>>> #define PCR_LPPOL (1 << 22) >>>>> >>>>> I take this opportunity to ask a question: >>>>> on APF27 with imx27, I found that the system freeze >>>>> when an interrupt handler is in place and an >>>>> interruption happens. >>>>> if interrupt handler is in place without interruption no >>>>> problems and if interruption happens without interrupt >>>>> handler it's ok too. >>>>> >>>>> If someone could help or advise me. >>>>> thank you very much >> The reason I knew for this problem was if handle_edge_irq was used for >> some interrupts. But problems of this kind for IMX GPIOs are fixed in >> 1.3-03. >> >> Do you have this problem with in-kernel or out-of-tree drivers? With >> real-time or non real-time interrupts? Could you show us >> /proc/interrupts before the system freezes? >> > I use an RTDM real-time driver for testing > GPIO interrupt don't appears in /proc/interrupts > > ~ # cat /proc/interrupts > CPU0 > 20: 69 - IMX-uart > 26: 1664 - i.MX Timer Tick > 29: 84439 - mxc_nd > 50: 75 - fec > 53: 0 - VPU_CODEC_IRQ > Err: 0 Appear in /proc/interrupts only the Linux domain interrupts. Xenomai domain interrupts appear in /proc/xenomai/irq. Now, if you are sure to use the latest I-pipe patch, you should try and follow the course of the interrupts masking, acking, calling the handler and unmasking, to understand what happens. The most likely problem is that your interrupt handler gets called in an infinite loop, for some reason. The reason may be that the interrupt controller is configured for the wrong type (edge/level) of irq, or simply that your interrupt handler is bogus, or something else. We could help you if you showed us the driver code, and told us what kind of interrupt (edge, level) the hardware is using. You could also try to implement the driver as a plain linux driver and run it over the xenomai patched kernel and a plain linux kernel, to see if the problem is in the interrupt handling code, I-pipe irq handling code, or something else. -- Gilles ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2009-11-12 14:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-11-12 14:34 [Xenomai-help] Xenomai on ARMadeus Gwenhaël Goavec-Merou -- strict thread matches above, loose matches on Subject: below -- 2009-10-20 11:37 [Xenomai-core] [PATCH v3 0/9] heap setup/cleanup fixes, refactorings & more Jan Kiszka 2009-10-20 11:37 ` [Xenomai-core] [PATCH v3 5/9] nucleus: Avoid returning errors from xnheap_destroy_mapped Jan Kiszka 2009-10-24 17:22 ` Philippe Gerum 2009-11-02 16:04 ` Philippe Gerum 2009-11-02 16:41 ` Jan Kiszka 2009-11-02 16:51 ` Philippe Gerum 2009-11-02 16:57 ` Jan Kiszka 2009-11-02 18:01 ` Jan Kiszka 2009-11-02 18:19 ` [Xenomai-help] Xenomai on ARMadeus Pierre Ficheux 2009-11-02 18:22 ` Gilles Chanteperdrix 2009-11-02 18:38 ` Gilles Chanteperdrix 2009-11-02 19:19 ` gwenhael.goavec 2009-11-02 22:29 ` Gilles Chanteperdrix 2009-11-03 7:36 ` gwenhael.goavec [not found] ` <20091103082204.248eed59@domain.hid> 2009-11-04 13:14 ` Gilles Chanteperdrix [not found] ` <fbc4f538a6f4d84cfe514aba0985a525.squirrel@domain.hid> 2009-11-12 14:59 ` Gilles Chanteperdrix
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.