Re: Why shoud we cause panic in scsi_da.c?
Subject: Re: Why shoud we cause panic in scsi_da.c?
Date: Tue, 14 Jul 2015 15:49:29 -0400
On Mon, Jul 13, 2015 at 18:29:36 +0300, Alexander Motin wrote:
Hi.
On 13.07.2015 11:51, Kohji Okuno wrote:
On 07/13/15 10:11, Kohji Okuno wrote:
Could you comment on my quesion?
I found panic() in scsi_da.c. Please find the following.
I think we should return with error without panic().
What do you think about this?
scsi_da.c:
3018} else if (bp != NULL) {
3019 if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) != 0)
3020panic(REQ_CMP with QFRZN);
It looks to me more like an KASSERT() is appropriate here.
As I can see, this panic() call was added by ken@ about 15 years ago.
I've added him to CC in case he has some idea why it was done. From my
personal opinion I don't see much reasons to allow CAM_DEV_QFRZN to be
returned only together with error. While is may have little sense in
case of successful command completion, I don't think it should be
treated as error. Simply removing this panic is probably a bad idea,
since if it happens device will just remain frozen forever, that will be
will be difficult to diagnose, but I would better just dropped device
freeze in that case same as in case of completion with error.
I put it there because it indicates a software error. The queue shouldn't
be frozen if the command is successful. The reason for freezing the queue
is to allow error recovery to happen. The queue will get unfrozen after
error recovery completes.
We could alternately just print a diagnostic message, unfreeze the queue
and move on, but the idea is to allow the driver writer to detect and
correct his error immediately.
As for the original poster's problem, he has uncovered a bug that needs to
be fixed. (And I don't mean in the da(4) driver. The bug is in the
component that left the queue frozen. Most likely in the USB driver, but
it will take a little more investigation.) The panic worked as intended. :)
I don't know the reaseon. When I accessed the specified sector on the
specified HDD, I encounter the panic. But, I can access other sectors
on the same HDD. And, I can access all sectors on the other HDD (same
model).
Even if Ken's logic is correct, I think that we should do panic in
da(4) driver.
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Why shoud we cause panic in scsi_da.c?
Hi HPS,
Thank you for your comment.
I will wait for the commnet.
Kohji Okuno
On 07/13/15 10:11, Kohji Okuno wrote:
Hi,
Could you comment on my quesion?
Best regards,
Kohji Okuno
Hi,
I found panic() in scsi_da.c. Please find the following.
I think we should return with error without panic().
What do you think about this?
scsi_da.c:
3018} else if (bp != NULL) {
3019 if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) != 0)
3020panic(REQ_CMP with QFRZN);
Hi,
It looks to me more like an KASSERT() is appropriate here.
Might be some people which can answer this are on vacation currently.
--HPS
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Why shoud we cause panic in scsi_da.c?
Hi,
From: Alexander Motin m...@freebsd.org
Date: Mon, 13 Jul 2015 18:29:36 +0300
Hi.
On 13.07.2015 11:51, Kohji Okuno wrote:
On 07/13/15 10:11, Kohji Okuno wrote:
Could you comment on my quesion?
I found panic() in scsi_da.c. Please find the following.
I think we should return with error without panic().
What do you think about this?
scsi_da.c:
3018 } else if (bp != NULL) {
3019 if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) != 0)
3020 panic(REQ_CMP with QFRZN);
It looks to me more like an KASSERT() is appropriate here.
As I can see, this panic() call was added by ken@ about 15 years ago.
I've added him to CC in case he has some idea why it was done. From my
personal opinion I don't see much reasons to allow CAM_DEV_QFRZN to be
returned only together with error. While is may have little sense in
case of successful command completion, I don't think it should be
treated as error. Simply removing this panic is probably a bad idea,
since if it happens device will just remain frozen forever, that will be
will be difficult to diagnose, but I would better just dropped device
freeze in that case same as in case of completion with error.
Thank you for your comment.
I have a strange USB HDD. When I access the specified sector, the
kernel causes panic(REQ_CMP with QFRZN) always.
After I modified the following, I think that I can recover from this
state, although the specified sector access fails. This recovery means
that I can access other sectors after this recovery.
What do you think about my idea?
@@ -3016,8 +3016,17 @@ dadone(struct cam_periph *periph, union ccb *done_ccb)
/*timeout*/0,
/*getcount_only*/0);
} else if (bp != NULL) {
+#if 0
if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) != 0)
panic(REQ_CMP with QFRZN);
+#else
+ if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) != 0)
+ cam_release_devq(done_ccb-ccb_h.path,
+/*relsim_flags*/0,
+/*reduction*/0,
+/*timeout*/0,
+/*getcount_only*/0);
+#endif
if (state == DA_CCB_DELETE)
bp-bio_resid = 0;
else
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Why shoud we cause panic in scsi_da.c?
Hi,
Could you comment on my quesion?
Best regards,
Kohji Okuno
Hi,
I found panic() in scsi_da.c. Please find the following.
I think we should return with error without panic().
What do you think about this?
scsi_da.c:
3018 } else if (bp != NULL) {
3019 if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) !=
0)
3020 panic(REQ_CMP with QFRZN);
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Why shoud we cause panic in scsi_da.c?
Hi,
I found panic() in scsi_da.c. Please find the following.
I think we should return with error without panic().
What do you think about this?
scsi_da.c:
3018} else if (bp != NULL) {
3019if ((done_ccb-ccb_h.status CAM_DEV_QFRZN) !=
0)
3020panic(REQ_CMP with QFRZN);
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi Konstantin,
I tested your patch. It was no problem.
Thank you for your kind correspondence.
Regards,
Kohji Okuno
On Sat, Oct 04, 2014 at 05:53:26PM +0900, Kohji Okuno wrote:
Hi, Konstantin,
At the end of the mail is commit candidate. I did not even compiled this.
Can you test and report back, please ?
Could you check the following?
(1) should use kernel_pmap-pm_stats.resident_count
^^^
I'm sorry. My patch was wrong.
As well as mine.
(2) should use pmap_resident_count_inc() in amd64.
Correct.
I will test, later.
In addtion, I have one question.
In current and 10-stable, is vm_map_delete() called by kva_free()?
No, kva_free() only releases the vmem backing, leaving the page
tables intact. This is why I only did the stable/9 patch.
If vm_map_delete() is called, this fix is needed in current and
10-stable, I think.
Updated patch below.
Index: amd64/amd64/pmap.c
===
--- amd64/amd64/pmap.c(revision 272506)
+++ amd64/amd64/pmap.c(working copy)
@@ -5040,6 +5040,9 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
pa = trunc_page(pa);
for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE)
pmap_kenter_attr(va + tmpsize, pa + tmpsize, mode);
+ PMAP_LOCK(kernel_pmap);
+ pmap_resident_count_inc(kernel_pmap, OFF_TO_IDX(size));
+ PMAP_UNLOCK(kernel_pmap);
pmap_invalidate_range(kernel_pmap, va, va + tmpsize);
pmap_invalidate_cache_range(va, va + tmpsize);
return ((void *)(va + offset));
Index: i386/i386/pmap.c
===
--- i386/i386/pmap.c (revision 272506)
+++ i386/i386/pmap.c (working copy)
@@ -5066,10 +5066,14 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
size = roundup(offset + size, PAGE_SIZE);
pa = pa PG_FRAME;
- if (pa KERNLOAD pa + size = KERNLOAD)
+ if (pa KERNLOAD pa + size = KERNLOAD) {
va = KERNBASE + pa;
- else
+ } else {
va = kmem_alloc_nofault(kernel_map, size);
+ PMAP_LOCK(kernel_pmap);
+ kernel_pmap-pm_stats.resident_count += OFF_TO_IDX(size);
+ PMAP_UNLOCK(kernel_pmap);
+ }
if (!va)
panic(pmap_mapdev: Couldn't alloc kernel virtual memory);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi Konstantin, Thank you very much for your detailed explanatin. I understood the policy of vmem. Many thanks, Kohji Okuno On Sun, Oct 05, 2014 at 01:15:12PM +0900, Kohji Okuno wrote: Hi, On Sat, Oct 04, 2014 at 08:53:35PM +0900, Kohji Okuno wrote: Hi Konstantin, Thank you for your prompt response. I will test and report from next monday. In addtion, I have one question. In current and 10-stable, is vm_map_delete() called by kva_free()? No, kva_free() only releases the vmem backing, leaving the page tables intact. This is why I only did the stable/9 patch. Where are PTEs allocated by pmap_mapdev() freed in current and 10-stable? Could you please explain me? They are not freed. The removal of the vmem which covers the address space managed by corresponding ptes, allows the reuse of both KVA region and corresponding PTEs in the tables. The only concern with the resident page tables is to avoid two kva_alloc() to step over each other, and this is ensured by vmem. I agree that normal pages are reusable. But, since the pages mapped by pmap_mapdev() are concerned with the physicall address of device (For example: video memory), these PTEs aren't reusable, I think. So, should we free these PTEs by pmap_unmapdev()? There is no hold on any physical pages which were referenced by the ptes. The only thing which is left out are the records in the page tables which map the KVA to said device memory. It is harmless. When the KVA is reused, the ptes in page tables are overwritten. It might be argued that clearing PTEs, or at least removing the PG_V bit catches erronous unintended accesses to the freed range, but by the cost of the overhead of re-polluting the caches. And since clearing is not effective without doing TLB flush, which requires broadcast IPI, it is more trouble than advantage. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi, Konstantin,
Thank you for your comment.
And, your change is better than mine.
Do you mean that this panic is related to missed pmap_remove() ?
I doubt it, since pmap_mapdev() does not establish managed mappings.
Yes, pmap_mapdev() does not establish managed mappings. But, if
kernel_pmap.pm_stats.resident_count is zero, then any managed pages
(for example pipe_map, exec_map, or etc.) are not able to change
unmanaged status, because pmap_remove() returns without calling
pmap_remove_pte().
In this result, I encounterd the panic. Could you refer the following?
Regards,
Kohji Okuno
int
vm_map_delete(vm_map_t map, vm_offset_t start, vm_offset_t end)
{
SNIP
/** this pmap_remove() does not change for mappings! **/
pmap_remove(map-pmap, entry-start, entry-end);
/*
* Delete the entry only after removing all pmap
* entries pointing to its pages. (Otherwise, its
* page frames may be reallocated, and any modify bits
* will be set in the wrong object!)
*/
/** this calls vm_page_free_toq() and causes panic! **/
vm_map_entry_delete(map, entry);
entry = next;
}
return (KERN_SUCCESS);
}
On Fri, Oct 03, 2014 at 05:25:33PM +0900, Kohji Okuno wrote:
Hi,
At least in i386 9-stable, when we call pmap_mapdev() and
pmap_unmapdev(), kernel_pmap.pm_stats.resident_count is decreased
incorrectly.
pmap_mapdev_attr()-pmap_kenter_attr():
In this path, kernel_pmap.pm_stats.resident_count is not increlmented.
pmap_unmapdev()-kmem_free(kernel_map)-vm_map_remove()-pmap_remove():
But, in this path, kernel_pmap.pm_stats.resident_count is decreased in
pmap_remove_pte().
While I called pmap_mapdev() and pmap_unmapdev() repeatedly and
kernel_pmap.pm_stats.resident_count became `0', since pmap_remove()
returned without removing ptes, I encountered various kernel panics.
I think you are right.
The problem seems to be fixed in HEAD and 10, since mapdev was switched
to use kva_alloc/kva_free. I added stable@ to Cc:.
And, when I enabled INVARIANTS, I looked the following panic message.
panic: vm_page_free_toq: freeing mapped page 0x.
Do you mean that this panic is related to missed pmap_remove() ?
I doubt it, since pmap_mapdev() does not establish managed mappings.
I think, I should change the following.
What do you think about this?
diff --git a/sys/i386/i386/pmap.c b/sys/i386/i386/pmap.c
index 2adc6f8..a0998e8 100644
--- a/sys/i386/i386/pmap.c
+++ b/sys/i386/i386/pmap.c
@@ -5061,6 +5061,7 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, int
mode)
{
vm_offset_t va, offset;
vm_size_t tmpsize;
+int kmem_allocated = 0;
offset = pa PAGE_MASK;
size = roundup(offset + size, PAGE_SIZE);
@@ -5068,13 +5069,18 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, int
mode)
if (pa KERNLOAD pa + size = KERNLOAD)
va = KERNBASE + pa;
-else
+else {
va = kmem_alloc_nofault(kernel_map, size);
+kmem_allocated = 1;
You could just do
PMAP_LOCK(kernel_pmap);
kernel_pmap.pm_stats.resident_count += OFF_TO_IDX(size);
PMAP_UNLOCK(kernel_pmap);
there. And, the same fix is needed for amd64.
+}
if (!va)
panic(pmap_mapdev: Couldn't alloc kernel virtual memory);
-for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE)
+for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE) {
pmap_kenter_attr(va + tmpsize, pa + tmpsize, mode);
+if (kmem_allocated)
+kernel_pmap.pm_stats.resident_count++;
+}
pmap_invalidate_range(kernel_pmap, va, va + tmpsize);
pmap_invalidate_cache_range(va, va + size);
return ((void *)(va + offset));
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi, Konstantin,
At the end of the mail is commit candidate. I did not even compiled this.
Can you test and report back, please ?
Could you check the following?
(1) should use kernel_pmap-pm_stats.resident_count
^^^
I'm sorry. My patch was wrong.
(2) should use pmap_resident_count_inc() in amd64.
I will test, later.
In addtion, I have one question.
In current and 10-stable, is vm_map_delete() called by kva_free()?
If vm_map_delete() is called, this fix is needed in current and
10-stable, I think.
Regards,
Kohji Okuno
On Sat, Oct 04, 2014 at 05:00:36PM +0900, Kohji Okuno wrote:
Hi, Konstantin,
Thank you for your comment.
And, your change is better than mine.
At the end of the mail is commit candidate. I did not even compiled this.
Can you test and report back, please ?
Do you mean that this panic is related to missed pmap_remove() ?
I doubt it, since pmap_mapdev() does not establish managed mappings.
Yes, pmap_mapdev() does not establish managed mappings. But, if
kernel_pmap.pm_stats.resident_count is zero, then any managed pages
(for example pipe_map, exec_map, or etc.) are not able to change
unmanaged status, because pmap_remove() returns without calling
pmap_remove_pte().
In this result, I encounterd the panic. Could you refer the following?
Yes, kmem_back() indeed uses managed mapping.
Index: amd64/amd64/pmap.c
===
--- amd64/amd64/pmap.c(revision 272506)
+++ amd64/amd64/pmap.c(working copy)
@@ -5040,6 +5040,9 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
pa = trunc_page(pa);
for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE)
pmap_kenter_attr(va + tmpsize, pa + tmpsize, mode);
+ PMAP_LOCK(kernel_pmap);
+ kernel_pmap.pm_stats.resident_count += OFF_TO_IDX(size);
+ PMAP_UNLOCK(kernel_pmap);
pmap_invalidate_range(kernel_pmap, va, va + tmpsize);
pmap_invalidate_cache_range(va, va + tmpsize);
return ((void *)(va + offset));
Index: i386/i386/pmap.c
===
--- i386/i386/pmap.c (revision 272506)
+++ i386/i386/pmap.c (working copy)
@@ -5066,10 +5066,14 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
size = roundup(offset + size, PAGE_SIZE);
pa = pa PG_FRAME;
- if (pa KERNLOAD pa + size = KERNLOAD)
+ if (pa KERNLOAD pa + size = KERNLOAD) {
va = KERNBASE + pa;
- else
+ } else {
va = kmem_alloc_nofault(kernel_map, size);
+ PMAP_LOCK(kernel_pmap);
+ kernel_pmap.pm_stats.resident_count += OFF_TO_IDX(size);
+ PMAP_UNLOCK(kernel_pmap);
+ }
if (!va)
panic(pmap_mapdev: Couldn't alloc kernel virtual memory);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi Konstantin,
Thank you for your prompt response.
I will test and report from next monday.
In addtion, I have one question.
In current and 10-stable, is vm_map_delete() called by kva_free()?
No, kva_free() only releases the vmem backing, leaving the page
tables intact. This is why I only did the stable/9 patch.
Where are PTEs allocated by pmap_mapdev() freed in current and 10-stable?
Could you please explain me?
Regards,
Kohji Okuno
On Sat, Oct 04, 2014 at 05:53:26PM +0900, Kohji Okuno wrote:
Hi, Konstantin,
At the end of the mail is commit candidate. I did not even compiled this.
Can you test and report back, please ?
Could you check the following?
(1) should use kernel_pmap-pm_stats.resident_count
^^^
I'm sorry. My patch was wrong.
As well as mine.
(2) should use pmap_resident_count_inc() in amd64.
Correct.
I will test, later.
In addtion, I have one question.
In current and 10-stable, is vm_map_delete() called by kva_free()?
No, kva_free() only releases the vmem backing, leaving the page
tables intact. This is why I only did the stable/9 patch.
If vm_map_delete() is called, this fix is needed in current and
10-stable, I think.
Updated patch below.
Index: amd64/amd64/pmap.c
===
--- amd64/amd64/pmap.c(revision 272506)
+++ amd64/amd64/pmap.c(working copy)
@@ -5040,6 +5040,9 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
pa = trunc_page(pa);
for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE)
pmap_kenter_attr(va + tmpsize, pa + tmpsize, mode);
+ PMAP_LOCK(kernel_pmap);
+ pmap_resident_count_inc(kernel_pmap, OFF_TO_IDX(size));
+ PMAP_UNLOCK(kernel_pmap);
pmap_invalidate_range(kernel_pmap, va, va + tmpsize);
pmap_invalidate_cache_range(va, va + tmpsize);
return ((void *)(va + offset));
Index: i386/i386/pmap.c
===
--- i386/i386/pmap.c (revision 272506)
+++ i386/i386/pmap.c (working copy)
@@ -5066,10 +5066,14 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, in
size = roundup(offset + size, PAGE_SIZE);
pa = pa PG_FRAME;
- if (pa KERNLOAD pa + size = KERNLOAD)
+ if (pa KERNLOAD pa + size = KERNLOAD) {
va = KERNBASE + pa;
- else
+ } else {
va = kmem_alloc_nofault(kernel_map, size);
+ PMAP_LOCK(kernel_pmap);
+ kernel_pmap-pm_stats.resident_count += OFF_TO_IDX(size);
+ PMAP_UNLOCK(kernel_pmap);
+ }
if (!va)
panic(pmap_mapdev: Couldn't alloc kernel virtual memory);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About pmap_mapdev() pmap_unmapdev()
Hi, On Sat, Oct 04, 2014 at 08:53:35PM +0900, Kohji Okuno wrote: Hi Konstantin, Thank you for your prompt response. I will test and report from next monday. In addtion, I have one question. In current and 10-stable, is vm_map_delete() called by kva_free()? No, kva_free() only releases the vmem backing, leaving the page tables intact. This is why I only did the stable/9 patch. Where are PTEs allocated by pmap_mapdev() freed in current and 10-stable? Could you please explain me? They are not freed. The removal of the vmem which covers the address space managed by corresponding ptes, allows the reuse of both KVA region and corresponding PTEs in the tables. The only concern with the resident page tables is to avoid two kva_alloc() to step over each other, and this is ensured by vmem. I agree that normal pages are reusable. But, since the pages mapped by pmap_mapdev() are concerned with the physicall address of device (For example: video memory), these PTEs aren't reusable, I think. So, should we free these PTEs by pmap_unmapdev()? Best regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
About pmap_mapdev() pmap_unmapdev()
Hi,
At least in i386 9-stable, when we call pmap_mapdev() and
pmap_unmapdev(), kernel_pmap.pm_stats.resident_count is decreased
incorrectly.
pmap_mapdev_attr()-pmap_kenter_attr():
In this path, kernel_pmap.pm_stats.resident_count is not increlmented.
pmap_unmapdev()-kmem_free(kernel_map)-vm_map_remove()-pmap_remove():
But, in this path, kernel_pmap.pm_stats.resident_count is decreased in
pmap_remove_pte().
While I called pmap_mapdev() and pmap_unmapdev() repeatedly and
kernel_pmap.pm_stats.resident_count became `0', since pmap_remove()
returned without removing ptes, I encountered various kernel panics.
And, when I enabled INVARIANTS, I looked the following panic message.
panic: vm_page_free_toq: freeing mapped page 0x.
I think, I should change the following.
What do you think about this?
diff --git a/sys/i386/i386/pmap.c b/sys/i386/i386/pmap.c
index 2adc6f8..a0998e8 100644
--- a/sys/i386/i386/pmap.c
+++ b/sys/i386/i386/pmap.c
@@ -5061,6 +5061,7 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, int mode)
{
vm_offset_t va, offset;
vm_size_t tmpsize;
+ int kmem_allocated = 0;
offset = pa PAGE_MASK;
size = roundup(offset + size, PAGE_SIZE);
@@ -5068,13 +5069,18 @@ pmap_mapdev_attr(vm_paddr_t pa, vm_size_t size, int
mode)
if (pa KERNLOAD pa + size = KERNLOAD)
va = KERNBASE + pa;
- else
+ else {
va = kmem_alloc_nofault(kernel_map, size);
+ kmem_allocated = 1;
+ }
if (!va)
panic(pmap_mapdev: Couldn't alloc kernel virtual memory);
- for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE)
+ for (tmpsize = 0; tmpsize size; tmpsize += PAGE_SIZE) {
pmap_kenter_attr(va + tmpsize, pa + tmpsize, mode);
+ if (kmem_allocated)
+ kernel_pmap.pm_stats.resident_count++;
+ }
pmap_invalidate_range(kernel_pmap, va, va + tmpsize);
pmap_invalidate_cache_range(va, va + size);
return ((void *)(va + offset));
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Does the xHCI driver has a spec violation?
Hi HPS, Could you refer to the following document (4.6.6 Configure Endpoint:P.99)? This document shows: If the Drop Context flag is `1' and the Add Context flag is `1', the xHC shall: o Release the current Resources and Bandwidth allocated to the endpoint and assign the new Resources and Bandwidth requested for the endpoint. Regards, Kohji Okuno. On 09/22/14 06:58, Kohji Okuno wrote: Hi, I encountered a issue for USB mic. In fist time, my host controller (xHCI) sends single IN-tokens every 8-SOFs. This is expected action. But, after I open, close and open, my host controller sends plural IN-tokens between SOF and SOF. In Intel Lynx Point, I could not reproduce this issue. I'm sorry. Unfortunately, I can't explain details about my proprietary host controler. I found the following explanation in the xHCI 1.1 specification http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/extensible-host-controler-interface-usb-xhci.pdf In 4.8.3 Endpoint Context State, 6. The Configure Endpoint Command (Add (A) = `1' and Drop (D) =`1') shall transition an endpoint, except the Default Control Endpoint, from the Stopped to the Running state.' So, I modify as the following, then I can run expectedly. What do you think about this change? Hi, I think we should issue the context drop separately. Are we certain that if both drop and add bits are set at the same time, that the drop bit will be processed before the add? This might be a bug in your hardware, which apparently doesn't check if the context has already been added or not. I'll be glad to make a workaround for it once we have settled on a solution. Can you test the attached patch using both your hardware and the Lynx Point. Thank you! --HPS Best regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Does the xHCI driver has a spec violation?
On 09/22/14 08:31, Kohji Okuno wrote: Hi HPS, Could you refer to the following document (4.6.6 Configure Endpoint:P.99)? This document shows: If the Drop Context flag is `1' and the Add Context flag is `1', the xHC shall: o Release the current Resources and Bandwidth allocated to the endpoint and assign the new Resources and Bandwidth requested for the endpoint. Hi, I see. Then what is missing to your patch is to mask away bits 0 and 1, because those are reserved for D0 and D1 and should be zero? Hi, HPS, You are correct, I think. We shold mask D0 and D1. My host controller works both. Thanks, Kohji Okuno. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Does the xHCI driver has a spec violation?
Hi, Please verify: http://svnweb.freebsd.org/changeset/base/271953 Thank you! --HPS Hi, HPS, I confirmed your commit. It was no problem. Many thanks, Kohji Okuno. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Does the xHCI driver has a spec violation?
Hi,
I encountered a issue for USB mic.
In fist time, my host controller (xHCI) sends single IN-tokens every
8-SOFs. This is expected action. But, after I open, close and open, my
host controller sends plural IN-tokens between SOF and SOF.
In Intel Lynx Point, I could not reproduce this issue.
I'm sorry. Unfortunately, I can't explain details about my proprietary
host controler.
I found the following explanation in the xHCI 1.1 specification
http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/extensible-host-controler-interface-usb-xhci.pdf
In 4.8.3 Endpoint Context State,
6. The Configure Endpoint Command (Add (A) = `1' and Drop (D) =`1')
shall transition an endpoint, except the Default Control
Endpoint, from the Stopped to the Running state.'
So, I modify as the following, then I can run expectedly.
What do you think about this change?
Best regards,
Kohji Okuno
static usb_error_t
xhci_configure_mask(struct usb_device *udev, uint32_t mask, uint8_t drop)
{
struct xhci_softc *sc = XHCI_BUS2SC(udev-bus);
struct usb_page_search buf_inp;
struct xhci_input_dev_ctx *pinp;
uint32_t temp;
uint8_t index;
uint8_t x;
index = udev-controller_slot_id;
usbd_get_page(sc-sc_hw.devs[index].input_pc, 0, buf_inp);
pinp = buf_inp.buffer;
if (drop) {
mask = XHCI_INCTX_NON_CTRL_MASK;
xhci_ctx_set_le32(sc, pinp-ctx_input.dwInCtx0, mask);
xhci_ctx_set_le32(sc, pinp-ctx_input.dwInCtx1, 0);
} else {
- xhci_ctx_set_le32(sc, pinp-ctx_input.dwInCtx0, 0);
+ xhci_ctx_set_le32(sc, pinp-ctx_input.dwInCtx0, mask);
xhci_ctx_set_le32(sc, pinp-ctx_input.dwInCtx1, mask);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
About the type of physaddr in struct usb_page.
Hi HPS,
I think the type of physaddr in struct usb_page is incorrect.
We shuld use bus_addr_t for physaddr.
What do you think about this?
60 * The following structure defines physical and non kernel virtual
61 * address of a memory page having size USB_PAGE_SIZE.
62 */
63 struct usb_page {
64 #if USB_HAVE_BUSDMA
65 bus_size_t physaddr;
66 void *buffer; /* non Kernel Virtual Address */
67 #endif
68 };
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
UFS SU+J bug? (Re: uninitialized journal data written in SU+J ?)
Hi,
If you use UFS SU+J, could you check your `.sujournal'?
(For example: strings .sujournal)
You will find memory that already has been released.
In addition, unfotunately, if the memory was used for journal is
re-used, although the journal is incorrect, fsck will missunderstand it.
What do you think about this?
Regards,
Kohji Okuno
From: takehara.mikih...@jp.panasonic.com
Subject: uninitialized journal data written in SU+J ?
Date: Tue, 22 Apr 2014 16:21:43 +0900
Hello,
I'm testing UFS with SU+J. But it seems sometimes broken journal data has
written.
In softdep_process_journal (ffs_softdep.c), there is a while code to build
jsegrec and each entry.
But by my test, sometimes there is no entry then break this while code
without building jsegrec.
If this happens, bp-b_data is not initialized but this bp is written, I
think.
I checked this behavior by following patch.
diff --git a/sys/ufs/ffs/ffs_softdep.c b/sys/ufs/ffs/ffs_softdep.c
index 585af50..2d4939c 100644
--- a/sys/ufs/ffs/ffs_softdep.c
+++ b/sys/ufs/ffs/ffs_softdep.c
@@ -3421,6 +3421,15 @@ softdep_process_journal(mp, needwk, flags)
data = bp-b_data + off;
cnt--;
}
+
+#if 1
+ if (off == 0) {
+ struct jsegrec *tmp = (struct jsegrec*)bp-b_data;
+ if (tmp-jsr_seq != jseg-js_seq) {
+ panic(test test);
+ }
+ }
+#endif
/*
* Write this one buffer and continue.
*/
If uninitialized data is valid by fsck suj, this may result filesystem
corruption, I think.
I think it's better to clear b_data before using it.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kevent has bug?
0x100 /* flux set in
kqueue_scan() */
int kn_sfflags; /* saved filter flags */
intptr_tkn_sdata; /* saved data field */
union {
Hi,
I think, we should add KN_SCAN after knote_attach() in
kqueue_register(), too. What do you think about this?
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kevent has bug?
From: Konstantin Belousov kostik...@gmail.com Date: Thu, 3 Apr 2014 16:48:14 +0300 On Thu, Apr 03, 2014 at 06:26:56PM +0900, Kohji Okuno wrote: The done_ev_add case is indeed missed in my patch, thank you for noting. The case of EV_ADD does not need the KN_SCAN workaround, IMO, since the race is possible just by the nature of adding the knote. I think, we should add KN_SCAN after knote_attach() in kqueue_register(), too. What do you think about this? See above, I noted this case in the previous mail. This may be elaborated. First, I think it is technically incorrect to allow the event notification before the f_attach() method is finished. So the KN_SCAN flag could be set only after f_attach() call, but due to both kq and knlist not locked there, we still have the same race. And this race is in fact acceptable, since it is the race between application calling EV_ADD, and external event occuring, which cannot be avoided. Until the kevent(EV_ADD) syscall returned, we do not have an obligation to report the event from the kqfd. Having the race somewhat bigger by not setting KN_SCAN is fine in my opinion. Hi, Thank you for your detailed commnet. And I uderstood about your opinion. By the way, do you commit your change to HEAD? Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kevent has bug?
From: John-Mark Gurney j...@funkthat.com
Date: Tue, 1 Apr 2014 23:15:51 -0700
Kohji Okuno wrote this message on Wed, Apr 02, 2014 at 11:45 +0900:
I think, kevent() has a bug.
I tested sample programs by attached sources.
This sample tests about EVFILT_SIGNAL.
I build sample programs by the following commands.
% gcc -O2 -o child child.c
% gcc -O2 -o parent parent.c
The expected result is the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
But, sometimes the result was the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
This result means the number of times the signal has occured was
incorrect.
I was able to reproduce this...
In case of EVFILT_SIGNAL, according to `man kevent', `data' retuns the
number of times the signal has occurred since the last call to
kevent(). This `data' is recorded by filt_signal() (This is f_event in
struct filterops).
The system call kevent()'s events are processed by kqueue_scan() in
kern_event.c. In kqueue_scan(), kn-kn_fop-f_event() is allways
called after KN_INFLUX is set to kn-kn_status.
On the other hand, kernel events are occured by knote() in
kern_event.c. (In EVFILT_SIGNAL, knote() is called from tdsendsignal()
in kern_sig.c.) In knote(), kn-kn_fop-f_event() is called only when
KN_INFLUX is not set in kn-kn_status.
In race condition between kqueue_scan() and knote(),
kn-kn_fop-f_event() from knote() may not be called, I think.
Considering that both are called w/ a lock, that cannot happen..
KN_LIST_LOCK(kn) locks the same lock that is asserted that is held
by knote...
In knote(), because the context holds knlist's lock, the context can
not sleep. So, KN_INFLUX should not be set on calling
kn-kn_fop-f_event() in kqueue_scan(), I think.
No, it needs to be set:
* Setting the KN_INFLUX flag enables you to unlock the kq that this knote
* is on, and modify kn_status as if you had the KQ lock.
As this comment says, _INFLUX allows you to unlock the KQ w/o fear that
the knote will disappear out from under you causing you to dereference
possibly free'd memory..
If you just tried to lock the list lock w/o unlocking the KQ lock, you
could end up w/ a dead lock, as you aren't maintaining lock order
properly.. The correct lock order if knlist - kq...
What do you think about this issue?
This is a real issue, but not due to the race you described
above...
I beleave it's the result of the race.
Could you try to add printf() in knote()?
Please refer to attached patch.
I have verified on my machine that it isn't because there is a knote
waiting that isn't getting woken up, and the knote on my hung process
has data == 0, so it definately lost one of the signals:
(kgdb) print $14.kq_knhash[20].slh_first[0]
$20 = {kn_link = {sle_next = 0x0}, kn_selnext = {sle_next = 0x0},
kn_knlist = 0xf8005a9c5840, kn_tqe = {tqe_next = 0xf801fdab4500,
tqe_prev = 0xf8004bb10038}, kn_kq = 0xf8004bb1, kn_kevent = {
ident = 20, filter = -6, flags = 32, fflags = 0, data = 0, udata = 0x0},
kn_status = 0, kn_sfflags = 0, kn_sdata = 0, kn_ptr = {
p_fp = 0xf8005a9c54b8, p_proc = 0xf8005a9c54b8,
p_aio = 0xf8005a9c54b8, p_lio = 0xf8005a9c54b8,
p_v = 0xf8005a9c54b8}, kn_fop = 0x81405ef0, kn_hook = 0x0,
kn_hookid = 0}
If you want to find this yourself, you can run kgdb on a live system,
switch to the thread of the parent (info threads, thread XXX), and
do:
frame 7
(or a frame that has td, which is struct thread *), then:
print *(struct kqueue *)td-td_proc[0].p_fd[0].fd_ofiles[3].fde_file[0].f_data
This will give you the struct kqueue * of the parent, and then:
print $XX.kq_knhash[0]@63
to figure out where the knote is in the hash, and then you can print
it out yourself...
I'm going to take a look at this a bit more later... I'm thinking of
using dtrace to collect the stacks where filt_signal is called, and
match them up... dtrace might even be able to get us the note's data
upon return helping to make sure things got tracked properly...
Thanks for finding this bug! Hopefully we can find a solution to it..
--
John-Mark GurneyVoice: +1 415 225 5579
All that I will do, has been done, All that I have, has not.
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index b3fb23d..7791447 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -1868,6 +1868,8 @@ knote(struct knlist *list, long hint, int lockflags)
if ((kn-kn_status KN_INFLUX) != KN_INFLUX) {
KQ_LOCK(kq);
if ((kn-kn_status KN_INFLUX) == KN_INFLUX) {
+if (hint NOTE_SIGNAL)
+ printf(Aee2\n);
KQ_UNLOCK(kq);
} else if ((lockflags KNF_NOKQLOCK) != 0) {
kn-kn_status |= KN_INFLUX;
@@ -1886,6 +1888,10 @@ knote(struct knlist *list, long hint, int lockflags)
KQ_UNLOCK
Re: kevent has bug?
From: Konstantin Belousov kostik...@gmail.com
Date: Wed, 2 Apr 2014 15:07:45 +0300
On Wed, Apr 02, 2014 at 04:06:16PM +0900, Kohji Okuno wrote:
From: John-Mark Gurney j...@funkthat.com
Date: Tue, 1 Apr 2014 23:15:51 -0700
Kohji Okuno wrote this message on Wed, Apr 02, 2014 at 11:45 +0900:
I think, kevent() has a bug.
I tested sample programs by attached sources.
This sample tests about EVFILT_SIGNAL.
I build sample programs by the following commands.
% gcc -O2 -o child child.c
% gcc -O2 -o parent parent.c
The expected result is the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
But, sometimes the result was the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
This result means the number of times the signal has occured was
incorrect.
I was able to reproduce this...
In case of EVFILT_SIGNAL, according to `man kevent', `data' retuns the
number of times the signal has occurred since the last call to
kevent(). This `data' is recorded by filt_signal() (This is f_event in
struct filterops).
The system call kevent()'s events are processed by kqueue_scan() in
kern_event.c. In kqueue_scan(), kn-kn_fop-f_event() is allways
called after KN_INFLUX is set to kn-kn_status.
On the other hand, kernel events are occured by knote() in
kern_event.c. (In EVFILT_SIGNAL, knote() is called from tdsendsignal()
in kern_sig.c.) In knote(), kn-kn_fop-f_event() is called only when
KN_INFLUX is not set in kn-kn_status.
In race condition between kqueue_scan() and knote(),
kn-kn_fop-f_event() from knote() may not be called, I think.
Considering that both are called w/ a lock, that cannot happen..
KN_LIST_LOCK(kn) locks the same lock that is asserted that is held
by knote...
In knote(), because the context holds knlist's lock, the context can
not sleep. So, KN_INFLUX should not be set on calling
kn-kn_fop-f_event() in kqueue_scan(), I think.
No, it needs to be set:
* Setting the KN_INFLUX flag enables you to unlock the kq that this knote
* is on, and modify kn_status as if you had the KQ lock.
As this comment says, _INFLUX allows you to unlock the KQ w/o fear that
the knote will disappear out from under you causing you to dereference
possibly free'd memory..
If you just tried to lock the list lock w/o unlocking the KQ lock, you
could end up w/ a dead lock, as you aren't maintaining lock order
properly.. The correct lock order if knlist - kq...
What do you think about this issue?
This is a real issue, but not due to the race you described
above...
I beleave it's the result of the race.
Could you try to add printf() in knote()?
Please refer to attached patch.
I have verified on my machine that it isn't because there is a knote
waiting that isn't getting woken up, and the knote on my hung process
has data == 0, so it definately lost one of the signals:
(kgdb) print $14.kq_knhash[20].slh_first[0]
$20 = {kn_link = {sle_next = 0x0}, kn_selnext = {sle_next = 0x0},
kn_knlist = 0xf8005a9c5840, kn_tqe = {tqe_next = 0xf801fdab4500,
tqe_prev = 0xf8004bb10038}, kn_kq = 0xf8004bb1, kn_kevent
= {
ident = 20, filter = -6, flags = 32, fflags = 0, data = 0, udata =
0x0},
kn_status = 0, kn_sfflags = 0, kn_sdata = 0, kn_ptr = {
p_fp = 0xf8005a9c54b8, p_proc = 0xf8005a9c54b8,
p_aio = 0xf8005a9c54b8, p_lio = 0xf8005a9c54b8,
p_v = 0xf8005a9c54b8}, kn_fop = 0x81405ef0, kn_hook = 0x0,
kn_hookid = 0}
If you want to find this yourself, you can run kgdb on a live system,
switch to the thread of the parent (info threads, thread XXX), and
do:
frame 7
(or a frame that has td, which is struct thread *), then:
print *(struct kqueue
*)td-td_proc[0].p_fd[0].fd_ofiles[3].fde_file[0].f_data
This will give you the struct kqueue * of the parent, and then:
print $XX.kq_knhash[0]@63
to figure out where the knote is in the hash, and then you can print
it out yourself...
I'm going to take a look at this a bit more later... I'm thinking of
using dtrace to collect the stacks where filt_signal is called, and
match them up... dtrace might even be able to get us the note's data
upon return helping to make sure things got tracked properly...
Thanks for finding this bug! Hopefully we can find a solution to it..
--
John-Mark Gurney Voice: +1 415 225 5579
All that I will do, has been done, All that I have, has not.
diff --git a/sys/kern/kern_event.c b/sys/kern/kern_event.c
index b3fb23d..7791447 100644
--- a/sys/kern/kern_event.c
+++ b/sys/kern/kern_event.c
@@ -1868,6 +1868,8 @@ knote(struct knlist *list, long hint, int lockflags)
if ((kn-kn_status KN_INFLUX) != KN_INFLUX
kevent has bug?
Hi,
I think, kevent() has a bug.
I tested sample programs by attached sources.
This sample tests about EVFILT_SIGNAL.
I build sample programs by the following commands.
% gcc -O2 -o child child.c
% gcc -O2 -o parent parent.c
The expected result is the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20
OK
But, sometimes the result was the following.
% ./parent
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19
This result means the number of times the signal has occured was
incorrect.
In case of EVFILT_SIGNAL, according to `man kevent', `data' retuns the
number of times the signal has occurred since the last call to
kevent(). This `data' is recorded by filt_signal() (This is f_event in
struct filterops).
The system call kevent()'s events are processed by kqueue_scan() in
kern_event.c. In kqueue_scan(), kn-kn_fop-f_event() is allways
called after KN_INFLUX is set to kn-kn_status.
On the other hand, kernel events are occured by knote() in
kern_event.c. (In EVFILT_SIGNAL, knote() is called from tdsendsignal()
in kern_sig.c.) In knote(), kn-kn_fop-f_event() is called only when
KN_INFLUX is not set in kn-kn_status.
In race condition between kqueue_scan() and knote(),
kn-kn_fop-f_event() from knote() may not be called, I think.
In knote(), because the context holds knlist's lock, the context can
not sleep. So, KN_INFLUX should not be set on calling
kn-kn_fop-f_event() in kqueue_scan(), I think.
What do you think about this issue?
Best regards,
Kohji Okuno
#include sys/types.h
#include stdlib.h
#include unistd.h
#include stdio.h
int
main()
{
sleep(1);
exit(0);
}
#include sys/types.h
#include sys/wait.h
#include sys/event.h
#include sys/time.h
#include stdlib.h
#include stdio.h
#include unistd.h
#include signal.h
#define NUM_CHILDREN20
int
main()
{
int i;
pid_t pid;
char *argv[2] = {child, NULL};
struct kevent kev;
int kqfd = kqueue();
int count;
int err;
int status;
EV_SET(kev, SIGCHLD, EVFILT_SIGNAL, EV_ADD, 0, 0, 0);
kevent(kqfd, kev, 1, NULL, 0, NULL);
while (1) {
count = 0;
for (i = 0; i NUM_CHILDREN; i++) {
pid = fork();
if (pid == 0) {
execve(./child, argv, NULL);
}
}
while (1) {
err = kevent(kqfd, NULL, 0, kev, 1, NULL);
if (err 0 kev.ident == SIGCHLD) {
for (i = 0; i kev.data; i++) {
pid = waitpid(-1, status, WNOHANG);
if (pid 0) {
count++;
printf(%d , count);
fflush(stdout);
if (count == NUM_CHILDREN) {
printf(\nOK\n);
goto next;
}
}
}
}
}
next:
;
}
exit(0);
}
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: About kevent
Kohji Okuno wrote this message on Fri, Feb 28, 2014 at 11:13 +0900: I have a question about kevent. How should the userland judge knote which is cleared from knlist by knlist_clear() or knlist_delete()? It looks like I need to read the code better... knlist_clear (killkn=0) and knlist_delete (killkn=1) are wrappers around knlist_cleardel... Looking at the code of knlist_cleardel, if killkn is set (knlist_delete) the knote will be dropped (free'd)... if it is not set, the flags _EOF and _ONESHOT will be set such that it'll be returned soon.. Now that I look at the code, KNOTE_ACTIVATE is never called to be put on the list to be delivered, so now I'm not sure if it works the way it's suppose to... I have a feeling that the notes might hang around forever until the kq fd is closed... I'm also puzzled as to why _DETACHED isn't set, which would seem to mean that we'll call f_detach when we close the kq, which I assume could cause a panic... This needs to be investigated/tested... -- John-Mark GurneyVoice: +1 415 225 5579 All that I will do, has been done, All that I have, has not. Hi, Thank you for your comment. I tried test by usb_dev. When a USB device is lost suddenly, I can receive EV_EOF|EV_ONESHOT on kevent-flags. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for usb_dev
Hi HPS and John-Mark, We should wait for empty of knlist before knlist_destroy(). When I tried 262551, the kernel panic at KN_LIST_LOCK(kn) in kern_event.c. Regards, Kohji Okuno On 02/27/14 10:00, Hans Petter Selasky wrote: Hi Kohji, Can you verify this commit: http://svnweb.freebsd.org/changeset/base/262550 Please test using both read and write direction. For example you can use the ULPT driver or a /dev/usb/X.X.X node which supports both read and write. Thank you! --HPS And this commit too: http://svnweb.freebsd.org/changeset/base/262551 Thank you! --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for usb_dev
Hi HPS and John-Mark, After I changed as the following, the kernel panic does not happen. What do you think about this change? + knlist_clear(f-selinfo.si_note, 0); knlist_destroy(f-selinfo.si_note); Regards, Kohji Okuno We should wait for empty of knlist before knlist_destroy(). When I tried 262551, the kernel panic at KN_LIST_LOCK(kn) in kern_event.c. Regards, Kohji Okuno On 02/27/14 10:00, Hans Petter Selasky wrote: Hi Kohji, Can you verify this commit: http://svnweb.freebsd.org/changeset/base/262550 Please test using both read and write direction. For example you can use the ULPT driver or a /dev/usb/X.X.X node which supports both read and write. Thank you! --HPS And this commit too: http://svnweb.freebsd.org/changeset/base/262551 Thank you! --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for KBD.
Hi John-Mark,
Thank you for your comment.
I added knote_clear() and knote_destroy() in kbd_detach(). I attached
patch, again. But, maybe this patch can not resolve all cases you
pointed.
Regards,
Kohji Okuno
Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:24 +0900:
I tried to add kqueue I/F to kbd.c. I attached patch.
What do you think about my patch?
So, knlist_destroy is missing in this patch too..
It also needs some style(9) loving in that some blank lines are missing
and there are some extra curly braces...
So, knlist_clear is usually used for something like close where it
cannot be used again... You use knlist_clear when the kbd goes away,
but this also means that the user will never be notified that the kbd
has gone, and could possibly end up leaking resources...
I guess I should maybe write a function knlist_clearerr or something
that detaches all the knotes from the knlist and sets the proper flag
so that they can be reaped by userland... I believe your usb patch
had a similar issue and some of the other drivers have this issue too..
Otherwise looks good...
--
John-Mark GurneyVoice: +1 415 225 5579
All that I will do, has been done, All that I have, has not.
diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
index 8036762..26dcaad 100644
--- a/sys/dev/kbd/kbd.c
+++ b/sys/dev/kbd/kbd.c
@@ -59,6 +59,7 @@ typedef struct genkbd_softc {
char gkb_q[KB_QSIZE]; /* input queue */
unsigned int gkb_q_start;
unsigned int gkb_q_length;
+ unsigned int gkb_index;
} genkbd_softc_t;
static SLIST_HEAD(, keyboard_driver) keyboard_drivers =
@@ -472,6 +473,7 @@ static d_read_t genkbdread;
static d_write_t genkbdwrite;
static d_ioctl_t genkbdioctl;
static d_poll_t genkbdpoll;
+static d_kqfilter_t genkbdkqfilter;
static struct cdevsw kbd_cdevsw = {
@@ -483,12 +485,14 @@ static struct cdevsw kbd_cdevsw = {
.d_write = genkbdwrite,
.d_ioctl = genkbdioctl,
.d_poll = genkbdpoll,
+ .d_kqfilter = genkbdkqfilter,
.d_name = kbd,
};
int
kbd_attach(keyboard_t *kbd)
{
+ genkbd_softc_t *sc;
if (kbd-kb_index = keyboards)
return (EINVAL);
@@ -498,8 +502,11 @@ kbd_attach(keyboard_t *kbd)
kbd-kb_dev = make_dev(kbd_cdevsw, kbd-kb_index, UID_ROOT, GID_WHEEL,
0600, %s%r, kbd-kb_name, kbd-kb_unit);
make_dev_alias(kbd-kb_dev, kbd%r, kbd-kb_index);
- kbd-kb_dev-si_drv1 = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
+ sc = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
M_WAITOK | M_ZERO);
+ kbd-kb_dev-si_drv1 = sc;
+ sc-gkb_index = KBD_INDEX(kbd-kb_dev);
+ knlist_init_mtx(sc-gkb_rsel.si_note, NULL);
printf(kbd%d at %s%d\n, kbd-kb_index, kbd-kb_name, kbd-kb_unit);
return (0);
}
@@ -507,12 +514,17 @@ kbd_attach(keyboard_t *kbd)
int
kbd_detach(keyboard_t *kbd)
{
+ genkbd_softc_t *sc;
if (kbd-kb_index = keyboards)
return (EINVAL);
if (keyboard[kbd-kb_index] != kbd)
return (EINVAL);
+ sc = kbd-kb_dev-si_drv1;
+ knlist_clear(sc-gkb_rsel.si_note, 0);
+ knlist_destroy(sc-gkb_rsel.si_note);
+
free(kbd-kb_dev-si_drv1, M_DEVBUF);
destroy_dev(kbd-kb_dev);
@@ -697,6 +709,71 @@ genkbdioctl(struct cdev *dev, u_long cmd, caddr_t arg, int flag, struct thread *
return (error);
}
+static void
+genkbd_kqops_detach(struct knote *kn)
+{
+ genkbd_softc_t *sc;
+ sc = kn-kn_hook;
+ knlist_remove(sc-gkb_rsel.si_note, kn, 0);
+}
+
+static int
+genkbd_kqops_event(struct knote *kn, long hint)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+
+ sc = kn-kn_hook;
+ kbd = kbd_get_keyboard(sc-gkb_index);
+
+ if ((kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ return 1; /* the keyboard has gone */
+ }
+ else {
+ if (sc-gkb_q_length 0)
+ return 1;
+ else
+ return 0;
+ }
+
+}
+static struct filterops genkbd_kqops =
+{
+ .f_isfd = 1,
+ .f_attach = NULL,
+ .f_detach = genkbd_kqops_detach,
+ .f_event = genkbd_kqops_event,
+};
+static int
+genkbdkqfilter(struct cdev *dev, struct knote *kn)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+ int error = 0;
+ int s;
+
+ s = spltty();
+ sc = dev-si_drv1;
+ kbd = kbd_get_keyboard(KBD_INDEX(dev));
+ if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ error = EIO;
+ }
+ else {
+ switch (kn-kn_filter) {
+ case EVFILT_READ:
+ kn-kn_fop = genkbd_kqops;
+ kn-kn_hook = (void *)sc;
+ knlist_add(sc-gkb_rsel.si_note, kn, 0);
+ break;
+ default:
+ error = EOPNOTSUPP;
+ break;
+ }
+ }
+ splx(s);
+ return (error);
+}
+
static int
genkbdpoll(struct cdev *dev, int events, struct thread *td)
{
@@ -744,6 +821,7 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+ KNOTE_UNLOCKED(sc-gkb_rsel.si_note, 0);
return (0);
default:
return (EINVAL);
@@ -814,6 +892,7 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+ KNOTE_UNLOCKED(sc-gkb_rsel.si_note, 0);
}
return (0);
___
freebsd-current
Re: kqueue for usb_dev
Hi HPS, Your patch did not resolve the kernel panic. I think, we should check knlist_clear() before knlist_destroy(). When a device is lost suddenly, usb_dev notify to a process in usb_fifo_close() and then calls knlist_destroy(). knlist_destroy() clears knlist-kn_lock and knlist-kn_unlock. But, the process that is notified will start over kqueue_scan() after knlist_destroy(). And, in KN_LIST_LOCK(kn), the context will call NULL function (kn-knlist-kn_lock). Regards, Kohji Okuno On 02/27/14 11:39, Kohji Okuno wrote: Hi HPS and John-Mark, After I changed as the following, the kernel panic does not happen. What do you think about this change? + knlist_clear(f-selinfo.si_note, 0); knlist_destroy(f-selinfo.si_note); Regards, Kohji Okuno Can you try the attached patch instead? --HPS ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
About kevent
Hi, I have a question about kevent. How should the userland judge knote which is cleared from knlist by knlist_clear() or knlist_delete()? Best regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
kqueue for usb_dev
Hi,
I tried add kqueue I/F to usb_dev.c. I attached my patch.
What do you think about my patch?
Best regards,
Kohji Okuno
diff --git a/sys/dev/usb/usb_dev.c b/sys/dev/usb/usb_dev.c
index f086a3c..4334be7 100644
--- a/sys/dev/usb/usb_dev.c
+++ b/sys/dev/usb/usb_dev.c
@@ -120,6 +120,9 @@ static d_ioctl_t usb_ioctl;
static d_read_t usb_read;
static d_write_t usb_write;
static d_poll_t usb_poll;
+#if 1
+static d_kqfilter_t usb_kqfilter;
+#endif
static d_ioctl_t usb_static_ioctl;
@@ -138,6 +141,10 @@ struct cdevsw usb_devsw = {
.d_read = usb_read,
.d_write = usb_write,
.d_poll = usb_poll
+#if 1
+ ,
+ .d_kqfilter = usb_kqfilter
+#endif
};
static struct cdev* usb_dev = NULL;
@@ -505,6 +512,9 @@ usb_fifo_create(struct usb_cdev_privdata *cpd,
f-fifo_index = n + USB_FIFO_TX;
f-dev_ep_index = e;
f-priv_mtx = udev-device_mtx;
+#if 1
+ knlist_init_mtx(f-selinfo.si_note, f-priv_mtx);
+#endif
f-priv_sc0 = ep;
f-methods = usb_ugen_methods;
f-iface_index = ep-iface_index;
@@ -532,6 +542,9 @@ usb_fifo_create(struct usb_cdev_privdata *cpd,
f-fifo_index = n + USB_FIFO_RX;
f-dev_ep_index = e;
f-priv_mtx = udev-device_mtx;
+#if 1
+ knlist_init_mtx(f-selinfo.si_note, f-priv_mtx);
+#endif
f-priv_sc0 = ep;
f-methods = usb_ugen_methods;
f-iface_index = ep-iface_index;
@@ -712,6 +725,11 @@ usb_fifo_open(struct usb_cdev_privdata *cpd,
/* reset select flag */
f-flag_isselect = 0;
+#if 1
+ /* reset kevent flag */
+ f-flag_iskevent = 0;
+#endif
+
/* reset flushing flag */
f-flag_flushing = 0;
@@ -772,6 +790,13 @@ usb_fifo_close(struct usb_fifo *f, int fflags)
/* clear current cdev private data pointer */
f-curr_cpd = NULL;
+#if 1
+ /* check if we are watched by kevent */
+ if (f-flag_iskevent) {
+ KNOTE_LOCKED(f-selinfo.si_note, 0);
+ f-flag_iskevent = 0;
+ }
+#endif
/* check if we are selected */
if (f-flag_isselect) {
selwakeup(f-selinfo);
@@ -1113,6 +1138,179 @@ done:
return (err);
}
+#if 1
+static void
+usb_filter_detach(struct knote *kn)
+{
+ struct usb_fifo *f = kn-kn_hook;
+ knlist_remove(f-selinfo.si_note, kn, 0);
+}
+
+static int
+usb_filter_write(struct knote *kn, long hint)
+{
+ struct usb_cdev_privdata* cpd;
+ struct usb_fifo *f;
+ struct usb_mbuf *m;
+ int is_usbfs = 0;
+
+ f = kn-kn_hook;
+ cpd = f-curr_cpd;
+ if (cpd == NULL) {
+ return (1);
+ }
+
+ if (f-fs_ep_max != 0) {
+ is_usbfs = 1;
+ }
+
+ if (!is_usbfs) {
+ if (f-flag_iserror) {
+ /* we got an error */
+ m = (void *)1;
+ } else {
+ if (f-queue_data == NULL) {
+/*
+ * start write transfer, if not
+ * already started
+ */
+(f-methods-f_start_write) (f);
+ }
+ /* check if any packets are available */
+ USB_IF_POLL(f-free_q, m);
+ }
+ } else {
+ if (f-flag_iscomplete) {
+ m = (void *)1;
+ } else {
+ m = NULL;
+ }
+ }
+
+ if (m) {
+ return (1);
+ } else {
+ return (0);
+ }
+}
+
+static int
+usb_filter_read(struct knote *kn, long hint)
+{
+ struct usb_cdev_privdata* cpd;
+ struct usb_fifo *f;
+ struct usb_mbuf *m;
+ int is_usbfs = 0;
+
+ f = kn-kn_hook;
+ cpd = f-curr_cpd;
+ if (cpd == NULL) {
+ return (1);
+ }
+
+ if (f-fs_ep_max != 0) {
+ is_usbfs = 1;
+ }
+
+ if (!is_usbfs) {
+ if (f-flag_iserror) {
+ /* we have and error */
+ m = (void *)1;
+ } else {
+ if (f-queue_data == NULL) {
+/*
+ * start read transfer, if not
+ * already started
+ */
+(f-methods-f_start_read) (f);
+ }
+ /* check if any packets are available */
+ USB_IF_POLL(f-used_q, m);
+ }
+ } else {
+ if (f-flag_iscomplete) {
+ m = (void *)1;
+ } else {
+ m = NULL;
+ }
+ }
+
+ if (m) {
+ return (1);
+ } else {
+ if (!is_usbfs) {
+ /* start reading data */
+ (f-methods-f_start_read) (f);
+ }
+ return (0);
+ }
+}
+
+static struct filterops usb_filtops_write =
+{
+ .f_isfd = 1,
+ .f_detach = usb_filter_detach,
+ .f_event = usb_filter_write,
+};
+
+static struct filterops usb_filtops_read =
+{
+ .f_isfd = 1,
+ .f_detach = usb_filter_detach,
+ .f_event = usb_filter_read,
+};
+
+
+/* ARGSUSED */
+static int
+usb_kqfilter(struct cdev* dev, struct knote *kn)
+{
+ struct usb_cdev_refdata refs;
+ struct usb_cdev_privdata* cpd;
+ struct usb_fifo *f;
+ int fflags;
+ int err = EINVAL;
+
+ if (devfs_get_cdevpriv((void **)cpd) != 0 ||
+ usb_ref_device(cpd, refs, 0) != 0)
+ return (ENXIO);
+
+ fflags = cpd-fflags;
+
+ /* Figure out who needs service */
+ switch (kn-kn_filter) {
+ case EVFILT_WRITE:
+ if (fflags FWRITE) {
+ f = refs.txfifo;
+ kn-kn_fop = usb_filtops_write;
+ err = 0;
+ }
+ break;
+ case EVFILT_READ:
+ if (fflags FREAD) {
+ f = refs.rxfifo;
+ kn-kn_fop = usb_filtops_read;
+ err = 0;
+ }
+ break;
+ default:
+ err = EOPNOTSUPP;
+ break;
+ }
+
+ if (err == 0) {
+ kn-kn_hook = f;
+ mtx_lock(f-priv_mtx);
+ knlist_add(f-selinfo.si_note, kn, 1);
+ f-flag_iskevent = 1;
+ mtx_unlock(f-priv_mtx);
+ }
+
+ usb_unref_device(cpd, refs);
+ return (err);
+}
+#endif
+
/* ARGSUSED */
static int
usb_poll
kqueue for KBD.
Hi,
I tried add kqueue I/F to kbd.c. I attached my patch.
What do you think about my patch?
Best regards,
Kohji Okuno
diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
index 8036762..df000ab 100644
--- a/sys/dev/kbd/kbd.c
+++ b/sys/dev/kbd/kbd.c
@@ -59,6 +59,9 @@ typedef struct genkbd_softc {
char gkb_q[KB_QSIZE]; /* input queue */
unsigned int gkb_q_start;
unsigned int gkb_q_length;
+#if 1
+ unsigned int gkb_index;
+#endif
} genkbd_softc_t;
static SLIST_HEAD(, keyboard_driver) keyboard_drivers =
@@ -472,6 +475,9 @@ static d_read_t genkbdread;
static d_write_t genkbdwrite;
static d_ioctl_t genkbdioctl;
static d_poll_t genkbdpoll;
+#if 1
+static d_kqfilter_t genkbdkqfilter;
+#endif
static struct cdevsw kbd_cdevsw = {
@@ -483,6 +489,9 @@ static struct cdevsw kbd_cdevsw = {
.d_write = genkbdwrite,
.d_ioctl = genkbdioctl,
.d_poll = genkbdpoll,
+#if 1
+ .d_kqfilter = genkbdkqfilter,
+#endif
.d_name = kbd,
};
@@ -500,6 +509,13 @@ kbd_attach(keyboard_t *kbd)
make_dev_alias(kbd-kb_dev, kbd%r, kbd-kb_index);
kbd-kb_dev-si_drv1 = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
M_WAITOK | M_ZERO);
+#if 1
+ {
+ genkbd_softc_t *sc = kbd-kb_dev-si_drv1;
+ sc-gkb_index = KBD_INDEX(kbd-kb_dev);
+ knlist_init_mtx(sc-gkb_rsel.si_note, NULL);
+ }
+#endif
printf(kbd%d at %s%d\n, kbd-kb_index, kbd-kb_name, kbd-kb_unit);
return (0);
}
@@ -697,6 +713,73 @@ genkbdioctl(struct cdev *dev, u_long cmd, caddr_t arg, int flag, struct thread *
return (error);
}
+#if 1
+static void
+genkbd_kqops_detach(struct knote *kn)
+{
+ genkbd_softc_t *sc;
+ sc = kn-kn_hook;
+ knlist_remove(sc-gkb_rsel.si_note, kn, 0);
+}
+
+static int
+genkbd_kqops_event(struct knote *kn, long hint)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+
+ sc = kn-kn_hook;
+ kbd = kbd_get_keyboard(sc-gkb_index);
+
+ if ((kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ return 1; /* the keyboard has gone */
+ }
+ else {
+ if (sc-gkb_q_length 0)
+ return 1;
+ else
+ return 0;
+ }
+
+}
+static struct filterops genkbd_kqops =
+{
+ .f_isfd = 1,
+ .f_attach = NULL,
+ .f_detach = genkbd_kqops_detach,
+ .f_event = genkbd_kqops_event,
+};
+static int
+genkbdkqfilter(struct cdev *dev, struct knote *kn)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+ int error = 0;
+ int s;
+
+ s = spltty();
+ sc = dev-si_drv1;
+ kbd = kbd_get_keyboard(KBD_INDEX(dev));
+ if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ error = EIO;
+ }
+ else {
+ switch (kn-kn_filter) {
+ case EVFILT_READ:
+ kn-kn_fop = genkbd_kqops;
+ kn-kn_hook = (void *)sc;
+ knlist_add(sc-gkb_rsel.si_note, kn, 0);
+ break;
+ default:
+ error = EOPNOTSUPP;
+ break;
+ }
+ }
+ splx(s);
+ return (error);
+}
+#endif
+
static int
genkbdpoll(struct cdev *dev, int events, struct thread *td)
{
@@ -744,6 +827,9 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+#if 1
+ knlist_clear(sc-gkb_rsel.si_note, 0);
+#endif
return (0);
default:
return (EINVAL);
@@ -814,6 +900,9 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+#if 1
+ KNOTE_UNLOCKED(sc-gkb_rsel.si_note, 0);
+#endif
}
return (0);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
kqueue for KBD.
Hi,
I tried to add kqueue I/F to kbd.c. I attached patch.
What do you think about my patch?
Best regards,
Kohji Okuno
diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
index 8036762..df000ab 100644
--- a/sys/dev/kbd/kbd.c
+++ b/sys/dev/kbd/kbd.c
@@ -59,6 +59,9 @@ typedef struct genkbd_softc {
char gkb_q[KB_QSIZE]; /* input queue */
unsigned int gkb_q_start;
unsigned int gkb_q_length;
+#if 1
+ unsigned int gkb_index;
+#endif
} genkbd_softc_t;
static SLIST_HEAD(, keyboard_driver) keyboard_drivers =
@@ -472,6 +475,9 @@ static d_read_t genkbdread;
static d_write_t genkbdwrite;
static d_ioctl_t genkbdioctl;
static d_poll_t genkbdpoll;
+#if 1
+static d_kqfilter_t genkbdkqfilter;
+#endif
static struct cdevsw kbd_cdevsw = {
@@ -483,6 +489,9 @@ static struct cdevsw kbd_cdevsw = {
.d_write = genkbdwrite,
.d_ioctl = genkbdioctl,
.d_poll = genkbdpoll,
+#if 1
+ .d_kqfilter = genkbdkqfilter,
+#endif
.d_name = kbd,
};
@@ -500,6 +509,13 @@ kbd_attach(keyboard_t *kbd)
make_dev_alias(kbd-kb_dev, kbd%r, kbd-kb_index);
kbd-kb_dev-si_drv1 = malloc(sizeof(genkbd_softc_t), M_DEVBUF,
M_WAITOK | M_ZERO);
+#if 1
+ {
+ genkbd_softc_t *sc = kbd-kb_dev-si_drv1;
+ sc-gkb_index = KBD_INDEX(kbd-kb_dev);
+ knlist_init_mtx(sc-gkb_rsel.si_note, NULL);
+ }
+#endif
printf(kbd%d at %s%d\n, kbd-kb_index, kbd-kb_name, kbd-kb_unit);
return (0);
}
@@ -697,6 +713,73 @@ genkbdioctl(struct cdev *dev, u_long cmd, caddr_t arg, int flag, struct thread *
return (error);
}
+#if 1
+static void
+genkbd_kqops_detach(struct knote *kn)
+{
+ genkbd_softc_t *sc;
+ sc = kn-kn_hook;
+ knlist_remove(sc-gkb_rsel.si_note, kn, 0);
+}
+
+static int
+genkbd_kqops_event(struct knote *kn, long hint)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+
+ sc = kn-kn_hook;
+ kbd = kbd_get_keyboard(sc-gkb_index);
+
+ if ((kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ return 1; /* the keyboard has gone */
+ }
+ else {
+ if (sc-gkb_q_length 0)
+ return 1;
+ else
+ return 0;
+ }
+
+}
+static struct filterops genkbd_kqops =
+{
+ .f_isfd = 1,
+ .f_attach = NULL,
+ .f_detach = genkbd_kqops_detach,
+ .f_event = genkbd_kqops_event,
+};
+static int
+genkbdkqfilter(struct cdev *dev, struct knote *kn)
+{
+ keyboard_t *kbd;
+ genkbd_softc_t *sc;
+ int error = 0;
+ int s;
+
+ s = spltty();
+ sc = dev-si_drv1;
+ kbd = kbd_get_keyboard(KBD_INDEX(dev));
+ if ((sc == NULL) || (kbd == NULL) || !KBD_IS_VALID(kbd)) {
+ error = EIO;
+ }
+ else {
+ switch (kn-kn_filter) {
+ case EVFILT_READ:
+ kn-kn_fop = genkbd_kqops;
+ kn-kn_hook = (void *)sc;
+ knlist_add(sc-gkb_rsel.si_note, kn, 0);
+ break;
+ default:
+ error = EOPNOTSUPP;
+ break;
+ }
+ }
+ splx(s);
+ return (error);
+}
+#endif
+
static int
genkbdpoll(struct cdev *dev, int events, struct thread *td)
{
@@ -744,6 +827,9 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+#if 1
+ knlist_clear(sc-gkb_rsel.si_note, 0);
+#endif
return (0);
default:
return (EINVAL);
@@ -814,6 +900,9 @@ genkbd_event(keyboard_t *kbd, int event, void *arg)
wakeup(sc);
}
selwakeuppri(sc-gkb_rsel, PZERO);
+#if 1
+ KNOTE_UNLOCKED(sc-gkb_rsel.si_note, 0);
+#endif
}
return (0);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for usb_dev
Hi John-Mark,
I tested the attached sample source with USB mouse.
Thanks,
Kohji Okuno
From: John-Mark Gurney j...@funkthat.com
Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:26 +0900:
I tried add kqueue I/F to usb_dev.c. I attached my patch.
What do you think about my patch?
Do you have test cases for these patches?
--
John-Mark GurneyVoice: +1 415 225 5579
All that I will do, has been done, All that I have, has not.
#include stdio.h
#include stdlib.h
#include stdint.h
#include sys/types.h
#include sys/event.h
#include sys/select.h
#include sys/time.h
#include unistd.h
#include fcntl.h
#include errno.h
#define DEV /dev/ums0
#if 0
int
main()
{
int i;
ssize_t ret;
uint8_t buf[128];
int fd = open(DEV, O_RDONLY);
fd_set readfds;
for (i = 0; i 10; i++) {
FD_ZERO(readfds);
FD_SET(fd, readfds);
ret = select(fd+1, readfds, NULL, NULL, NULL);
printf(select=%d\n, ret);
ret = read(fd, buf, sizeof(buf));
printf(%d:%02x %02x %02x\n, ret, buf[0], buf[1], buf[2]);
}
close(fd);
exit(0);
}
#else
int
main()
{
int i;
int err;
ssize_t ret;
uint8_t buf[128];
int fd = open(DEV, O_RDONLY);
int kqfd = kqueue();
struct kevent evlist[1];
EV_SET(evlist[0], fd, EVFILT_READ, EV_ADD, 0, 0, 0);
err = kevent(kqfd, evlist, 1, 0, 0, 0);
if (err) {
perror(kevent);
close(fd);
close(kqfd);
exit(1);
}
for (i = 0; i 10; i++) {
ret = kevent(kqfd, 0, 0, evlist, 1, 0);
printf(kev=%d\n, ret);
ret = read(fd, buf, sizeof(buf));
printf(%d:%02x %02x %02x\n, ret, buf[0], buf[1], buf[2]);
}
close(fd);
exit(0);
}
#endif
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for usb_dev
Hi John-Mark, Thank you for you comment. From: John-Mark Gurney j...@funkthat.com Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:26 +0900: I tried add kqueue I/F to usb_dev.c. I attached my patch. What do you think about my patch? A few comments... 1) You should just drop the use of flag_iskevent and just unconditionally call KNOTE... since you have the lock already held, the cost is minimal (and w/ modern branch prediction, may be cheaper)... Should we set the use of flag_iskevent, when usb_filter_read() and usb_filter_write() return `0'? 2) Why do you try to start read/write transfers in the _filter? You should just check to see if data is available and not do work.. This is also important since kqueue calls the filter just before delivering the knote to userland to verify that there is still data, and it will call your _event function for each knote on the fd... The work should be started through other mechanisms, like read/write syscall or interrupt or timeout/callout... If it's required to get results from USB_IF_POLL, then it's fine.. I copied from usb_poll(). Should we try to start read/write transfers in usb_kqfilter()? Or should not we try to start read/write transfers in poll and kqueue? 3) I don't see any calls to knlist_destroy... These calls are needed to clean up the knlist... I understood. Obviously the #if 1's will need to go... Also, I don't think your change is against HEAD.. The line numbers in my version of usb_dev.c are different... I'm sorry. Many thanks, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: kqueue for usb_dev
From: Hans Petter Selasky h...@bitfrost.no On 02/27/14 08:13, Kohji Okuno wrote: Hi John-Mark, Thank you for you comment. From: John-Mark Gurney j...@funkthat.com Kohji Okuno wrote this message on Thu, Feb 27, 2014 at 14:26 +0900: I tried add kqueue I/F to usb_dev.c. I attached my patch. What do you think about my patch? A few comments... 1) You should just drop the use of flag_iskevent and just unconditionally call KNOTE... since you have the lock already held, the cost is minimal (and w/ modern branch prediction, may be cheaper)... Should we set the use of flag_iskevent, when usb_filter_read() and usb_filter_write() return `0'? 2) Why do you try to start read/write transfers in the _filter? You should just check to see if data is available and not do work.. This is also important since kqueue calls the filter just before delivering the knote to userland to verify that there is still data, and it will call your _event function for each knote on the fd... The work should be started through other mechanisms, like read/write syscall or interrupt or timeout/callout... If it's required to get results from USB_IF_POLL, then it's fine.. I copied from usb_poll(). Should we try to start read/write transfers in usb_kqfilter()? Or should not we try to start read/write transfers in poll and kqueue? 3) I don't see any calls to knlist_destroy... These calls are needed to clean up the knlist... I understood. Obviously the #if 1's will need to go... Also, I don't think your change is against HEAD.. The line numbers in my version of usb_dev.c are different... I'm sorry. Hi, I've found two bugs: 1) +#if 1 + knlist_init_mtx(f_tx-selinfo.si_note, f_rx-priv_mtx); +#endif Should be: +#if 1 + knlist_init_mtx(f_tx-selinfo.si_note, f_tx-priv_mtx); +#endif 2) Event filters need to lock the FIFO's mutex. BTW: I'm working on getting the code into -HEAD. I'll run some test before it goes in. Hi, Thank you for you comment. 1) You are right. 2) I think that priv_mtx is hold in caller function. Would you refer to kqueue_scan() in kern/kern_event.c? Thanks, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Mon, 16 Dec 2013 11:53:31 +0100 Hi Kohji, A regression issue has been reported when using the CHAIN-BIT patch. Can you verify this additional patch on you hardware and report back? http://svnweb.freebsd.org/changeset/base/259462 Thank you! --HPS Hi HPS, Thank you for informing your commit. I tried your latest patch. It is OK in my environment. Many thanks, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Thu, 12 Dec 2013 09:37:29 +0100 On 12/12/13 08:40, Kohji Okuno wrote: From: Hans Petter Selasky h...@bitfrost.no Hi HPS, The endpoint type is BULK, and the direction is OUT. I checked by using a USB analyzer. When I did not set CHAIN bit in LINK TRB, my host controller sent illegal packets sometimes. But, ZLPs were sent. Hi, Test OK here aswell. Does this commit look OK to you: http://svnweb.freebsd.org/changeset/base/259248 Hi HPS, I confirmed that your commit is OK. Many thanks, Kohji Okuno. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
spec violation of xHCI?
Hi,
I think the xHCI host controller driver has a spec violation.
Could you refer to
``Table 126: Offset 0Ch – Link TRB Field Definitions''
in xHCI_Specification_for_USB.pdf(Revision 1.0)?
The following is an excerpt about the CHAIN BIT.
Chain bit (CH). Set to ‘1’ by software to associate this TRB with
the next TRB on the Ring. A Transfer Descriptor (TD) is defined as
one or more TRBs. The Chain bit is used to identify the TRBs that
comprise a TD. Refer to section 4.11.7 for more information on Link
TRB placement within a TD. On a Command Ring this bit is ignored by
the xHC.
I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895.
How do you think?
src/sys/dev/usb/controller/xhci.c:
1879/* fill out link TRB */
1880
1881if (td_next != NULL) {
1882/* link the current TD with the next one */
1883td-td_trb[x].qwTrb0 =
htole64((uint64_t)td_next-td_self);
1884DPRINTF(LINK=0x%08llx\n, (long
long)td_next-td_self);
1885} else {
1886/* this field will get updated later */
1887DPRINTF(NOLINK\n);
1888}
1889
1890dword = XHCI_TRB_2_IRQ_SET(0);
1891
1892td-td_trb[x].dwTrb2 = htole32(dword);
1893
1894dword = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
1895XHCI_TRB_3_CYCLE_BIT | XHCI_TRB_3_IOC_BIT;
1896
1897td-td_trb[x].dwTrb3 = htole32(dword);
1898
1899td-alt_next = td_alt_next;
--
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
On 12/11/13 11:12, Kohji Okuno wrote:
Hi,
I think the xHCI host controller driver has a spec violation.
Could you refer to
``Table 126: Offset 0Ch – Link TRB Field Definitions''
in xHCI_Specification_for_USB.pdf(Revision 1.0)?
The following is an excerpt about the CHAIN BIT.
Chain bit (CH). Set to ‘1’ by software to associate this TRB with
the next TRB on the Ring. A Transfer Descriptor (TD) is defined as
one or more TRBs. The Chain bit is used to identify the TRBs that
comprise a TD. Refer to section 4.11.7 for more information on Link
TRB placement within a TD. On a Command Ring this bit is ignored by
the xHC.
I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895.
How do you think?
Hi Kohji,
The double word written at line 1895 does not set the chain bit because this
is the end of a transfer descriptor, TD. I'm unsure how hardware interprets
this bit, if setting the bit on the previous TRB makes the next one connect to
the previous one, or the other way around. If setting this bit makes the TRB
connect to the previous one, you are correct. Else the current code is
correct.
Hi, HPS,
Thank you for your comment.
I think that this (line 1895) is not the end of a transfer descriptor.
When the device driver needs a Zero Length Packet, this is not the
end. And, If xfer has nframes, this is not the end, too.
Regards,
Kohji Okuno
src/sys/dev/usb/controller/xhci.c:
1879 /* fill out link TRB */
1880
1881 if (td_next != NULL) {
1882 /* link the current TD with the next one */
1883 td-td_trb[x].qwTrb0 = htole64((uint64_t)td_next-td_self);
1884 DPRINTF(LINK=0x%08llx\n, (long long)td_next-td_self);
1885 } else {
1886 /* this field will get updated later */
1887 DPRINTF(NOLINK\n);
1888 }
1889
1890 dword = XHCI_TRB_2_IRQ_SET(0);
1891
1892 td-td_trb[x].dwTrb2 = htole32(dword);
1893
1894 dword = XHCI_TRB_3_TYPE_SET(XHCI_TRB_TYPE_LINK) |
1895 XHCI_TRB_3_CYCLE_BIT | XHCI_TRB_3_IOC_BIT;
1896
1897 td-td_trb[x].dwTrb3 = htole32(dword);
1898
1899 td-alt_next = td_alt_next;
--
Best regards,
Kohji Okuno
___
freebsd-...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-usb
To unsubscribe, send any mail to freebsd-usb-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Wed, 11 Dec 2013 13:44:37 +0100 On 12/11/13 12:12, Kohji Okuno wrote: On 12/11/13 11:12, Kohji Okuno wrote: Hi, I think the xHCI host controller driver has a spec violation. Could you refer to ``Table 126: Offset 0Ch – Link TRB Field Definitions'' in xHCI_Specification_for_USB.pdf(Revision 1.0)? The following is an excerpt about the CHAIN BIT. Chain bit (CH). Set to ‘1’ by software to associate this TRB with the next TRB on the Ring. A Transfer Descriptor (TD) is defined as one or more TRBs. The Chain bit is used to identify the TRBs that comprise a TD. Refer to section 4.11.7 for more information on Link TRB placement within a TD. On a Command Ring this bit is ignored by the xHC. I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. How do you think? Hi Kohji, The double word written at line 1895 does not set the chain bit because this is the end of a transfer descriptor, TD. I'm unsure how hardware interprets this bit, if setting the bit on the previous TRB makes the next one connect to the previous one, or the other way around. If setting this bit makes the TRB connect to the previous one, you are correct. Else the current code is correct. Hi, HPS, Thank you for your comment. I think that this (line 1895) is not the end of a transfer descriptor. When the device driver needs a Zero Length Packet, this is not the end. And, If xfer has nframes, this is not the end, too. Regards, Kohji Okuno Hi Kohji, Yes, you are right that if nframes is greater than one, and/or if a ZLP needs to be sent this is not the end of the USB transfers. Are we sure that if the XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will receive a completion TRB-event for each of the nframes, or will the chain bit result in loss of TRB completion events? Does setting this bit have any impact on performance? Thank you! --HPS Hi HPS, I don't know about any impact on performance. But, in linux, link trb has CHAIN BIT if necessary, I think. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Wed, 11 Dec 2013 13:50:50 +0100 On 12/11/13 13:44, Hans Petter Selasky wrote: On 12/11/13 12:12, Kohji Okuno wrote: On 12/11/13 11:12, Kohji Okuno wrote: Hi, I think the xHCI host controller driver has a spec violation. Could you refer to ``Table 126: Offset 0Ch – Link TRB Field Definitions'' in xHCI_Specification_for_USB.pdf(Revision 1.0)? The following is an excerpt about the CHAIN BIT. Chain bit (CH). Set to ‘1’ by software to associate this TRB with the next TRB on the Ring. A Transfer Descriptor (TD) is defined as one or more TRBs. The Chain bit is used to identify the TRBs that comprise a TD. Refer to section 4.11.7 for more information on Link TRB placement within a TD. On a Command Ring this bit is ignored by the xHC. I think that we should add XHCI_TRB_3_CHAIN_BIT to line 1895. How do you think? Hi Kohji, The double word written at line 1895 does not set the chain bit because this is the end of a transfer descriptor, TD. I'm unsure how hardware interprets this bit, if setting the bit on the previous TRB makes the next one connect to the previous one, or the other way around. If setting this bit makes the TRB connect to the previous one, you are correct. Else the current code is correct. Hi, HPS, Thank you for your comment. I think that this (line 1895) is not the end of a transfer descriptor. When the device driver needs a Zero Length Packet, this is not the end. And, If xfer has nframes, this is not the end, too. Regards, Kohji Okuno Hi Kohji, Yes, you are right that if nframes is greater than one, and/or if a ZLP needs to be sent this is not the end of the USB transfers. Are we sure that if the XHCI_TRB_3_CHAIN_BIT is added at line 1895, that we will receive a completion TRB-event for each of the nframes, or will the chain bit result in loss of TRB completion events? Does setting this bit have any impact on performance? Thank you! --HPS Some more thoughts: The code in question handle all four USB transfer types. Are you saying that the CHAIN bit should be set for isochronous transfers too, so all the packets send in all the intervals are chained together? Or is this only for BULK traffic you want to add the CHAIN bit? Thank you! --HPS Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Wed, 11 Dec 2013 15:04:42 +0100 On 12/11/13 14:06, Kohji Okuno wrote: Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno Hi Kohji, I went through the Linux codes a bit, and I see they have some quirks for the chaining bit. Unfortunately Linux does the queuing quite differently than in FreeBSD and Shara Sharp which is the author of that code, stated recently a need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that means there are more bugs in there or not. Let me explain a bit how things work in FreeBSD and why I did not put the chaining bit in line 1895 which you suggest. In my design the chaining bit should not be set at line 1895, because if you receive a short packet and nframes 1, the XHCI should not go to the end of the frame list, but rather the next frame, nframes + 1. If the single short OK flag is set on a BULK transfer, yes, it would be correct to set the chaining bit here, but it is not required, because we are already are handling activation of the next frame in the function xhci_activate_transfer() and xhci_skip_transfer(). Transfer here means zero or more TRBs. We use the cycle bit on the TRB to single step the frames in software, although you are right that we might optimise this by setting the chaining bit instead for the BULK case so that we don't need software intervention to handle the job. If the multi short OK flag is set, multiple short terminated frames can be received and then the chaining bit should not be set. Are you seeing a real problem because of the chain bit not being set, or is this more the result of code review? Thank you for the interest in my XHCI driver. --HPS Hi HPS, Unfortunately, I can not explain in detail. But, I encountered a real problem for ZLP. And, when I added CHAIN bit, this problem was resolved. When a device driver needs force_short(ZLP), your device driver creates TRB in the followings. NORMAL_TRB- LINK_TRB - NORMAL_TRB - LINK_TRB = Kick DOORBELL (with payload) (#1) (ZLP)(#2) In this case, I think LINK_TRB #1 needs chain bit. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: spec violation of xHCI?
From: Hans Petter Selasky h...@bitfrost.no Date: Thu, 12 Dec 2013 08:15:02 +0100 On 12/12/13 01:59, Kohji Okuno wrote: From: Hans Petter Selasky h...@bitfrost.no Date: Wed, 11 Dec 2013 15:04:42 +0100 On 12/11/13 14:06, Kohji Okuno wrote: Hi HPS, All link trbs which are not the end need CHAIN bit, I think. But, this is errata in xHCI ver 0.95. So, linux has quirk for chain bit. Could you check linux codes? Regards, Kohji Okuno Hi Kohji, I went through the Linux codes a bit, and I see they have some quirks for the chaining bit. Unfortunately Linux does the queuing quite differently than in FreeBSD and Shara Sharp which is the author of that code, stated recently a need for rewrite of the TRB/TD stuff in Linux, so I'm not sure if that means there are more bugs in there or not. Let me explain a bit how things work in FreeBSD and why I did not put the chaining bit in line 1895 which you suggest. In my design the chaining bit should not be set at line 1895, because if you receive a short packet and nframes 1, the XHCI should not go to the end of the frame list, but rather the next frame, nframes + 1. If the single short OK flag is set on a BULK transfer, yes, it would be correct to set the chaining bit here, but it is not required, because we are already are handling activation of the next frame in the function xhci_activate_transfer() and xhci_skip_transfer(). Transfer here means zero or more TRBs. We use the cycle bit on the TRB to single step the frames in software, although you are right that we might optimise this by setting the chaining bit instead for the BULK case so that we don't need software intervention to handle the job. If the multi short OK flag is set, multiple short terminated frames can be received and then the chaining bit should not be set. Are you seeing a real problem because of the chain bit not being set, or is this more the result of code review? Thank you for the interest in my XHCI driver. --HPS Hi HPS, Unfortunately, I can not explain in detail. But, I encountered a real problem for ZLP. And, when I added CHAIN bit, this problem was resolved. When a device driver needs force_short(ZLP), your device driver creates TRB in the followings. NORMAL_TRB- LINK_TRB - NORMAL_TRB - LINK_TRB = Kick DOORBELL (with payload) (#1) (ZLP)(#2) In this case, I think LINK_TRB #1 needs chain bit. Hi Kohji, Did you check using a USB analyzer what the difference is when setting the CHAIN bit and not setting the chain bit? I would guess that if you set the CHAIN-bit in this case, no ZLP will be sent, because the TRB is associated with the previous one. What endpoint type is this? BULK/CONTROL/INTR/ISOC What direction is this? IN or OUT? --HPS Hi HPS, The endpoint type is BULK, and the direction is OUT. I checked by using a USB analyzer. When I did not set CHAIN bit in LINK TRB, my host controller sent illegal packets sometimes. But, ZLPs were sent. Regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Are pthread_setcancelstate() and pthread_setcanceltype() cancellation points?
Hi,
I have a question.
Are pthread_setcancelstate() and pthread_setcanceltype() cancellation
points?
I refered to the following URL.
http://pubs.opengroup.org/onlinepubs/7908799/xsh/threads.html
This document shows that cancellation points in the pthread library
are only pthread_cond_timedwait(), pthread_cond_wait(), pthread_join()
and pthread_testcancel().
But, current implementation in FreeBSD is the following.
(Please take notice of (*) marks).
Would you check this?
lib/libthr/thread/thr_cancel.c:
77 int
78 _pthread_setcancelstate(int state, int *oldstate)
79 {
80 struct pthread *curthread = _get_curthread();
81 int oldval;
82
83 oldval = curthread-cancel_enable;
84 switch (state) {
85 case PTHREAD_CANCEL_DISABLE:
86 curthread-cancel_enable = 0;
87 break;
88 case PTHREAD_CANCEL_ENABLE:
89 curthread-cancel_enable = 1;
90 (*) testcancel(curthread);
91 break;
92 default:
93 return (EINVAL);
94 }
95
96 if (oldstate) {
97 *oldstate = oldval ? PTHREAD_CANCEL_ENABLE :
98 PTHREAD_CANCEL_DISABLE;
99 }
100 return (0);
101 }
102
103 int
104 _pthread_setcanceltype(int type, int *oldtype)
105 {
106 struct pthread *curthread = _get_curthread();
107 int oldval;
108
109 oldval = curthread-cancel_async;
110 switch (type) {
111 case PTHREAD_CANCEL_ASYNCHRONOUS:
112 curthread-cancel_async = 1;
113 (*) testcancel(curthread);
114 break;
115 case PTHREAD_CANCEL_DEFERRED:
116 curthread-cancel_async = 0;
117 break;
118 default:
119 return (EINVAL);
120 }
121
122 if (oldtype) {
123 *oldtype = oldval ? PTHREAD_CANCEL_ASYNCHRONOUS :
124 PTHREAD_CANCEL_DEFERRED;
125 }
126 return (0);
127 }
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Are pthread_setcancelstate() and pthread_setcanceltype() cancellation points?
Hi,
This is newer document.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
Hi,
I have a question.
Are pthread_setcancelstate() and pthread_setcanceltype() cancellation
points?
I refered to the following URL.
http://pubs.opengroup.org/onlinepubs/7908799/xsh/threads.html
This document shows that cancellation points in the pthread library
are only pthread_cond_timedwait(), pthread_cond_wait(), pthread_join()
and pthread_testcancel().
But, current implementation in FreeBSD is the following.
(Please take notice of (*) marks).
Would you check this?
lib/libthr/thread/thr_cancel.c:
77int
78_pthread_setcancelstate(int state, int *oldstate)
79{
80struct pthread *curthread = _get_curthread();
81int oldval;
82
83oldval = curthread-cancel_enable;
84switch (state) {
85case PTHREAD_CANCEL_DISABLE:
86curthread-cancel_enable = 0;
87break;
88case PTHREAD_CANCEL_ENABLE:
89curthread-cancel_enable = 1;
90 (*)testcancel(curthread);
91break;
92default:
93return (EINVAL);
94}
95
96if (oldstate) {
97*oldstate = oldval ? PTHREAD_CANCEL_ENABLE :
98PTHREAD_CANCEL_DISABLE;
99}
100 return (0);
101 }
102
103 int
104 _pthread_setcanceltype(int type, int *oldtype)
105 {
106 struct pthread *curthread = _get_curthread();
107 int oldval;
108
109 oldval = curthread-cancel_async;
110 switch (type) {
111 case PTHREAD_CANCEL_ASYNCHRONOUS:
112 curthread-cancel_async = 1;
113 (*) testcancel(curthread);
114 break;
115 case PTHREAD_CANCEL_DEFERRED:
116 curthread-cancel_async = 0;
117 break;
118 default:
119 return (EINVAL);
120 }
121
122 if (oldtype) {
123 *oldtype = oldval ? PTHREAD_CANCEL_ASYNCHRONOUS :
124 PTHREAD_CANCEL_DEFERRED;
125 }
126 return (0);
127 }
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Are pthread_setcancelstate() and pthread_setcanceltype() cancellation points?
Hi, Konstantin,
Thank you for your prompt response.
The reason for the testcancel() calls there is that async cancellation
is defined as to be acted upon at any time after the cancellation is
enabled. If we have pending cancellation request, the async cancellation
must proceed after the pthread_setcancelstate(PTHREAD_CANCEL_ENABLE).
I see a bug there, namely, we should not process the cancellation if
the type is deferred. Is this is real issue you are concerned with ?
I think your change is right.
I found this issue in my sample source. My sample source in
8.1-RELEASE worked good, but it caused dead lock in current, since
my_mutex did not release. This change was committed after 8.1-RELEASE.
void
my_cleanup(void *arg)
{
int oldstate;
pthread_mutex_lock(my_mutex);
pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, oldstate);
...
pthread_setcancelstate(oldstate, NULL);
pthread_mutex_unlock(my_mutex);
}
And I investigated about glibc.
In glibc, if PTHREAD_CANCEL_ENABLE ASYNCHRONOUS then __do_cancel()
is executed in pthread_setcancelstate().
glibc-2.17/nptl/pthread_setcancelstate.c:
24 int
25 __pthread_setcancelstate (state, oldstate)
26 int state;
27 int *oldstate;
28 {
29 volatile struct pthread *self;
30
31 if (state PTHREAD_CANCEL_ENABLE || state PTHREAD_CANCEL_DISABLE)
32 return EINVAL;
33
34 self = THREAD_SELF;
35
36 int oldval = THREAD_GETMEM (self, cancelhandling);
37 while (1)
38 {
39 int newval = (state == PTHREAD_CANCEL_DISABLE
40 ? oldval | CANCELSTATE_BITMASK
41 : oldval ~CANCELSTATE_BITMASK);
42
43 /* Store the old value. */
44 if (oldstate != NULL)
45 *oldstate = ((oldval CANCELSTATE_BITMASK)
46 ? PTHREAD_CANCEL_DISABLE : PTHREAD_CANCEL_ENABLE);
47
48 /* Avoid doing unnecessary work. The atomic operation can
49 potentially be expensive if the memory has to be locked and
50 remote cache lines have to be invalidated. */
51 if (oldval == newval)
52 break;
53
54 /* Update the cancel handling word. This has to be done
55 atomically since other bits could be modified as well. */
56 int curval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling, newval,
57 oldval);
58 if (__builtin_expect (curval == oldval, 1))
59 {
60 if (CANCEL_ENABLED_AND_CANCELED_AND_ASYNCHRONOUS (newval))
61 __do_cancel ();
62
63 break;
64 }
65
66 /* Prepare for the next round. */
67 oldval = curval;
68 }
69
70 return 0;
71 }
Many thanks,
Kohji Okuno
On Tue, Jun 18, 2013 at 06:15:56PM +0900, Kohji Okuno wrote:
Hi,
This is newer document.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05_02
Hi,
I have a question.
Are pthread_setcancelstate() and pthread_setcanceltype() cancellation
points?
I refered to the following URL.
http://pubs.opengroup.org/onlinepubs/7908799/xsh/threads.html
This document shows that cancellation points in the pthread library
are only pthread_cond_timedwait(), pthread_cond_wait(), pthread_join()
and pthread_testcancel().
But, current implementation in FreeBSD is the following.
(Please take notice of (*) marks).
Would you check this?
The reason for the testcancel() calls there is that async cancellation
is defined as to be acted upon at any time after the cancellation is
enabled. If we have pending cancellation request, the async cancellation
must proceed after the pthread_setcancelstate(PTHREAD_CANCEL_ENABLE).
I see a bug there, namely, we should not process the cancellation if
the type is deferred. Is this is real issue you are concerned with ?
diff --git a/lib/libthr/thread/thr_cancel.c b/lib/libthr/thread/thr_cancel.c
index 89f0ee1..beae707 100644
--- a/lib/libthr/thread/thr_cancel.c
+++ b/lib/libthr/thread/thr_cancel.c
@@ -87,7 +87,8 @@ _pthread_setcancelstate(int state, int *oldstate)
break;
case PTHREAD_CANCEL_ENABLE:
curthread-cancel_enable = 1;
- testcancel(curthread);
+ if (curthread-cancel_async)
+ testcancel(curthread);
break;
default:
return (EINVAL);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: deadlock between g_event and a thread on removing a device.
Hi Konstantin,
Thank you for your comment.
I don't have any solution for this issue.
And when a device is removed suddenly, there are other problems, I think.
On Fri, Jan 18, 2013 at 02:45:38PM +0900, Kohji Okuno wrote:
Hi,
When I removed a device (ex. /dev/da0), I have encounterd a
dead-lock between ``g_event'' thread and a thread that is opening
device file (I call this thread as A).
Would you refer the following?
When the device is removed between dev_refthread() and g_dev_open(),
thread A incremented dev-si_threadcount, but can't acquire
topology_lock.
On the other hand, g_event is waiting to set dev-si_threadcount to 0
with topology_lock.
Regards,
Kohji Okuno
Thread A
...
devfs_open()
{
...
dsw = dev_refthread(dev, ref); = increment dev-si_threadcount
...
error = dsw-d_open(...); = call g_dev_open()
...
dev_relthread(dev, ref);= decrement dev-si_threadcount
}
g_dev_open()
{
...
g_topology_lock(); = Thread A couldn't acquire
...topology_lock.
}
g_event
g_run_events()
{
...
g_topology_lock(); = g_event acuired topology_lock here.
...
one_event()
...
}
one_event()
g_orphan_register()
g_dev_orphan()
destroy_dev()
destroy_dev()
destroy_devl()
{
...
while (dev-si_threadcount != 0) { = this count was incremented by Thread
A
/* Use unique dummy wait ident */
msleep(csw, devmtx, PRIBIO, devdrn, hz / 10);
}
...
}
Yes, you are absolutely right.
I believe there were some patches floating around which changed the
destroy_dev() call in the g_dev_orphan() to destroy_dev_sched(). I do
not remember who was the author.
My reply was that naive substitution of the destroy_dev() to
destroy_dev_sched() is racy, because some requests might still come
in after the call to destroy_dev_sched(). Despite destroy_dev_sched()
setting the CDP_SCHED_DTR flag on the devfs node, some thread might
already entered the cdevsw method. I do not believe that there was
further progress there.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
deadlock between g_event and a thread on removing a device.
Hi,
When I removed a device (ex. /dev/da0), I have encounterd a
dead-lock between ``g_event'' thread and a thread that is opening
device file (I call this thread as A).
Would you refer the following?
When the device is removed between dev_refthread() and g_dev_open(),
thread A incremented dev-si_threadcount, but can't acquire
topology_lock.
On the other hand, g_event is waiting to set dev-si_threadcount to 0
with topology_lock.
Regards,
Kohji Okuno
Thread A
...
devfs_open()
{
...
dsw = dev_refthread(dev, ref); = increment dev-si_threadcount
...
error = dsw-d_open(...); = call g_dev_open()
...
dev_relthread(dev, ref);= decrement dev-si_threadcount
}
g_dev_open()
{
...
g_topology_lock(); = Thread A couldn't acquire
...topology_lock.
}
g_event
g_run_events()
{
...
g_topology_lock(); = g_event acuired topology_lock here.
...
one_event()
...
}
one_event()
g_orphan_register()
g_dev_orphan()
destroy_dev()
destroy_dev()
destroy_devl()
{
...
while (dev-si_threadcount != 0) { = this count was incremented by Thread A
/* Use unique dummy wait ident */
msleep(csw, devmtx, PRIBIO, devdrn, hz / 10);
}
...
}
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
arm: cpu_switch() has bug?
Hi,
I have doubt if cpu_switch() of arm has a bug.
In swtch.S:L.334, if newtd-td_pcb (this is in stack pointer for
kernel) has an address accessed first for the old(current) thread,
data_abort_fault may occur.
When data_abort_fault occurs, data_abort_handler() tries to solve this
address from kernel_map. In this time, curthread and curpcb are
already updated in swtch.S:L.223-231. As this result,
data_abort_handler() will occur data_abort_fault in trap.c:L.301, again.
When I check, in other CPUs, after updating the root pointer of MMU,
curthread and curpcb are updated.
Would you please check this?
Thanks,
Kohji Okuno
arm/arm/swtch.S:
215 ENTRY(cpu_switch)
216 stmfd sp!, {r4-r7, lr}
217 mov r6, r2 /* Save the mutex */
218
219 .Lswitch_resume:
220 /* rem: r0 = old lwp */
221 /* rem: interrupts are disabled */
222
223 /* Process is now on a processor. */
224 /* We have a new curthread now so make a note it */
225 GET_CURTHREAD_PTR(r7)
226 str r1, [r7]
227
228 /* Hook in a new pcb */
229 GET_PCPU(r7)
230 ldr r2, [r1, #TD_PCB]
231 str r2, [r7, #PC_CURPCB]
232
233 /* rem: r1 = new process */
234 /* rem: interrupts are enabled */
SNIP
298 /* rem: r2 = old PCB */
299 /* rem: r9 = new PCB */
300 /* rem: interrupts are enabled */
301
302 #ifdef ARM_VFP_SUPPORT
303 /*
304 * vfp_store will clear pcpu-pc_vfpcthread, save
305 * registers and state, and modify the control as needed.
306 * a future exception will bounce the backup settings in the fp
unit.
307 * XXX vfp_store can't change r4
308 */
309 GET_PCPU(r7)
310 ldr r8, [r7, #(PC_VFPCTHREAD)]
311 cmp r4, r8 /* old thread used vfp?
*/
312 bne 1f /* no, don't save */
313 cmp r1, r4 /* same thread ? */
314 beq 1f /* yes, skip vfp store
*/
315 #ifdef SMP
316 ldr r8, [r7, #(PC_CPU)] /* last used on this
cpu? */
317 ldr r3, [r2, #(PCB_VFPCPU)]
318 cmp r8, r3 /* last cpu to use these registers? */
319 bne 1f /* no. these values are stale */
320 #endif
321 add r0, r2, #(PCB_VFPSTATE)
322 bl _C_LABEL(vfp_store)
323 1:
324 #endif /* ARM_VFP_SUPPORT */
325
326 /* r1 now free! */
327
328 /* Third phase : restore saved context */
329
330 /* rem: r2 = old PCB */
331 /* rem: r9 = new PCB */
332 /* rem: interrupts are enabled */
333
334 ldr r5, [r9, #(PCB_DACR)] /* r5 = new DACR */
335 mov r2, #DOMAIN_CLIENT
336 cmp r5, r2, lsl #(PMAP_DOMAIN_KERNEL * 2) /* Sw to kernel
thread? */
337 beq .Lcs_context_switched/* Yup. Don't flush cache
*/
338 mrc p15, 0, r0, c3, c0, 0 /* r0 = old DACR */
arm/arm/trap.c
224 void
225 data_abort_handler(trapframe_t *tf)
226 {
227 struct vm_map *map;
228 struct pcb *pcb;
229 struct thread *td;
230 u_int user, far, fsr;
231 vm_prot_t ftype;
232 void *onfault;
233 vm_offset_t va;
234 int error = 0;
235 struct ksig ksig;
236 struct proc *p;
237
238
239 /* Grab FAR/FSR before enabling interrupts */
240 far = cpu_faultaddress();
241 fsr = cpu_faultstatus();
242 #if 0
243 printf(data abort: %p (from %p %p)\n, (void*)far,
(void*)tf-tf_pc,
244 (void*)tf-tf_svc_lr);
245 #endif
246
247 /* Update vmmeter statistics */
248 #if 0
249 vmexp.traps++;
250 #endif
251
252 td = curthread;
253 p = td-td_proc;
254
255 PCPU_INC(cnt.v_trap);
256 /* Data abort came from user mode? */
257 user = TRAP_USERMODE(tf);
258
259 if (user) {
260 td-td_pticks = 0;
261 td-td_frame = tf;
262 if (td-td_ucred != td-td_proc-p_ucred)
263 cred_update_thread(td);
264
265 }
266 /* Grab the current pcb */
267 pcb = td-td_pcb;
SNIP
299
300 /* fusubailout is used by [fs]uswintr to avoid page faulting */
301 if (__predict_false(pcb-pcb_onfault == fusubailout
About 802.1Q tag
Hi,
Would someone check the following code?
If the hardware do not process an 802.1Q tag, the kernel repacks mbuf
in line 578-580. But, `struct ether_header *eh' was assigned at line 484.
And, in line 611-637, because of the kernel refers old eh pointer, the
kernel will misjudges its ether packet.
I think that `eh = mtod(m, struct ether_header *);' is needed after
line 580.
Thanks,
Kohji Okuno
sys/net/if_ethersubr.c:
448 static void
449 ether_input_internal(struct ifnet *ifp, struct mbuf *m)
450 {
451 struct ether_header *eh;
484 eh = mtod(m, struct ether_header *);
554 /*
555 * If the hardware did not process an 802.1Q tag, do this now,
556 * to allow 802.1P priority frames to be passed to the main
input
557 * path correctly.
558 * TODO: Deal with Q-in-Q frames, but not arbitrary nesting
levels.
559 */
560 if ((m-m_flags M_VLANTAG) == 0 etype == ETHERTYPE_VLAN) {
578 bcopy((char *)evl, (char *)evl + ETHER_VLAN_ENCAP_LEN,
579 ETHER_HDR_LEN - ETHER_TYPE_LEN);
580 m_adj(m, ETHER_VLAN_ENCAP_LEN);
581 }
610
611 #if defined(INET) || defined(INET6)
612 /*
613 * Clear M_PROMISC on frame so that carp(4) will see it when the
614 * mbuf flows up to Layer 3.
615 * FreeBSD's implementation of carp(4) uses the inprotosw
616 * to dispatch IPPROTO_CARP. carp(4) also allocates its own
617 * Ethernet addresses of the form 00:00:5e:00:01:xx, which
618 * is outside the scope of the M_PROMISC test below.
619 * TODO: Maintain a hash table of ethernet addresses other than
620 * ether_dhost which may be active on this ifp.
621 */
622 if (ifp-if_carp (*carp_forus_p)(ifp, eh-ether_dhost)) {
623 m-m_flags = ~M_PROMISC;
624 } else
625 #endif
626 {
627 /*
628 * If the frame received was not for our MAC address,
set the
629 * M_PROMISC flag on the mbuf chain. The frame may need
to
630 * be seen by the rest of the Ethernet input path in
case of
631 * re-entry (e.g. bridge, vlan, netgraph) but should
not be
632 * seen by upper protocol layers.
633 */
634 if (!ETHER_IS_MULTICAST(eh-ether_dhost)
635 bcmp(IF_LLADDR(ifp), eh-ether_dhost,
ETHER_ADDR_LEN) != 0)
636 m-m_flags |= M_PROMISC;
637 }
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
The current libc/locale/toupper.c is mistaken
Hi,
I think that libc/locale/toupper.c is mistaken.
Could you check it?
@@ -51,7 +51,7 @@ ___toupper_l(c, l)
{
size_t lim;
FIX_LOCALE(l);
- _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__maplower_ext;
+ _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__mapupper_ext;
_RuneEntry *base, *re;
if (c 0 || c == EOF)
Best regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: The current libc/locale/toupper.c is mistaken
Hi Eric,
I'm left wondering how this was not caught by the libc++ test
suite. The current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
This test suite checks only popular characters.
__mapupper_ext is used in case of special characters.
For example, Turkish 'i' (0x0131) should convert 'I' (0x49).
Regards,
Kohji Okuno
Den 10/05/2012 kl. 12.03 skrev Dimitry Andric:
On 2012-05-10 11:02, Kohji Okuno wrote:
I think that libc/locale/toupper.c is mistaken.
Could you check it?
@@ -51,7 +51,7 @@ ___toupper_l(c, l)
{
size_t lim;
FIX_LOCALE(l);
- _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__maplower_ext;
+ _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__mapupper_ext;
_RuneEntry *base, *re;
if (c 0 || c == EOF)
Yes, this definitely looks like a copy/paste error, introduced here:
http://svnweb.freebsd.org/base/head/lib/libc/locale/toupper.c?r1=165903r2=227753
I'll commit the fix tonight (CEST), if David isn't faster than me. :)
I'm left wondering how this was not caught by the libc++ test suite. The
current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
Thanks,
Erik___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: The current libc/locale/toupper.c is mistaken
Hi David,
From: David Chisnall thera...@freebsd.org
If you have a test case, I can commit it to the libc++ test suite.
David
I attached my test source.
This test program shoud output as below.
towupper_l
0049, 0049
0131, 0049
0130, 0130
0069, 0049
towlower_l
0049, 0069
0131, 0131
0130, 0069
0069, 0069
But, when I use original toupper.c, this proguram output as below.
towupper_l
0049, 0049
0131, 0131
0130, 0069
0069, 0049
towlower_l
0049, 0069
0131, 0131
0130, 0069
0069, 0069
Regards,
Kohji Okuno
On 10 May 2012, at 21:42, Kohji Okuno wrote:
Hi Eric,
I'm left wondering how this was not caught by the libc++ test
suite. The current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
This test suite checks only popular characters.
__mapupper_ext is used in case of special characters.
For example, Turkish 'i' (0x0131) should convert 'I' (0x49).
Regards,
Kohji Okuno
Den 10/05/2012 kl. 12.03 skrev Dimitry Andric:
On 2012-05-10 11:02, Kohji Okuno wrote:
I think that libc/locale/toupper.c is mistaken.
Could you check it?
@@ -51,7 +51,7 @@ ___toupper_l(c, l)
{
size_t lim;
FIX_LOCALE(l);
- _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__maplower_ext;
+ _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__mapupper_ext;
_RuneEntry *base, *re;
if (c 0 || c == EOF)
Yes, this definitely looks like a copy/paste error, introduced here:
http://svnweb.freebsd.org/base/head/lib/libc/locale/toupper.c?r1=165903r2=227753
I'll commit the fix tonight (CEST), if David isn't faster than me. :)
I'm left wondering how this was not caught by the libc++ test suite. The
current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
Thanks,
Erik___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: The current libc/locale/toupper.c is mistaken
Hi,
I'm sorry. I forgot to attach a file.
Regards,
Kohji Okuno
Hi David,
From: David Chisnall thera...@freebsd.org
If you have a test case, I can commit it to the libc++ test suite.
David
I attached my test source.
This test program shoud output as below.
towupper_l
0049, 0049
0131, 0049
0130, 0130
0069, 0049
towlower_l
0049, 0069
0131, 0131
0130, 0069
0069, 0069
But, when I use original toupper.c, this proguram output as below.
towupper_l
0049, 0049
0131, 0131
0130, 0069
0069, 0049
towlower_l
0049, 0069
0131, 0131
0130, 0069
0069, 0069
Regards,
Kohji Okuno
On 10 May 2012, at 21:42, Kohji Okuno wrote:
Hi Eric,
I'm left wondering how this was not caught by the libc++ test
suite. The current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
This test suite checks only popular characters.
__mapupper_ext is used in case of special characters.
For example, Turkish 'i' (0x0131) should convert 'I' (0x49).
Regards,
Kohji Okuno
Den 10/05/2012 kl. 12.03 skrev Dimitry Andric:
On 2012-05-10 11:02, Kohji Okuno wrote:
I think that libc/locale/toupper.c is mistaken.
Could you check it?
@@ -51,7 +51,7 @@ ___toupper_l(c, l)
{
size_t lim;
FIX_LOCALE(l);
- _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__maplower_ext;
+ _RuneRange *rr = XLOCALE_CTYPE(l)-runes-__mapupper_ext;
_RuneEntry *base, *re;
if (c 0 || c == EOF)
Yes, this definitely looks like a copy/paste error, introduced here:
http://svnweb.freebsd.org/base/head/lib/libc/locale/toupper.c?r1=165903r2=227753
I'll commit the fix tonight (CEST), if David isn't faster than me. :)
I'm left wondering how this was not caught by the libc++ test suite. The
current toupper.c shouldn't pass
http://llvm.org/svn/llvm-project/libcxx/trunk/test/localization/locales/locale.convenience/conversions/conversions.character/toupper.pass.cpp
Thanks,
Erik___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
#include stdlib.h
#include stdio.h
#include locale.h
#include wctype.h
#include xlocale.h
int
main()
{
int i;
wint_t test[] = {0x0049, 0x0131, 0x0130, 0x0069};
wint_t ret;
locale_t x;
x = newlocale(0x1f, tr_TR, NULL);
printf(towupper_l\n);
for (i = 0; i sizeof(test)/sizeof(wint_t); i++) {
ret = towupper_l(test[i], x);
printf(%04x, %04x\n, test[i], ret);
}
printf(\n);
printf(towlower_l\n);
for (i = 0; i sizeof(test)/sizeof(wint_t); i++) {
ret = towlower_l(test[i], x);
printf(%04x, %04x\n, test[i], ret);
}
exit(0);
}
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Is UPS_PORT_POWER wrong?
Hi HPS,
Do you have better idea?
From: Kohji Okuno okuno.ko...@jp.panasonic.com
Date: Tue, 24 Jan 2012 09:53:29 +0900 (JST)
Hi HPS,
On Monday 23 January 2012 09:12:46 Kohji Okuno wrote:
Hi HPS,
I think that UPS_PORT_POWER and UPS_PORT_LINK_STATE overlap.
And, in xhci.c you set UPS_PORT_POWER as folows.
When UPS_PORT_POWER is set, UPS_PORT_LINK_STATE_GET() macro will
return incorrect value.
if (v XHCI_PS_PP) {
/*
* The USB 3.0 RH is using the
* USB 2.0's power bit
*/
i |= UPS_PORT_POWER;
}
Hi,
The USB 3.0 root HUB is special because it defines FULL/HIGH and LOW speed,
so
I had to merge that into the port status register of the XHCI root HUB like
this:
0: CONNECT_STATUS
1: PORT_ENABLED
2: SUSPEND
3: OVERCURRENT_INDICATOR
4: LINK STATE (USB 3.0)
5: -
6: -
7: -
8: PORT_POWER (USB 2.0)
# Bit 9+10 have 4 combinations which are defined: FS, LW, HS, SS
9: LOW_SPEED (USB 2.0)
10: HIGH_SPEED (USB 2.0)
11: not implemented
12: PORT_INDICATOR
13:
14:
15: MODE_DEVICE (FreeBSD specific)
If you have a better idea, it is possible to change this.
I have a idea.
-#define UPS_PORT_LINK_STATE_GET(x) (((x) 5) 0xF)
-#define UPS_PORT_LINK_STATE_SET(x) (((x) 0xF) 5)
+#define UPS_PORT_LINK_STATE_GET(x) x) 5) 0x7)|(((x) 11)
0x8))
+#define UPS_PORT_LINK_STATE_SET(x) x) 0x7) 5)|(((x) 0x8)
11))
+#define UPS_PORT_LS_SS 0x4000 /* currently FreeBSD
specific */
But, this is not cool.
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Is UPS_PORT_POWER wrong?
Hi HPS,
I think that UPS_PORT_POWER and UPS_PORT_LINK_STATE overlap.
And, in xhci.c you set UPS_PORT_POWER as folows.
When UPS_PORT_POWER is set, UPS_PORT_LINK_STATE_GET() macro will
return incorrect value.
if (v XHCI_PS_PP) {
/*
* The USB 3.0 RH is using the
* USB 2.0's power bit
*/
i |= UPS_PORT_POWER;
}
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Is UPS_PORT_POWER wrong?
Hi HPS,
On Monday 23 January 2012 09:12:46 Kohji Okuno wrote:
Hi HPS,
I think that UPS_PORT_POWER and UPS_PORT_LINK_STATE overlap.
And, in xhci.c you set UPS_PORT_POWER as folows.
When UPS_PORT_POWER is set, UPS_PORT_LINK_STATE_GET() macro will
return incorrect value.
if (v XHCI_PS_PP) {
/*
* The USB 3.0 RH is using the
* USB 2.0's power bit
*/
i |= UPS_PORT_POWER;
}
Hi,
The USB 3.0 root HUB is special because it defines FULL/HIGH and LOW speed,
so
I had to merge that into the port status register of the XHCI root HUB like
this:
0: CONNECT_STATUS
1: PORT_ENABLED
2: SUSPEND
3: OVERCURRENT_INDICATOR
4: LINK STATE (USB 3.0)
5: -
6: -
7: -
8: PORT_POWER (USB 2.0)
# Bit 9+10 have 4 combinations which are defined: FS, LW, HS, SS
9: LOW_SPEED (USB 2.0)
10: HIGH_SPEED (USB 2.0)
11: not implemented
12: PORT_INDICATOR
13:
14:
15: MODE_DEVICE (FreeBSD specific)
If you have a better idea, it is possible to change this.
I have a idea.
-#define UPS_PORT_LINK_STATE_GET(x) (((x) 5) 0xF)
-#define UPS_PORT_LINK_STATE_SET(x) (((x) 0xF) 5)
+#define UPS_PORT_LINK_STATE_GET(x) x) 5) 0x7)|(((x) 11)
0x8))
+#define UPS_PORT_LINK_STATE_SET(x) x) 0x7) 5)|(((x) 0x8)
11))
+#define UPS_PORT_LS_SS 0x4000 /* currently FreeBSD specific
*/
But, this is not cool.
Regards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Can you use a USB3.0 hub?
Hi HPS, From: Hans Petter Selasky hsela...@c2i.net Subject: Re: Can you use a USB3.0 hub? Date: Thu, 12 Jan 2012 22:23:22 +0100 On Thursday 12 January 2012 07:15:17 Kohji Okuno wrote: Hi, Can you use a USB3.0 hub? I tried a USB3.0 hub (BUFFALO BSH4A04U3BK). And I used 8-stable and PCI-E card (BUFFALO IFC-PCIE2U3) The hub is for only japanese market. The card is NEC’s 720200 chip http://www.buffalotech.com/products/accessories/interface-card-adapters/usb -30-pci-express-interface-card/ The kernel could not recognize USB3.0 HDD that connected to this hub as the following log. But, the kernel could reconize USB2.0 HDD that connected to this hub. Regards, Kohji Okuno Hi, There is a problem with USB 3.0 HUBs, most likely something related to the XHCI route string or USB HUB set depth. I don't have a USB 3.0 analyzer, so I cannot find this out quickly. If you could help debug, would be great. Here is a patch which you can put on top of 8/9- or 10- stable: http://svn.freebsd.org/changeset/base/230032 It fixes a few issues, but not all. I think your commit is wrong about UPS_PORT_POWER_SS. - #define UPS_PORT_POWER_SS 0x0200 /* super-speed only */ #define UPS_LOW_SPEED 0x0200 + #define UPS_PORT_POWER_SS 0x2000 /* super-speed only */ #define UPS_LOW_SPEED 0x0200 Now, usb3.0 HDD was not able to recognize. I have USB 3.0 analyzer(LeCroy Voyager), so I can help debug. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Can you use a USB3.0 hub?
Hi, Can you use a USB3.0 hub? I tried a USB3.0 hub (BUFFALO BSH4A04U3BK). And I used 8-stable and PCI-E card (BUFFALO IFC-PCIE2U3) The hub is for only japanese market. The card is NEC’s 720200 chip http://www.buffalotech.com/products/accessories/interface-card-adapters/usb-30-pci-express-interface-card/ The kernel could not recognize USB3.0 HDD that connected to this hub as the following log. But, the kernel could reconize USB2.0 HDD that connected to this hub. Regards, Kohji Okuno -- log xhci0: XHCI (generic) USB 3.0 controller mem 0xf7ffe000-0xf7ff irq 28 at d evice 0.0 on pci1 xhci0: [ITHREAD] xhci0: 32 byte context size. usbus0 on xhci0 ... ugen0.2: VIA Labs, Inc. at usbus0 uhub11: VIA Labs, Inc. 4-Port USB 3.0 Hub, class 9/0, rev 3.00/3.74, addr 1 on usbus0 uhub11: 4 ports with 4 removable, self powered usb_alloc_device: set address 3 failed (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=3, set address failed! (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=3, set address failed! (USB_ERR_IOERROR, ignored) ugen0.3: Unknown at usbus0 (disconnected) uhub_reattach_port: could not allocate new device uhub_reattach_port: device problem (USB_ERR_STALLED), disabling port 4 ugen0.3: vendor 0x2109 at usbus0 uhub12: vendor 0x2109 USB2.0 Hub, class 9/0, rev 2.00/2.74, addr 2 on usbus0 uhub12: 4 ports with 4 removable, self powered usb_alloc_device: set address 4 failed (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=4, set address failed! (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=4, set address failed! (USB_ERR_IOERROR, ignored) ugen0.4: Unknown at usbus0 (disconnected) uhub_reattach_port: could not allocate new device uhub_reattach_port: device problem (USB_ERR_STALLED), disabling port 4 usb_alloc_device: set address 4 failed (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=4, set address failed! (USB_ERR_IOERROR, ignored) usbd_req_re_enumerate: addr=4, set address failed! (USB_ERR_IOERROR, ignored) ugen0.4: Unknown at usbus0 (disconnected) uhub_reattach_port: could not allocate new device ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
about XHCI_PS_PP
Hi Selasky, I think XHCI_PS_PP is wrong. - #define XHCI_PS_PP 0x0100 /* RW - port power */ + #define XHCI_PS_PP 0x0200 /* RW - port power */ Could you check it? Best regards, Kohji Okuno ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug: devfs is sure to have the bug.
Hi Kostic and Jaakko,
On Fri, Aug 05, 2011 at 06:45:22PM +0300, Jaakko Heinonen wrote:
On 2011-08-03, Kostik Belousov wrote:
On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote:
devfs_populate(), and the context holds only dm-dm_lock in
devfs_populate().
On the other hand, devfs_generation is incremented in devfs_create()
and devfs_destroy() the context holds only devmtx in devfs_create()
and devfs_destroy().
If a context executes devfs_create() when other context is executing
(***), then dm-dm_generation is updated incorrect value.
As a result, we can not open the last detected device (we receive
ENOENT).
I think the problem you described is real, and suggested change is right.
Initially, I thought that we should work with devfs_generation as with
the atomic type due to unlocked access in the devfs_populate(), but then
convinced myself that this is not needed.
But also, I think there is another half of the problem. Namely,
devfs_lookup() calls devfs_populate_vp(), and then does lookup with the
help of devfs_lookupx(). We will miss the generation update
happen after the drop of the dm_lock in devfs_populate_vp() to reacquire
the directory vnode lock.
I don't understand this. devfs_generation is not protected with dm_lock
in devfs_create() and devfs_destroy(). On the other hand if you mean
that another thread calls devfs_populate() while we drop dm_lock in
devfs_populate_vp(), isn't the mount point up to date when we re-lock
dm_lock?
Yes, I was not quite exact in describing what I mean, and the reference
to dm_lock drop is both vague and not correct.
I am after the fact that we do allow the situation where it is externally
visible that new cdev node was successfully created before the lookup
returns ENOENT for the path of the node.
@@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int
cleanup)
void
devfs_populate(struct devfs_mount *dm)
{
+ unsigned gen;
sx_assert(dm-dm_lock, SX_XLOCKED);
- if (dm-dm_generation == devfs_generation)
+ gen = devfs_generation;
+ if (dm-dm_generation == gen)
return;
while (devfs_populate_loop(dm, 0))
continue;
- dm-dm_generation = devfs_generation;
+ dm-dm_generation = gen;
}
After this change dm-dm_generation may be stale although the mount
point is up to date? This is probably harmless, though.
This must be harmless, in the worst case it will cause more calls to
the populate. In fact, this even allows the dm_generation to roll backward,
which is again harmless.
After all, do we only have to fix only devfs_populate()?
I think that there is no problem while I am testing the 8.1-RELEASE
environment that fix only devfs_populate().
Many thanks,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug: devfs is sure to have the bug.
Hello Kostik,
On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote:
Hello Kostik,
From: Kostik Belousov kostik...@gmail.com
Subject: Re: Bug: devfs is sure to have the bug.
Date: Wed, 3 Aug 2011 16:50:44 +0300
I think the problem you described is real, and suggested change is right.
Initially, I thought that we should work with devfs_generation as with
the atomic type due to unlocked access in the devfs_populate(), but then
convinced myself that this is not needed.
But also, I think there is another half of the problem. Namely,
devfs_lookup() calls devfs_populate_vp(), and then does lookup with the
help of devfs_lookupx(). We will miss the generation update
happen after the drop of the dm_lock in devfs_populate_vp() to reacquire
the directory vnode lock.
I propose the change below, consisting of your fix and also retry of
population and lookup in case generations do not match for ENOENT
case.
diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c
index d72ada0..8ff9bc2 100644
--- a/sys/fs/devfs/devfs_devs.c
+++ b/sys/fs/devfs/devfs_devs.c
@@ -63,7 +63,7 @@ static MALLOC_DEFINE(M_CDEVP, DEVFS1, DEVFS cdev_priv
storage);
static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, DEVFS
filesystem);
-static unsigned devfs_generation;
+unsigned devfs_generation;
SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD,
devfs_generation, 0, DEVFS generation number);
@@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int
cleanup)
void
devfs_populate(struct devfs_mount *dm)
{
+ unsigned gen;
sx_assert(dm-dm_lock, SX_XLOCKED);
- if (dm-dm_generation == devfs_generation)
+ gen = devfs_generation;
+ if (dm-dm_generation == gen)
return;
while (devfs_populate_loop(dm, 0))
continue;
- dm-dm_generation = devfs_generation;
+ dm-dm_generation = gen;
}
/*
diff --git a/sys/fs/devfs/devfs_int.h b/sys/fs/devfs/devfs_int.h
index cdc6aba..cb01ad1 100644
--- a/sys/fs/devfs/devfs_int.h
+++ b/sys/fs/devfs/devfs_int.h
@@ -71,6 +71,8 @@ struct cdev_priv {
#define cdev2priv(c)member2struct(cdev_priv, cdp_c, c)
+extern unsigned devfs_generation;
+
struct cdev *devfs_alloc(int);
int devfs_dev_exists(const char *);
void devfs_free(struct cdev *);
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index 955bd8b..2603caa 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -188,7 +188,7 @@ devfs_clear_cdevpriv(void)
* On success devfs_populate_vp() returns with dmp-dm_lock held.
*/
static int
-devfs_populate_vp(struct vnode *vp)
+devfs_populate_vp(struct vnode *vp, int dm_locked)
{
struct devfs_dirent *de;
struct devfs_mount *dmp;
@@ -199,7 +199,8 @@ devfs_populate_vp(struct vnode *vp)
dmp = VFSTODEVFS(vp-v_mount);
locked = VOP_ISLOCKED(vp);
- sx_xlock(dmp-dm_lock);
+ if (!dm_locked)
+ sx_xlock(dmp-dm_lock);
DEVFS_DMP_HOLD(dmp);
/* Can't call devfs_populate() with the vnode lock held. */
@@ -242,7 +243,7 @@ devfs_vptocnp(struct vop_vptocnp_args *ap)
dmp = VFSTODEVFS(vp-v_mount);
- error = devfs_populate_vp(vp);
+ error = devfs_populate_vp(vp, 0);
if (error != 0)
return (error);
@@ -643,7 +644,7 @@ devfs_getattr(struct vop_getattr_args *ap)
struct devfs_mount *dmp;
struct cdev *dev;
- error = devfs_populate_vp(vp);
+ error = devfs_populate_vp(vp, 0);
if (error != 0)
return (error);
@@ -903,7 +904,7 @@ devfs_lookupx(struct vop_lookup_args *ap, int
*dm_unlock)
if (cdev == NULL)
sx_xlock(dmp-dm_lock);
- else if (devfs_populate_vp(dvp) != 0) {
+ else if (devfs_populate_vp(dvp, 0) != 0) {
*dm_unlock = 0;
sx_xlock(dmp-dm_lock);
if (DEVFS_DMP_DROP(dmp)) {
@@ -966,19 +967,30 @@ devfs_lookupx(struct vop_lookup_args *ap, int
*dm_unlock)
static int
devfs_lookup(struct vop_lookup_args *ap)
{
- int j;
struct devfs_mount *dmp;
- int dm_unlock;
+ int error, dm_unlock;
- if (devfs_populate_vp(ap-a_dvp) != 0)
+ dm_unlock = 0;
+retry:
+ if (devfs_populate_vp(ap-a_dvp, dm_unlock) != 0)
return (ENOTDIR);
dmp = VFSTODEVFS(ap-a_dvp-v_mount);
dm_unlock = 1;
- j = devfs_lookupx(ap, dm_unlock);
- if (dm_unlock == 1)
+ error = devfs_lookupx(ap, dm_unlock);
+ if (error == ENOENT) {
+ if (dm_unlock)
+ sx_assert(dmp-dm_lock, SA_XLOCKED);
+ else {
+ sx_xlock(dmp-dm_lock);
+ dm_unlock = 1;
+ }
+ if (devfs_generation != dmp-dm_generation)
+ goto retry;
+ }
+ if (dm_unlock
Re: Bug: devfs is sure to have the bug.
Hello Kostik,
On Thu, Aug 04, 2011 at 11:41:39AM +0900, Kohji Okuno wrote:
But, now I'm using 8.1-RELEASE. May I have advice about 8.X ?
Do you mean a patch for the stable/8 ? I believe it is enough to
apply rev. 211628 to stable/8, then the patch I posted yesterday
should be compilable.
I'm sorry.
I need a patch for 8.1-RELEASE. Could you propose patch?
Did you tried to apply the 211628 and the patch I mailed, to 8.1 ?
I am not very interested in porting this stuff for such old system.
On the other hand, I am unaware of large changes in devfs between
8.1 and latest stable.
No. Because the difference was large as you were poingint out, I did
not your patch. Now, I'm trying the following patch.
devfs_populate(struct devfs_mount *dm)
{
+#if 1
+ unsigned gen;
+ sx_assert(dm-dm_lock, SX_XLOCKED);
+ gen = devfs_generation;
+ if (dm-dm_generation == gen)
+ return;
+ while (devfs_populate_loop(dm, 0))
+ continue;
+ dm-dm_generation = gen;
+#else
sx_assert(dm-dm_lock, SX_XLOCKED);
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
dm-dm_generation = devfs_generation;
+#endif
}
/*
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug: devfs is sure to have the bug.
Hello Kostik,
From: Kostik Belousov kostik...@gmail.com
Subject: Re: Bug: devfs is sure to have the bug.
Date: Wed, 3 Aug 2011 16:50:44 +0300
Message-ID: 20110803135044.gm17...@deviant.kiev.zoral.com.ua
On Wed, Aug 03, 2011 at 02:44:23PM +0900, Kohji Okuno wrote:
Hello,
Hello,
I think that devfs is sure to have the bug.
I found that I couldn't open /dev/XXX though the kernel detected XXX
device.
dm-dm_generation is updated with devfs_generation in
devfs_populate(), and the context holds only dm-dm_lock in
devfs_populate().
On the other hand, devfs_generation is incremented in devfs_create()
and devfs_destroy() the context holds only devmtx in devfs_create()
and devfs_destroy().
If a context executes devfs_create() when other context is executing
(***), then dm-dm_generation is updated incorrect value.
As a result, we can not open the last detected device (we receive ENOENT).
I think that we should change the lock method.
May I have advice?
void
devfs_populate(struct devfs_mount *dm)
{
sx_assert(dm-dm_lock, SX_XLOCKED);
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
(***)
dm-dm_generation = devfs_generation;
}
void
devfs_create(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags |= CDP_ACTIVE;
cdp-cdp_inode = alloc_unrl(devfs_inos);
dev_refl(dev);
TAILQ_INSERT_TAIL(cdevp_list, cdp, cdp_list);
devfs_generation++;
}
void
devfs_destroy(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags = ~CDP_ACTIVE;
devfs_generation++;
}
Thanks.
I propose the solution. May I have advice?
void
devfs_populate(struct devfs_mount *dm)
{
sx_assert(dm-dm_lock, SX_XLOCKED);
#if 1 /* I propose... */
int tmp_generation;
retry:
tmp_generation = devfs_generation;
if (dm-dm_generation == tmp_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
if (tmp_generation != devfs_generation)
goto retry;
dm-dm_generation = tmp_generation;
#else /* Original */
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
dm-dm_generation = devfs_generation;
#endif
}
I think the problem you described is real, and suggested change is right.
Initially, I thought that we should work with devfs_generation as with
the atomic type due to unlocked access in the devfs_populate(), but then
convinced myself that this is not needed.
But also, I think there is another half of the problem. Namely,
devfs_lookup() calls devfs_populate_vp(), and then does lookup with the
help of devfs_lookupx(). We will miss the generation update
happen after the drop of the dm_lock in devfs_populate_vp() to reacquire
the directory vnode lock.
I propose the change below, consisting of your fix and also retry of
population and lookup in case generations do not match for ENOENT
case.
diff --git a/sys/fs/devfs/devfs_devs.c b/sys/fs/devfs/devfs_devs.c
index d72ada0..8ff9bc2 100644
--- a/sys/fs/devfs/devfs_devs.c
+++ b/sys/fs/devfs/devfs_devs.c
@@ -63,7 +63,7 @@ static MALLOC_DEFINE(M_CDEVP, DEVFS1, DEVFS cdev_priv
storage);
static SYSCTL_NODE(_vfs, OID_AUTO, devfs, CTLFLAG_RW, 0, DEVFS filesystem);
-static unsigned devfs_generation;
+unsigned devfs_generation;
SYSCTL_UINT(_vfs_devfs, OID_AUTO, generation, CTLFLAG_RD,
devfs_generation, 0, DEVFS generation number);
@@ -630,13 +630,15 @@ devfs_populate_loop(struct devfs_mount *dm, int cleanup)
void
devfs_populate(struct devfs_mount *dm)
{
+ unsigned gen;
sx_assert(dm-dm_lock, SX_XLOCKED);
- if (dm-dm_generation == devfs_generation)
+ gen = devfs_generation;
+ if (dm-dm_generation == gen)
return;
while (devfs_populate_loop(dm, 0))
continue;
- dm-dm_generation = devfs_generation;
+ dm-dm_generation = gen;
}
/*
diff --git a/sys/fs/devfs/devfs_int.h b/sys/fs/devfs/devfs_int.h
index cdc6aba..cb01ad1 100644
--- a/sys/fs/devfs/devfs_int.h
+++ b/sys/fs/devfs/devfs_int.h
@@ -71,6 +71,8 @@ struct cdev_priv {
#define cdev2priv(c)member2struct(cdev_priv, cdp_c, c)
+extern unsigned devfs_generation;
+
struct cdev *devfs_alloc(int);
int devfs_dev_exists(const char *);
void devfs_free(struct cdev *);
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index 955bd8b..2603caa 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -188,7 +188,7
Bug: devfs is sure to have the bug.
Hello,
I think that devfs is sure to have the bug.
I found that I couldn't open /dev/XXX though the kernel detected XXX
device.
dm-dm_generation is updated with devfs_generation in
devfs_populate(), and the context holds only dm-dm_lock in
devfs_populate().
On the other hand, devfs_generation is incremented in devfs_create()
and devfs_destroy() the context holds only devmtx in devfs_create()
and devfs_destroy().
If a context executes devfs_create() when other context is executing
(***), then dm-dm_generation is updated incorrect value.
As a result, we can not open the last detected device (we receive ENOENT).
I think that we should change the lock method.
May I have advice?
void
devfs_populate(struct devfs_mount *dm)
{
sx_assert(dm-dm_lock, SX_XLOCKED);
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
(***)
dm-dm_generation = devfs_generation;
}
void
devfs_create(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags |= CDP_ACTIVE;
cdp-cdp_inode = alloc_unrl(devfs_inos);
dev_refl(dev);
TAILQ_INSERT_TAIL(cdevp_list, cdp, cdp_list);
devfs_generation++;
}
void
devfs_destroy(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags = ~CDP_ACTIVE;
devfs_generation++;
}
Thanks.
--
Kohji Okuno (okuno.ko...@jp.panasonic.com)
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug: devfs is sure to have the bug.
Hello,
Hello,
I think that devfs is sure to have the bug.
I found that I couldn't open /dev/XXX though the kernel detected XXX
device.
dm-dm_generation is updated with devfs_generation in
devfs_populate(), and the context holds only dm-dm_lock in
devfs_populate().
On the other hand, devfs_generation is incremented in devfs_create()
and devfs_destroy() the context holds only devmtx in devfs_create()
and devfs_destroy().
If a context executes devfs_create() when other context is executing
(***), then dm-dm_generation is updated incorrect value.
As a result, we can not open the last detected device (we receive ENOENT).
I think that we should change the lock method.
May I have advice?
void
devfs_populate(struct devfs_mount *dm)
{
sx_assert(dm-dm_lock, SX_XLOCKED);
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
(***)
dm-dm_generation = devfs_generation;
}
void
devfs_create(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags |= CDP_ACTIVE;
cdp-cdp_inode = alloc_unrl(devfs_inos);
dev_refl(dev);
TAILQ_INSERT_TAIL(cdevp_list, cdp, cdp_list);
devfs_generation++;
}
void
devfs_destroy(struct cdev *dev)
{
struct cdev_priv *cdp;
mtx_assert(devmtx, MA_OWNED);
cdp = cdev2priv(dev);
cdp-cdp_flags = ~CDP_ACTIVE;
devfs_generation++;
}
Thanks.
I propose the solution. May I have advice?
void
devfs_populate(struct devfs_mount *dm)
{
sx_assert(dm-dm_lock, SX_XLOCKED);
#if 1 /* I propose... */
int tmp_generation;
retry:
tmp_generation = devfs_generation;
if (dm-dm_generation == tmp_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
if (tmp_generation != devfs_generation)
goto retry;
dm-dm_generation = tmp_generation;
#else /* Original */
if (dm-dm_generation == devfs_generation)
return;
while (devfs_populate_loop(dm, 0))
continue;
dm-dm_generation = devfs_generation;
#endif
}
Thanks,
Kohji Okuno (okuno.ko...@jp.panasonic.com)
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug about devfs?
Hello,
From: Kostik Belousov kostik...@gmail.com
Date: Tue, 12 Jul 2011 17:57:53 +0300
On Tue, Jul 12, 2011 at 03:02:44PM +0200, Attilio Rao wrote:
2011/7/12 Kostik Belousov kostik...@gmail.com:
On Tue, Jul 12, 2011 at 07:10:28PM +0900, Kohji Okuno wrote:
Hello,
I think that devfs has a problem.
I encountered the problem that open(/dev/AAA) returned ENOENT.
Of course, /dev/AAA exists.
ENOENT was created by the point(***) in devfs_allocv().
I think that the race condition had occurred between process A and
vnlru kernel thread.
Please check the following.
If vnlru set VI_DOOMED to vp-v_iflag but vnlru didn't still execute
VOP_RECLAIM(), process A cat get valid vp from de-de_vnode.
But, vget() will return ENOENT, because vp-v_iflag has VI_DOOMED.
When I set the break point to (***), I checked that de-de_vnode and
vp-v_data were NULL.
process A: vnlru:
devfs_allocv() {
vgonel(vp) {
... ...
vp-v_iflag |= VI_DOOMED;
mtx_lock(devfs_de_interlock); ...
vp = de-de_vnode;
if (vp != NULL) { VI_UNLOCK(vp);
_/ ...
VI_LOCK(vp); / if (VOP_RECLAIM(vp, td))
mtx_unlock(devfs_de_interlock); ...
... \ devfs_reclaim(ap) {
error = vget(vp,...); \
... \__
mtx_lock(devfs_de_interlock);
if (devfs_allocv_drop_refs(...)) { de = vp-v_data;
... if (de != NULL) {
} de-de_vnode = NULL;
else if (error) { vp-v_data = NULL;
... }
rturn (error); (***)
mtx_unlock(devfs_de_interlock);
} ...
}
I think that devfs_allocv() should be fixed as below.
How do you think?
devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode
**vpp)
{
int error;
struct vnode *vp;
struct cdev *dev;
struct devfs_mount *dmp;
dmp = VFSTODEVFS(mp);
+#if 1
+ retry:
+#endif
if (de-de_flags DE_DOOMED) {
...
mtx_lock(devfs_de_interlock);
vp = de-de_vnode;
if (vp != NULL) {
VI_LOCK(vp);
mtx_unlock(devfs_de_interlock);
sx_xunlock(dmp-dm_lock);
error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, curthread);
sx_xlock(dmp-dm_lock);
if (devfs_allocv_drop_refs(0, dmp, de)) {
if (error == 0)
vput(vp);
return (ENOENT);
}
else if (error) {
+#if 1
+ if (error == ENOENT)
+ goto retry;
+#endif
sx_xunlock(dmp-dm_lock);
return (error);
}
Thank you for the report.
The proposed change would revert r179247, which also caused some issues.
Are you able to reproduce the problem ?
Could you try the following patch ? I cannot reproduce your situation,
so the patch is untested by me.
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index bf6dab8..bbbfff4 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -397,6 +397,7 @@ devfs_allocv(struct devfs_dirent *de, struct mount
*mp, int lockmode,
sx_xunlock(dmp-dm_lock);
return (ENOENT);
}
+loop:
DEVFS_DE_HOLD(de);
DEVFS_DMP_HOLD(dmp);
mtx_lock(devfs_de_interlock);
@@ -412,7 +413,16 @@ devfs_allocv(struct devfs_dirent *de, struct mount
*mp, int lockmode,
vput(vp);
return (ENOENT);
}
- else if (error) {
+ else if (error != 0) {
+ if (error == ENOENT) {
+ mtx_lock(devfs_de_interlock);
+ while (de-de_vnode != NULL) {
+ msleep(de-de_vnode,
+ devfs_de_interlock, 0,
dvall, 0);
+ }
+ mtx_unlock(devfs_de_interlock);
+ goto loop;
+ }
sx_xunlock(dmp-dm_lock);
return (error);
}
@@ -1259,6 +1269,7 @@ devfs_reclaim(struct vop_reclaim_args *ap)
if (de != NULL
Bug about devfs?
Hello,
I think that devfs has a problem.
I encountered the problem that open(/dev/AAA) returned ENOENT.
Of course, /dev/AAA exists.
ENOENT was created by the point(***) in devfs_allocv().
I think that the race condition had occurred between process A and
vnlru kernel thread.
Please check the following.
If vnlru set VI_DOOMED to vp-v_iflag but vnlru didn't still execute
VOP_RECLAIM(), process A cat get valid vp from de-de_vnode.
But, vget() will return ENOENT, because vp-v_iflag has VI_DOOMED.
When I set the break point to (***), I checked that de-de_vnode and
vp-v_data were NULL.
process A: vnlru:
devfs_allocv() {
vgonel(vp) {
... ...
vp-v_iflag |= VI_DOOMED;
mtx_lock(devfs_de_interlock); ...
vp = de-de_vnode;
if (vp != NULL) { VI_UNLOCK(vp);
_/ ...
VI_LOCK(vp); / if (VOP_RECLAIM(vp, td))
mtx_unlock(devfs_de_interlock); ...
... \ devfs_reclaim(ap) {
error = vget(vp,...); \
...\__ mtx_lock(devfs_de_interlock);
if (devfs_allocv_drop_refs(...)) {de = vp-v_data;
... if (de != NULL) {
} de-de_vnode = NULL;
else if (error) { vp-v_data = NULL;
... }
rturn (error); (***)mtx_unlock(devfs_de_interlock);
} ...
}
I think that devfs_allocv() should be fixed as below.
How do you think?
devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp)
{
int error;
struct vnode *vp;
struct cdev *dev;
struct devfs_mount *dmp;
dmp = VFSTODEVFS(mp);
+#if 1
+ retry:
+#endif
if (de-de_flags DE_DOOMED) {
...
mtx_lock(devfs_de_interlock);
vp = de-de_vnode;
if (vp != NULL) {
VI_LOCK(vp);
mtx_unlock(devfs_de_interlock);
sx_xunlock(dmp-dm_lock);
error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, curthread);
sx_xlock(dmp-dm_lock);
if (devfs_allocv_drop_refs(0, dmp, de)) {
if (error == 0)
vput(vp);
return (ENOENT);
}
else if (error) {
+#if 1
+ if (error == ENOENT)
+ goto retry;
+#endif
sx_xunlock(dmp-dm_lock);
return (error);
}
Thanks,
Kohji Okuno.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: Bug about devfs?
Hello,
From: Kostik Belousov kostik...@gmail.com
Subject: Re: Bug about devfs?
Date: Tue, 12 Jul 2011 14:19:25 +0300
Message-ID: 20110712111925.gh43...@deviant.kiev.zoral.com.ua
Thank you for the report.
The proposed change would revert r179247, which also caused some issues.
Are you able to reproduce the problem ?
Could you try the following patch ? I cannot reproduce your situation,
so the patch is untested by me.
Thank you for quick your comment.
I think that your change is beter than mine.
I will test it, and I will report the result.
On Tue, Jul 12, 2011 at 07:10:28PM +0900, Kohji Okuno wrote:
Hello,
I think that devfs has a problem.
I encountered the problem that open(/dev/AAA) returned ENOENT.
Of course, /dev/AAA exists.
ENOENT was created by the point(***) in devfs_allocv().
I think that the race condition had occurred between process A and
vnlru kernel thread.
Please check the following.
If vnlru set VI_DOOMED to vp-v_iflag but vnlru didn't still execute
VOP_RECLAIM(), process A cat get valid vp from de-de_vnode.
But, vget() will return ENOENT, because vp-v_iflag has VI_DOOMED.
When I set the break point to (***), I checked that de-de_vnode and
vp-v_data were NULL.
process A: vnlru:
devfs_allocv() {
vgonel(vp) {
... ...
vp-v_iflag |= VI_DOOMED;
mtx_lock(devfs_de_interlock);...
vp = de-de_vnode;
if (vp != NULL) {VI_UNLOCK(vp);
_/ ...
VI_LOCK(vp); / if (VOP_RECLAIM(vp, td))
mtx_unlock(devfs_de_interlock); ...
... \ devfs_reclaim(ap) {
error = vget(vp,...); \
... \__
mtx_lock(devfs_de_interlock);
if (devfs_allocv_drop_refs(...)) {de = vp-v_data;
... if (de != NULL) {
}de-de_vnode = NULL;
else if (error) {vp-v_data = NULL;
... }
rturn (error); (***) mtx_unlock(devfs_de_interlock);
}...
}
I think that devfs_allocv() should be fixed as below.
How do you think?
devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp)
{
int error;
struct vnode *vp;
struct cdev *dev;
struct devfs_mount *dmp;
dmp = VFSTODEVFS(mp);
+#if 1
+ retry:
+#endif
if (de-de_flags DE_DOOMED) {
...
mtx_lock(devfs_de_interlock);
vp = de-de_vnode;
if (vp != NULL) {
VI_LOCK(vp);
mtx_unlock(devfs_de_interlock);
sx_xunlock(dmp-dm_lock);
error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, curthread);
sx_xlock(dmp-dm_lock);
if (devfs_allocv_drop_refs(0, dmp, de)) {
if (error == 0)
vput(vp);
return (ENOENT);
}
else if (error) {
+#if 1
+if (error == ENOENT)
+goto retry;
+#endif
sx_xunlock(dmp-dm_lock);
return (error);
}
Thank you for the report.
The proposed change would revert r179247, which also caused some issues.
Are you able to reproduce the problem ?
Could you try the following patch ? I cannot reproduce your situation,
so the patch is untested by me.
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index bf6dab8..bbbfff4 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -397,6 +397,7 @@ devfs_allocv(struct devfs_dirent *de, struct mount *mp,
int lockmode,
sx_xunlock(dmp-dm_lock);
return (ENOENT);
}
+loop:
DEVFS_DE_HOLD(de);
DEVFS_DMP_HOLD(dmp);
mtx_lock(devfs_de_interlock);
@@ -412,7 +413,16 @@ devfs_allocv(struct devfs_dirent *de, struct mount *mp,
int lockmode,
vput(vp);
return (ENOENT);
}
- else if (error) {
+ else if (error != 0) {
+ if (error == ENOENT) {
+ mtx_lock(devfs_de_interlock);
+ while (de-de_vnode != NULL) {
+ msleep(de-de_vnode,
+ devfs_de_interlock, 0, dvall, 0);
+ }
+ mtx_unlock(devfs_de_interlock);
+ goto loop
Re: Bug about devfs?
Hello,
From: Kostik Belousov kostik...@gmail.com
Date: Tue, 12 Jul 2011 17:57:53 +0300
On Tue, Jul 12, 2011 at 03:02:44PM +0200, Attilio Rao wrote:
2011/7/12 Kostik Belousov kostik...@gmail.com:
On Tue, Jul 12, 2011 at 07:10:28PM +0900, Kohji Okuno wrote:
Hello,
I think that devfs has a problem.
I encountered the problem that open(/dev/AAA) returned ENOENT.
Of course, /dev/AAA exists.
ENOENT was created by the point(***) in devfs_allocv().
I think that the race condition had occurred between process A and
vnlru kernel thread.
Please check the following.
If vnlru set VI_DOOMED to vp-v_iflag but vnlru didn't still execute
VOP_RECLAIM(), process A cat get valid vp from de-de_vnode.
But, vget() will return ENOENT, because vp-v_iflag has VI_DOOMED.
When I set the break point to (***), I checked that de-de_vnode and
vp-v_data were NULL.
process A: vnlru:
devfs_allocv() {
vgonel(vp) {
... ...
vp-v_iflag |= VI_DOOMED;
mtx_lock(devfs_de_interlock); ...
vp = de-de_vnode;
if (vp != NULL) { VI_UNLOCK(vp);
_/ ...
VI_LOCK(vp); / if (VOP_RECLAIM(vp, td))
mtx_unlock(devfs_de_interlock); ...
... \ devfs_reclaim(ap) {
error = vget(vp,...); \
... \__
mtx_lock(devfs_de_interlock);
if (devfs_allocv_drop_refs(...)) { de = vp-v_data;
... if (de != NULL) {
} de-de_vnode = NULL;
else if (error) { vp-v_data = NULL;
... }
rturn (error); (***) mtx_unlock(devfs_de_interlock);
} ...
}
I think that devfs_allocv() should be fixed as below.
How do you think?
devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode
**vpp)
{
int error;
struct vnode *vp;
struct cdev *dev;
struct devfs_mount *dmp;
dmp = VFSTODEVFS(mp);
+#if 1
+ retry:
+#endif
if (de-de_flags DE_DOOMED) {
...
mtx_lock(devfs_de_interlock);
vp = de-de_vnode;
if (vp != NULL) {
VI_LOCK(vp);
mtx_unlock(devfs_de_interlock);
sx_xunlock(dmp-dm_lock);
error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, curthread);
sx_xlock(dmp-dm_lock);
if (devfs_allocv_drop_refs(0, dmp, de)) {
if (error == 0)
vput(vp);
return (ENOENT);
}
else if (error) {
+#if 1
+ if (error == ENOENT)
+ goto retry;
+#endif
sx_xunlock(dmp-dm_lock);
return (error);
}
Thank you for the report.
The proposed change would revert r179247, which also caused some issues.
Are you able to reproduce the problem ?
Could you try the following patch ? I cannot reproduce your situation,
so the patch is untested by me.
diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index bf6dab8..bbbfff4 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
@@ -397,6 +397,7 @@ devfs_allocv(struct devfs_dirent *de, struct mount
*mp, int lockmode,
sx_xunlock(dmp-dm_lock);
return (ENOENT);
}
+loop:
DEVFS_DE_HOLD(de);
DEVFS_DMP_HOLD(dmp);
mtx_lock(devfs_de_interlock);
@@ -412,7 +413,16 @@ devfs_allocv(struct devfs_dirent *de, struct mount
*mp, int lockmode,
vput(vp);
return (ENOENT);
}
- else if (error) {
+ else if (error != 0) {
+ if (error == ENOENT) {
+ mtx_lock(devfs_de_interlock);
+ while (de-de_vnode != NULL) {
+ msleep(de-de_vnode,
+ devfs_de_interlock, 0,
dvall, 0);
+ }
+ mtx_unlock(devfs_de_interlock);
+ goto loop;
+ }
sx_xunlock(dmp-dm_lock);
return (error);
}
@@ -1259,6 +1269,7 @@ devfs_reclaim(struct vop_reclaim_args *ap)
if (de != NULL
Re: About 32bit binary on amd64
Thank you for your comments. In my usage, it works good by the patch appended to this mail. On May 24, 2010, at 2:49 AM, pluknet wrote: On 24 May 2010 08:49, Kohji Okuno okuno.ko...@jp.panasonic.com wrote: Hi all, I want to compile 32bit binary on amd64, but I met with the problem. Could you teach me the best solution, please? My environment is FreeBSD 8.1-PRERELEASE #0: Tue May 18 12:01:26 JST 2010. I compiled and executed test.c as below on amd64. [...] % gcc -m32 -B/usr/lib32 test.c % ./a.out mmap: Invalid argument AFAIK, it still doesn't work on FreeBSD. You need something like 32bit chroot environment. There's also about: http://www.freebsd.org/projects/ideas/#p-freebsd-amd64-gcc-m32 -m32 is busted on FreeBSD; I don't remember the full details but I think it had something to do with the linking stage of things... Thanks, -Garrett diff -Nur machine.org/_inttypes.h machine/_inttypes.h --- machine.org/_inttypes.h 2009-08-03 17:13:06.0 +0900 +++ machine/_inttypes.h 2010-05-24 10:16:12.130753024 +0900 @@ -37,6 +37,9 @@ * $FreeBSD: src/sys/amd64/include/_inttypes.h,v 1.3.34.1 2009/08/03 08:13:06 kensmith Exp $ */ +#ifdef __i386__ +#include machine/_inttypes32.h +#else #ifndef _MACHINE_INTTYPES_H_ #define _MACHINE_INTTYPES_H_ @@ -218,3 +221,4 @@ #define SCNxPTR lx /* uintptr_t */ #endif /* !_MACHINE_INTTYPES_H_ */ +#endif /* __i386__ */ diff -Nur machine.org/_inttypes32.h machine/_inttypes32.h --- machine.org/_inttypes32.h 1970-01-01 09:00:00.0 +0900 +++ machine/_inttypes32.h 2010-05-24 10:17:17.382704652 +0900 @@ -0,0 +1,220 @@ +/*- + * Copyright (c) 2001 The NetBSD Foundation, Inc. + * All rights reserved. + * + * This code is derived from software contributed to The NetBSD Foundation + * by Klaus Klein. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + *notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + *notice, this list of conditions and the following disclaimer in the + *documentation and/or other materials provided with the distribution. + * 3. All advertising materials mentioning features or use of this software + *must display the following acknowledgement: + *This product includes software developed by the NetBSD + *Foundation, Inc. and its contributors. + * 4. Neither the name of The NetBSD Foundation nor the names of its + *contributors may be used to endorse or promote products derived + *from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTORS + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTORS + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + * + * From: $NetBSD: int_fmtio.h,v 1.2 2001/04/26 16:25:21 kleink Exp $ + * $FreeBSD: src/sys/i386/include/_inttypes.h,v 1.2.36.1 2009/08/03 08:13:06 kensmith Exp $ + */ + +#ifndef _MACHINE_INTTYPES_H_ +#define _MACHINE_INTTYPES_H_ + +/* + * Macros for format specifiers. + */ + +/* fprintf(3) macros for signed integers. */ + +#define PRId8 d /* int8_t */ +#define PRId16 d /* int16_t */ +#define PRId32 d /* int32_t */ +#define PRId64 lld /* int64_t */ +#define PRIdLEAST8 d /* int_least8_t */ +#define PRIdLEAST16 d /* int_least16_t */ +#define PRIdLEAST32 d /* int_least32_t */ +#define PRIdLEAST64 lld /* int_least64_t */ +#define PRIdFAST8 d /* int_fast8_t */ +#define PRIdFAST16 d /* int_fast16_t */ +#define PRIdFAST32 d /* int_fast32_t */ +#define PRIdFAST64 lld /* int_fast64_t */ +#define PRIdMAX jd /* intmax_t */ +#define PRIdPTR d /* intptr_t */ + +#define PRIi8 i /* int8_t */ +#define PRIi16 i /* int16_t */ +#define PRIi32 i /* int32_t */ +#define PRIi64 lli /* int64_t */ +#define PRIiLEAST8 i /* int_least8_t */ +#define PRIiLEAST16 i /* int_least16_t */ +#define PRIiLEAST32 i /* int_least32_t */ +#define PRIiLEAST64 lli /* int_least64_t */ +#define PRIiFAST8 i /* int_fast8_t */ +#define PRIiFAST16 i /* int_fast16_t */ +#define PRIiFAST32 i /* int_fast32_t */ +#define PRIiFAST64 lli /* int_fast64_t */ +#define PRIiMAX ji /* intmax_t */ +#define PRIiPTR i /* intptr_t
About 32bit binary on amd64
Hi all,
I want to compile 32bit binary on amd64, but I met with the problem.
Could you teach me the best solution, please?
My environment is FreeBSD 8.1-PRERELEASE #0: Tue May 18 12:01:26 JST 2010.
I compiled and executed test.c as below on amd64.
-- begin -- test.c --
#include stdlib.h
#include stdio.h
#include sys/mman.h
#include unistd.h
int main()
{
void *ptr;
ptr = mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_ANON, -1, 0);
if (ptr == MAP_FAILED) {
perror(mmap); exit(1);
}
printf(%lx\n, (unsigned long)ptr);
munmap(ptr, 4096);
exit(0);
}
-- end -- test.c
% gcc -m32 -B/usr/lib32 test.c
% ./a.out
mmap: Invalid argument
I disassembled a.out. I think that 'movl $0x0,0x18(%esp)' is needed.
This error is occured by 'off_t offset'.
80484ce: 83 ec 34sub$0x34,%esp
80484d1: c7 44 24 14 00 00 00movl $0x0,0x14(%esp)
80484d8: 00
80484d9: c7 44 24 10 ff ff ffmovl $0x,0x10(%esp)
80484e0: ff
80484e1: c7 44 24 0c 00 10 00movl $0x1000,0xc(%esp)
80484e8: 00
80484e9: c7 44 24 08 03 00 00movl $0x3,0x8(%esp)
80484f0: 00
80484f1: c7 44 24 04 00 10 00movl $0x1000,0x4(%esp)
80484f8: 00
80484f9: c7 04 24 00 00 00 00movl $0x0,(%esp)
8048500: e8 43 fe ff ff call 8048348 m...@plt
I found this solution.
I modified machine depended header as below.
# cd /usr/include/machine
# cp /usr/src/sys/i386/include/_types.h _types32.h
# patch _types.h.diff
-- begin -- _types.h.diff
--- _types.h.org2010-05-24 13:34:55.406874258 +0900
+++ _types.h2010-05-24 13:35:33.790522354 +0900
@@ -36,6 +36,10 @@
* $FreeBSD: src/sys/amd64/include/_types.h,v 1.12.2.1 2009/08/03 08:13:06
kensmith Exp $
*/
+#ifdef __i386__
+#include machine/_types32.h
+#else
+
#ifndef _MACHINE__TYPES_H_
#define_MACHINE__TYPES_H_
@@ -115,3 +119,4 @@
#endif
#endif /* !_MACHINE__TYPES_H_ */
+#endif /* __i386__ */
-- end -- _types.h.diff --
In this case, 'off_t offset' is set correctly as below.
80484d1: c7 44 24 14 00 00 00movl $0x0,0x14(%esp)
80484d8: 00
80484d9: c7 44 24 18 00 00 00movl $0x0,0x18(%esp)
80484e0: 00
80484e1: c7 44 24 10 ff ff ffmovl $0x,0x10(%esp)
80484e8: ff
80484e9: c7 44 24 0c 00 10 00movl $0x1000,0xc(%esp)
80484f0: 00
80484f1: c7 44 24 08 03 00 00movl $0x3,0x8(%esp)
80484f8: 00
80484f9: c7 44 24 04 00 10 00movl $0x1000,0x4(%esp)
8048500: 00
8048501: c7 04 24 00 00 00 00movl $0x0,(%esp)
8048508: e8 3b fe ff ff call 8048348 m...@plt
How should we deal with this problem?
Best reards,
Kohji Okuno
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
