Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Hemant Gupta]

Hi Marcel,

On Thu, Apr 5, 2012 at 11:39 PM, Hemant Gupta
<hemant.gupta@stericsson.com> wrote:
> This patch uses the corect packet type for ESCO Link.
> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
> resulting in setting bits that are not supported by controller
> to 0 which means that corresponding EDR ESCO packet type is
> supported(EDR Packet types are inverted as per BT Spec) which might
> not be the case.
>
> For eg:
> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
> packet types and does not support 2-EV3 packet type. This would mean
> that while creating the esco_type in function
> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
> all other EDR eSCO bits would be set resulting in
> hdev->esco_type =3D 0x0380
>
> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
> Link, wrong calculation would take place as below:
>
> conn->pkt_type =3D hdev->esco_type & ~EDR_ESCO_MASK;
> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 & ~0x03C0 =3D 0x0380 & 0xFC3F
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0000
> Since the EDR eSCO bits are inverted, this would indicate that all
> EDR eSCO packet types are supported, which is not correct as local
> controller is not supporting the 2-EV3 packet type.
>
> As per calculations of the patch
> conn->pkt_type =3D hdev->esco_type ^ EDR_ESCO_MASK;
> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 ^ 0x03C0
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0040
> which correctly indicates that packet type used excludes the 2-EV3
> packet type not supported by local controller.
>
Any comments on the updated patch with commit message.

> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
> ---
> =A0net/bluetooth/hci_conn.c | =A0 =A02 +-
> =A01 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 947172b..1060fb6 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, i=
nt type, bdaddr_t *dst)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conn->pkt_type =3D hdev->p=
kt_type & SCO_PTYPE_MASK;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0case ESCO_LINK:
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type & ~EDR_E=
SCO_MASK;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type ^ EDR_ES=
CO_MASK;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0}
>
> --
> 1.7.0.4
>

--=20
Best Regards
Hemant Gupta
ST-Ericsson India

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Marcel Holtmann]

Hi Hemant,

> > This patch uses the corect packet type for ESCO Link.
> > Without this patch esco packet types were anded with ~EDR_ESCO_MASK
> > resulting in setting bits that are not supported by controller
> > to 0 which means that corresponding EDR ESCO packet type is
> > supported(EDR Packet types are inverted as per BT Spec) which might
> > not be the case.
> >
> > For eg:
> > Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
> > packet types and does not support 2-EV3 packet type. This would mean
> > that while creating the esco_type in function
> > hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
> > all other EDR eSCO bits would be set resulting in
> > hdev->esco_type = 0x0380
> >
> > Now in hci_conn_add() when the pkt_type is being calculated for eSCO
> > Link, wrong calculation would take place as below:
> >
> > conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
> >                = 0x0380 & ~0x03C0 = 0x0380 & 0xFC3F
> >               = 0x0000
> > Since the EDR eSCO bits are inverted, this would indicate that all
> > EDR eSCO packet types are supported, which is not correct as local
> > controller is not supporting the 2-EV3 packet type.
> >
> > As per calculations of the patch
> > conn->pkt_type = hdev->esco_type ^ EDR_ESCO_MASK;
> >                = 0x0380 ^ 0x03C0
> >               = 0x0040
> > which correctly indicates that packet type used excludes the 2-EV3
> > packet type not supported by local controller.
> >
> Any comments on the updated patch with commit message.

for some strange reason, I have not seen v4 so far. However this message
made it through.

> > Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
> > ---
> >  net/bluetooth/hci_conn.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

You might need to resend it in case it never reached Johan and Gustavo
either.

Regards

Marcel

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Hemant Gupta]

Hi Marcel,

