On Wed, Feb 17, 2010 at 9:31 AM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Nick,
>
>> >> As a first step, can we get consensus on the userspace API:
>> >>
>> >> --- a/include/net/bluetooth/hci.h
>> >> +++ b/include/net/bluetooth/hci.h
>> >> @@ -139,8 +139,11 @@ enum {
>> >> +#define ALL_SCO_PKTS (SCO_ESCO_MASK | EDR_ESCO_MASK)
>> >> +#define ALL_ESCO_PKTS (SCO_ESCO_MASK | ESCO_EV3 | ESCO_EV4 | ESCO_EV5)
>> >>
>> >> --- a/include/net/bluetooth/sco.h
>> >> +++ b/include/net/bluetooth/sco.h
>> >> @@ -37,6 +37,7 @@
>> >> struct sockaddr_sco {
>> >> sa_family_t sco_family;
>> >> bdaddr_t sco_bdaddr;
>> >> + __u16 sco_pkt_type;
>> >> };
>> >>
>> >> This will at least unblock my work.
>> >
>> > Would it be better to add new sockopt for sco socket?
>>
>> What advantage does that have?
>>
>> Putting it in struct sockaddr_sco seems to make more sense since
>> packet types can only be selected during SCO connection establishment.
>> They can't be changed once the socket is connected.
>
> in theory you can change the allowed packet types for ACL, SCO and eSCO
> after the connection is active. However the usefulness here is fairly
> limited. In case of ACL it is purely academical and most link manager
> will just ignore you. Mainly because the host stack can't really make a
> good decision here anyway.
>
> Personally I think it is a total brain dead concept to give this into
> the control of the host stack anyway. For eSCO packet types this make a
> bit more sense since you might wanna control the bandwidth. However
> changing them later is just pointless. And I don't recall of any profile
> actually mentioning about it. I think they had a great idea behind eSCO
> support, but since it is impossible to get the negotiation parts right,
> everybody sticks with simple eSCO channels and doesn't bother to change
> them.
>
> And even if we would be going for a setsockopt(), that would be blocking
> and then again pretty much pointless API. The sockaddr is most logical
> thing that fits into what we wanna achieve. Disallow/allow certain
> packet types and essentially force SCO over eSCO.
Ok seems like there is agreement on using struct sockaddr_sco for sco_pkt_type.
A more controversial question is whether to follow the SIG convention
of reversing the logic on the EDR bits in the packet type mask (1
means do *not* use this packet for the EDR bits), or to use consistent
logic for each bit (1 always means *allow* this packet type) for
packet selection in scokaddr_sco.sco_pkt_type.
The original patch I posted followed the SIG convention. However after
more thinking I am leaning towards using consistent logic for each
bit. 1 will always mean 'allow this packet'. My rationale is that the
most common use for sco_pkt_type will be to request only SCO packet
types allowed. If however the SIG adds more reverse logic packet
types, and we follow the SIG convention, then old userspace code that
just requested the SCO packet types will now end up with the new
packet types as well. So I think it is best to avoid this situation
and for 1 to always mean 'allow this packet' in
sockaddr_sco.sco_pkt_type.
Attached is a new patch with the consistent bit logic.
Comments?
Cheers,
Nick
Attachment:
0001-Bluetooth-Allow-SCO-eSCO-packet-type-selection-for-o.patch
Description: Binary data
