2018-08-02 11:36:47

by Amit K Bag

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Release RF resource on BT shutdown

SGkgTWFyY2VsLA0KDQo+IEhpIEFtaXQsDQo+DQo+ID4gSW4gY2FzZSBvZiBCVCB0dXJuIG9mZiBm
cm9tIFVJIGl0IGlzIHNhZmUgdG8gYWRkIEhDSSByZXNldCBjb21tYW5kIA0KPiA+IHdoaWNoIHdp
bGwgbWFpbnRhaW4gdGhlIGNvbnRyb2xsZXIgc3RhdGUuDQo+DQo+IHRoaXMgbmVlZHMgdG8gYmUg
YSBiaXQgbW9yZSBkZXRhaWxzIHNpbmNlIGhkZXYtPnNodXRkb3duIGhhbmRsaW5nIGlzIHJlYWxs
eSBvbmx5IGZvciB0aGUgb2xkZXIgSW50ZWwgY29udHJvbGxlcnMuDQoNCj4gQWxzbyBJIGFtIGZh
aWxpbmcgdG8gc2VlIHdoeSBIQ0kgUmVzZXQgaGVscHMgb3Igd2hhdCBpdCBkb2VzIGhlcmUuIEl0
IGlzIGdvaW5nIHRvIGJlIGNhbGxlZCBhZ2FpbiBvbiBoZGV2LT5vcGVuIHRoZSBuZXh0IHRpbWUg
dGhlIGNvbnRyb2xsZXIgaXMgYWN0aXZhdGVkLg0KPg0KPiA+IA0KPiA+IFNpZ25lZC1vZmYtYnk6
IEFtaXQgSyBCYWcgPGFtaXQuay5iYWdAaW50ZWwuY29tPg0KPiA+IC0tLQ0KPiA+IGRyaXZlcnMv
Ymx1ZXRvb3RoL2J0dXNiLmMgfCAxMiArKysrKysrKysrKysNCj4gPiAxIGZpbGUgY2hhbmdlZCwg
MTIgaW5zZXJ0aW9ucygrKQ0KPiA+IA0KPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2JsdWV0b290
aC9idHVzYi5jIGIvZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYyANCj4gPiBpbmRleCA1NzJmZDc1
ZmJjZjYuLjU5OWVjOWIxMmQzZiAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2JsdWV0b290aC9i
dHVzYi5jDQo+ID4gKysrIGIvZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYw0KPiA+IEBAIC0yMzY5
LDYgKzIzNjksMTggQEAgc3RhdGljIGludCBidHVzYl9zaHV0ZG93bl9pbnRlbChzdHJ1Y3QgaGNp
X2RldiAqaGRldikNCj4gPiAJc3RydWN0IHNrX2J1ZmYgKnNrYjsNCj4gPiAJbG9uZyByZXQ7DQo+
ID4gDQo+ID4gKwkvKiBJbiBjYXNlIG9mIEJUIG9mZiBmcm9tIFVJIGl0IGlzIHNhZmUgdG8gZG8g
SENJIHJlc2V0Lg0KPiA+ICsJICogVGhpcyB3aWxsIG1haW50YWluIHRoZSBjb250cm9sbGVyIHN0
YXRlLg0KPiA+ICsJICovDQo+DQo+IEkgZG8gbm90IGNhcmUgd2hhdCB0aGUgVUkgZG9lcyBoZXJl
LiBUaGlzIGlzIHBvd2VyIGRvd24gb3BlcmF0aW9uIGFuZCBzbyBpdCBuZWVkcyB0byBiZSBleHBs
YWluZWQgcHJvcGVybHkuIEFuZCB3aXRoIGEgbG90IG1vcmUgZGV0YWlscyBzaW5jZSBJIGFtIGZh
aWxpbmcgdG8gdW5kZXJzdGFuZCB3aGF0IHN0YXRlIGlzIG1haW50YWluZWQgb24gSENJIFJlc2V0
LiBBcyBwZXIgc3BlY2lmaWNhdGlvbiBpdCB3aWxsIHJlc2V0IGFsbCBCbHVldG9vdGggPiA+IHN0
YXRlcyBiYWNrIHRvIHRoZSBkZWZhdWx0cy4NCj4NCj4gPiArCXNrYiA9IF9faGNpX2NtZF9zeW5j
KGhkZXYsIEhDSV9PUF9SRVNFVCwgMCwgTlVMTCwgSENJX0lOSVRfVElNRU9VVCk7DQo+ID4gKyAg
ICAgICAgaWYgKElTX0VSUihza2IpKSB7DQo+ID4gKwkJcmV0ID0gUFRSX0VSUihza2IpOw0KPiA+
ICsJCUJUX0VSUigiJXM6IEJUIGNvbnRyb2xsZXIgIEhDSSByZXNldCBmYWlsZWQgKCVsZCkiLA0K
PiA+ICsJCSAgICAgICBoZGV2LT5uYW1lLCByZXQpOw0KPiA+ICsJCXJldHVybiByZXQ7DQo+ID4g
Kwl9DQo+ID4gKwlrZnJlZV9za2Ioc2tiKTsNCj4gPiArDQo+DQo+ID4gVGhlcmUgaXMgd2hpdGVz
cGFjZSBkYW1hZ2UgaGVyZS4gQW5kIGlmIHRoaXMgaGFzbuKAmXQgYmVlbiBkb25lIHlldCwgcGxl
YXNlIGZpcnN0IHN0YXJ0IHVzaW5nIGJ0X2Rldl9lcnIgaW5zdGVhZCBvZiBCVF9FUlIgZm9yIHRo
ZSBJbnRlbCBwYXJ0cy4NCj4NCj4gPiAJLyogU29tZSBwbGF0Zm9ybXMgaGF2ZSBhbiBpc3N1ZSB3
aXRoIEJUIExFRCB3aGVuIHRoZSBpbnRlcmZhY2UgaXMNCj4gPiAJICogZG93biBvciBCVCByYWRp
byBpcyB0dXJuZWQgb2ZmLCB3aGljaCB0YWtlcyA1IHNlY29uZHMgdG8gQlQgTEVEDQo+ID4gCSAq
IGdvZXMgb2ZmLiBUaGlzIGNvbW1hbmQgdHVybnMgb2ZmIHRoZSBCVCBMRUQgaW1tZWRpYXRlbHku
DQo+DQo+IFJlZ2FyZHMNCj4NCj4gTWFyY2VsDQoNCg0KSSBoYXZlIHVwZGF0ZWQgdGhlIHBhdGNo
IGFzIHBlciB5b3VyIGNvbW1lbnRzLiBQbGVhc2UgZmluZCB0aGUgcGF0Y2ggYXMgYmVsb3cuDQoN
Cklzc3VlIGRlc2NyaXB0aW9uOiBJbnRlbCAyNzY1IHNoYXJlcyB0aGUgc2FtZSBSRiB3aXRoIFdp
ZmkgYW5kIEJULg0KSW4gdGhlIHNodXRkb3duIHNjZW5hcmlvIHR1cm4gb2ZmIEJULCBmb2xsb3dl
ZCBieSB0dXJuIFdpRmkgb2ZmDQphbmQgb24gY2F1c2luZyBlcnJvciBpbiBSRiBjYWxpYnJhdGlv
biBpbiBXaUZpIE1vZHVsZQ0KDQpTb2x1dGlvbjogYmVmb3JlIHNodXRkb3duIEJUIGVuc3VyZSBh
bnkgUkYgYWN0aXZpdHkgdG8gY2xlYXIgYnkNCkhDSSByZXNldCBjb21tYW5kLg0KDQpSZWZlcmVu
Y2UgTG9nczoNCkVSUiBrZXJuZWw6IFsgMzg2LjE5MzI4NF0gaXdsd2lmaSAwMDAwOjAxOjAwLjA6
IEZhaWxlZCB0byBydW4gSU5JVCBjYWxpYnJhdGlvbnM6IC01DQpFUlIga2VybmVsOiBbIDM4Ni4x
OTMyOThdIGl3bHdpZmkgMDAwMDowMTowMC4wOiBGYWlsZWQgdG8gcnVuIElOSVQgdWNvZGU6IC01
DQpFUlIga2VybmVsOiBbIDM4Ni4xOTMzMDldIGl3bHdpZmkgMDAwMDowMTowMC4wOiBGYWlsZWQg
dG8gc3RhcnQgUlQgdWNvZGU6IC01DQoNClNpZ25lZC1vZmYtYnk6IEFtaXQgSyBCYWcgPGFtaXQu
ay5iYWdAaW50ZWwuY29tPg0KLS0tDQogZHJpdmVycy9ibHVldG9vdGgvYnR1c2IuYyB8IDE1ICsr
KysrKysrKysrKysrKw0KIDEgZmlsZSBjaGFuZ2VkLCAxNSBpbnNlcnRpb25zKCspDQoNCmRpZmYg
LS1naXQgYS9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jIGIvZHJpdmVycy9ibHVldG9vdGgvYnR1
c2IuYw0KaW5kZXggNTcyZmQ3NWZiY2Y2Li40YTNiNmE3ZGM4NmYgMTAwNjQ0DQotLS0gYS9kcml2
ZXJzL2JsdWV0b290aC9idHVzYi5jDQorKysgYi9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jDQpA
QCAtMjM2OSw2ICsyMzY5LDIxIEBAIHN0YXRpYyBpbnQgYnR1c2Jfc2h1dGRvd25faW50ZWwoc3Ry
dWN0IGhjaV9kZXYgKmhkZXYpDQogICAgICAgIHN0cnVjdCBza19idWZmICpza2I7DQogICAgICAg
IGxvbmcgcmV0Ow0KDQorICAgICAgIC8qIElTU1VFOiBJbiBzaHV0ZG93biBzZXF1ZW5jZSB3aGVy
ZSBCVCB0dXJuIG9mZiBhbmQgZm9sbG93ZWQNCisgICAgICAgICogYnkgV2lGaSB0dXJuIG9mZiwg
YW5kIGlmIFdpZmkgaXMgdHVybiBvbiBiYWNrLCBXaWZpIGlzIHVuYWJsZQ0KKyAgICAgICAgKiB0
byBkbyB0aGUgUkYgY2FsaWJyYXRpb24uKG9ic2VydmUgdGhpcyBvbiBJTlRFTCA3MjY1KQ0KKyAg
ICAgICAgKg0KKyAgICAgICAgKiBTb2x1dGlvbjogYmVmb3JlIHNodXRkb3duIEJUIGVuc3VyZSBh
bnkgUkYgYWN0aXZpdHkgdG8gY2xlYXINCisgICAgICAgICogYnkgc2VuZCB0aGlzIGNvbW1hbmQu
DQorICAgICAgICAqLw0KKyAgICAgICBza2IgPSBfX2hjaV9jbWRfc3luYyhoZGV2LCBIQ0lfT1Bf
UkVTRVQsIDAsIE5VTEwsIEhDSV9JTklUX1RJTUVPVVQpOw0KKyAgICAgICBpZiAoSVNfRVJSKHNr
YikpIHsNCisgICAgICAgICAgICAgICByZXQgPSBQVFJfRVJSKHNrYik7DQorICAgICAgICAgICAg
ICAgYnRfZGV2X2VycihoZGV2LCAiJXM6IEhDSSByZXNldCBmYWlsZWRcbiIsIF9fZnVuY19fKTsN
CisgICAgICAgICAgICAgICByZXR1cm4gcmV0Ow0KKyAgICAgICB9DQorICAgICAgIGtmcmVlX3Nr
Yihza2IpOw0KKw0KICAgICAgICAvKiBTb21lIHBsYXRmb3JtcyBoYXZlIGFuIGlzc3VlIHdpdGgg
QlQgTEVEIHdoZW4gdGhlIGludGVyZmFjZSBpcw0KICAgICAgICAgKiBkb3duIG9yIEJUIHJhZGlv
IGlzIHR1cm5lZCBvZmYsIHdoaWNoIHRha2VzIDUgc2Vjb25kcyB0byBCVCBMRUQNCiAgICAgICAg
ICogZ29lcyBvZmYuIFRoaXMgY29tbWFuZCB0dXJucyBvZmYgdGhlIEJUIExFRCBpbW1lZGlhdGVs
eS4NCi0tDQoyLjcuNA0KDQo=