On Wed, Apr 11, 2012 at 2:49 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Hemant,
>
>> > This patch uses the corect packet type for ESCO Link.
>> > Without this patch esco packet types were anded with ~EDR_ESCO_MASK
>> > resulting in setting bits that are not supported by controller
>> > to 0 which means that corresponding EDR ESCO packet type is
>> > supported(EDR Packet types are inverted as per BT Spec) which might
>> > not be the case.
>> >
>> > For eg:
>> > Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
>> > packet types and does not support 2-EV3 packet type. This would mean
>> > that while creating the esco_type in function
>> > hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
>> > all other EDR eSCO bits would be set resulting in
>> > hdev->esco_type =3D 0x0380
>> >
>> > Now in hci_conn_add() when the pkt_type is being calculated for eSCO
>> > Link, wrong calculation would take place as below:
>> >
>> > conn->pkt_type =3D hdev->esco_type & ~EDR_ESCO_MASK;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D 0x0380 & ~0x03C0 =3D 0x0380 & 0xFC3=
F
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0000
>> > Since the EDR eSCO bits are inverted, this would indicate that all
>> > EDR eSCO packet types are supported, which is not correct as local
>> > controller is not supporting the 2-EV3 packet type.
>> >
>> > As per calculations of the patch
>> > conn->pkt_type =3D hdev->esco_type ^ EDR_ESCO_MASK;
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D 0x0380 ^ 0x03C0
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0040
>> > which correctly indicates that packet type used excludes the 2-EV3
>> > packet type not supported by local controller.
>> >
>> Any comments on the updated patch with commit message.
>
> for some strange reason, I have not seen v4 so far. However this message
> made it through.
>
>> > Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
>> > ---
>> > =A0net/bluetooth/hci_conn.c | =A0 =A02 +-
>> > =A01 files changed, 1 insertions(+), 1 deletions(-)
>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>
> You might need to resend it in case it never reached Johan and Gustavo
> either.
>
Thanks for letting me know that somehow v4 of patch never made to
mailing list :(
I have now sent the v4 patch keeping your ACK.
> Regards
>
> Marcel
>
>



--=20
Best Regards
Hemant Gupta
ST-Ericsson India

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Hemant Gupta]

Hi Johan,

