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=
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
Hi Gustavo,
On Sat, 2011-07-02 at 21:49 -0400, Gustavo F. Padovan wrote:
> * Marcel Holtmann <[email protected]> [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 <[email protected]>
> > > ---
> > > 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
T24gU2F0LCAyMDExLTA3LTAyIGF0IDE4OjExIC0wNDAwLCBNYXJjZWwgSG9sdG1hbm4gd3JvdGU6
DQo+IEhpIFBldGVyLA0KPiANCj4gPiAgLyogLS0tLSBIQ0kgSW9jdGwgcmVxdWVzdHMgc3RydWN0
dXJlcyAtLS0tICovDQo+ID4gIHN0cnVjdCBoY2lfZGV2X3N0YXRzIHsNCj4gPiArCV9fdTMyIG51
bGxfcng7CQkvKiAjIE5VTEwgcGt0cyByZWN2ZCAqLw0KPiA+ICAJX191MzIgZXJyX3J4Ow0KPiA+
ICAJX191MzIgZXJyX3R4Ow0KPiA+ICAJX191MzIgY21kX3R4Ow0KPiANCj4geW91IGNhbiBub3Qg
ZG8gaXQgbGlrZSB0aGlzLiBUaGlzIHdpbGwgYnJlYWsgdXNlcnNwYWNlIEFQSS9BQkkuDQoNClRo
YW5rcyBmb3IgdGhlIGRlZmluaXRpdmUgYW5zd2VyLCBNYXJjZWwuDQoNCkkgc3VzcGVjdGVkIGFz
IG11Y2ggKHdoaWNoIGlzIHdoeSBJIG5vdGVkIGluIHRoZSBhY2NvbXBhbnlpbmcgY292ZXIgdGhh
dA0KdGhpcyB3b3VsZCBiZSBpbmNvbXBhdGlibGUgd2l0aCBwcmV2aW91cyB1c2Vyc3BhY2UgdmVy
c2lvbnMgb2YgdGhpcw0KaW9jdGwpLg0KDQpJcyB0aGUgZ2VuZXJhbCBwbGFuIHRoZW4gdG8gZnJl
ZXplIHRoZSBleGlzdGluZyByYXcgSENJIGludGVyZmFjZSBhbmQNCm9ubHkgaW1wbGVtZW50IG5l
dyB1c2Vyc3BhY2UgPC0+IGtlcm5lbCBpbnRlcmZhY2VzIG9uIG1nbXRvcHM/DQoNClJlZ2FyZHMs
DQpQZXRlcg0KDQoNCg==
* Marcel Holtmann <[email protected]> [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 <[email protected]>
> > ---
> > 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
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 <[email protected]>
> ---
> 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