2018-08-02 16:21:30

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Release RF resource on BT shutdown

Hi Amit,

>>>>> In case of BT turn off from UI it is safe to add HCI reset command
>>>>> which will maintain the controller state.
>>>>
>>>> this needs to be a bit more details since hdev->shutdown handling is really only for the older Intel controllers.
>>>
>>>> Also I am failing to see why HCI Reset helps or what it does here. It is going to be called again on hdev->open the next time the controller is activated.
>>>>
>>>>>
>>>>> Signed-off-by: Amit K Bag <[email protected]>
>>>>> ---
>>>>> drivers/bluetooth/btusb.c | 12 ++++++++++++
>>>>> 1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 572fd75fbcf6..599ec9b12d3f 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -2369,6 +2369,18 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
>>>>> struct sk_buff *skb;
>>>>> long ret;
>>>>>
>>>>> + /* In case of BT off from UI it is safe to do HCI reset.
>>>>> + * This will maintain the controller state.
>>>>> + */
>>>>
>>>> I do not care what the UI does here. This is power down operation and so it needs to be explained properly. And with a lot more details since I am failing to understand what state is maintained on HCI Reset. As per specification it will reset all Bluetooth >> > states back to the defaults.
>>>>
>>>>> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>>>> + if (IS_ERR(skb)) {
>>>>> + ret = PTR_ERR(skb);
>>>>> + BT_ERR("%s: BT controller HCI reset failed (%ld)",
>>>>> + hdev->name, ret);
>>>>> + return ret;
>>>>> + }
>>>>> + kfree_skb(skb);
>>>>> +
>>>>
>>>>> There is whitespace damage here. And if this hasn’t been done yet, please first start using bt_dev_err instead of BT_ERR for the Intel parts.
>>>>
>>>>> /* Some platforms have an issue with BT LED when the interface is
>>>>> * down or BT radio is turned off, which takes 5 seconds to BT LED
>>>>> * goes off. This command turns off the BT LED immediately.
>>>>
>>>> Regards
>>>>
>>>> Marcel
>>>
>>>
>>> I have updated the patch as per your comments. Please find the patch as below.
>>>
>>> Issue description: Intel 2765 shares the same RF with Wifi and BT.
>>
>> 7265 ?
>>
>>> In the shutdown scenario turn off BT, followed by turn WiFi off and on
>>> causing error in RF calibration in WiFi Module
>>>
>>> Solution: before shutdown BT ensure any RF activity to clear by HCI
>>> reset command.
>>>
>>> Reference Logs:
>>> ERR kernel: [ 386.193284] iwlwifi 0000:01:00.0: Failed to run INIT
>>> calibrations: -5 ERR kernel: [ 386.193298] iwlwifi 0000:01:00.0:
>>> Failed to run INIT ucode: -5 ERR kernel: [ 386.193309] iwlwifi
>>> 0000:01:00.0: Failed to start RT ucode: -5
>>>
>>> Signed-off-by: Amit K Bag <[email protected]>
>>> ---
>>> drivers/bluetooth/btusb.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 572fd75fbcf6..4a3b6a7dc86f 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -2369,6 +2369,21 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
>>> struct sk_buff *skb;
>>> long ret;
>>>
>>> + /* ISSUE: In shutdown sequence where BT turn off and followed
>>
>> Scrap the word ISSUE: since that is not needed. Just describe why we are doing this.
>>
>>> + * by WiFi turn off, and if Wifi is turn on back, Wifi is unable
>>> + * to do the RF calibration.(observe this on INTEL 7265)
>>
>> No need for the text in ()
>>
>>> + *
>>> + * Solution: before shutdown BT ensure any RF activity to clear
>>> + * by send this command.
>>> + */
>>
>> Scrap the Solution: and bring this in as proper sentence.
>>
>>> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + ret = PTR_ERR(skb);
>>> + bt_dev_err(hdev, "%s: HCI reset failed\n", __func__);
>>
>> Scrap the __func__ usage. We are not doing this. Also no \n with the bt_dev_err. If you want this clear then "HCI reset on shutdown failed”.
>>
>>> + return ret;
>>> + }
>>> + kfree_skb(skb);
>>> +
>>> /* Some platforms have an issue with BT LED when the interface is
>>> * down or BT radio is turned off, which takes 5 seconds to BT LED
>>> * goes off. This command turns off the BT LED immediately.
>
> I have updated the patch as per your comments. Please find the patch as below.
>
> Issue description: Intel 7265 shares the same RF with Wifi and BT.
> In the shutdown scenario turn off BT, followed by turn WiFi off
> and on causing error in RF calibration in WiFi Module
>
> Solution: HCI reset command should clear any RF activity before shutdown BT.
>
> Reference Logs:
> ERR kernel: [ 386.193284] iwlwifi 0000:01:00.0: Failed to run INIT calibrations: -5
> ERR kernel: [ 386.193298] iwlwifi 0000:01:00.0: Failed to run INIT ucode: -5
> ERR kernel: [ 386.193309] iwlwifi 0000:01:00.0: Failed to start RT ucode: -5
>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 572fd75fbcf6..5a59dd91062d 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2369,6 +2369,20 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
> struct sk_buff *skb;
> long ret;
>
> + /* In shutdown sequence where BT turn off and followed
> + * by WiFi turn off, and if Wifi is turn on back, Wifi is unable
> + * to do the RF calibration.
> + *
> + * This command should clear any RF activity before shutdown BT.
> + */