On Mon, Apr 16, 2012 at 3:59 PM, Johan Hedberg <johan.hedberg@gmail.com> wr=
ote:
> Hi,
>
> On Thu, Apr 12, 2012, Mike wrote:
>> > On Tue, Apr 10, 2012 at 11:13 PM, Hemant Gupta
>> > <hemantgupta.ste@gmail.com> wrote:
>> >> Hi Marcel, Johan, Gustavo,
>> >>
>> >> On Wed, Apr 11, 2012 at 9:32 AM, Hemant Gupta
>> >> <hemant.gupta@stericsson.com> wrote:
>> >>> This patch uses the corect packet type for ESCO Link.
>> >>> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
>> >>> resulting in setting bits that are not supported by controller
>> >>> to 0 which means that corresponding EDR ESCO packet type is
>> >>> supported(EDR Packet types are inverted as per BT Spec) which might
>> >>> not be the case.
>> >>>
>> >>> For eg:
>> >>> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSC=
O
>> >>> packet types and does not support 2-EV3 packet type. This would mean
>> >>> that while creating the esco_type in function
>> >>> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
>> >>> all other EDR eSCO bits would be set resulting in
>> >>> hdev->esco_type =3D 0x0380
>> >>>
>> >>> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
>> >>> Link, wrong calculation would take place as below:
>> >>>
>> >>> conn->pkt_type =3D hdev->esco_type & ~EDR_ESCO_MASK;
>> >>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 & ~0x03C0 =3D 0x0380 & 0xF=
C3F
>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0000
>> >>> Since the EDR eSCO bits are inverted, this would indicate that all
>> >>> EDR eSCO packet types are supported, which is not correct as local
>> >>> controller is not supporting the 2-EV3 packet type.
>> >>>
>> >>> As per calculations of the patch
>> >>> conn->pkt_type =3D hdev->esco_type ^ EDR_ESCO_MASK;
>> >>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 ^ 0x03C0
>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0040
>> >>> which correctly indicates that packet type used excludes the 2-EV3
>> >>> packet type not supported by local controller.
>> >>>
>> >>> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
>> >>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>> >>> ---
>> >>> =A0net/bluetooth/hci_conn.c | =A0 =A02 +-
>> >>> =A01 files changed, 1 insertions(+), 1 deletions(-)
>> >>>
>> >>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> >>> index 947172b..1060fb6 100644
>> >>> --- a/net/bluetooth/hci_conn.c
>> >>> +++ b/net/bluetooth/hci_conn.c
>> >>> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hd=
ev, int type, bdaddr_t *dst)
>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conn->pkt_type =3D hd=
ev->pkt_type & SCO_PTYPE_MASK;
>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>> >>> =A0 =A0 =A0 =A0case ESCO_LINK:
>> >>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type & ~=
EDR_ESCO_MASK;
>> >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type ^ E=
DR_ESCO_MASK;
>> >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>> >>> =A0 =A0 =A0 =A0}
>> >>>
>> >>> --
>> >>> 1.7.0.4
>> >>>
>> >>
>> >> Resending the patch with Marcel's ACK, because I am not sure if it
>> >> made to the mailing list again or not.
>> >
>> > While this is better, it does not appear complete based on my
>> > analysis. =A0The spec reads as such:
>> >
>> > The Packet_Type parameter is a bit mask specifying the syn-
>> > chronous packet types that are allowed on the link and shall be met. T=
he
>> > reserved bits in the Packet_Type field shall be set to one. If all
>> > bits are set in
>> > the packet type field then all packets types shall be allowed.
>> >
>> > So, in addition to this problem, the code does not set the reserved
>> > bits to 1. =A0I think that packet_type should be set to 0xFFC0 by
>> > default, and then explicitly enable data rates as appropriate. =A0In
>> > this case, the code would then be:
>> >
>> > conn->pkt_type =3D 0xFFC0; // before the switch statement
>> > conn->pkt_type ^=3D hdev->esco_type & EDR_ESCO_MASK; // in place of
>> > proposed change above
>>
>> Ah, this would actually be bad because EV3/4/5 are not part of any
>> mask and would then get lost. =A0Perhaps it would be better to invert
>> the defines so that they are named like ESCO_NO_2EV3 and then we
>> wouldn't need such funny math? =A0Then EV3/4/5 could be added to the
>> mask.
>>
>> > This would also need to be corrected for the other case statements.
>>
>> The SCO_LINK case should not include the (hdev->esco_type &
>> EDR_ESCO_MASK) because EDR rates are not supported for SCO. =A0I'm not
>> sure what the intended math was there (just happens in the case that
>> all are supported that they are listed as not supported if link is
>> SCO_LINK). =A0Similarly for SCO_LINK in hci_event.c
>> hci_sync_conn_complete_evt where the pkt_type is redefined. =A0But it
>> still also does not follow spec on reserved bits.
>
> So what's the status with the evolution of this patch. Should the v4 be
> applied and improvements be done on top of that, or is there a v5 coming
> up soon, or what?
>
As per my understanding, v4 does not add any problems compared to
existing kernel behavior, infact fixes issue with respect to current
kernel, that has problem if controller does not support some eSCO
packet types.

Problem now is with RFU bits, which needs to be corrected, and a new
patch is required for that.
> Johan



--=20
Best Regards
Hemant Gupta
ST-Ericsson India

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Johan Hedberg]

Hi,

