* [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling @ 2011-07-02 17:44 Peter Hurley 2011-07-02 22:11 ` Marcel Holtmann 0 siblings, 1 reply; 6+ messages in thread From: Peter Hurley @ 2011-07-02 17:44 UTC (permalink / raw) To: linux-bluetooth QSAwLWxlbmd0aCBBQ0wgY29udGludWF0aW9uLWZyYWdtZW50IGlzIGEgdmFsaWQgTlVMTCBwYWNr ZXQuIFJlbW90ZQ0KZGV2aWNlcyBjYW4gdXNlIHRoZSBGTE9XIGluZGljYXRvciBpbiB0aGUgQUNM IHBhY2tldCBoZWFkZXIgdG8NCmZsb3ctY29udHJvbCBBQ0wgcGFja2V0cyB3aXRob3V0IHNlbmRp bmcgYSBwYXlsb2FkLg0KDQpUcmFjayBhcyBhIGRldmljZSBzdGF0IGluc3RlYWQgb2YgbG9nZ2lu Zy4NCg0KU2lnbmVkLW9mZi1ieTogUGV0ZXIgSHVybGV5IDxwZXRlckBodXJsZXlzb2Z0d2FyZS5j b20+DQotLS0NCiBpbmNsdWRlL25ldC9ibHVldG9vdGgvaGNpLmggfCAgICAxICsNCiBuZXQvYmx1 ZXRvb3RoL2wyY2FwX2NvcmUuYyAgfCAgICA4ICsrKysrKysrDQogMiBmaWxlcyBjaGFuZ2VkLCA5 IGluc2VydGlvbnMoKyksIDAgZGVsZXRpb25zKC0pDQoNCmRpZmYgLS1naXQgYS9pbmNsdWRlL25l dC9ibHVldG9vdGgvaGNpLmggYi9pbmNsdWRlL25ldC9ibHVldG9vdGgvaGNpLmgNCmluZGV4IDBj MjAyMjcuLmRlN2VkODEgMTAwNjQ0DQotLS0gYS9pbmNsdWRlL25ldC9ibHVldG9vdGgvaGNpLmgN CisrKyBiL2luY2x1ZGUvbmV0L2JsdWV0b290aC9oY2kuaA0KQEAgLTExNDIsNiArMTE0Miw3IEBA IHN0cnVjdCBoY2lfdWZpbHRlciB7DQogDQogLyogLS0tLSBIQ0kgSW9jdGwgcmVxdWVzdHMgc3Ry dWN0dXJlcyAtLS0tICovDQogc3RydWN0IGhjaV9kZXZfc3RhdHMgew0KKwlfX3UzMiBudWxsX3J4 OwkJLyogIyBOVUxMIHBrdHMgcmVjdmQgKi8NCiAJX191MzIgZXJyX3J4Ow0KIAlfX3UzMiBlcnJf dHg7DQogCV9fdTMyIGNtZF90eDsNCmRpZmYgLS1naXQgYS9uZXQvYmx1ZXRvb3RoL2wyY2FwX2Nv cmUuYyBiL25ldC9ibHVldG9vdGgvbDJjYXBfY29yZS5jDQppbmRleCBlNjRhMWMyLi40ODY5MTgx IDEwMDY0NA0KLS0tIGEvbmV0L2JsdWV0b290aC9sMmNhcF9jb3JlLmMNCisrKyBiL25ldC9ibHVl dG9vdGgvbDJjYXBfY29yZS5jDQpAQCAtNDEwOSw2ICs0MTA5LDE0IEBAIHN0YXRpYyBpbnQgbDJj YXBfcmVjdl9hY2xkYXRhKHN0cnVjdCBoY2lfY29ubiAqaGNvbiwgc3RydWN0IHNrX2J1ZmYgKnNr YiwgdTE2IGZsDQogCQlCVF9EQkcoIkNvbnQ6IGZyYWcgbGVuICVkIChleHBlY3RpbmcgJWQpIiwg c2tiLT5sZW4sIGNvbm4tPnJ4X2xlbik7DQogDQogCQlpZiAoIWNvbm4tPnJ4X2xlbikgew0KKwkJ CS8qIEEgMC1sZW5ndGgsIGNvbnRpbnVhdGlvbiBmcmFnbWVudCBpcyBhIE5VTEwgcGFja2V0DQor CQkJICogKENvcmUgMi4xLCBWb2wgMiwgUGFydCBCLCA2LjUuMS4yLCA2LjQuMyAmIDYuNi4yKQ0K KwkJCSAqIFRoZSByZW1vdGUgZGV2aWNlIGlzIGxpa2VseSBjb250cm9sbGluZyBwYWNrZXQgZmxv dw0KKwkJCSAqIHdpdGggQUNMIHBheWxvYWQgaGVhZGVyIEZMT1cgaW5kaWNhdG9yLiAqLw0KKwkJ CWlmICghc2tiLT5sZW4pIHsNCisJCQkJaGNvbi0+aGRldi0+c3RhdC5udWxsX3J4Kys7DQorCQkJ CWdvdG8gZHJvcDsNCisJCQl9DQogCQkJQlRfRVJSKCJVbmV4cGVjdGVkIGNvbnRpbnVhdGlvbiBm cmFtZSAobGVuICVkKSIsIHNrYi0+bGVuKTsNCiAJCQlsMmNhcF9jb25uX3VucmVsaWFibGUoY29u biwgRUNPTU0pOw0KIAkJCWdvdG8gZHJvcDsNCi0tIA0KMS43LjQuMQ0KDQo= ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling 2011-07-02 17:44 [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling Peter Hurley @ 2011-07-02 22:11 ` Marcel Holtmann 2011-07-03 1:49 ` Gustavo F. Padovan 2011-07-05 14:55 ` Peter Hurley 0 siblings, 2 replies; 6+ messages in thread From: Marcel Holtmann @ 2011-07-02 22:11 UTC (permalink / raw) To: Peter Hurley; +Cc: linux-bluetooth Hi Peter, > A 0-length ACL continuation-fragment is a valid NULL packet. Remote > devices can use the FLOW indicator in the ACL packet header to > flow-control ACL packets without sending a payload. > > Track as a device stat instead of logging. > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > --- > include/net/bluetooth/hci.h | 1 + > net/bluetooth/l2cap_core.c | 8 ++++++++ > 2 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 0c20227..de7ed81 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -1142,6 +1142,7 @@ struct hci_ufilter { > > /* ---- HCI Ioctl requests structures ---- */ > struct hci_dev_stats { > + __u32 null_rx; /* # NULL pkts recvd */ > __u32 err_rx; > __u32 err_tx; > __u32 cmd_tx; you can not do it like this. This will break userspace API/ABI. Regards Marcel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling 2011-07-02 22:11 ` Marcel Holtmann @ 2011-07-03 1:49 ` Gustavo F. Padovan 2011-07-05 15:13 ` Peter Hurley 2011-07-05 14:55 ` Peter Hurley 1 sibling, 1 reply; 6+ messages in thread From: Gustavo F. Padovan @ 2011-07-03 1:49 UTC (permalink / raw) To: Marcel Holtmann; +Cc: Peter Hurley, linux-bluetooth * Marcel Holtmann <marcel@holtmann.org> [2011-07-02 15:11:51 -0700]: > Hi Peter, > > > A 0-length ACL continuation-fragment is a valid NULL packet. Remote > > devices can use the FLOW indicator in the ACL packet header to > > flow-control ACL packets without sending a payload. > > > > Track as a device stat instead of logging. > > > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > > --- > > include/net/bluetooth/hci.h | 1 + > > net/bluetooth/l2cap_core.c | 8 ++++++++ > > 2 files changed, 9 insertions(+), 0 deletions(-) > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 0c20227..de7ed81 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -1142,6 +1142,7 @@ struct hci_ufilter { > > > > /* ---- HCI Ioctl requests structures ---- */ > > struct hci_dev_stats { > > + __u32 null_rx; /* # NULL pkts recvd */ > > __u32 err_rx; > > __u32 err_tx; > > __u32 cmd_tx; > > you can not do it like this. This will break userspace API/ABI. Actually I don't see a point to have null_rx counter, just drop the packet and we are done. Gustavo ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling 2011-07-03 1:49 ` Gustavo F. Padovan @ 2011-07-05 15:13 ` Peter Hurley 0 siblings, 0 replies; 6+ messages in thread From: Peter Hurley @ 2011-07-05 15:13 UTC (permalink / raw) To: Gustavo F. Padovan; +Cc: Marcel Holtmann, linux-bluetooth Hi Gustavo, On Sat, 2011-07-02 at 21:49 -0400, Gustavo F. Padovan wrote: > * Marcel Holtmann <marcel@holtmann.org> [2011-07-02 15:11:51 -0700]: >=20 > > Hi Peter, > >=20 > > > A 0-length ACL continuation-fragment is a valid NULL packet. Remote > > > devices can use the FLOW indicator in the ACL packet header to > > > flow-control ACL packets without sending a payload. > > >=20 > > > Track as a device stat instead of logging. > > >=20 > > > Signed-off-by: Peter Hurley <peter@hurleysoftware.com> > > > --- > > > include/net/bluetooth/hci.h | 1 + > > > net/bluetooth/l2cap_core.c | 8 ++++++++ > > > 2 files changed, 9 insertions(+), 0 deletions(-) > > >=20 > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.= h > > > index 0c20227..de7ed81 100644 > > > --- a/include/net/bluetooth/hci.h > > > +++ b/include/net/bluetooth/hci.h > > > @@ -1142,6 +1142,7 @@ struct hci_ufilter { > > > =20 > > > /* ---- HCI Ioctl requests structures ---- */ > > > struct hci_dev_stats { > > > + __u32 null_rx; /* # NULL pkts recvd */ > > > __u32 err_rx; > > > __u32 err_tx; > > > __u32 cmd_tx; > >=20 > > you can not do it like this. This will break userspace API/ABI. >=20 > Actually I don't see a point to have null_rx counter, just drop the packe= t and > we are done. Although a null_rx counter doesn't really qualify as critical or even necessary, I thought it might be useful in helping users troubleshoot apparent link quality issues - especially those relating to A2DP, like dropouts and sputter. Currently, some users experience significant audio quality issues that I suspect is actually *not* related to link quality nor old LMP implementations. Often these are accompanied by logs full of "Unexpected continuation frame ..." messages. By 'full', I mean on the order of 30,000 messages in < 10 minutes. Although I thought it appropriate to not log an error message for an accepted flow control method, it seems a shame to simply ignore it. Regards, Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling 2011-07-02 22:11 ` Marcel Holtmann 2011-07-03 1:49 ` Gustavo F. Padovan @ 2011-07-05 14:55 ` Peter Hurley 2011-07-05 16:35 ` Marcel Holtmann 1 sibling, 1 reply; 6+ messages in thread From: Peter Hurley @ 2011-07-05 14:55 UTC (permalink / raw) To: Marcel Holtmann; +Cc: linux-bluetooth T24gU2F0LCAyMDExLTA3LTAyIGF0IDE4OjExIC0wNDAwLCBNYXJjZWwgSG9sdG1hbm4gd3JvdGU6 DQo+IEhpIFBldGVyLA0KPiANCj4gPiAgLyogLS0tLSBIQ0kgSW9jdGwgcmVxdWVzdHMgc3RydWN0 dXJlcyAtLS0tICovDQo+ID4gIHN0cnVjdCBoY2lfZGV2X3N0YXRzIHsNCj4gPiArCV9fdTMyIG51 bGxfcng7CQkvKiAjIE5VTEwgcGt0cyByZWN2ZCAqLw0KPiA+ICAJX191MzIgZXJyX3J4Ow0KPiA+ ICAJX191MzIgZXJyX3R4Ow0KPiA+ICAJX191MzIgY21kX3R4Ow0KPiANCj4geW91IGNhbiBub3Qg ZG8gaXQgbGlrZSB0aGlzLiBUaGlzIHdpbGwgYnJlYWsgdXNlcnNwYWNlIEFQSS9BQkkuDQoNClRo YW5rcyBmb3IgdGhlIGRlZmluaXRpdmUgYW5zd2VyLCBNYXJjZWwuDQoNCkkgc3VzcGVjdGVkIGFz IG11Y2ggKHdoaWNoIGlzIHdoeSBJIG5vdGVkIGluIHRoZSBhY2NvbXBhbnlpbmcgY292ZXIgdGhh dA0KdGhpcyB3b3VsZCBiZSBpbmNvbXBhdGlibGUgd2l0aCBwcmV2aW91cyB1c2Vyc3BhY2UgdmVy c2lvbnMgb2YgdGhpcw0KaW9jdGwpLg0KDQpJcyB0aGUgZ2VuZXJhbCBwbGFuIHRoZW4gdG8gZnJl ZXplIHRoZSBleGlzdGluZyByYXcgSENJIGludGVyZmFjZSBhbmQNCm9ubHkgaW1wbGVtZW50IG5l dyB1c2Vyc3BhY2UgPC0+IGtlcm5lbCBpbnRlcmZhY2VzIG9uIG1nbXRvcHM/DQoNClJlZ2FyZHMs DQpQZXRlcg0KDQoNCg== ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling 2011-07-05 14:55 ` Peter Hurley @ 2011-07-05 16:35 ` Marcel Holtmann 0 siblings, 0 replies; 6+ messages in thread From: Marcel Holtmann @ 2011-07-05 16:35 UTC (permalink / raw) To: Peter Hurley; +Cc: linux-bluetooth Hi Peter, > > > /* ---- HCI Ioctl requests structures ---- */ > > > struct hci_dev_stats { > > > + __u32 null_rx; /* # NULL pkts recvd */ > > > __u32 err_rx; > > > __u32 err_tx; > > > __u32 cmd_tx; > > > > you can not do it like this. This will break userspace API/ABI. > > Thanks for the definitive answer, Marcel. > > I suspected as much (which is why I noted in the accompanying cover that > this would be incompatible with previous userspace versions of this > ioctl). > > Is the general plan then to freeze the existing raw HCI interface and > only implement new userspace <-> kernel interfaces on mgmtops? we can extend the existing ioctl, but only in an API/ABI compatible fashion. So adding the null_rx at the end of the struct might work. And we might wanna add null_rx + null_tx to be consistent. The problem part here is that the stats struct is also nested in the info struct. We might get lucky here since it is nested at the end. Regards Marcel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-05 16:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-07-02 17:44 [PATCH v2 1/1] Bluetooth: l2cap: fix NULL ACL packet handling Peter Hurley 2011-07-02 22:11 ` Marcel Holtmann 2011-07-03 1:49 ` Gustavo F. Padovan 2011-07-05 15:13 ` Peter Hurley 2011-07-05 14:55 ` Peter Hurley 2011-07-05 16:35 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).