Re: RFC: Allow Bluez to select flushable or non-flushable ACL packets with L2CAP_LM_RELIABLE

[Nick Pelly <npelly@google.com>]

On Fri, Dec 18, 2009 at 3:05 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>
> Hi Nick,
>
> > >> >> >> Right now Bluez always requests flushable ACL packets (but doe=
s not
> > >> >> >> set a flush timeout, so effectively they are non-flushable):
> > >> >> >>
> > >> >> >> However it is desirable to use an ACL flush timeout on A2DP pa=
ckets so
> > >> >> >> that if the ACL packets block for some reason then the LM can =
flush
> > >> >> >> them to make room for newer packets.
> > >> >> >>
> > >> >> >> Is it reasonable for Bluez to use the 0x00 ACL packet boundary=
 flag by
> > >> >> >> default (non-flushable packet), and let userspace request flus=
hable
> > >> >> >> packets on A2DP L2CAP sockets with the socket option
> > >> >> >> L2CAP_LM_RELIABLE.
> > >> >> >
> > >> >> > the reliable option has a different meaning. It comes back from=
 the old
> > >> >> > Bluetooth 1.1 qualification days where we had to tests on L2CAP=
 that had
> > >> >> > to confirm that we can detect malformed packets and report them=
. These
> > >> >> > days it is just fine to drop them.
> > >> >>
> > >> >> Got it, how about introducing
> > >> >>
> > >> >> #define L2CAP_LM_FLUSHABLE 0x0040
> > >> >
> > >> > that l2cap_sock_setsockopt_old() sets this didn't give you a hint =
that
> > >> > we might wanna deprecate this socket options ;)
> > >> >
> > >> > I need to read up on the flushable stuff, but in the end it deserv=
es its
> > >> > own socket option. Also an ioctl() to actually trigger Enhanced fl=
ush
> > >> > might be needed.
> > >> >
> > >> >> struct l2cap_pinfo {
> > >> >> =A0 =A0...
> > >> >> =A0 =A0__u8 flushable;
> > >> >> }
> > >> >
> > >> > Sure. In the long run we need to turn this into a bitmask. We are =
just
> > >> > wasting memory here.
> > >>
> > >> Attached is an updated patch, that checks the LMP features bitmask
> > >> before using the new non-flushable packet type.
> > >>
> > >> I am still using L2CAP_LM_FLUSHABLE socket option in
> > >> l2cap_sock_setsockopt_old(), which I don't think you are happy with.
> > >> So how about a new option:
> > >>
> > >> SOL_L2CAP, L2CAP_ACL_FLUSH
> > >> which has a default value of 0, and can be set to 1 to make the ACL
> > >> data sent by this L2CAP socket flushable.
> >
> > Was this proposal ok?
>
> Even SOL_L2CAP goes away. Use SOL_BLUETOOTH for this.
>
> > >> In a later commit we would then add
> > >> SOL_ACL, ACL_FLUSH_TIMEOUT
> > >> That is used to set an automatic flush timeout for the ACL link on a
> > >> L2CAP socket. Note that SOL_ACL is new.
> > >
> > > can I stop you right here (without even looking at the patch). We do
> > > have the generic SOL_BLUETOOTH that you should be using. So adding
> > > SOL_ACL is not a viable option at all.
> >
> > This would be in a later patch, and SOL_BLUETOOTH, ACL_FLUSH_TIMEOUT
> > is fine too, or whatever you prefer.
>
> Why not just use BT_FLUSHABLE and have it always take a timeout option
> and then 0 means not flushable. And advantage of having it separated?

I think keeping them=A0separate=A0makes it clear that the flush timeout is
global for a given ACL link, whereas the flushable/non-flushable
boolean is specific to a L2CAP channel. (Which is why I suggested
introducing a new level SOL_ACL for the ACL_FLUSH_TIMEOUT option -
since this option applies at the ACL level in the stack).

A specific advantage of this is that flushable packets can be enabled
without over-writing a previous flush timeout that was set on a
different L2CAP socket on the same ACL link. I guess this can also be
achieved with getsockopt() but that is racy.

> Regards
>
> Marcel
>
>