On Thu, Apr 12, 2012, Mike wrote:
> > On Tue, Apr 10, 2012 at 11:13 PM, Hemant Gupta
> > <hemantgupta.ste@gmail.com> wrote:
> >> Hi Marcel, Johan, Gustavo,
> >>
> >> On Wed, Apr 11, 2012 at 9:32 AM, Hemant Gupta
> >> <hemant.gupta@stericsson.com> wrote:
> >>> This patch uses the corect packet type for ESCO Link.
> >>> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
> >>> resulting in setting bits that are not supported by controller
> >>> to 0 which means that corresponding EDR ESCO packet type is
> >>> supported(EDR Packet types are inverted as per BT Spec) which might
> >>> not be the case.
> >>>
> >>> For eg:
> >>> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
> >>> packet types and does not support 2-EV3 packet type. This would mean
> >>> that while creating the esco_type in function
> >>> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
> >>> all other EDR eSCO bits would be set resulting in
> >>> hdev->esco_type = 0x0380
> >>>
> >>> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
> >>> Link, wrong calculation would take place as below:
> >>>
> >>> conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
> >>>                = 0x0380 & ~0x03C0 = 0x0380 & 0xFC3F
> >>>               = 0x0000
> >>> Since the EDR eSCO bits are inverted, this would indicate that all
> >>> EDR eSCO packet types are supported, which is not correct as local
> >>> controller is not supporting the 2-EV3 packet type.
> >>>
> >>> As per calculations of the patch
> >>> conn->pkt_type = hdev->esco_type ^ EDR_ESCO_MASK;
> >>>                = 0x0380 ^ 0x03C0
> >>>               = 0x0040
> >>> which correctly indicates that packet type used excludes the 2-EV3
> >>> packet type not supported by local controller.
> >>>
> >>> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
> >>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> >>> ---
> >>>  net/bluetooth/hci_conn.c |    2 +-
> >>>  1 files changed, 1 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >>> index 947172b..1060fb6 100644
> >>> --- a/net/bluetooth/hci_conn.c
> >>> +++ b/net/bluetooth/hci_conn.c
> >>> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
> >>>                        conn->pkt_type = hdev->pkt_type & SCO_PTYPE_MASK;
> >>>                break;
> >>>        case ESCO_LINK:
> >>> -               conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
> >>> +               conn->pkt_type = hdev->esco_type ^ EDR_ESCO_MASK;
> >>>                break;
> >>>        }
> >>>
> >>> --
> >>> 1.7.0.4
> >>>
> >>
> >> Resending the patch with Marcel's ACK, because I am not sure if it
> >> made to the mailing list again or not.
> >
> > While this is better, it does not appear complete based on my
> > analysis.  The spec reads as such:
> >
> > The Packet_Type parameter is a bit mask specifying the syn-
> > chronous packet types that are allowed on the link and shall be met. The
> > reserved bits in the Packet_Type field shall be set to one. If all
> > bits are set in
> > the packet type field then all packets types shall be allowed.
> >
> > So, in addition to this problem, the code does not set the reserved
> > bits to 1.  I think that packet_type should be set to 0xFFC0 by
> > default, and then explicitly enable data rates as appropriate.  In
> > this case, the code would then be:
> >
> > conn->pkt_type = 0xFFC0; // before the switch statement
> > conn->pkt_type ^= hdev->esco_type & EDR_ESCO_MASK; // in place of
> > proposed change above
> 
> Ah, this would actually be bad because EV3/4/5 are not part of any
> mask and would then get lost.  Perhaps it would be better to invert
> the defines so that they are named like ESCO_NO_2EV3 and then we
> wouldn't need such funny math?  Then EV3/4/5 could be added to the
> mask.
> 
> > This would also need to be corrected for the other case statements.
> 
> The SCO_LINK case should not include the (hdev->esco_type &
> EDR_ESCO_MASK) because EDR rates are not supported for SCO.  I'm not
> sure what the intended math was there (just happens in the case that
> all are supported that they are listed as not supported if link is
> SCO_LINK).  Similarly for SCO_LINK in hci_event.c
> hci_sync_conn_complete_evt where the pkt_type is redefined.  But it
> still also does not follow spec on reserved bits.

So what's the status with the evolution of this patch. Should the v4 be
applied and improvements be done on top of that, or is there a v5 coming
up soon, or what?

Johan

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Mike]

