* [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context @ 2011-12-20 19:15 Gustavo F. Padovan 2011-12-20 19:15 ` [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM Gustavo F. Padovan 2011-12-20 20:58 ` [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Marcel Holtmann 0 siblings, 2 replies; 8+ messages in thread From: Gustavo F. Padovan @ 2011-12-20 19:15 UTC (permalink / raw) To: linux-bluetooth; +Cc: Gustavo F. Padovan From: "Gustavo F. Padovan" <padovan@profusion.mobi> No local_bh_disable is needed there once we run everything in process context. The same goes for the replacement of bh_lock_sock() by lock_sock(). Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi> --- net/bluetooth/af_bluetooth.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-) diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c index 062124c..cdcfcab 100644 --- a/net/bluetooth/af_bluetooth.c +++ b/net/bluetooth/af_bluetooth.c @@ -199,15 +199,14 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock) BT_DBG("parent %p", parent); - local_bh_disable(); list_for_each_safe(p, n, &bt_sk(parent)->accept_q) { sk = (struct sock *) list_entry(p, struct bt_sock, accept_q); - bh_lock_sock(sk); + lock_sock(sk); /* FIXME: Is this check still needed */ if (sk->sk_state == BT_CLOSED) { - bh_unlock_sock(sk); + release_sock(sk); bt_accept_unlink(sk); continue; } @@ -218,14 +217,12 @@ struct sock *bt_accept_dequeue(struct sock *parent, struct socket *newsock) if (newsock) sock_graft(sk, newsock); - bh_unlock_sock(sk); - local_bh_enable(); + release_sock(sk); return sk; } - bh_unlock_sock(sk); + release_sock(sk); } - local_bh_enable(); return NULL; } -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2011-12-20 19:15 [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Gustavo F. Padovan @ 2011-12-20 19:15 ` Gustavo F. Padovan 2011-12-20 21:01 ` Marcel Holtmann 2011-12-20 20:58 ` [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Marcel Holtmann 1 sibling, 1 reply; 8+ messages in thread From: Gustavo F. Padovan @ 2011-12-20 19:15 UTC (permalink / raw) To: linux-bluetooth; +Cc: Gustavo F. Padovan From: "Gustavo F. Padovan" <padovan@profusion.mobi> RFCOMM needs a proper priority mechanism inside itself and not try to use l2cap priority to fix its own problem. Acked-by: Marcel Holtmann <marcel@holtmann.org> Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi> --- net/bluetooth/rfcomm/core.c | 47 ++++++++++++++++++++---------------------- 1 files changed, 22 insertions(+), 25 deletions(-) diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c index be6288c..a795d96 100644 --- a/net/bluetooth/rfcomm/core.c +++ b/net/bluetooth/rfcomm/core.c @@ -65,8 +65,7 @@ static DEFINE_MUTEX(rfcomm_mutex); static LIST_HEAD(session_list); -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len, - u32 priority); +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len); static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci); static int rfcomm_send_disc(struct rfcomm_session *s, u8 dlci); static int rfcomm_queue_disc(struct rfcomm_dlc *d); @@ -748,32 +747,23 @@ void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src, bdaddr_t *d } /* ---- RFCOMM frame sending ---- */ -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len, - u32 priority) +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len) { - struct socket *sock = s->sock; - struct sock *sk = sock->sk; struct kvec iv = { data, len }; struct msghdr msg; - BT_DBG("session %p len %d priority %u", s, len, priority); - - if (sk->sk_priority != priority) { - lock_sock(sk); - sk->sk_priority = priority; - release_sock(sk); - } + BT_DBG("session %p len %d", s, len); memset(&msg, 0, sizeof(msg)); - return kernel_sendmsg(sock, &msg, &iv, 1, len); + return kernel_sendmsg(s->sock, &msg, &iv, 1, len); } static int rfcomm_send_cmd(struct rfcomm_session *s, struct rfcomm_cmd *cmd) { BT_DBG("%p cmd %u", s, cmd->ctrl); - return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd), HCI_PRIO_MAX); + return rfcomm_send_frame(s, (void *) cmd, sizeof(*cmd)); } static int rfcomm_send_sabm(struct rfcomm_session *s, u8 dlci) @@ -878,7 +868,7 @@ static int rfcomm_send_nsc(struct rfcomm_session *s, int cr, u8 type) *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d) @@ -920,7 +910,7 @@ static int rfcomm_send_pn(struct rfcomm_session *s, int cr, struct rfcomm_dlc *d *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci, @@ -958,7 +948,7 @@ int rfcomm_send_rpn(struct rfcomm_session *s, int cr, u8 dlci, *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status) @@ -985,7 +975,7 @@ static int rfcomm_send_rls(struct rfcomm_session *s, int cr, u8 dlci, u8 status) *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig) @@ -1012,7 +1002,7 @@ static int rfcomm_send_msc(struct rfcomm_session *s, int cr, u8 dlci, u8 v24_sig *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr) @@ -1034,7 +1024,7 @@ static int rfcomm_send_fcoff(struct rfcomm_session *s, int cr) *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_fcon(struct rfcomm_session *s, int cr) @@ -1056,7 +1046,7 @@ static int rfcomm_send_fcon(struct rfcomm_session *s, int cr) *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static int rfcomm_send_test(struct rfcomm_session *s, int cr, u8 *pattern, int len) @@ -1107,7 +1097,7 @@ static int rfcomm_send_credits(struct rfcomm_session *s, u8 addr, u8 credits) *ptr = __fcs(buf); ptr++; - return rfcomm_send_frame(s, buf, ptr - buf, HCI_PRIO_MAX); + return rfcomm_send_frame(s, buf, ptr - buf); } static void rfcomm_make_uih(struct sk_buff *skb, u8 addr) @@ -1786,8 +1776,15 @@ static inline int rfcomm_process_tx(struct rfcomm_dlc *d) return skb_queue_len(&d->tx_queue); while (d->tx_credits && (skb = skb_dequeue(&d->tx_queue))) { - err = rfcomm_send_frame(d->session, skb->data, skb->len, - skb->priority); + struct socket *sock = d->session->sock; + struct sock *sk = sock->sk; + + if (sk->sk_priority != skb->priority) { + lock_sock(sk); + sk->sk_priority = skb->priority; + release_sock(sk); + } + err = rfcomm_send_frame(d->session, skb->data, skb->len); if (err < 0) { skb_queue_head(&d->tx_queue, skb); break; -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2011-12-20 19:15 ` [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM Gustavo F. Padovan @ 2011-12-20 21:01 ` Marcel Holtmann 2011-12-29 15:16 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2011-12-20 21:01 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth Hi Gustavo, > RFCOMM needs a proper priority mechanism inside itself and not try to use > l2cap priority to fix its own problem. <snip> > static void rfcomm_make_uih(struct sk_buff *skb, u8 addr) > @@ -1786,8 +1776,15 @@ static inline int rfcomm_process_tx(struct rfcomm_dlc *d) > return skb_queue_len(&d->tx_queue); > > while (d->tx_credits && (skb = skb_dequeue(&d->tx_queue))) { > - err = rfcomm_send_frame(d->session, skb->data, skb->len, > - skb->priority); > + struct socket *sock = d->session->sock; > + struct sock *sk = sock->sk; > + > + if (sk->sk_priority != skb->priority) { > + lock_sock(sk); > + sk->sk_priority = skb->priority; > + release_sock(sk); > + } > + err = rfcomm_send_frame(d->session, skb->data, skb->len); > if (err < 0) { > skb_queue_head(&d->tx_queue, skb); > break; coming to think about this, we might not even bother setting the sk->sk_priority at all here. Lets just forget about RFCOMM. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2011-12-20 21:01 ` Marcel Holtmann @ 2011-12-29 15:16 ` Luiz Augusto von Dentz 2011-12-29 17:45 ` Marcel Holtmann 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2011-12-29 15:16 UTC (permalink / raw) To: Marcel Holtmann; +Cc: Gustavo F. Padovan, linux-bluetooth Hi Gustavo, Marcel, On Tue, Dec 20, 2011 at 11:01 PM, Marcel Holtmann <marcel@holtmann.org> wro= te: > Hi Gustavo, > >> RFCOMM needs a proper priority mechanism inside itself and not try to us= e >> l2cap priority to fix its own problem. > > <snip> > >> =A0static void rfcomm_make_uih(struct sk_buff *skb, u8 addr) >> @@ -1786,8 +1776,15 @@ static inline int rfcomm_process_tx(struct rfcomm= _dlc *d) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 return skb_queue_len(&d->tx_queue); >> >> =A0 =A0 =A0 while (d->tx_credits && (skb =3D skb_dequeue(&d->tx_queue)))= { >> - =A0 =A0 =A0 =A0 =A0 =A0 err =3D rfcomm_send_frame(d->session, skb->dat= a, skb->len, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb->priority); >> + =A0 =A0 =A0 =A0 =A0 =A0 struct socket *sock =3D d->session->sock; >> + =A0 =A0 =A0 =A0 =A0 =A0 struct sock *sk =3D sock->sk; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 if (sk->sk_priority !=3D skb->priority) { >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 lock_sock(sk); >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sk->sk_priority =3D skb->prior= ity; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_sock(sk); >> + =A0 =A0 =A0 =A0 =A0 =A0 } >> + =A0 =A0 =A0 =A0 =A0 =A0 err =3D rfcomm_send_frame(d->session, skb->dat= a, skb->len); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (err < 0) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb_queue_head(&d->tx_queue,= skb); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break; > > coming to think about this, we might not even bother setting the > sk->sk_priority at all here. Lets just forget about RFCOMM. I guess you guys don't remember, but I tried to explain why the priority has to be set even for RFCOMM, any frame sent by the kernel with a timeout needs to be scheduled as soon as possible otherwise it may timeout due to quote being consumed by other channels with higher priority, so in this case if an A2DP stream in ongoing you will never be able to complete a RFCOMM connection because there wont be enough quote to schedule them before they timeout (usually 10 out 10 attempt end up like that in my setup). Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that should get rid of this resetting sk_priority on every frame, but that means not depending on socket API at all to pass frames from RFCOMM to L2CAP, so we need an API to create L2CAP frames directly in RFCOMM modules and queue them in its hci_chan. --=20 Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2011-12-29 15:16 ` Luiz Augusto von Dentz @ 2011-12-29 17:45 ` Marcel Holtmann 2012-01-02 13:16 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 8+ messages in thread From: Marcel Holtmann @ 2011-12-29 17:45 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Gustavo F. Padovan, linux-bluetooth Hi Luiz, > >> RFCOMM needs a proper priority mechanism inside itself and not try to use > >> l2cap priority to fix its own problem. > > > > <snip> > > > >> static void rfcomm_make_uih(struct sk_buff *skb, u8 addr) > >> @@ -1786,8 +1776,15 @@ static inline int rfcomm_process_tx(struct rfcomm_dlc *d) > >> return skb_queue_len(&d->tx_queue); > >> > >> while (d->tx_credits && (skb = skb_dequeue(&d->tx_queue))) { > >> - err = rfcomm_send_frame(d->session, skb->data, skb->len, > >> - skb->priority); > >> + struct socket *sock = d->session->sock; > >> + struct sock *sk = sock->sk; > >> + > >> + if (sk->sk_priority != skb->priority) { > >> + lock_sock(sk); > >> + sk->sk_priority = skb->priority; > >> + release_sock(sk); > >> + } > >> + err = rfcomm_send_frame(d->session, skb->data, skb->len); > >> if (err < 0) { > >> skb_queue_head(&d->tx_queue, skb); > >> break; > > > > coming to think about this, we might not even bother setting the > > sk->sk_priority at all here. Lets just forget about RFCOMM. > > I guess you guys don't remember, but I tried to explain why the > priority has to be set even for RFCOMM, any frame sent by the kernel > with a timeout needs to be scheduled as soon as possible otherwise it > may timeout due to quote being consumed by other channels with higher > priority, so in this case if an A2DP stream in ongoing you will never > be able to complete a RFCOMM connection because there wont be enough > quote to schedule them before they timeout (usually 10 out 10 attempt > end up like that in my setup). > > Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that > should get rid of this resetting sk_priority on every frame, but that > means not depending on socket API at all to pass frames from RFCOMM to > L2CAP, so we need an API to create L2CAP frames directly in RFCOMM > modules and queue them in its hci_chan. yes, we need exactly this. Not using the L2CAP socket and interfacing with L2CAP directly is a good idea. Same came up with A2MP as well. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2011-12-29 17:45 ` Marcel Holtmann @ 2012-01-02 13:16 ` Luiz Augusto von Dentz 2012-01-02 18:43 ` Marcel Holtmann 0 siblings, 1 reply; 8+ messages in thread From: Luiz Augusto von Dentz @ 2012-01-02 13:16 UTC (permalink / raw) To: Marcel Holtmann; +Cc: Gustavo F. Padovan, linux-bluetooth Hi Marcel, On Thu, Dec 29, 2011 at 7:45 PM, Marcel Holtmann <marcel@holtmann.org> wrote: >> >> I guess you guys don't remember, but I tried to explain why the >> priority has to be set even for RFCOMM, any frame sent by the kernel >> with a timeout needs to be scheduled as soon as possible otherwise it >> may timeout due to quote being consumed by other channels with higher >> priority, so in this case if an A2DP stream in ongoing you will never >> be able to complete a RFCOMM connection because there wont be enough >> quote to schedule them before they timeout (usually 10 out 10 attempt >> end up like that in my setup). >> >> Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that >> should get rid of this resetting sk_priority on every frame, but that >> means not depending on socket API at all to pass frames from RFCOMM to >> L2CAP, so we need an API to create L2CAP frames directly in RFCOMM >> modules and queue them in its hci_chan. > > yes, we need exactly this. Not using the L2CAP socket and interfacing > with L2CAP directly is a good idea. Same came up with A2MP as well. Right, so I will work on this next, but in the meantime I think we should keep the current logic (it doesn't seems to cause any problem although the locking is pretty bad) until we got a proper solution and at least it is able to complete connections. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM 2012-01-02 13:16 ` Luiz Augusto von Dentz @ 2012-01-02 18:43 ` Marcel Holtmann 0 siblings, 0 replies; 8+ messages in thread From: Marcel Holtmann @ 2012-01-02 18:43 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: Gustavo F. Padovan, linux-bluetooth Hi Luiz, > >> I guess you guys don't remember, but I tried to explain why the > >> priority has to be set even for RFCOMM, any frame sent by the kernel > >> with a timeout needs to be scheduled as soon as possible otherwise it > >> may timeout due to quote being consumed by other channels with higher > >> priority, so in this case if an A2DP stream in ongoing you will never > >> be able to complete a RFCOMM connection because there wont be enough > >> quote to schedule them before they timeout (usually 10 out 10 attempt > >> end up like that in my setup). > >> > >> Perhaps if we assign a dedicated queue/hci_chan per RFCOMM socket that > >> should get rid of this resetting sk_priority on every frame, but that > >> means not depending on socket API at all to pass frames from RFCOMM to > >> L2CAP, so we need an API to create L2CAP frames directly in RFCOMM > >> modules and queue them in its hci_chan. > > > > yes, we need exactly this. Not using the L2CAP socket and interfacing > > with L2CAP directly is a good idea. Same came up with A2MP as well. > > Right, so I will work on this next, but in the meantime I think we > should keep the current logic (it doesn't seems to cause any problem > although the locking is pretty bad) until we got a proper solution and > at least it is able to complete connections. the locking is broken with the move to workqueues. So feel free to figure out a locking that works, but right now it is screwed up. Be my guest to set a MAX_PRIO - 1 by default for RFCOMM and see how that works out. Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context 2011-12-20 19:15 [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Gustavo F. Padovan 2011-12-20 19:15 ` [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM Gustavo F. Padovan @ 2011-12-20 20:58 ` Marcel Holtmann 1 sibling, 0 replies; 8+ messages in thread From: Marcel Holtmann @ 2011-12-20 20:58 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: linux-bluetooth Hi Gustavo, > No local_bh_disable is needed there once we run everything in process > context. The same goes for the replacement of bh_lock_sock() by > lock_sock(). > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi> > --- > net/bluetooth/af_bluetooth.c | 11 ++++------- > 1 files changed, 4 insertions(+), 7 deletions(-) Acked-by: Marcel Holtmann <marcel@holtmann.org> Regards Marcel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-01-02 18:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-12-20 19:15 [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Gustavo F. Padovan 2011-12-20 19:15 ` [PATCH 2/2] Bluetooth: Remove HCI_PRIO_MAX from ctrl cdm in RFCOMM Gustavo F. Padovan 2011-12-20 21:01 ` Marcel Holtmann 2011-12-29 15:16 ` Luiz Augusto von Dentz 2011-12-29 17:45 ` Marcel Holtmann 2012-01-02 13:16 ` Luiz Augusto von Dentz 2012-01-02 18:43 ` Marcel Holtmann 2011-12-20 20:58 ` [PATCH 1/2] Bluetooth: fix bt_accept_dequeue() to work in process context Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).