* [PATCH v4] Bluetooth: Fix packet type for ESCO Link [not found] <1334116936-16171-1-git-send-email-hemant.gupta@stericsson.com> @ 2012-04-11 4:11 ` Hemant GUPTA 2012-04-11 4:13 ` Hemant Gupta 1 sibling, 0 replies; 9+ messages in thread From: Hemant GUPTA @ 2012-04-11 4:11 UTC (permalink / raw) To: linux-bluetooth; +Cc: Marcel Holtmann, Johan Hedberg, gustavo 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link [not found] <1334116936-16171-1-git-send-email-hemant.gupta@stericsson.com> 2012-04-11 4:11 ` [PATCH v4] Bluetooth: Fix packet type for ESCO Link Hemant GUPTA @ 2012-04-11 4:13 ` Hemant Gupta 2012-04-11 23:58 ` Mike 1 sibling, 1 reply; 9+ messages in thread From: Hemant Gupta @ 2012-04-11 4:13 UTC (permalink / raw) To: Marcel Holtmann, Johan Hedberg, gustavo; +Cc: linux-bluetooth, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-11 4:13 ` Hemant Gupta @ 2012-04-11 23:58 ` Mike 2012-04-12 17:10 ` Mike 0 siblings, 1 reply; 9+ messages in thread From: Mike @ 2012-04-11 23:58 UTC (permalink / raw) To: Hemant Gupta Cc: Marcel Holtmann, Johan Hedberg, gustavo, linux-bluetooth, Hemant Gupta 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-11 23:58 ` Mike @ 2012-04-12 17:10 ` Mike 2012-04-16 10:29 ` Johan Hedberg 0 siblings, 1 reply; 9+ messages in thread From: Mike @ 2012-04-12 17:10 UTC (permalink / raw) To: Hemant Gupta Cc: Marcel Holtmann, Johan Hedberg, gustavo, linux-bluetooth, Hemant Gupta 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-12 17:10 ` Mike @ 2012-04-16 10:29 ` Johan Hedberg 2012-04-16 10:33 ` Hemant Gupta 0 siblings, 1 reply; 9+ messages in thread From: Johan Hedberg @ 2012-04-16 10:29 UTC (permalink / raw) To: Mike Cc: Hemant Gupta, Marcel Holtmann, gustavo, linux-bluetooth, Hemant Gupta 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-16 10:29 ` Johan Hedberg @ 2012-04-16 10:33 ` Hemant Gupta 0 siblings, 0 replies; 9+ messages in thread From: Hemant Gupta @ 2012-04-16 10:33 UTC (permalink / raw) To: Mike, Hemant Gupta, Marcel Holtmann, gustavo, linux-bluetooth, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <1333649382-16220-1-git-send-email-hemant.gupta@stericsson.com>]
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link [not found] <1333649382-16220-1-git-send-email-hemant.gupta@stericsson.com> @ 2012-04-10 15:27 ` Hemant Gupta 2012-04-10 21:19 ` Marcel Holtmann 0 siblings, 1 reply; 9+ messages in thread From: Hemant Gupta @ 2012-04-10 15:27 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-10 15:27 ` Hemant Gupta @ 2012-04-10 21:19 ` Marcel Holtmann 2012-04-11 4:04 ` Hemant Gupta 0 siblings, 1 reply; 9+ messages in thread From: Marcel Holtmann @ 2012-04-10 21:19 UTC (permalink / raw) To: Hemant Gupta; +Cc: linux-bluetooth, Hemant Gupta 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] Bluetooth: Fix packet type for ESCO Link 2012-04-10 21:19 ` Marcel Holtmann @ 2012-04-11 4:04 ` Hemant Gupta 0 siblings, 0 replies; 9+ messages in thread From: Hemant Gupta @ 2012-04-11 4:04 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth, 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-04-16 10:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1334116936-16171-1-git-send-email-hemant.gupta@stericsson.com>
2012-04-11 4:11 ` [PATCH v4] Bluetooth: Fix packet type for ESCO Link Hemant GUPTA
2012-04-11 4:13 ` Hemant Gupta
2012-04-11 23:58 ` Mike
2012-04-12 17:10 ` Mike
2012-04-16 10:29 ` Johan Hedberg
2012-04-16 10:33 ` Hemant Gupta
[not found] <1333649382-16220-1-git-send-email-hemant.gupta@stericsson.com>
2012-04-10 15:27 ` Hemant Gupta
2012-04-10 21:19 ` Marcel Holtmann
2012-04-11 4:04 ` Hemant Gupta
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.