On Wed, Apr 11, 2012 at 6:58 PM, Mike <puffy.taco@gmail.com> wrote:
> Hi,
>
> On Tue, Apr 10, 2012 at 11:13 PM, Hemant Gupta
> <hemantgupta.ste@gmail.com> wrote:
>> Hi Marcel, Johan, Gustavo,
>>
>> On Wed, Apr 11, 2012 at 9:32 AM, Hemant Gupta
>> <hemant.gupta@stericsson.com> wrote:
>>> This patch uses the corect packet type for ESCO Link.
>>> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
>>> resulting in setting bits that are not supported by controller
>>> to 0 which means that corresponding EDR ESCO packet type is
>>> supported(EDR Packet types are inverted as per BT Spec) which might
>>> not be the case.
>>>
>>> For eg:
>>> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
>>> packet types and does not support 2-EV3 packet type. This would mean
>>> that while creating the esco_type in function
>>> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
>>> all other EDR eSCO bits would be set resulting in
>>> hdev->esco_type =3D 0x0380
>>>
>>> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
>>> Link, wrong calculation would take place as below:
>>>
>>> conn->pkt_type =3D hdev->esco_type & ~EDR_ESCO_MASK;
>>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 & ~0x03C0 =3D 0x0380 & 0xFC3F
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0000
>>> Since the EDR eSCO bits are inverted, this would indicate that all
>>> EDR eSCO packet types are supported, which is not correct as local
>>> controller is not supporting the 2-EV3 packet type.
>>>
>>> As per calculations of the patch
>>> conn->pkt_type =3D hdev->esco_type ^ EDR_ESCO_MASK;
>>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 ^ 0x03C0
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0040
>>> which correctly indicates that packet type used excludes the 2-EV3
>>> packet type not supported by local controller.
>>>
>>> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
>>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>>> ---
>>> =A0net/bluetooth/hci_conn.c | =A0 =A02 +-
>>> =A01 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>>> index 947172b..1060fb6 100644
>>> --- a/net/bluetooth/hci_conn.c
>>> +++ b/net/bluetooth/hci_conn.c
>>> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev,=
 int type, bdaddr_t *dst)
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conn->pkt_type =3D hdev-=
>pkt_type & SCO_PTYPE_MASK;
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>> =A0 =A0 =A0 =A0case ESCO_LINK:
>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type & ~EDR=
_ESCO_MASK;
>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type ^ EDR_=
ESCO_MASK;
>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>>> =A0 =A0 =A0 =A0}
>>>
>>> --
>>> 1.7.0.4
>>>
>>
>> Resending the patch with Marcel's ACK, because I am not sure if it
>> made to the mailing list again or not.
>
> While this is better, it does not appear complete based on my
> analysis. =A0The spec reads as such:
>
> The Packet_Type parameter is a bit mask specifying the syn-
> chronous packet types that are allowed on the link and shall be met. The
> reserved bits in the Packet_Type field shall be set to one. If all
> bits are set in
> the packet type field then all packets types shall be allowed.
>
> So, in addition to this problem, the code does not set the reserved
> bits to 1. =A0I think that packet_type should be set to 0xFFC0 by
> default, and then explicitly enable data rates as appropriate. =A0In
> this case, the code would then be:
>
> conn->pkt_type =3D 0xFFC0; // before the switch statement
> conn->pkt_type ^=3D hdev->esco_type & EDR_ESCO_MASK; // in place of
> proposed change above

Ah, this would actually be bad because EV3/4/5 are not part of any
mask and would then get lost.  Perhaps it would be better to invert
the defines so that they are named like ESCO_NO_2EV3 and then we
wouldn't need such funny math?  Then EV3/4/5 could be added to the
mask.

> This would also need to be corrected for the other case statements.

The SCO_LINK case should not include the (hdev->esco_type &
EDR_ESCO_MASK) because EDR rates are not supported for SCO.  I'm not
sure what the intended math was there (just happens in the case that
all are supported that they are listed as not supported if link is
SCO_LINK).  Similarly for SCO_LINK in hci_event.c
hci_sync_conn_complete_evt where the pkt_type is redefined.  But it
still also does not follow spec on reserved bits.

Mike

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Mike]

Hi,

