Hi Luiz,
> >> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> >> > ---
> >> > net/bluetooth/rfcomm/core.c | 51 +++++++++++++++++++++++++++++-------------
> >> > net/bluetooth/rfcomm/sock.c | 2 +
> >> > 2 files changed, 37 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> >> > index 85580f2..bfc6bce 100644
> >> > --- a/net/bluetooth/rfcomm/core.c
> >> > +++ b/net/bluetooth/rfcomm/core.c
> >> > @@ -65,7 +65,8 @@ static DEFINE_MUTEX(rfcomm_mutex);
> >> >
> >> > static LIST_HEAD(session_list);
> >> >
> >> > -static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len);
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > + u32 priority);
> >> > 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);
> >> > @@ -747,19 +748,34 @@ 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)
> >> > +static int rfcomm_send_frame(struct rfcomm_session *s, u8 *data, int len,
> >> > + u32 priority)
> >> > {
> >> > struct socket *sock = s->sock;
> >> > + struct sock *sk = sock->sk;
> >> > struct kvec iv = { data, len };
> >> > struct msghdr msg;
> >> >
> >> > - BT_DBG("session %p len %d", s, len);
> >> > + 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);
> >> > + }
> >> >
> >> > memset(&msg, 0, sizeof(msg));
> >> >
> >> > return kernel_sendmsg(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);
> >>
> >>
> >> What's the idea here? Prioritize commands over data? But does this really
> >> happen? Because we have only one queue to receive the data in L2CAP. There
> >> is no separation between data and cmd there.
> >>
> >> Also, did you check if we can send RFCOMM commands and data out of order?
> >>
> >> I really would like to rewrite l2cap-rfcomm iteraction before adding new
> >> features here.
> >
> > lets just forget RFCOMM for now and make SO_PRIORITY setsockopt calls
> > return an error code. Priority on RFCOMM links is also rather pointless
> > since they all go via the same L2CAP PSM anyway. You would end up
> > prioritizing all RFCOMM connections over any other L2CAP connection.
>
> Currently the priority is set per skb not per channel, so it is not as
> broken as you may think it is. Other than that you can't really ignore
> the priority for RFCOMM because as the priority will be not set in its
> skb it will be left 0 so it is low priority which for RFCOMM commands
> may cause problems due latency being increased (remember it not a
> simple fair scheduler anymore), also iirc SO_PRIORITY is not RFCOMM
> specific and currently no error is return.
and we have to super careful with any sort of potential re-ordering
here. I rather not touch SKBs coming in from RFCOMM. They should stay as
they are.
> > So if you try to prioritize HFP then you also prioritize PBAP in the end
> > and we are back where we started. We could implement the 27.007 priority
> > support inside RFCOMM, but that seems even more pointless endeavor.
>
> That is not the way it work, take a look at rfcomm_send_frame:
>
> + if (sk->sk_priority != priority) {
> + lock_sock(sk);
> + sk->sk_priority = priority;
> + release_sock(sk);
> + }
>
> This is suppose to dynamically updates the priority if it changes.
As I said above, I rather not mess with this. Keep the RFCOMM stream as
it is. L2CAP priority is essentially something different than RFCOMM
priority. And I do not wanna go there right now.
> > What we really want is prioritized L2CAP links and hopefully in the
> > future everything moves over to use L2CAP directly anyway and RFCOMM
> > will be slowly dying out.
>
> The problem here is not really RFCOMM as for L2CAP we also give
> maximum priority to commands to avoid latency problems, this is
> specially important when connecting because it will page but if either
> L2CAP or RFCOMM commands are not properly prioritized they may timeout
> so all the paging is wasted, btw this was very easy to emulate with
> hid+a2dp then try to connect anything else.
As pointed our earlier, we have to be really careful to not reorder
command wrongly. If that happens we have a serious problem with the
overall system.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