/* In the shutdown sequence where Bluetooth is turned off followed
* by WiFi being turned off, turning WiFi back on causes issue with
* the RF calibration.
*
* To ensure that any RF activity has been stopped, issue HCI Reset
* command to clear all ongoing activity including advertising,
* scanning etc.
*/

> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + ret = PTR_ERR(skb);
> + bt_dev_err(hdev, "HCI reset on shutdown failed");
> + return ret;
> + }
> + kfree_skb(skb);
> +
> /* Some platforms have an issue with BT LED when the interface is
> * down or BT radio is turned off, which takes 5 seconds to BT LED
> * goes off. This command turns off the BT LED immediately.

And then send a proper v2 of the patch.

Regards

Marcel


2018-08-02 15:42:12

by Amit K Bag

[permalink] [raw]
Subject: RE: [PATCH] Bluetooth: btusb: Release RF resource on BT shutdown

DQogSGkgTWFyY2VsLA0KDQo+Pj4+IEluIGNhc2Ugb2YgQlQgdHVybiBvZmYgZnJvbSBVSSBpdCBp
cyBzYWZlIHRvIGFkZCBIQ0kgcmVzZXQgY29tbWFuZCANCj4+Pj4gd2hpY2ggd2lsbCBtYWludGFp
biB0aGUgY29udHJvbGxlciBzdGF0ZS4NCj4+PiANCj4+PiB0aGlzIG5lZWRzIHRvIGJlIGEgYml0
IG1vcmUgZGV0YWlscyBzaW5jZSBoZGV2LT5zaHV0ZG93biBoYW5kbGluZyBpcyByZWFsbHkgb25s
eSBmb3IgdGhlIG9sZGVyIEludGVsIGNvbnRyb2xsZXJzLg0KPj4gDQo+Pj4gQWxzbyBJIGFtIGZh
aWxpbmcgdG8gc2VlIHdoeSBIQ0kgUmVzZXQgaGVscHMgb3Igd2hhdCBpdCBkb2VzIGhlcmUuIEl0
IGlzIGdvaW5nIHRvIGJlIGNhbGxlZCBhZ2FpbiBvbiBoZGV2LT5vcGVuIHRoZSBuZXh0IHRpbWUg
dGhlIGNvbnRyb2xsZXIgaXMgYWN0aXZhdGVkLg0KPj4+IA0KPj4+PiANCj4+Pj4gU2lnbmVkLW9m
Zi1ieTogQW1pdCBLIEJhZyA8YW1pdC5rLmJhZ0BpbnRlbC5jb20+DQo+Pj4+IC0tLQ0KPj4+PiBk
cml2ZXJzL2JsdWV0b290aC9idHVzYi5jIHwgMTIgKysrKysrKysrKysrDQo+Pj4+IDEgZmlsZSBj
aGFuZ2VkLCAxMiBpbnNlcnRpb25zKCspDQo+Pj4+IA0KPj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVy
cy9ibHVldG9vdGgvYnR1c2IuYyBiL2RyaXZlcnMvYmx1ZXRvb3RoL2J0dXNiLmMgDQo+Pj4+IGlu
ZGV4IDU3MmZkNzVmYmNmNi4uNTk5ZWM5YjEyZDNmIDEwMDY0NA0KPj4+PiAtLS0gYS9kcml2ZXJz
L2JsdWV0b290aC9idHVzYi5jDQo+Pj4+ICsrKyBiL2RyaXZlcnMvYmx1ZXRvb3RoL2J0dXNiLmMN
Cj4+Pj4gQEAgLTIzNjksNiArMjM2OSwxOCBAQCBzdGF0aWMgaW50IGJ0dXNiX3NodXRkb3duX2lu
dGVsKHN0cnVjdCBoY2lfZGV2ICpoZGV2KQ0KPj4+PiAJc3RydWN0IHNrX2J1ZmYgKnNrYjsNCj4+
Pj4gCWxvbmcgcmV0Ow0KPj4+PiANCj4+Pj4gKwkvKiBJbiBjYXNlIG9mIEJUIG9mZiBmcm9tIFVJ
IGl0IGlzIHNhZmUgdG8gZG8gSENJIHJlc2V0Lg0KPj4+PiArCSAqIFRoaXMgd2lsbCBtYWludGFp
biB0aGUgY29udHJvbGxlciBzdGF0ZS4NCj4+Pj4gKwkgKi8NCj4+PiANCj4+PiBJIGRvIG5vdCBj
YXJlIHdoYXQgdGhlIFVJIGRvZXMgaGVyZS4gVGhpcyBpcyBwb3dlciBkb3duIG9wZXJhdGlvbiBh
bmQgc28gaXQgbmVlZHMgdG8gYmUgZXhwbGFpbmVkIHByb3Blcmx5LiBBbmQgd2l0aCBhIGxvdCBt
b3JlIGRldGFpbHMgc2luY2UgSSBhbSBmYWlsaW5nIHRvIHVuZGVyc3RhbmQgd2hhdCBzdGF0ZSBp
cyBtYWludGFpbmVkIG9uIEhDSSBSZXNldC4gQXMgcGVyIHNwZWNpZmljYXRpb24gaXQgd2lsbCBy
ZXNldCBhbGwgQmx1ZXRvb3RoID4+ID4gc3RhdGVzIGJhY2sgdG8gdGhlIGRlZmF1bHRzLg0KPj4+
IA0KPj4+PiArCXNrYiA9IF9faGNpX2NtZF9zeW5jKGhkZXYsIEhDSV9PUF9SRVNFVCwgMCwgTlVM
TCwgSENJX0lOSVRfVElNRU9VVCk7DQo+Pj4+ICsgICAgICAgIGlmIChJU19FUlIoc2tiKSkgew0K
Pj4+PiArCQlyZXQgPSBQVFJfRVJSKHNrYik7DQo+Pj4+ICsJCUJUX0VSUigiJXM6IEJUIGNvbnRy
b2xsZXIgIEhDSSByZXNldCBmYWlsZWQgKCVsZCkiLA0KPj4+PiArCQkgICAgICAgaGRldi0+bmFt
ZSwgcmV0KTsNCj4+Pj4gKwkJcmV0dXJuIHJldDsNCj4+Pj4gKwl9DQo+Pj4+ICsJa2ZyZWVfc2ti
KHNrYik7DQo+Pj4+ICsNCj4+PiANCj4+Pj4gVGhlcmUgaXMgd2hpdGVzcGFjZSBkYW1hZ2UgaGVy
ZS4gQW5kIGlmIHRoaXMgaGFzbsOiwoDCmXQgYmVlbiBkb25lIHlldCwgcGxlYXNlIGZpcnN0IHN0
YXJ0IHVzaW5nIGJ0X2Rldl9lcnIgaW5zdGVhZCBvZiBCVF9FUlIgZm9yIHRoZSBJbnRlbCBwYXJ0
cy4NCj4+PiANCj4+Pj4gCS8qIFNvbWUgcGxhdGZvcm1zIGhhdmUgYW4gaXNzdWUgd2l0aCBCVCBM
RUQgd2hlbiB0aGUgaW50ZXJmYWNlIGlzDQo+Pj4+IAkgKiBkb3duIG9yIEJUIHJhZGlvIGlzIHR1
cm5lZCBvZmYsIHdoaWNoIHRha2VzIDUgc2Vjb25kcyB0byBCVCBMRUQNCj4+Pj4gCSAqIGdvZXMg
b2ZmLiBUaGlzIGNvbW1hbmQgdHVybnMgb2ZmIHRoZSBCVCBMRUQgaW1tZWRpYXRlbHkuDQo+Pj4g
DQo+Pj4gUmVnYXJkcw0KPj4+IA0KPj4+IE1hcmNlbA0KPj4gDQo+PiANCj4+IEkgaGF2ZSB1cGRh
dGVkIHRoZSBwYXRjaCBhcyBwZXIgeW91ciBjb21tZW50cy4gUGxlYXNlIGZpbmQgdGhlIHBhdGNo
IGFzIGJlbG93Lg0KPj4gDQo+PiBJc3N1ZSBkZXNjcmlwdGlvbjogSW50ZWwgMjc2NSBzaGFyZXMg
dGhlIHNhbWUgUkYgd2l0aCBXaWZpIGFuZCBCVC4NCj4NCj43MjY1ID8NCj4NCj4+IEluIHRoZSBz
aHV0ZG93biBzY2VuYXJpbyB0dXJuIG9mZiBCVCwgZm9sbG93ZWQgYnkgdHVybiBXaUZpIG9mZiBh
bmQgb24gDQo+PiBjYXVzaW5nIGVycm9yIGluIFJGIGNhbGlicmF0aW9uIGluIFdpRmkgTW9kdWxl
DQo+PiANCj4+IFNvbHV0aW9uOiBiZWZvcmUgc2h1dGRvd24gQlQgZW5zdXJlIGFueSBSRiBhY3Rp
dml0eSB0byBjbGVhciBieSBIQ0kgDQo+PiByZXNldCBjb21tYW5kLg0KPj4gDQo+PiBSZWZlcmVu
Y2UgTG9nczoNCj4+IEVSUiBrZXJuZWw6IFsgMzg2LjE5MzI4NF0gaXdsd2lmaSAwMDAwOjAxOjAw
LjA6IEZhaWxlZCB0byBydW4gSU5JVCANCj4+IGNhbGlicmF0aW9uczogLTUgRVJSIGtlcm5lbDog
WyAzODYuMTkzMjk4XSBpd2x3aWZpIDAwMDA6MDE6MDAuMDogDQo+PiBGYWlsZWQgdG8gcnVuIElO
SVQgdWNvZGU6IC01IEVSUiBrZXJuZWw6IFsgMzg2LjE5MzMwOV0gaXdsd2lmaSANCj4+IDAwMDA6
MDE6MDAuMDogRmFpbGVkIHRvIHN0YXJ0IFJUIHVjb2RlOiAtNQ0KPj4gDQo+PiBTaWduZWQtb2Zm
LWJ5OiBBbWl0IEsgQmFnIDxhbWl0LmsuYmFnQGludGVsLmNvbT4NCj4+IC0tLQ0KPj4gZHJpdmVy
cy9ibHVldG9vdGgvYnR1c2IuYyB8IDE1ICsrKysrKysrKysrKysrKw0KPj4gMSBmaWxlIGNoYW5n
ZWQsIDE1IGluc2VydGlvbnMoKykNCj4+IA0KPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvYmx1ZXRv
b3RoL2J0dXNiLmMgYi9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jIA0KPj4gaW5kZXggNTcyZmQ3
NWZiY2Y2Li40YTNiNmE3ZGM4NmYgMTAwNjQ0DQo+PiAtLS0gYS9kcml2ZXJzL2JsdWV0b290aC9i
dHVzYi5jDQo+PiArKysgYi9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jDQo+PiBAQCAtMjM2OSw2
ICsyMzY5LDIxIEBAIHN0YXRpYyBpbnQgYnR1c2Jfc2h1dGRvd25faW50ZWwoc3RydWN0IGhjaV9k
ZXYgKmhkZXYpDQo+PiAgICAgICAgc3RydWN0IHNrX2J1ZmYgKnNrYjsNCj4+ICAgICAgICBsb25n
IHJldDsNCj4+IA0KPj4gKyAgICAgICAvKiBJU1NVRTogSW4gc2h1dGRvd24gc2VxdWVuY2Ugd2hl
cmUgQlQgdHVybiBvZmYgYW5kIGZvbGxvd2VkDQo+DQo+U2NyYXAgdGhlIHdvcmQgSVNTVUU6IHNp
bmNlIHRoYXQgaXMgbm90IG5lZWRlZC4gSnVzdCBkZXNjcmliZSB3aHkgd2UgYXJlIGRvaW5nIHRo
aXMuDQo+DQo+PiArICAgICAgICAqIGJ5IFdpRmkgdHVybiBvZmYsIGFuZCBpZiBXaWZpIGlzIHR1
cm4gb24gYmFjaywgV2lmaSBpcyB1bmFibGUNCj4+ICsgICAgICAgICogdG8gZG8gdGhlIFJGIGNh
bGlicmF0aW9uLihvYnNlcnZlIHRoaXMgb24gSU5URUwgNzI2NSkNCj4NCj5ObyBuZWVkIGZvciB0
aGUgdGV4dCBpbiAoKQ0KPg0KPj4gKyAgICAgICAgKg0KPj4gKyAgICAgICAgKiBTb2x1dGlvbjog
YmVmb3JlIHNodXRkb3duIEJUIGVuc3VyZSBhbnkgUkYgYWN0aXZpdHkgdG8gY2xlYXINCj4+ICsg
ICAgICAgICogYnkgc2VuZCB0aGlzIGNvbW1hbmQuDQo+PiArICAgICAgICAqLw0KPg0KPlNjcmFw
IHRoZSBTb2x1dGlvbjogYW5kIGJyaW5nIHRoaXMgaW4gYXMgcHJvcGVyIHNlbnRlbmNlLg0KPg0K
Pj4gKyAgICAgICBza2IgPSBfX2hjaV9jbWRfc3luYyhoZGV2LCBIQ0lfT1BfUkVTRVQsIDAsIE5V
TEwsIEhDSV9JTklUX1RJTUVPVVQpOw0KPj4gKyAgICAgICBpZiAoSVNfRVJSKHNrYikpIHsNCj4+
ICsgICAgICAgICAgICAgICByZXQgPSBQVFJfRVJSKHNrYik7DQo+PiArICAgICAgICAgICAgICAg
YnRfZGV2X2VycihoZGV2LCAiJXM6IEhDSSByZXNldCBmYWlsZWRcbiIsIF9fZnVuY19fKTsNCj4N
Cj5TY3JhcCB0aGUgX19mdW5jX18gdXNhZ2UuIFdlIGFyZSBub3QgZG9pbmcgdGhpcy4gQWxzbyBu
byBcbiB3aXRoIHRoZSBidF9kZXZfZXJyLiBJZiB5b3Ugd2FudCB0aGlzIGNsZWFyIHRoZW4gIkhD
SSByZXNldCBvbiBzaHV0ZG93biBmYWlsZWTigJ0uDQo+DQo+PiArICAgICAgICAgICAgICAgcmV0
dXJuIHJldDsNCj4+ICsgICAgICAgfQ0KPj4gKyAgICAgICBrZnJlZV9za2Ioc2tiKTsNCj4+ICsN
Cj4+ICAgICAgICAvKiBTb21lIHBsYXRmb3JtcyBoYXZlIGFuIGlzc3VlIHdpdGggQlQgTEVEIHdo
ZW4gdGhlIGludGVyZmFjZSBpcw0KPj4gICAgICAgICAqIGRvd24gb3IgQlQgcmFkaW8gaXMgdHVy
bmVkIG9mZiwgd2hpY2ggdGFrZXMgNSBzZWNvbmRzIHRvIEJUIExFRA0KPj4gICAgICAgICAqIGdv
ZXMgb2ZmLiBUaGlzIGNvbW1hbmQgdHVybnMgb2ZmIHRoZSBCVCBMRUQgaW1tZWRpYXRlbHkuDQoN
CkkgaGF2ZSB1cGRhdGVkIHRoZSBwYXRjaCBhcyBwZXIgeW91ciBjb21tZW50cy4gUGxlYXNlIGZp
bmQgdGhlIHBhdGNoIGFzIGJlbG93Lg0KDQpJc3N1ZSBkZXNjcmlwdGlvbjogSW50ZWwgNzI2NSBz
aGFyZXMgdGhlIHNhbWUgUkYgd2l0aCBXaWZpIGFuZCBCVC4NCkluIHRoZSBzaHV0ZG93biBzY2Vu
YXJpbyB0dXJuIG9mZiBCVCwgZm9sbG93ZWQgYnkgdHVybiBXaUZpIG9mZg0KYW5kIG9uIGNhdXNp
bmcgZXJyb3IgaW4gUkYgY2FsaWJyYXRpb24gaW4gV2lGaSBNb2R1bGUNCg0KU29sdXRpb246IEhD
SSByZXNldCBjb21tYW5kIHNob3VsZCBjbGVhciBhbnkgUkYgYWN0aXZpdHkgIGJlZm9yZSBzaHV0
ZG93biBCVC4NCg0KUmVmZXJlbmNlIExvZ3M6DQpFUlIga2VybmVsOiBbIDM4Ni4xOTMyODRdIGl3
bHdpZmkgMDAwMDowMTowMC4wOiBGYWlsZWQgdG8gcnVuIElOSVQgY2FsaWJyYXRpb25zOiAtNQ0K
RVJSIGtlcm5lbDogWyAzODYuMTkzMjk4XSBpd2x3aWZpIDAwMDA6MDE6MDAuMDogRmFpbGVkIHRv
IHJ1biBJTklUIHVjb2RlOiAtNQ0KRVJSIGtlcm5lbDogWyAzODYuMTkzMzA5XSBpd2x3aWZpIDAw
MDA6MDE6MDAuMDogRmFpbGVkIHRvIHN0YXJ0IFJUIHVjb2RlOiAtNQ0KDQpTaWduZWQtb2ZmLWJ5
OiBBbWl0IEsgQmFnIDxhbWl0LmsuYmFnQGludGVsLmNvbT4NCi0tLQ0KIGRyaXZlcnMvYmx1ZXRv
b3RoL2J0dXNiLmMgfCAxNCArKysrKysrKysrKysrKw0KIDEgZmlsZSBjaGFuZ2VkLCAxNCBpbnNl
cnRpb25zKCspDQoNCmRpZmYgLS1naXQgYS9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jIGIvZHJp
dmVycy9ibHVldG9vdGgvYnR1c2IuYw0KaW5kZXggNTcyZmQ3NWZiY2Y2Li41YTU5ZGQ5MTA2MmQg
MTAwNjQ0DQotLS0gYS9kcml2ZXJzL2JsdWV0b290aC9idHVzYi5jDQorKysgYi9kcml2ZXJzL2Js
dWV0b290aC9idHVzYi5jDQpAQCAtMjM2OSw2ICsyMzY5LDIwIEBAIHN0YXRpYyBpbnQgYnR1c2Jf
c2h1dGRvd25faW50ZWwoc3RydWN0IGhjaV9kZXYgKmhkZXYpDQogICAgICAgIHN0cnVjdCBza19i
dWZmICpza2I7DQogICAgICAgIGxvbmcgcmV0Ow0KDQorICAgICAgIC8qIEluIHNodXRkb3duIHNl
cXVlbmNlIHdoZXJlIEJUIHR1cm4gb2ZmIGFuZCBmb2xsb3dlZA0KKyAgICAgICAgKiBieSBXaUZp
IHR1cm4gb2ZmLCBhbmQgaWYgV2lmaSBpcyB0dXJuIG9uIGJhY2ssIFdpZmkgaXMgdW5hYmxlDQor
ICAgICAgICAqIHRvIGRvIHRoZSBSRiBjYWxpYnJhdGlvbi4NCisgICAgICAgICoNCisgICAgICAg
ICogVGhpcyBjb21tYW5kIHNob3VsZCBjbGVhciBhbnkgUkYgYWN0aXZpdHkgIGJlZm9yZSBzaHV0
ZG93biBCVC4NCisgICAgICAgICovDQorICAgICAgIHNrYiA9IF9faGNpX2NtZF9zeW5jKGhkZXYs
IEhDSV9PUF9SRVNFVCwgMCwgTlVMTCwgSENJX0lOSVRfVElNRU9VVCk7DQorICAgICAgIGlmIChJ
U19FUlIoc2tiKSkgew0KKyAgICAgICAgICAgICAgIHJldCA9IFBUUl9FUlIoc2tiKTsNCisgICAg
ICAgICAgICAgICBidF9kZXZfZXJyKGhkZXYsICJIQ0kgcmVzZXQgb24gc2h1dGRvd24gZmFpbGVk
Iik7DQorICAgICAgICAgICAgICAgcmV0dXJuIHJldDsNCisgICAgICAgfQ0KKyAgICAgICBrZnJl
ZV9za2Ioc2tiKTsNCisNCiAgICAgICAgLyogU29tZSBwbGF0Zm9ybXMgaGF2ZSBhbiBpc3N1ZSB3
aXRoIEJUIExFRCB3aGVuIHRoZSBpbnRlcmZhY2UgaXMNCiAgICAgICAgICogZG93biBvciBCVCBy
YWRpbyBpcyB0dXJuZWQgb2ZmLCB3aGljaCB0YWtlcyA1IHNlY29uZHMgdG8gQlQgTEVEDQogICAg
ICAgICAqIGdvZXMgb2ZmLiBUaGlzIGNvbW1hbmQgdHVybnMgb2ZmIHRoZSBCVCBMRUQgaW1tZWRp
YXRlbHkuDQo=

2018-08-02 12:29:59

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: btusb: Release RF resource on BT shutdown

Hi Amit,

>>> In case of BT turn off from UI it is safe to add HCI reset command
>>> which will maintain the controller state.
>>
>> this needs to be a bit more details since hdev->shutdown handling is really only for the older Intel controllers.
>
>> Also I am failing to see why HCI Reset helps or what it does here. It is going to be called again on hdev->open the next time the controller is activated.
>>
>>>
>>> Signed-off-by: Amit K Bag <[email protected]>
>>> ---
>>> drivers/bluetooth/btusb.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 572fd75fbcf6..599ec9b12d3f 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -2369,6 +2369,18 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
>>> struct sk_buff *skb;
>>> long ret;
>>>
>>> + /* In case of BT off from UI it is safe to do HCI reset.
>>> + * This will maintain the controller state.
>>> + */
>>
>> I do not care what the UI does here. This is power down operation and so it needs to be explained properly. And with a lot more details since I am failing to understand what state is maintained on HCI Reset. As per specification it will reset all Bluetooth > > states back to the defaults.
>>
>>> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
>>> + if (IS_ERR(skb)) {
>>> + ret = PTR_ERR(skb);
>>> + BT_ERR("%s: BT controller HCI reset failed (%ld)",
>>> + hdev->name, ret);
>>> + return ret;
>>> + }
>>> + kfree_skb(skb);
>>> +
>>
>>> There is whitespace damage here. And if this hasn’t been done yet, please first start using bt_dev_err instead of BT_ERR for the Intel parts.
>>
>>> /* Some platforms have an issue with BT LED when the interface is
>>> * down or BT radio is turned off, which takes 5 seconds to BT LED
>>> * goes off. This command turns off the BT LED immediately.
>>
>> Regards
>>
>> Marcel
>
>
> I have updated the patch as per your comments. Please find the patch as below.
>
> Issue description: Intel 2765 shares the same RF with Wifi and BT.

7265 ?

> In the shutdown scenario turn off BT, followed by turn WiFi off
> and on causing error in RF calibration in WiFi Module
>
> Solution: before shutdown BT ensure any RF activity to clear by
> HCI reset command.
>
> Reference Logs:
> ERR kernel: [ 386.193284] iwlwifi 0000:01:00.0: Failed to run INIT calibrations: -5
> ERR kernel: [ 386.193298] iwlwifi 0000:01:00.0: Failed to run INIT ucode: -5
> ERR kernel: [ 386.193309] iwlwifi 0000:01:00.0: Failed to start RT ucode: -5
>
> Signed-off-by: Amit K Bag <[email protected]>
> ---
> drivers/bluetooth/btusb.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 572fd75fbcf6..4a3b6a7dc86f 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -2369,6 +2369,21 @@ static int btusb_shutdown_intel(struct hci_dev *hdev)
> struct sk_buff *skb;
> long ret;
>
> + /* ISSUE: In shutdown sequence where BT turn off and followed

Scrap the word ISSUE: since that is not needed. Just describe why we are doing this.

> + * by WiFi turn off, and if Wifi is turn on back, Wifi is unable
> + * to do the RF calibration.(observe this on INTEL 7265)

No need for the text in ()

> + *
> + * Solution: before shutdown BT ensure any RF activity to clear
> + * by send this command.
> + */

Scrap the Solution: and bring this in as proper sentence.

> + skb = __hci_cmd_sync(hdev, HCI_OP_RESET, 0, NULL, HCI_INIT_TIMEOUT);
> + if (IS_ERR(skb)) {
> + ret = PTR_ERR(skb);
> + bt_dev_err(hdev, "%s: HCI reset failed\n", __func__);

Scrap the __func__ usage. We are not doing this. Also no \n with the bt_dev_err. If you want this clear then "HCI reset on shutdown failed”.

> + return ret;
> + }
> + kfree_skb(skb);
> +
> /* Some platforms have an issue with BT LED when the interface is
> * down or BT radio is turned off, which takes 5 seconds to BT LED
> * goes off. This command turns off the BT LED immediately.

Regards

Marcel