On Tue, Apr 10, 2012 at 11:13 PM, Hemant Gupta
<hemantgupta.ste@gmail.com> wrote:
> Hi Marcel, Johan, Gustavo,
>
> On Wed, Apr 11, 2012 at 9:32 AM, Hemant Gupta
> <hemant.gupta@stericsson.com> wrote:
>> This patch uses the corect packet type for ESCO Link.
>> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
>> resulting in setting bits that are not supported by controller
>> to 0 which means that corresponding EDR ESCO packet type is
>> supported(EDR Packet types are inverted as per BT Spec) which might
>> not be the case.
>>
>> For eg:
>> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
>> packet types and does not support 2-EV3 packet type. This would mean
>> that while creating the esco_type in function
>> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
>> all other EDR eSCO bits would be set resulting in
>> hdev->esco_type =3D 0x0380
>>
>> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
>> Link, wrong calculation would take place as below:
>>
>> conn->pkt_type =3D hdev->esco_type & ~EDR_ESCO_MASK;
>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 & ~0x03C0 =3D 0x0380 & 0xFC3F
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0000
>> Since the EDR eSCO bits are inverted, this would indicate that all
>> EDR eSCO packet types are supported, which is not correct as local
>> controller is not supporting the 2-EV3 packet type.
>>
>> As per calculations of the patch
>> conn->pkt_type =3D hdev->esco_type ^ EDR_ESCO_MASK;
>> =A0=A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0380 ^ 0x03C0
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D 0x0040
>> which correctly indicates that packet type used excludes the 2-EV3
>> packet type not supported by local controller.
>>
>> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
>> Acked-by: Marcel Holtmann <marcel@holtmann.org>
>> ---
>> =A0net/bluetooth/hci_conn.c | =A0 =A02 +-
>> =A01 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 947172b..1060fb6 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, =
int type, bdaddr_t *dst)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0conn->pkt_type =3D hdev->=
pkt_type & SCO_PTYPE_MASK;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>> =A0 =A0 =A0 =A0case ESCO_LINK:
>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type & ~EDR_=
ESCO_MASK;
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 conn->pkt_type =3D hdev->esco_type ^ EDR_E=
SCO_MASK;
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
>> =A0 =A0 =A0 =A0}
>>
>> --
>> 1.7.0.4
>>
>
> Resending the patch with Marcel's ACK, because I am not sure if it
> made to the mailing list again or not.

While this is better, it does not appear complete based on my
analysis.  The spec reads as such:

The Packet_Type parameter is a bit mask specifying the syn-
chronous packet types that are allowed on the link and shall be met. The
reserved bits in the Packet_Type field shall be set to one. If all
bits are set in
the packet type field then all packets types shall be allowed.

So, in addition to this problem, the code does not set the reserved
bits to 1.  I think that packet_type should be set to 0xFFC0 by
default, and then explicitly enable data rates as appropriate.  In
this case, the code would then be:

conn->pkt_type =3D 0xFFC0; // before the switch statement
conn->pkt_type ^=3D hdev->esco_type & EDR_ESCO_MASK; // in place of
proposed change above

This would also need to be corrected for the other case statements.

Does this make sense?

Mike

Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Hemant Gupta]

Hi Marcel, Johan, Gustavo,

On Wed, Apr 11, 2012 at 9:32 AM, Hemant Gupta
<hemant.gupta@stericsson.com> wrote:
> This patch uses the corect packet type for ESCO Link.
> Without this patch esco packet types were anded with ~EDR_ESCO_MASK
> resulting in setting bits that are not supported by controller
> to 0 which means that corresponding EDR ESCO packet type is
> supported(EDR Packet types are inverted as per BT Spec) which might
> not be the case.
>
> For eg:
> Local Controller supports only 3-EV5, 2-EV5 and 3-EV3 of the EDR eSCO
> packet types and does not support 2-EV3 packet type. This would mean
> that while creating the esco_type in function
> hci_cc_read_local_features() the ESCO_2EV3 bit would not be set and
> all other EDR eSCO bits would be set resulting in
> hdev->esco_type = 0x0380
>
> Now in hci_conn_add() when the pkt_type is being calculated for eSCO
> Link, wrong calculation would take place as below:
>
> conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
>                = 0x0380 & ~0x03C0 = 0x0380 & 0xFC3F
>               = 0x0000
> Since the EDR eSCO bits are inverted, this would indicate that all
> EDR eSCO packet types are supported, which is not correct as local
> controller is not supporting the 2-EV3 packet type.
>
> As per calculations of the patch
> conn->pkt_type = hdev->esco_type ^ EDR_ESCO_MASK;
>                = 0x0380 ^ 0x03C0
>               = 0x0040
> which correctly indicates that packet type used excludes the 2-EV3
> packet type not supported by local controller.
>
> Signed-off-by: Hemant Gupta <hemant.gupta@stericsson.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  net/bluetooth/hci_conn.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 947172b..1060fb6 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -396,7 +396,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>                        conn->pkt_type = hdev->pkt_type & SCO_PTYPE_MASK;
>                break;
>        case ESCO_LINK:
> -               conn->pkt_type = hdev->esco_type & ~EDR_ESCO_MASK;
> +               conn->pkt_type = hdev->esco_type ^ EDR_ESCO_MASK;
>                break;
>        }
>
> --
> 1.7.0.4
>

Resending the patch with Marcel's ACK, because I am not sure if it
made to the mailing list again or not.

-- 
Best Regards
Hemant Gupta
ST-Ericsson India

[PATCH v4] Bluetooth: Fix packet type for ESCO Link

[Hemant GUPTA]

VGhpcyBwYXRjaCB1c2VzIHRoZSBjb3JlY3QgcGFja2V0IHR5cGUgZm9yIEVTQ08gTGluay4NCldp
dGhvdXQgdGhpcyBwYXRjaCBlc2NvIHBhY2tldCB0eXBlcyB3ZXJlIGFuZGVkIHdpdGggfkVEUl9F
U0NPX01BU0sgcmVzdWx0aW5nIGluIHNldHRpbmcgYml0cyB0aGF0IGFyZSBub3Qgc3VwcG9ydGVk
IGJ5IGNvbnRyb2xsZXIgdG8gMCB3aGljaCBtZWFucyB0aGF0IGNvcnJlc3BvbmRpbmcgRURSIEVT
Q08gcGFja2V0IHR5cGUgaXMgc3VwcG9ydGVkKEVEUiBQYWNrZXQgdHlwZXMgYXJlIGludmVydGVk
IGFzIHBlciBCVCBTcGVjKSB3aGljaCBtaWdodCBub3QgYmUgdGhlIGNhc2UuDQoNCkZvciBlZzoN
CkxvY2FsIENvbnRyb2xsZXIgc3VwcG9ydHMgb25seSAzLUVWNSwgMi1FVjUgYW5kIDMtRVYzIG9m
IHRoZSBFRFIgZVNDTyBwYWNrZXQgdHlwZXMgYW5kIGRvZXMgbm90IHN1cHBvcnQgMi1FVjMgcGFj
a2V0IHR5cGUuIFRoaXMgd291bGQgbWVhbiB0aGF0IHdoaWxlIGNyZWF0aW5nIHRoZSBlc2NvX3R5
cGUgaW4gZnVuY3Rpb24NCmhjaV9jY19yZWFkX2xvY2FsX2ZlYXR1cmVzKCkgdGhlIEVTQ09fMkVW
MyBiaXQgd291bGQgbm90IGJlIHNldCBhbmQgYWxsIG90aGVyIEVEUiBlU0NPIGJpdHMgd291bGQg
YmUgc2V0IHJlc3VsdGluZyBpbg0KaGRldi0+ZXNjb190eXBlID0gMHgwMzgwDQoNCk5vdyBpbiBo
Y2lfY29ubl9hZGQoKSB3aGVuIHRoZSBwa3RfdHlwZSBpcyBiZWluZyBjYWxjdWxhdGVkIGZvciBl
U0NPIExpbmssIHdyb25nIGNhbGN1bGF0aW9uIHdvdWxkIHRha2UgcGxhY2UgYXMgYmVsb3c6DQoN
CmNvbm4tPnBrdF90eXBlID0gaGRldi0+ZXNjb190eXBlICYgfkVEUl9FU0NPX01BU0s7DQogwqAg
wqAgwqAgwqAgwqAgwqAgwqAgPSAweDAzODAgJiB+MHgwM0MwID0gMHgwMzgwICYgMHhGQzNGDQog
ICAgICAgICAgICAgICA9IDB4MDAwMA0KU2luY2UgdGhlIEVEUiBlU0NPIGJpdHMgYXJlIGludmVy
dGVkLCB0aGlzIHdvdWxkIGluZGljYXRlIHRoYXQgYWxsIEVEUiBlU0NPIHBhY2tldCB0eXBlcyBh
cmUgc3VwcG9ydGVkLCB3aGljaCBpcyBub3QgY29ycmVjdCBhcyBsb2NhbCBjb250cm9sbGVyIGlz
IG5vdCBzdXBwb3J0aW5nIHRoZSAyLUVWMyBwYWNrZXQgdHlwZS4NCg0KQXMgcGVyIGNhbGN1bGF0
aW9ucyBvZiB0aGUgcGF0Y2gNCmNvbm4tPnBrdF90eXBlID0gaGRldi0+ZXNjb190eXBlIF4gRURS
X0VTQ09fTUFTSzsNCiDCoCDCoCDCoCDCoCDCoCDCoCDCoCA9IDB4MDM4MCBeIDB4MDNDMA0KICAg
ICAgICAgICAgICAgPSAweDAwNDANCndoaWNoIGNvcnJlY3RseSBpbmRpY2F0ZXMgdGhhdCBwYWNr
ZXQgdHlwZSB1c2VkIGV4Y2x1ZGVzIHRoZSAyLUVWMyBwYWNrZXQgdHlwZSBub3Qgc3VwcG9ydGVk
IGJ5IGxvY2FsIGNvbnRyb2xsZXIuDQoNClNpZ25lZC1vZmYtYnk6IEhlbWFudCBHdXB0YSA8aGVt
YW50Lmd1cHRhQHN0ZXJpY3Nzb24uY29tPg0KQWNrZWQtYnk6IE1hcmNlbCBIb2x0bWFubiA8bWFy
Y2VsQGhvbHRtYW5uLm9yZz4NCi0tLQ0KIG5ldC9ibHVldG9vdGgvaGNpX2Nvbm4uYyB8ICAgIDIg
Ky0NCiAxIGZpbGVzIGNoYW5nZWQsIDEgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbnMoLSkNCg0K
ZGlmZiAtLWdpdCBhL25ldC9ibHVldG9vdGgvaGNpX2Nvbm4uYyBiL25ldC9ibHVldG9vdGgvaGNp
X2Nvbm4uYyBpbmRleCA5NDcxNzJiLi4xMDYwZmI2IDEwMDY0NA0KLS0tIGEvbmV0L2JsdWV0b290
aC9oY2lfY29ubi5jDQorKysgYi9uZXQvYmx1ZXRvb3RoL2hjaV9jb25uLmMNCkBAIC0zOTYsNyAr
Mzk2LDcgQEAgc3RydWN0IGhjaV9jb25uICpoY2lfY29ubl9hZGQoc3RydWN0IGhjaV9kZXYgKmhk
ZXYsIGludCB0eXBlLCBiZGFkZHJfdCAqZHN0KQ0KIAkJCWNvbm4tPnBrdF90eXBlID0gaGRldi0+
cGt0X3R5cGUgJiBTQ09fUFRZUEVfTUFTSzsNCiAJCWJyZWFrOw0KIAljYXNlIEVTQ09fTElOSzoN
Ci0JCWNvbm4tPnBrdF90eXBlID0gaGRldi0+ZXNjb190eXBlICYgfkVEUl9FU0NPX01BU0s7DQor
CQljb25uLT5wa3RfdHlwZSA9IGhkZXYtPmVzY29fdHlwZSBeIEVEUl9FU0NPX01BU0s7DQogCQli
cmVhazsNCiAJfQ0KIA0KLS0NCjEuNy4wLjQNCg0K