Bug 11795
| Summary: | ks959-sir dongle no longer works under 2.6.27 (REGRESSION) | ||
|---|---|---|---|
| Product: | Networking | Reporter: | Alex Villacis Lasso (avillaci) |
| Component: | Other | Assignee: | Samuel Ortiz (samuel) |
| Status: | CLOSED CODE_FIX | ||
| Severity: | normal | CC: | anarsoul, patrick.stadelmann, rjw |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.27 | Subsystem: | |
| Regression: | Yes | Bisected commit-id: | |
| Bug Depends on: | |||
| Bug Blocks: | 11167 | ||
| Attachments: |
usbmon trace with dongle inserted, shows speed command being correctly sent
usbmon trace with dongle inserted, 2.6.27, shows absent speed change command dmesg output of affected kernel with proposed patchset skb headromm check [1/3] Create new field tx_extra in skbuff [2/3] IRDA stack should call irda_get_skb_cb (tx_extra implementation) [3/3] IRDA drivers should call irda_get_skb_cb dmesg output after patches for rc4 and running minicom /dev/ircomm0 dmesg output after patches for rc6 and running minicom /dev/ircomm0 |
||
|
Description
Alex Villacis Lasso
2008-10-20 10:49:05 UTC
Created attachment 18378 [details]
usbmon trace with dongle inserted, shows speed command being correctly sent
Created attachment 18379 [details]
usbmon trace with dongle inserted, 2.6.27, shows absent speed change command
It seems not only ks959 support is broken, but that complete irda subsystem is disfuntional in 2.6.27 Guys, could you try this patchset: http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc2/ It reserves an additional skbuff header for the irda_skb_cb. The latter should thus be safe accross dev_queue_xmit. Works just fine on 2.6.28-rc2 with this patchset. Thank you! (In reply to comment #5) > Works just fine on 2.6.28-rc2 with this patchset. Thank you! > No go, no go. The patchset still does not solve the problem in my setup. I tested with the kingsun-sir dongle and the ks959-sir dongles. With both of them, discovery seems to work fine, but the act of connecting with obexftp fails inmediately. Vasily, could you please describe your IRDA setup? What kind of dongle are you using? Do any strange messages appear? How did you test that the patchset actually works? Did you use obexftp/gnokii/other, or just ifconfig? Regarding the patchset, I think there might not be a guarantee that all the sk_buff structs allocated were actually allocated with the new functions. Created attachment 18692 [details]
dmesg output of affected kernel with proposed patchset
Notice the strange LMP messages
> Vasily, could you please describe your IRDA setup? What kind of dongle are
> you
> using? Do any strange messages appear? How did you test that the patchset
> actually works? Did you use obexftp/gnokii/other, or just ifconfig?
I tested this code with ircp, and ftpobex.
ftpobex connects to my nokia phone fines, and I could download an Image from there with ircp.
You're right though, we could be missing some skbuff down the stack.
I'll send an additional patch to check for the skbuff header validity, today or tomorrow.
Created attachment 18695 [details]
skb headromm check
> You're right though, we could be missing some skbuff down the stack.
> I'll send an additional patch to check for the skbuff header validity, today
> or
> tomorrow.
I just added a patch that will simply write some error message whenever we dont have enough skb headroom.
Alex, could you please apply this patch, and try again ?
2 Alex Villacis Lasso: I'm using ks959 dongle, I've used gnokii (with nokia 6020 phone) and kbeam (with 6020 and my pda) to test irda, both works fine. Btw, I got some strange messages on dmesg too: irlap_change_speed(), setting speed to 9600 irlap_change_speed(), setting speed to 57600 irlmp_state_dtr(), Unknown event LM_LAP_CONNECT_CONFIRM on LSAP 0x11 irlmp_state_setup() WATCHDOG_TIMEOUT! irda_connect(), connect failed! irlmp_state_dtr(), Unknown event LM_LAP_CONNECT_CONFIRM on LSAP 0x11 irlap_change_speed(), setting speed to 9600 irda_connect(), connect failed! irlap_change_speed(), setting speed to 57600 (In reply to comment #12) > Btw, I got some strange messages on dmesg too: > > irlap_change_speed(), setting speed to 9600 > irlap_change_speed(), setting speed to 57600 > irlmp_state_dtr(), Unknown event LM_LAP_CONNECT_CONFIRM on LSAP 0x11 > irlmp_state_setup() WATCHDOG_TIMEOUT! > irda_connect(), connect failed! > irlmp_state_dtr(), Unknown event LM_LAP_CONNECT_CONFIRM on LSAP 0x11 > irlap_change_speed(), setting speed to 9600 > irda_connect(), connect failed! > irlap_change_speed(), setting speed to 57600 Weird...you confirm that you never had those messages with a working kernel (e.g. 2.6.26) ? I never got "Uknown event" and "WATCHDOG_TIMEOUT/connection failed" on 2.6.26 (In reply to comment #14) > I never got "Uknown event" and "WATCHDOG_TIMEOUT/connection failed" on 2.6.26 Do you mind applying the kernel patch I just attached to this bug and give it another try ? Thanks. With this patch I get a lot of ***** Not enough headroom, skb will be corrupted ***** messages on dmesg Handled-By : Samuel Ortiz <samuel@sortiz.org> Notify-Also : Vasily <anarsoul@gmail.com> Created attachment 18781 [details]
[1/3] Create new field tx_extra in skbuff
The previous patchsets attempt to fix the clobbering of the irda information by storing it within the data payload itself, as an additional header. For this, space has been allocated via skb_pull (?!) and later, skb_reserve(). The problem with this approach is that we do not have a guarantee that all skbuffs that are processed in the irda stack are actually allocated with functions that reserve the required space for the irda metadata, especially in the tx route. In addition, this approach mixes the payload data with metadata that should not be transmitted at all, which is a bit disorganized.
This is the first of 3 patches that try a different approach. Instead of allocating an additional "header" within the data buffer itself, it introduces a new field within the skbuff, named tx_extra. This field should be used for passing data from the higher layers that is required for the drivers to transmit the packet correctly, and formalizes the previous usage of the cb field by the irda stack. The only issue I see is that every single skbuff carries an additional 32 bytes which are not put to any use in other stacks (for now). I was thinking about a pointer field to on-the-fly allocated data, but that means messing around with the skbuff allocation functions, the cloning functions (involving deciding how to behave on cloning), etc. This way is simpler to understand.
This patch and the other two that follow fix the issue for me under 2.6.28-rc3.
Created attachment 18782 [details]
[2/3] IRDA stack should call irda_get_skb_cb (tx_extra implementation)
A rewrite of the original patch by Samuel Ortiz, for the tx_extra implementation. Does away with irda_alloc_skb which is irrelevant for this implementation.
Created attachment 18783 [details]
[3/3] IRDA drivers should call irda_get_skb_cb
The original patch by Samuel Ortiz, which applies without changes for the tx_extra implementation.
(In reply to comment #18) > Created an attachment (id=18781) [details] > [1/3] Create new field tx_extra in skbuff > > The previous patchsets attempt to fix the clobbering of the irda information > by > storing it within the data payload itself, as an additional header. For this, > space has been allocated via skb_pull (?!) and later, skb_reserve(). No, the additional space is allocated with alloc_skb(), see irda_alloc_skb() in: http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc2/0001-irda-Introduce-irda_alloc_skb.patch > The > problem with this approach is that we do not have a guarantee that all > skbuffs > that are processed in the irda stack are actually allocated with functions > that > reserve the required space for the irda metadata, especially in the tx route. > In addition, this approach mixes the payload data with metadata that should > not > be transmitted at all, which is a bit disorganized. > > This is the first of 3 patches that try a different approach. Instead of > allocating an additional "header" within the data buffer itself, it > introduces > a new field within the skbuff, named tx_extra. This field should be used for > passing data from the higher layers that is required for the drivers to > transmit the packet correctly, and formalizes the previous usage of the cb > field by the irda stack. The only issue I see is that every single skbuff > carries an additional 32 bytes which are not put to any use in other stacks > (for now). I was thinking about a pointer field to on-the-fly allocated data, > but that means messing around with the skbuff allocation functions, the > cloning > functions (involving deciding how to behave on cloning), etc. This way is > simpler to understand. > > This patch and the other two that follow fix the issue for me under > 2.6.28-rc3. > Ok, so I managed to reproduce the case where I was getting insufficient head room with my patches. I think I fixed it now, and it would be very nice if Alex and Vasily could try my fixes. It's a serie of 8 patches, to be applied on top of any recent 2.8.28-rc kernel. They're here: http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc4/ along with a series file to apply them in the right order. If you guys have time to test those patches, please let me know how it works for you. Cheers, Samuel. Works for me with latest patchset applied on the top of gentoo's 2.6.27-r2 kernel The most recent patches from Samuel Ortiz do fix the problem. Tested with ks959-sir and kingsun-sir on a 2.6.27-rc4 kernel. Right before submitting this patch set to Dave Miller, I realized that having skb->head point to irda_skb_cb is in theory a bad idea. For dev_alloc_skb() created skbs, packet schedulers could in theory overwrite skb->head. However, we reserve enough head room for both the packet schedulers and our data to fit. Thus, our irda_skb_cb should live right before skb->data, instead of at the beginning of the skb's headroom. The changes are minor, and in fact, only affect pacthes #1 and #8. It works for me (TM), but I'd appreciate to have a confirmation from either Alex ot Vasily. If that's the case, I'll submit this patchset upstream. Thanks for your help. (In reply to comment #26) > Right before submitting this patch set to Dave Miller, I realized that having > skb->head point to irda_skb_cb is in theory a bad idea. > For dev_alloc_skb() created skbs, packet schedulers could in theory overwrite > skb->head. However, we reserve enough head room for both the packet > schedulers > and our data to fit. Thus, our irda_skb_cb should live right before > skb->data, > instead of at the beginning of the skb's headroom. > > The changes are minor, and in fact, only affect pacthes #1 and #8. It works > for > me (TM), but I'd appreciate to have a confirmation from either Alex ot > Vasily. > If that's the case, I'll submit this patchset upstream. > Thanks for your help. > Could you please indicate where can I get updated patches? As of 2008/11/24 11:41 GMT -5 the patches at the last URL are the exact same ones as previously tested. > Could you please indicate where can I get updated patches? As of 2008/11/24 > 11:41 GMT -5 the patches at the last URL are the exact same ones as > previously > tested. Oops, forgot that, sorry. The patches are here: http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc6/ Thanks, Samuel. (In reply to comment #28) > > Could you please indicate where can I get updated patches? As of 2008/11/24 > > 11:41 GMT -5 the patches at the last URL are the exact same ones as > previously > > tested. Tested with ks959-sir. I think I spoke too soon when saying that the patches fix the problem. They do fix the problem when using openobex to communicate with the phone. However, when using minicom with /dev/ircomm0, the WARN_ON again triggers. This happens both with the patches at http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc4/ and the ones at http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc6/ . I will attach the dmesg output next. Created attachment 19065 [details]
dmesg output after patches for rc4 and running minicom /dev/ircomm0
Created attachment 19066 [details]
dmesg output after patches for rc6 and running minicom /dev/ircomm0
Last patchset doesn't work for me at all (applied on gentoo's gentoo-sources-2.6.27-r4) > Tested with ks959-sir. I think I spoke too soon when saying that the patches > fix the problem. They do fix the problem when using openobex to communicate > with the phone. However, when using minicom with /dev/ircomm0, the WARN_ON > again triggers. This happens both with the patches at > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc4/ > and the ones at > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc6/ > . > I will attach the dmesg output next. Ok, with the patches here: http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc7/ I couldnt reproduce those warnings. Note that the patches should also apply against an -rc6. Would you mind giving them a try ? Cheers, Samuel. (In reply to comment #33) > > Tested with ks959-sir. I think I spoke too soon when saying that the > patches > > fix the problem. They do fix the problem when using openobex to communicate > > with the phone. However, when using minicom with /dev/ircomm0, the WARN_ON > > again triggers. This happens both with the patches at > > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc4/ > > and the ones at > > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc6/ > . > > I will attach the dmesg output next. > Ok, with the patches here: > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc7/ > > I couldnt reproduce those warnings. Note that the patches should also apply > against an -rc6. > Would you mind giving them a try ? > > Cheers, > Samuel. > Tested with 2.6.28-rc7. There are no warnings anymore with obex_test from openobex package, as well as minicom /dev/ircomm0. (In reply to comment #34) > (In reply to comment #33) > > > Tested with ks959-sir. I think I spoke too soon when saying that the > patches > > > fix the problem. They do fix the problem when using openobex to > communicate > > > with the phone. However, when using minicom with /dev/ircomm0, the > WARN_ON > > > again triggers. This happens both with the patches at > > > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc4/ > > > and the ones at > > > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc6/ > . > > > I will attach the dmesg output next. > > Ok, with the patches here: > > > > > http://www.kernel.org/pub/linux/kernel/people/sameo/patches/irda/2.6.28-rc7/ > > > > I couldnt reproduce those warnings. Note that the patches should also apply > > against an -rc6. > > Would you mind giving them a try ? > > > > Cheers, > > Samuel. > > > > Tested with 2.6.28-rc7. There are no warnings anymore with obex_test from > openobex package, as well as minicom /dev/ircomm0. Awesome, thanks a lot for testing. I'll submit these patches upstream tomorrow, you guys still have 24 hours to show the red light. Cheers, Samuel. Just for the record, my latest patchset has not been accepted upstream. Instead, davem took a hckish fix in order to have a functional IrDA stack with 2.6.28: http://git.kernel.org/?p=linux/kernel/git/davem/net-2.6.git;a=commit;h=69c30e1e7492192f882a3fc11888b320fde5206a |