SCO flow control
PATCH 1/1 SCO flow control
Hi all,
I just finished a patch to bluez that implements flow control for SCO.
If you are interested in the subject, take a deep breath a read on... :-)
The goal of the "flow control" feature is the following :
- improve robustness : using this patch, user space software is not
able to crash your Linux box anymore by flooding it with SCO packets.
- ease of programmation for user space software. Without this feature
user space software has to use one of the following tricks:
1°) Send a sco packet to the peer only when it receives a sco
packet from the peer. This is the trick used by hstest software shipped with bluez-utils for instance.
2°) Throttle itself using some kind of sleeping routines. This
is the approach i used when writing bluez-headset (See one of my precedent posts for more information on that).
- This patch has also the side effect to be able to know how many
packets are "in the pipe", i.e. in the socket queue, but not yet sent over the air.
This is the kind of information expected by traditionnal PCM applications (e.g. this is called the "buffer size" in ALSA language). Thus it makes PCM emulation over bluetooth *much* simpler. :-)
How it works
- SCO socket layer has been further extented, and now uses thesk_sndbuf and sk_rcvbuf socket variables to count the number of packets that are "in the pipe". When the sk_sndbuf has reached its maximum value and the application tries to send more, it goes to sleep (or the send call returns with -EAGAIN if non blocking mode was selected). The maximum values can be changed using two new SCO socket options: SCO_RXBUFS and SCO_TXBUFS.
- After a SCO packet has been sent to HCI, a timer is started. This
timer expires when the SCO packet has been sent. Timer expired event then triggers the tx tasklet, that will dequeue one packet from the SCO socket queue, send it to the HCI, and eventually wake up the corresponding user process.
- Scheduling algorithm has been changed for SCO, so that there is a
maximum of 1 hardware SCO buffer used per SCO connection. This may seem
stupid to have only *one* packet. I tried to use more SCO buffers, however it really didn't improve sound quality. So supposedly one is big enough. :-)
Other nice features:
- I extended the sco modules with two new parameters:
"tx_quality_tune", and "rx_quality_tune". These parameters, when set to 1, will make the sco modules print a warning message when the SCO stream
gets interrupted for whatever reason (aka 'audio cut'), respectively in tx an rx. This feature is to be used by application developpers to fine tune their SCO_TXBUFS and SCO_RXBUFS parameters.
So, (we're close to the end...)
I ran the following tests on this patch:
- Play a a sound using hstest
- Play/Record using asound/arecord + bluez-headset
I would very much like feedback from people, using bluetooth devices other than USB though, as i only have USB dongles to do my testing.
To sum-up, this is non-trivial patch.
I'm would be very much interested in a review from kernel hackers, and would be quite happy to see this patch merged at some point in the future.
Now a question for Marcel : Could you have a look ? Can you tell what you would do with it ? (e.g: drop it - it's crap. OR cool! but you should change x and y and z...)
Cheers,
Fabien
PS: I'm not yet used to linux kernel processes, but will be happy to improve the patch/reformat things so that it fits into best practices. :-)
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/hci.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/hci.h +7 -0
@@ -712,12 +712,15 @@__u32 sco_tx;__u32 sco_rx;__u32 byte_rx;__u32 byte_tx; }; +/* Fields down there are mostly the same as hci_dev, +as this structure is meant to communicate info +to userspace */ struct hci_dev_info {__u16 dev_id;char name[8 ];bdaddr_t bdaddr;
@@ -727,15 +730,19 @@__u8 features[8 ];__u32 pkt_type;__u32 link_policy;__u32 link_mode; +/* Maximum transmition unit for ACL packets */ __u16 acl_mtu; +/* Number of ACL packets the baseband is able to buffer */ __u16 acl_pkts; +/* Maximum transmition unit for SCO packets */ __u16 sco_mtu; +/* Number of SCO packets the baseband is able to buffer */ __u16 sco_pkts;struct hci_dev_stats stat; };struct hci_conn_info {
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/hci_core.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/hci_core.h +36 -16
149 lines not on this page · raw message
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/sco.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/sco.h +3 -5
@@ -23,18 +23,13 @@*/ #ifndef __SCO_H#define __SCO_H/* SCO defaults */ -#define SCO_DEFAULT_MTU 500 -#define SCO_DEFAULT_FLUSH_TO 0xFFFF -#define SCO_CONN_TIMEOUT (HZ * 40) -#define SCO_DISCONN_TIMEOUT (HZ * 2) -#define SCO_CONN_IDLE_TIMEOUT (HZ * 60)/* SCO socket address */ struct sockaddr_sco {sa_family_t sco_family;bdaddr_t sco_bdaddr; };
@@ -48,12 +43,15 @@#define SCO_CONNINFO 0x02struct sco_conninfo {__u16 hci_handle;__u8 dev_class[3 ]; }; +#define SCO_TXBUFS 0x03 +#define SCO_RXBUFS 0x04 +/* ---- SCO connections ---- */ struct sco_conn {struct hci_conn *hcon;bdaddr_t *dst;bdaddr_t *src;
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/af_bluetooth.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/af_bluetooth.c +1 -1
@@ -46,13 +46,13 @@#ifndef CONFIG_BT_SOCK_DEBUG #undef BT_DBG #define BT_DBG(D...)#endif -#define VERSION "2.8" +#define VERSION "2.9"/* Bluetooth sockets */ #define BT_MAX_PROTO 8static struct net_proto_family *bt_proto[BT_MAX_PROTO ];int bt_sock_register (int proto,struct net_proto_family *ops)
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/hci_conn.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/hci_conn.c +8 -6
@@ -130,17 +130,17 @@else conn->state =BT_CLOSED ;hci_dev_unlock (hdev);return ; } -static void hci_conn_init_timer (struct hci_conn *conn) +static void hci_conn_init_disc_timer (struct hci_conn *conn) { -init_timer (&conn->timer); - conn->timer.function = hci_conn_timeout; - conn->timer.data = (unsigned long )conn; +init_timer (&conn->disc_timer); + conn->disc_timer.function = hci_conn_timeout; + conn->disc_timer.data = (unsigned long )conn; }struct hci_conn *hci_conn_add (struct hci_dev *hdev,int type,bdaddr_t *dst) {struct hci_conn *conn;
@@ -153,13 +153,14 @@bacpy (&conn->dst, dst); conn->type = type; conn->hdev = hdev; conn->state =BT_OPEN ;skb_queue_head_init (&conn->data_q); -hci_conn_init_timer (conn); +hci_conn_init_disc_timer (conn); +init_timer (&conn->data_timer);atomic_set (&conn->refcnt,0 );hci_dev_hold (hdev);tasklet_disable (&hdev->tx_task);
@@ -176,13 +177,14 @@int hci_conn_del (struct hci_conn *conn) {struct hci_dev *hdev = conn->hdev;BT_DBG ("%s conn %p handle %d" , hdev->name, conn, conn->handle); -hci_conn_del_timer (conn); +hci_conn_del_disc_timer (conn); +del_timer_sync (&conn->data_timer);if (conn->type ==SCO_LINK ) {struct hci_conn *acl = conn->link;if (acl) { acl->link =NULL ;hci_conn_put (acl);
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/hci_core.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/hci_core.c +75 -41
201 lines not on this page · raw message
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/sco.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/sco.c +300 -42
537 lines not on this page · raw message
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 1 line
I just finished a patch to bluez that implements flow control for SCO.
please split the changes to Bluetooth core and SCO module. Your changes to the core will conflict with the automatic sniff mode support and I need to sort that out.
And for the core part, please be brief with any additional comments. My rule for comments is to put them where the code is not clear by itself. What you have done is actually over-commenting and that makes the code harder to read and understand.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Quoted 6 lines · Marcel Holtmann
And for the core part, please be brief with any additional comments. My rule for comments is to put them where the code is not clear by itself. What you have done is actually over-commenting and that makes the code harder to read and understand.
I've read the code and for me the comments are very welcome. As well as your enthousiasm & code, Fabian.
Cheers, Pieter
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Pieter,
Quoted 7 lines
Quoted 5 lines
And for the core part, please be brief with any additional comments. My rule for comments is to put them where the code is not clear by itself. What you have done is actually over-commenting and that makes the code harder to read and understand.
I've read the code and for me the comments are very welcome. As well as your enthousiasm & code, Fabian.
the right amount of comments and the coding style has always been something people disagree on. However the kernel follows the policy to have clean and easy code. You should understand the code and there should be no need for comments. The comments are used to explain parts that are not easy to guess.
Example for these are the reasons why to put a memory barrier here or why not using some kind of locking or the reason why this isn't a race conditions even if it looks like one etc.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hello Marcel&All,
Quoted 3 lines
please split the changes to Bluetooth core and SCO module. Your changes to the core will conflict with the automatic sniff mode support and I need to sort that out.
Ok, i've just done that, i've split the patch into three smaller bits.
- Patch part 1 are changes to hci core required for SCO flow control
itself.
- Patch part 2 are the changes to the SCO socket layer.
- Patch part 3 are additionnal comments on hci core code.
Please note however that part 1 and part 2 can not be applied seperately.
Quoted 3 lines
And for the core part, please be brief with any additional comments. My rule for comments is to put them where the code is not clear by itself.
I would tend to agree with the general philosophy of the thing, however
for my point of view bluez kernel code lacks proper fields structure explanation. Comparing for skbuff.h & usb.h to hci_core.h for instance is just night and day... skbuff.h & usb.h are just well commented files, while hci.h and hci_core.h have hardly any comment. :-(
Quoted 2 lines
What you have done is actually over-commenting and that makes the code harder to read and understand.
Could you give me an example ? I don't see which part is not lined up with kernel coding style :-(
anyway,
All this is really *cosmetic* compared to the feature itself. :-)
On another level, does anyone here have anything to add on the feature itself, or the way i designed it ??
Cheers,
Fabien
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/hci_core.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/hci_core.h +30 -16
@@ -142,26 +148,39 @@atomic_t refcnt;spinlock_t lock;bdaddr_t dst;__u16 handle;__u16 state; +/* type : ACL or SCO */ __u8 type;__u8 out;__u8 dev_class[3 ];__u32 link_mode;unsigned long pend; +/* sent represents the number of packets this connections +has "on the wire" : .... oh f.... there are no wire +with bluetooth. By on the wire, i mean packets that have been sent +to the HCI device, and that are still in its buffers */ unsigned int sent; +/* Queued packets for this connection +Starting with bluez core version 2.9, SCO packets are not in this queue anymore, +but remain in the sockets send queue until they are forwared to the driver by +the hci scheduler */ struct sk_buff_head data_q; -struct timer_list timer; +/* Data timer : used only for SCO */ +struct timer_list data_timer; +/* Disconnect timer */ +struct timer_list disc_timer;struct hci_dev *hdev;void *l2cap_data; +/* private use for sco */ void *sco_data;void *priv;struct hci_conn *link; };
@@ -283,37 +302,37 @@struct hci_conn *hci_connect (struct hci_dev *hdev,int type,bdaddr_t *src);int hci_conn_auth (struct hci_conn *conn);int hci_conn_encrypt (struct hci_conn *conn);int hci_conn_change_link_key (struct hci_conn *conn);int hci_conn_switch_role (struct hci_conn *conn,uint8_t role); -static inline void hci_conn_set_timer (struct hci_conn *conn,unsigned long timeout) +static inline void hci_conn_set_disc_timer (struct hci_conn *conn,unsigned long timeout) { -mod_timer (&conn->timer, jiffies + timeout); +mod_timer (&conn->disc_timer, jiffies + timeout); } -static inline void hci_conn_del_timer (struct hci_conn *conn) +static inline void hci_conn_del_disc_timer (struct hci_conn *conn) { -del_timer (&conn->timer); +del_timer (&conn->disc_timer); }static inline void hci_conn_hold (struct hci_conn *conn) {atomic_inc (&conn->refcnt); -hci_conn_del_timer (conn); +hci_conn_del_disc_timer (conn); }static inline void hci_conn_put (struct hci_conn *conn) {if (atomic_dec_and_test (&conn->refcnt)) {if (conn->type ==ACL_LINK ) {unsigned long timeo = (conn->out) ?HCI_DISCONN_TIMEOUT :HCI_DISCONN_TIMEOUT *2 ; -hci_conn_set_timer (conn, timeo); +hci_conn_set_disc_timer (conn, timeo); }else -hci_conn_set_timer (conn,HZ /100 ); +hci_conn_set_disc_timer (conn,HZ /100 ); } }/* ----- HCI tasks ----- */ static inline void hci_sched_cmd (struct hci_dev *hdev) {
@@ -419,19 +438,20 @@char *name;unsigned int id;unsigned long flags;void *priv; -int (*connect_ind) (struct hci_dev *hdev,bdaddr_t *bdaddr,__u8 type); -int (*connect_cfm) (struct hci_conn *conn,__u8 status); -int (*disconn_ind) (struct hci_conn *conn,__u8 reason); -int (*recv_acldata) (struct hci_conn *conn,struct sk_buff *skb,__u16 flags); -int (*recv_scodata) (struct hci_conn *conn,struct sk_buff *skb); -int (*auth_cfm) (struct hci_conn *conn,__u8 status); -int (*encrypt_cfm) (struct hci_conn *conn,__u8 status); +int (*connect_ind) (struct hci_dev *hdev,bdaddr_t *bdaddr,__u8 type); +int (*connect_cfm) (struct hci_conn *conn,__u8 status); +int (*disconn_ind) (struct hci_conn *conn,__u8 reason); +int (*recv_acldata) (struct hci_conn *conn,struct sk_buff *skb,__u16 flags); +int (*recv_scodata) (struct hci_conn *conn,struct sk_buff *skb); +struct sk_buff * (*get_scodata) (struct hci_conn *conn); +int (*auth_cfm) (struct hci_conn *conn,__u8 status); +int (*encrypt_cfm) (struct hci_conn *conn,__u8 status); };static inline int hci_proto_connect_ind (struct hci_dev *hdev,bdaddr_t *bdaddr,__u8 type) { registerstruct hci_proto *hp;int mask =0 ;
@@ -575,13 +595,13 @@int hci_register_notifier (struct notifier_block *nb);int hci_unregister_notifier (struct notifier_block *nb);int hci_send_cmd (struct hci_dev *hdev,__u16 ogf,__u16 ocf,__u32 plen,void *param);int hci_send_acl (struct hci_conn *conn,struct sk_buff *skb,__u16 flags); -int hci_send_sco (struct hci_conn *conn,struct sk_buff *skb); +void hci_stream_sco (struct hci_conn *conn);void *hci_sent_cmd_data (struct hci_dev *hdev,__u16 ogf,__u16 ocf);void hci_si_event (struct hci_dev *hdev,int type,int dlen,void *data);/* ----- HCI Sockets ----- */
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/af_bluetooth.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/af_bluetooth.c +1 -1
@@ -46,13 +46,13 @@#ifndef CONFIG_BT_SOCK_DEBUG #undef BT_DBG #define BT_DBG(D...)#endif -#define VERSION "2.8" +#define VERSION "2.9"/* Bluetooth sockets */ #define BT_MAX_PROTO 8static struct net_proto_family *bt_proto[BT_MAX_PROTO ];int bt_sock_register (int proto,struct net_proto_family *ops)
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/hci_conn.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/hci_conn.c +8 -6
@@ -130,17 +130,17 @@else conn->state =BT_CLOSED ;hci_dev_unlock (hdev);return ; } -static void hci_conn_init_timer (struct hci_conn *conn) +static void hci_conn_init_disc_timer (struct hci_conn *conn) { -init_timer (&conn->timer); - conn->timer.function = hci_conn_timeout; - conn->timer.data = (unsigned long )conn; +init_timer (&conn->disc_timer); + conn->disc_timer.function = hci_conn_timeout; + conn->disc_timer.data = (unsigned long )conn; }struct hci_conn *hci_conn_add (struct hci_dev *hdev,int type,bdaddr_t *dst) {struct hci_conn *conn;
@@ -153,13 +153,14 @@bacpy (&conn->dst, dst); conn->type = type; conn->hdev = hdev; conn->state =BT_OPEN ;skb_queue_head_init (&conn->data_q); -hci_conn_init_timer (conn); +hci_conn_init_disc_timer (conn); +init_timer (&conn->data_timer);atomic_set (&conn->refcnt,0 );hci_dev_hold (hdev);tasklet_disable (&hdev->tx_task);
@@ -176,13 +177,14 @@int hci_conn_del (struct hci_conn *conn) {struct hci_dev *hdev = conn->hdev;BT_DBG ("%s conn %p handle %d" , hdev->name, conn, conn->handle); -hci_conn_del_timer (conn); +hci_conn_del_disc_timer (conn); +del_timer_sync (&conn->data_timer);if (conn->type ==SCO_LINK ) {struct hci_conn *acl = conn->link;if (acl) { acl->link =NULL ;hci_conn_put (acl);
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/hci_core.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/hci_core.c +75 -41
201 lines not on this page · raw message
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/sco.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/sco.h +3 -5
@@ -23,18 +23,13 @@*/ #ifndef __SCO_H#define __SCO_H/* SCO defaults */ -#define SCO_DEFAULT_MTU 500 -#define SCO_DEFAULT_FLUSH_TO 0xFFFF -#define SCO_CONN_TIMEOUT (HZ * 40) -#define SCO_DISCONN_TIMEOUT (HZ * 2) -#define SCO_CONN_IDLE_TIMEOUT (HZ * 60)/* SCO socket address */ struct sockaddr_sco {sa_family_t sco_family;bdaddr_t sco_bdaddr; };
@@ -48,12 +43,15 @@#define SCO_CONNINFO 0x02struct sco_conninfo {__u16 hci_handle;__u8 dev_class[3 ]; }; +#define SCO_TXBUFS 0x03 +#define SCO_RXBUFS 0x04 +/* ---- SCO connections ---- */ struct sco_conn {struct hci_conn *hcon;bdaddr_t *dst;bdaddr_t *src;
home/fchevalier/tmp/linux-2.6.16.16/net/bluetooth/sco.c renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/net/bluetooth/sco.c +308 -43
545 lines not on this page · raw message
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/hci.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/hci.h +7 -0
@@ -712,12 +712,15 @@__u32 sco_tx;__u32 sco_rx;__u32 byte_rx;__u32 byte_tx; }; +/* Fields down there are mostly the same as hci_dev, +as this structure is meant to communicate info +to userspace */ struct hci_dev_info {__u16 dev_id;char name[8 ];bdaddr_t bdaddr;
@@ -727,15 +730,19 @@__u8 features[8 ];__u32 pkt_type;__u32 link_policy;__u32 link_mode; +/* Maximum transmition unit for ACL packets */ __u16 acl_mtu; +/* Number of ACL packets the baseband is able to buffer */ __u16 acl_pkts; +/* Maximum transmition unit for SCO packets */ __u16 sco_mtu; +/* Number of SCO packets the baseband is able to buffer */ __u16 sco_pkts;struct hci_dev_stats stat; };struct hci_conn_info {
home/fchevalier/tmp/linux-2.6.16.16/include/net/bluetooth/hci_core.h renamed from home/fchevalier/tmp/linux-2.6.16.16-orig/include/net/bluetooth/hci_core.h +6 -0
@@ -81,18 +81,24 @@__u16 link_policy;__u16 link_mode;unsigned long quirks;atomic_t cmd_cnt; +/* Number of available controller buffers for ACL packets */ unsigned int acl_cnt; +/* Number of available controller buffers for SCO packets */ unsigned int sco_cnt; +/* Maximum transmition unit for ACL packets */ unsigned int acl_mtu; +/* Maximum transmition unit for SCO packets */ unsigned int sco_mtu; +/* Maximum number of ACL packets the controller is able to buffer */ unsigned int acl_pkts; +/* Maximum number of SCO packets the controller is able to buffer */ unsigned int sco_pkts;unsigned long cmd_last_tx;unsigned long acl_last_tx;unsigned long sco_last_tx;
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 11 lines
Quoted 3 lines
please split the changes to Bluetooth core and SCO module. Your changes to the core will conflict with the automatic sniff mode support and I need to sort that out.
Ok, i've just done that, i've split the patch into three smaller bits.
- Patch part 1 are changes to hci core required for SCO flow control
itself.
- Patch part 2 are the changes to the SCO socket layer.
- Patch part 3 are additionnal comments on hci core code.Please note however that part 1 and part 2 can not be applied seperately.
so far I only looked at the changes to the HCI core. Please don't rename the disconnect timer. This will collide with the changes for automatic sniff mode support.
Can we change the core and add a hci_stream_sco() without breaking the API. This would make it easier for me to apply the changes.
Another question is if we really should queue the data in sk_write_queue and not directly in the core. I would prefer to send the data to the core and then stream it from there.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
Sorry to be a little late on this answer, however a busy work schedule and a sunny Sunday prevented me to answer my e-mails for a few days :-)
Quoted 3 lines
so far I only looked at the changes to the HCI core. Please don't rename the disconnect timer. This will collide with the changes for automatic sniff mode support.
Ok, however as now sniff mode is out, i would suggest i build on top of it :-)
Quoted 3 lines
Can we change the core and add a
hci_stream_sco()without breaking the API. This would make it easier for me to apply the changes.
I see your point, however i don't see how to bring this feature in without breaking the hci core API for SCO sockets... :-(
Quoted 4 lines
Another question is if we really should queue the data in
sk_write_queueand not directly in the core. I would prefer to send the data to the core and then stream it from there.
That's an interesting question... :-)
I asked it myself when i started to design this feature.
In fact having outgoing packets stored in a socket queue has some advantages:
* it makes use of the BSD socket send queue, and thus can reuse some
generic socket queue management related functions, which rely on some well known sk_buff variable (sk_sndbuff, sk_wmem_alloc, sk_rcvbuff, sk_rmem_alloc ...)
* it makes the thing symetrical, both uplink and downlink path make
use socket queues to buffer data.
As such I thought there were not any valid reason to put in the core... however if you have suggestions i would be very happy to hear them. :-)
For now i would suggest we do the following:
- for you: have a look at the "whole picture", including the SCO part
patch. This way you would be able to better understand the interaction between SCO sockets & hci layer. This way you will be able to send in other comments too. :-)
- for me: * port the patch on top of the 2.6.16-mh1 :-)
* have it to work also when HZ % 333 != 0 ( i've just seen
it wouldn't work properly in this case... )
What do you think of the idea ?
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 2 lines
Sorry to be a little late on this answer, however a busy work schedule and a sunny Sunday prevented me to answer my e-mails for a few days :-)
no big deal. I should do this more often by myself ;)
Quoted 5 lines
Quoted 2 lines
Can we change the core and add a
hci_stream_sco()without breaking the API. This would make it easier for me to apply the changes.I see your point, however i don't see how to bring this feature in without breaking the hci core API for SCO sockets... :-(
No problem. We can break the API, but at least I need some good reason for it.
Quoted 15 lines
Quoted 3 lines
Another question is if we really should queue the data in
sk_write_queueand not directly in the core. I would prefer to send the data to the core and then stream it from there.That's an interesting question... :-)
I asked it myself when i started to design this feature.
In fact having outgoing packets stored in a socket queue has some advantages:
* it makes use of the BSD socket send queue, and thus can reuse some
generic socket queue management related functions, which rely on some well knownsk_buffvariable (sk_sndbuff,sk_wmem_alloc,sk_rcvbuff,sk_rmem_alloc...)
* it makes the thing symetrical, both uplink and downlink path make
use socket queues to buffer data.
Quoted 2 lines
As such I thought there were not any valid reason to put in the core... however if you have suggestions i would be very happy to hear them. :-)
Actually I never thought about SCO as socket is the best interface. So we might wanna move away from it sometime in the future. And in this case it would be great if it uses simple SKB queues or maybe kfifo for it. Even if this sounds like more work right now, I might be worth it.
Quoted 8 lines
For now i would suggest we do the following:
- for you: have a look at the "whole picture", including the SCO part
patch. This way you would be able to better understand the interaction between SCO sockets & hci layer. This way you will be able to send in other comments too. :-)
- for me: * port the patch on top of the 2.6.16-mh1 :-)
* have it to work also when HZ % 333 != 0 ( i've just seen
it wouldn't work properly in this case... )
Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna adapt scotest for testing it.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
Quoted 14 lines
Quoted 14 lines
Quoted 3 lines
Another question is if we really should queue the data in
sk_write_queueand not directly in the core. I would prefer to send the data to the core and then stream it from there.That's an interesting question... :-)
I asked it myself when i started to design this feature.
In fact having outgoing packets stored in a socket queue has some advantages:
* it makes use of the BSD socket send queue, and thus can reuse some
generic socket queue management related functions, which rely on some well knownsk_buffvariable (sk_sndbuff,sk_wmem_alloc,sk_rcvbuff,sk_rmem_alloc...)
* it makes the thing symetrical, both uplink and downlink path make
use socket queues to buffer data.
Quoted 7 lines
Quoted 2 lines
As such I thought there were not any valid reason to put in the core... however if you have suggestions i would be very happy to hear them. :-)
Actually I never thought about SCO as socket is the best interface. So we might wanna move away from it sometime in the future. And in this case it would be great if it uses simple SKB queues or maybe kfifo for it. Even if this sounds like more work right now, I might be worth it.
It might make sense: however we must take into account the following constraints:
- we must be aple to setup fifo lengths from userspace, so that to
simulate a variable size ring buffer as found on sound card hardware.
- we must be able to put the sending task to sleep, and wake it up
later, when the fifo gets 'full'.
I'm not against this, even if it means more work...
However i don't have any precise idea on which communication mechanism to use between userspace/kernelspace. Any ideas ??
Quoted 4 lines
Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna adapt scotest for testing it.
Ok, i'm gonna work on that then...
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 22 lines
Quoted 14 lines
Quoted 14 lines
Quoted 3 lines
Another question is if we really should queue the data in
sk_write_queueand not directly in the core. I would prefer to send the data to the core and then stream it from there.That's an interesting question... :-)
I asked it myself when i started to design this feature.
In fact having outgoing packets stored in a socket queue has some advantages:
* it makes use of the BSD socket send queue, and thus can reuse some
generic socket queue management related functions, which rely on some well knownsk_buffvariable (sk_sndbuff,sk_wmem_alloc,sk_rcvbuff,sk_rmem_alloc...)
* it makes the thing symetrical, both uplink and downlink path make
use socket queues to buffer data.Quoted 7 lines
Quoted 2 lines
As such I thought there were not any valid reason to put in the core... however if you have suggestions i would be very happy to hear them. :-)
Actually I never thought about SCO as socket is the best interface. So we might wanna move away from it sometime in the future. And in this case it would be great if it uses simple SKB queues or maybe kfifo for it. Even if this sounds like more work right now, I might be worth it.
Quoted 6 lines
It might make sense: however we must take into account the following constraints:
- we must be aple to setup fifo lengths from userspace, so that to
simulate a variable size ring buffer as found on sound card hardware.
- we must be able to put the sending task to sleep, and wake it up
later, when the fifo gets 'full'.
I don't see any problems with both. We can use workqueues or tasklets for the second one.
Quoted 1 line
I'm not against this, even if it means more work...
I think it will be worth it.
Quoted 2 lines
However i don't have any precise idea on which communication mechanism to use between userspace/kernelspace. Any ideas ??
We might leave the socket for now. The alternative is a character device or directly an ALSA sound card. Haven't fully thought about it either.
Quoted 4 lines
Quoted 3 lines
Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna adapt scotest for testing it.
Ok, i'm gonna work on that then...
The 2.6.16-mh2 is missing the automatic sniff mode patch, but only for testing. It is coming back for sure.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
Quoted 9 lines
Quoted 1 line
I'm not against this, even if it means more work...
I think it will be worth it.
Quoted 2 lines
However i don't have any precise idea on which communication mechanism to use between userspace/kernelspace. Any ideas ??
We might leave the socket for now. The alternative is a character device or directly an ALSA sound card. Haven't fully thought about it either.
For the ALSA sound card, this is basically what snd-bt-sco does. The issue with this alternative, is that the headset&handsfree profile mandates the use of an RFCOMM channel before to use SCO. Doing this at kernel level will mean that SCO layer will be dependant on RFCOMM sockets, or that user space will have to handle RFCOMM channel before to open ALSA device. This is roughly the way btsco works today. The result is not very pretty :-(
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna adapt scotest for testing it.
Ok, i'm gonna work on that then...
The 2.6.16-mh2 is missing the automatic sniff mode patch, but only for testing. It is coming back for sure.
Ok, i will stick with mh1 then....
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 10 lines
Quoted 2 lines
We might leave the socket for now. The alternative is a character device or directly an ALSA sound card. Haven't fully thought about it either.
For the ALSA sound card, this is basically what snd-bt-sco does. The issue with this alternative, is that the headset&handsfree profile mandates the use of an RFCOMM channel before to use SCO. Doing this at kernel level will mean that SCO layer will be dependant on RFCOMM sockets, or that user space will have to handle RFCOMM channel before to open ALSA device. This is roughly the way btsco works today. The result is not very pretty :-(
except that we don't would use the SCO socket interface in the kernel like the current snd-bt-sco is doing. And yes, we would need an extra configuration channel, because using AT commands the kernel is not an option at all.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
A just thought to a few more things related to a sco socket. I just though to a real advantage of using socket for SCO (vs ALSA device or character device).
Using a character or block device usually means that the device is connected. You can plug your USB mass storage device, and the /dev/sda will be usefull.
Sockets are usually used to connect to devices that the OS does not know if they exist or not (think to a distant IP host: the kernel has no way of knowing if it is valid are not. The connect call will just ask the kernel to 'give a try', and bail out if the device does not exist). Having said that, this is really quite similar to the bluetooth technology, where before you have tryed to connect, you don't know if the device is on or off...)
And for kfifo use: it make sense, however the bad point is that all bluez stack is coded with sk_buffs all over the place. Using kfifo today will mean having to do the following convertion:
(hci core) kfifo --> (hci device) sk_buff
I think for now i'm gonna stick with sk_buffs :-) However i will have a look to see how to queue sk_buffs at hci core layer instead of socket layer :-)
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann
maintainerHi Fabien,
Quoted 5 lines
A just thought to a few more things related to a sco socket. I just though to a real advantage of using socket for SCO (vs ALSA device or character device).
Using a character or block device usually means that the device is connected. You can plug your USB mass storage device, and the/dev/sda
Quoted 8 lines
will be usefull.
Sockets are usually used to connect to devices that the OS does not know if they exist or not (think to a distant IP host: the kernel has no way of knowing if it is valid are not. The connect call will just ask the kernel to 'give a try', and bail out if the device does not exist). Having said that, this is really quite similar to the bluetooth technology, where before you have tryed to connect, you don't know if the device is on or off...)
lets stay with the sockets for now.
Quoted 9 lines
And for kfifo use: it make sense, however the bad point is that all bluez stack is coded with
sk_buffsall over the place. Using kfifo today will mean having to do the following convertion:(hci core) kfifo --> (hci device)
sk_buffI think for now i'm gonna stick with
sk_buffs:-) However i will have a look to see how to queuesk_buffsat hci core layer instead of socket layer :-)
Sounds like a good plan.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel