From: Zijun Hu <[email protected]>
This patch series are to fix below 2 regression issues for QCA controllers
1) BT can't be enabled once BT is disabled for several QCA controllers
2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
the links for these issues are shown below:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Zijun Hu (2):
Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with
gpiod_get_optional()"
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
then warm reboot
drivers/bluetooth/hci_qca.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
--
2.7.4
From: Zijun Hu <[email protected]>
Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
not configured by DT or ACPI, the regression issue is that BT can't be
enabled after disable then warm reboot, fixed by correcting conditions
for sending the VSC reset controller within qca_serdev_shutdown().
Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
Reported-by: Wren Turkal <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
drivers/bluetooth/hci_qca.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 160175a23a49..2a47a60ecc25 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
struct hci_uart *hu = &qcadev->serdev_hu;
struct hci_dev *hdev = hu->hdev;
- struct qca_data *qca = hu->priv;
const u8 ibs_wake_cmd[] = { 0xFD };
const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
if (qcadev->btsoc_type == QCA_QCA6390) {
- if (test_bit(QCA_BT_OFF, &qca->flags) ||
- !test_bit(HCI_RUNNING, &hdev->flags))
+ if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
+ BT_INFO("QCA do not need to send EDL_RESET_REQ");
return;
+ }
+
+ if (hci_dev_test_flag(hdev, HCI_SETUP)) {
+ BT_INFO("QCA do not send EDL_RESET_REQ");
+ return;
+ }
+ BT_INFO("QCA start to send EDL_RESET_REQ");
serdev_device_write_flush(serdev);
ret = serdev_device_write_buf(serdev, ibs_wake_cmd,
sizeof(ibs_wake_cmd));
--
2.7.4
On 18/04/2024 16:06, Zijun Hu wrote:
> From: Zijun Hu <[email protected]>
>
> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
> not configured by DT or ACPI, the regression issue is that BT can't be
> enabled after disable then warm reboot, fixed by correcting conditions
> for sending the VSC reset controller within qca_serdev_shutdown().
I have trouble understanding what is the bug. Can you rephrase it?
>
> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
> Reported-by: Wren Turkal <[email protected]>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Signed-off-by: Zijun Hu <[email protected]>
> Tested-by: Wren Turkal <[email protected]>
> ---
> drivers/bluetooth/hci_qca.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 160175a23a49..2a47a60ecc25 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
> struct hci_uart *hu = &qcadev->serdev_hu;
> struct hci_dev *hdev = hu->hdev;
> - struct qca_data *qca = hu->priv;
> const u8 ibs_wake_cmd[] = { 0xFD };
> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>
> if (qcadev->btsoc_type == QCA_QCA6390) {
> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
> - !test_bit(HCI_RUNNING, &hdev->flags))
> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
> return;
> + }
> +
> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
Commit msg does not help me at all to understand why you are changing
the test bits.
> + BT_INFO("QCA do not send EDL_RESET_REQ");
> + return;
> + }
>
> + BT_INFO("QCA start to send EDL_RESET_REQ");
Why debugging info is part of the fix?
Best regards,
Krzysztof
On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
> On 18/04/2024 16:06, Zijun Hu wrote:
>> From: Zijun Hu <[email protected]>
>>
>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>> not configured by DT or ACPI, the regression issue is that BT can't be
>> enabled after disable then warm reboot, fixed by correcting conditions
>> for sending the VSC reset controller within qca_serdev_shutdown().
>
> I have trouble understanding what is the bug. Can you rephrase it?
>
Think about below sequences:
cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
BT is failed to be enabled after warm reboot.
>>
>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>> Reported-by: Wren Turkal <[email protected]>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Signed-off-by: Zijun Hu <[email protected]>
>> Tested-by: Wren Turkal <[email protected]>
>> ---
>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 160175a23a49..2a47a60ecc25 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>> struct hci_uart *hu = &qcadev->serdev_hu;
>> struct hci_dev *hdev = hu->hdev;
>> - struct qca_data *qca = hu->priv;
>> const u8 ibs_wake_cmd[] = { 0xFD };
>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>
>> if (qcadev->btsoc_type == QCA_QCA6390) {
>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>> - !test_bit(HCI_RUNNING, &hdev->flags))
>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>> return;
>> + }
>> +
>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>
> Commit msg does not help me at all to understand why you are changing
> the test bits.
it is test bits not changing bits.
>
>> + BT_INFO("QCA do not send EDL_RESET_REQ");
>> + return;
>> + }
>>
>> + BT_INFO("QCA start to send EDL_RESET_REQ");
>
> Why debugging info is part of the fix?
>
they are reserved intentionally to print critical info.
> Best regards,
> Krzysztof
>
On 18/04/2024 22:34, quic_zijuhu wrote:
> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>> On 18/04/2024 16:06, Zijun Hu wrote:
>>> From: Zijun Hu <[email protected]>
>>>
>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>> enabled after disable then warm reboot, fixed by correcting conditions
>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>
>> I have trouble understanding what is the bug. Can you rephrase it?
>>
> Think about below sequences:
> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
> BT is failed to be enabled after warm reboot.
Still not. Please get someone to help you rephrase it. One long sentence
is not good approach. Describe the problem, how it can be reproduced and
then come with brief explanation how you fixed it (because it is not
obvious to me).
>>>
>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>> Reported-by: Wren Turkal <[email protected]>
>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>> Signed-off-by: Zijun Hu <[email protected]>
>>> Tested-by: Wren Turkal <[email protected]>
>>> ---
>>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>> index 160175a23a49..2a47a60ecc25 100644
>>> --- a/drivers/bluetooth/hci_qca.c
>>> +++ b/drivers/bluetooth/hci_qca.c
>>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>> struct hci_uart *hu = &qcadev->serdev_hu;
>>> struct hci_dev *hdev = hu->hdev;
>>> - struct qca_data *qca = hu->priv;
>>> const u8 ibs_wake_cmd[] = { 0xFD };
>>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>
>>> if (qcadev->btsoc_type == QCA_QCA6390) {
>>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>>> - !test_bit(HCI_RUNNING, &hdev->flags))
>>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>> return;
>>> + }
>>> +
>>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>
>> Commit msg does not help me at all to understand why you are changing
>> the test bits.
> it is test bits not changing bits.
Previously we tested bits for BT off and HCI running. Now not, so you
changed the logic. Maybe it is correct, maybe not, I don't understand why.
>>
>>> + BT_INFO("QCA do not send EDL_RESET_REQ");
>>> + return;
>>> + }
>>>
>>> + BT_INFO("QCA start to send EDL_RESET_REQ");
>>
>> Why debugging info is part of the fix?
>>
> they are reserved intentionally to print critical info.
No, it's downstream coding style. Please don't bring it upstream. Or
explain why this deserves informing user. Drivers should be quiet mostly.
Best regards,
Krzysztof
On 4/19/2024 4:58 AM, Krzysztof Kozlowski wrote:
> On 18/04/2024 22:34, quic_zijuhu wrote:
>> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>>> On 18/04/2024 16:06, Zijun Hu wrote:
>>>> From: Zijun Hu <[email protected]>
>>>>
>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>>> enabled after disable then warm reboot, fixed by correcting conditions
>>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>>
>>> I have trouble understanding what is the bug. Can you rephrase it?
>>>
>> Think about below sequences:
>> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
>> BT is failed to be enabled after warm reboot.
>
> Still not. Please get someone to help you rephrase it. One long sentence
> is not good approach. Describe the problem, how it can be reproduced and
> then come with brief explanation how you fixed it (because it is not
> obvious to me).
>
thanks for your suggestions. will optimize commit message based on your suggestions.
>>>>
>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>> Reported-by: Wren Turkal <[email protected]>
>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>> Tested-by: Wren Turkal <[email protected]>
>>>> ---
>>>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index 160175a23a49..2a47a60ecc25 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>>>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>>> struct hci_uart *hu = &qcadev->serdev_hu;
>>>> struct hci_dev *hdev = hu->hdev;
>>>> - struct qca_data *qca = hu->priv;
>>>> const u8 ibs_wake_cmd[] = { 0xFD };
>>>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>>
>>>> if (qcadev->btsoc_type == QCA_QCA6390) {
>>>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>>>> - !test_bit(HCI_RUNNING, &hdev->flags))
>>>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>>> return;
>>>> + }
>>>> +
>>>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>
>>> Commit msg does not help me at all to understand why you are changing
>>> the test bits.
>> it is test bits not changing bits.
>
> Previously we tested bits for BT off and HCI running. Now not, so you
> changed the logic. Maybe it is correct, maybe not, I don't understand why.
>
if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means we can and need to do initialization
by calling hdev->setup() for every BT enable, so we don't need to send VSC to reset controller
here.
if HCI_QUIRK_NON_PERSISTENT_SETUP is cleared. it means we only can or need to do initialization
for the first BT enable operation. if HCI_SETUP is set, that means we don't do any BT enable yet
and the initialization operations are never performed. so we also don't need to send VSC any more.
otherwise, we need to send VSC to reset controller since initialization have been Done regardless of
BT state. for this case the serdev have still be opened. so it also don't meet the crash the 272970be3dab
fixed.
>>>
>>>> + BT_INFO("QCA do not send EDL_RESET_REQ");
>>>> + return;
>>>> + }
>>>>
>>>> + BT_INFO("QCA start to send EDL_RESET_REQ");
>>>
>>> Why debugging info is part of the fix?
>>>
>> they are reserved intentionally to print critical info.
>
> No, it's downstream coding style. Please don't bring it upstream. Or
> explain why this deserves informing user. Drivers should be quiet mostly.
>
>
okay, will remove BT_INFO("QCA start to send EDL_RESET_REQ");
>
> Best regards,
> Krzysztof
>
On 19/04/2024 00:05, quic_zijuhu wrote:
> On 4/19/2024 4:58 AM, Krzysztof Kozlowski wrote:
>> On 18/04/2024 22:34, quic_zijuhu wrote:
>>> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>>>> On 18/04/2024 16:06, Zijun Hu wrote:
>>>>> From: Zijun Hu <[email protected]>
>>>>>
>>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>>>> enabled after disable then warm reboot, fixed by correcting conditions
>>>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>>>
>>>> I have trouble understanding what is the bug. Can you rephrase it?
>>>>
>>> Think about below sequences:
>>> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
>>> BT is failed to be enabled after warm reboot.
>>
>> Still not. Please get someone to help you rephrase it. One long sentence
>> is not good approach. Describe the problem, how it can be reproduced and
>> then come with brief explanation how you fixed it (because it is not
>> obvious to me).
>>
> thanks for your suggestions. will optimize commit message based on your suggestions.
>>>>>
>>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>> ---
>>>>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>> index 160175a23a49..2a47a60ecc25 100644
>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>>>>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>>>> struct hci_uart *hu = &qcadev->serdev_hu;
>>>>> struct hci_dev *hdev = hu->hdev;
>>>>> - struct qca_data *qca = hu->priv;
>>>>> const u8 ibs_wake_cmd[] = { 0xFD };
>>>>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>>>
>>>>> if (qcadev->btsoc_type == QCA_QCA6390) {
>>>>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>>>>> - !test_bit(HCI_RUNNING, &hdev->flags))
>>>>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>>>> return;
>>>>> + }
>>>>> +
>>>>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>>
>>>> Commit msg does not help me at all to understand why you are changing
>>>> the test bits.
>>> it is test bits not changing bits.
>>
>> Previously we tested bits for BT off and HCI running. Now not, so you
>> changed the logic. Maybe it is correct, maybe not, I don't understand why.
>>
> if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means we can and need to do initialization
> by calling hdev->setup() for every BT enable, so we don't need to send VSC to reset controller
> here.
>
> if HCI_QUIRK_NON_PERSISTENT_SETUP is cleared. it means we only can or need to do initialization
> for the first BT enable operation. if HCI_SETUP is set, that means we don't do any BT enable yet
> and the initialization operations are never performed. so we also don't need to send VSC any more.
>
> otherwise, we need to send VSC to reset controller since initialization have been Done regardless of
> BT state. for this case the serdev have still be opened. so it also don't meet the crash the 272970be3dab
> fixed.
Please read again what I request here: your change is not obvious and is
not explained in commit msg.
Best regards,
Krzysztof
On 4/19/2024 6:38 AM, Krzysztof Kozlowski wrote:
> On 19/04/2024 00:05, quic_zijuhu wrote:
>> On 4/19/2024 4:58 AM, Krzysztof Kozlowski wrote:
>>> On 18/04/2024 22:34, quic_zijuhu wrote:
>>>> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>>>>> On 18/04/2024 16:06, Zijun Hu wrote:
>>>>>> From: Zijun Hu <[email protected]>
>>>>>>
>>>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>>>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>>>>> enabled after disable then warm reboot, fixed by correcting conditions
>>>>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>>>>
>>>>> I have trouble understanding what is the bug. Can you rephrase it?
>>>>>
>>>> Think about below sequences:
>>>> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
>>>> BT is failed to be enabled after warm reboot.
>>>
>>> Still not. Please get someone to help you rephrase it. One long sentence
>>> is not good approach. Describe the problem, how it can be reproduced and
>>> then come with brief explanation how you fixed it (because it is not
>>> obvious to me).
>>>
>> thanks for your suggestions. will optimize commit message based on your suggestions.
>>>>>>
>>>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>> ---
>>>>>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index 160175a23a49..2a47a60ecc25 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -2437,15 +2437,21 @@ static void qca_serdev_shutdown(struct device *dev)
>>>>>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>>>>> struct hci_uart *hu = &qcadev->serdev_hu;
>>>>>> struct hci_dev *hdev = hu->hdev;
>>>>>> - struct qca_data *qca = hu->priv;
>>>>>> const u8 ibs_wake_cmd[] = { 0xFD };
>>>>>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>>>>
>>>>>> if (qcadev->btsoc_type == QCA_QCA6390) {
>>>>>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>>>>>> - !test_bit(HCI_RUNNING, &hdev->flags))
>>>>>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks)) {
>>>>>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>>>>> return;
>>>>>> + }
>>>>>> +
>>>>>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>>>
>>>>> Commit msg does not help me at all to understand why you are changing
>>>>> the test bits.
>>>> it is test bits not changing bits.
>>>
>>> Previously we tested bits for BT off and HCI running. Now not, so you
>>> changed the logic. Maybe it is correct, maybe not, I don't understand why.
>>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means we can and need to do initialization
>> by calling hdev->setup() for every BT enable, so we don't need to send VSC to reset controller
>> here.
>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is cleared. it means we only can or need to do initialization
>> for the first BT enable operation. if HCI_SETUP is set, that means we don't do any BT enable yet
>> and the initialization operations are never performed. so we also don't need to send VSC any more.
>>
>> otherwise, we need to send VSC to reset controller since initialization have been Done regardless of
>> BT state. for this case the serdev have still be opened. so it also don't meet the crash the 272970be3dab
>> fixed.
>
> Please read again what I request here: your change is not obvious and is
> not explained in commit msg.
>
>
i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and HCI_SETUP is generic flag.
>
>
> Best regards,
> Krzysztof
>
This patch series are to fix below 2 regression issues for QCA controllers
1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
the links for these issues are shown below:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Zijun Hu (1):
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
drivers/bluetooth/hci_qca.c | 3 ++-
net/bluetooth/mgmt.c | 20 +++++++++++++++-----
2 files changed, 17 insertions(+), 6 deletions(-)
--
2.7.4
From: Luiz Augusto von Dentz <[email protected]>
These commands don't require the adapter to be up and running so don't
use hci_cmd_sync_queue which would check that flag, instead use
hci_cmd_sync_submit which would ensure mgmt_class_complete is set
properly regardless if any command was actually run or not.
Link: https://github.com/bluez/bluez/issues/809
Fixes: d883a4669a1d ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
net/bluetooth/mgmt.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 7129e70a0253..68deec968405 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2623,7 +2623,11 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
goto failed;
}
- err = hci_cmd_sync_queue(hdev, add_uuid_sync, cmd, mgmt_class_complete);
+ /* MGMT_OP_ADD_UUID don't require adapter the UP/Running so use
+ * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
+ */
+ err = hci_cmd_sync_submit(hdev, add_uuid_sync, cmd,
+ mgmt_class_complete);
if (err < 0) {
mgmt_pending_free(cmd);
goto failed;
@@ -2717,8 +2721,11 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- err = hci_cmd_sync_queue(hdev, remove_uuid_sync, cmd,
- mgmt_class_complete);
+ /* MGMT_OP_REMOVE_UUID don't require adapter the UP/Running so use
+ * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
+ */
+ err = hci_cmd_sync_submit(hdev, remove_uuid_sync, cmd,
+ mgmt_class_complete);
if (err < 0)
mgmt_pending_free(cmd);
@@ -2784,8 +2791,11 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
goto unlock;
}
- err = hci_cmd_sync_queue(hdev, set_class_sync, cmd,
- mgmt_class_complete);
+ /* MGMT_OP_SET_DEV_CLASS don't require adapter the UP/Running so use
+ * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
+ */
+ err = hci_cmd_sync_submit(hdev, set_class_sync, cmd,
+ mgmt_class_complete);
if (err < 0)
mgmt_pending_free(cmd);
--
2.7.4
This patch series are to fix below 2 regression issues for QCA controllers
1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
the links for these issues are shown below:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Zijun Hu (2):
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
then warm reboot
drivers/bluetooth/hci_qca.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
--
2.7.4
Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
with gpiod_get_optional()") will cause below serious regression issue:
BT can't be enabled any more after below steps:
cold boot -> enable BT -> disable BT -> BT enable failure
if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
The mentioned commit wrongly set quirk HCI_QUIRK_NON_PERSISTENT_SETUP
within qca_serdev_probe() for this case and cause this serious issue.
Fixed by reverting the mentioned commit for QCA_QCA6390.
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reported-by: Wren Turkal <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
drivers/bluetooth/hci_qca.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..0934e74112a6 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2357,7 +2357,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
if (IS_ERR(qcadev->bt_en)) {
dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
power_ctrl_enabled = false;
- }
+ } else if (!qcadev->bt_en && qcadev->btsoc_type == QCA_QCA6390)
+ power_ctrl_enabled = false;
qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
if (IS_ERR(qcadev->susclk)) {
--
2.7.4
From: Zijun Hu <[email protected]>
Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev") will cause below regression issue:
BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
once BT was ever enabled.
Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
Reported-by: Wren Turkal <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
drivers/bluetooth/hci_qca.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 0934e74112a6..3f5173f1180b 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2438,13 +2438,12 @@ static void qca_serdev_shutdown(struct device *dev)
struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
struct hci_uart *hu = &qcadev->serdev_hu;
struct hci_dev *hdev = hu->hdev;
- struct qca_data *qca = hu->priv;
const u8 ibs_wake_cmd[] = { 0xFD };
const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
if (qcadev->btsoc_type == QCA_QCA6390) {
- if (test_bit(QCA_BT_OFF, &qca->flags) ||
- !test_bit(HCI_RUNNING, &hdev->flags))
+ if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) ||
+ hci_dev_test_flag(hdev, HCI_SETUP))
return;
serdev_device_write_flush(serdev);
--
2.7.4
On 4/20/2024 5:48 AM, Zijun Hu wrote:
> From: Luiz Augusto von Dentz <[email protected]>
>
> These commands don't require the adapter to be up and running so don't
> use hci_cmd_sync_queue which would check that flag, instead use
> hci_cmd_sync_submit which would ensure mgmt_class_complete is set
> properly regardless if any command was actually run or not.
>
> Link: https://github.com/bluez/bluez/issues/809
> Fixes: d883a4669a1d ("Bluetooth: hci_sync: Only allow hci_cmd_sync_queue if running")
> Signed-off-by: Luiz Augusto von Dentz <[email protected]>
> ---
> net/bluetooth/mgmt.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 7129e70a0253..68deec968405 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2623,7 +2623,11 @@ static int add_uuid(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> goto failed;
> }
>
> - err = hci_cmd_sync_queue(hdev, add_uuid_sync, cmd, mgmt_class_complete);
> + /* MGMT_OP_ADD_UUID don't require adapter the UP/Running so use
> + * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
> + */
> + err = hci_cmd_sync_submit(hdev, add_uuid_sync, cmd,
> + mgmt_class_complete);
> if (err < 0) {
> mgmt_pending_free(cmd);
> goto failed;
> @@ -2717,8 +2721,11 @@ static int remove_uuid(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - err = hci_cmd_sync_queue(hdev, remove_uuid_sync, cmd,
> - mgmt_class_complete);
> + /* MGMT_OP_REMOVE_UUID don't require adapter the UP/Running so use
> + * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
> + */
> + err = hci_cmd_sync_submit(hdev, remove_uuid_sync, cmd,
> + mgmt_class_complete);
> if (err < 0)
> mgmt_pending_free(cmd);
>
> @@ -2784,8 +2791,11 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> - err = hci_cmd_sync_queue(hdev, set_class_sync, cmd,
> - mgmt_class_complete);
> + /* MGMT_OP_SET_DEV_CLASS don't require adapter the UP/Running so use
> + * hci_cmd_sync_submit instead of hci_cmd_sync_queue.
> + */
> + err = hci_cmd_sync_submit(hdev, set_class_sync, cmd,
> + mgmt_class_complete);
> if (err < 0)
> mgmt_pending_free(cmd);
>
sorry to send wrong patch sets please ignore it.
This is an automated email and please do not reply to this email.
Dear Submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.
----- Output -----
error: patch failed: net/bluetooth/mgmt.c:2623
error: net/bluetooth/mgmt.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=846241
---Test result---
Test Summary:
CheckPatch FAIL 1.55 seconds
GitLint FAIL 1.15 seconds
SubjectPrefix PASS 0.24 seconds
BuildKernel PASS 29.65 seconds
CheckAllWarning PASS 32.54 seconds
CheckSparse PASS 37.97 seconds
CheckSmatch FAIL 35.74 seconds
BuildKernel32 PASS 28.71 seconds
TestRunnerSetup PASS 515.72 seconds
TestRunner_l2cap-tester PASS 18.28 seconds
TestRunner_iso-tester PASS 28.86 seconds
TestRunner_bnep-tester PASS 4.67 seconds
TestRunner_mgmt-tester FAIL 108.91 seconds
TestRunner_rfcomm-tester PASS 7.24 seconds
TestRunner_sco-tester PASS 14.94 seconds
TestRunner_ioctl-tester PASS 7.64 seconds
TestRunner_mesh-tester PASS 5.74 seconds
TestRunner_smp-tester PASS 6.80 seconds
TestRunner_userchan-tester PASS 4.87 seconds
IncrementalBuild PASS 33.04 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v3,1/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#112:
Reported-by: Wren Turkal <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
total: 0 errors, 1 warnings, 9 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13636786.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v3,1/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
17: B1 Line exceeds max length (139>80): "Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
[v3,2/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable then warm reboot
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (93>80): "[v3,2/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable then warm reboot"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2
Failed Test Cases
LL Privacy - Add Device 7 (AL is full) Failed 0.191 seconds
---
Regards,
Linux Bluetooth
On 20/04/2024 00:03, Zijun Hu wrote:
> This patch series are to fix below 2 regression issues for QCA controllers
> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>
> the links for these issues are shown below:
> https://bugzilla.kernel.org/show_bug.cgi?id=218726
> https://lore.kernel.org/linux-bluetooth/ea20
Provide changelog, either in cover letter or in individual patches under
---.
Do not attach (thread) your patchsets to some other threads (unrelated
or older versions). This buries them deep in the mailbox and might
interfere with applying entire sets.
Best regards,
Krzysztof
On 20/04/2024 00:03, Zijun Hu wrote:
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()") will cause below serious regression issue:
>
> BT can't be enabled any more after below steps:
> cold boot -> enable BT -> disable BT -> BT enable failure
> if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
>
> The mentioned commit wrongly set quirk HCI_QUIRK_NON_PERSISTENT_SETUP
> within qca_serdev_probe() for this case and cause this serious issue.
>
> Fixed by reverting the mentioned commit for QCA_QCA6390.
>
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reported-by: Wren Turkal <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <[email protected]>
> Tested-by: Wren Turkal <[email protected]>
> ---
> drivers/bluetooth/hci_qca.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..0934e74112a6 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2357,7 +2357,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> if (IS_ERR(qcadev->bt_en)) {
> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> power_ctrl_enabled = false;
> - }
> + } else if (!qcadev->bt_en && qcadev->btsoc_type == QCA_QCA6390)
> + power_ctrl_enabled = false;
>
Please use kernel coding style, so {}.
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.
Best regards,
Krzysztof
On 20/04/2024 00:03, Zijun Hu wrote:
> From: Zijun Hu <[email protected]>
>
> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev") will cause below regression issue:
>
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
> if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
Please mention if QCA6390 requires reset pin, according to datasheet or
some hardware guideline.
>
> Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
> once BT was ever enabled.
>
> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
> Reported-by: Wren Turkal <[email protected]>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Signed-off-by: Zijun Hu <[email protected]>
> Tested-by: Wren Turkal <[email protected]>
> ---
> drivers/bluetooth/hci_qca.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 0934e74112a6..3f5173f1180b 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2438,13 +2438,12 @@ static void qca_serdev_shutdown(struct device *dev)
> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
> struct hci_uart *hu = &qcadev->serdev_hu;
> struct hci_dev *hdev = hu->hdev;
> - struct qca_data *qca = hu->priv;
> const u8 ibs_wake_cmd[] = { 0xFD };
> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>
> if (qcadev->btsoc_type == QCA_QCA6390) {
> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
> - !test_bit(HCI_RUNNING, &hdev->flags))
> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) ||
> + hci_dev_test_flag(hdev, HCI_SETUP))
> return;
I am sorry, but why do we need to perform shutdown procedure now if
device is off?
I raised questions about this and I still don't understand. Not much got
better comparing to previous version. Actually, I have no clue what
changed. Where is the changelog?
Best regards,
Krzysztof
On 4/20/2024 7:03 PM, Krzysztof Kozlowski wrote:
> On 20/04/2024 00:03, Zijun Hu wrote:
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()") will cause below serious regression issue:
>>
>> BT can't be enabled any more after below steps:
>> cold boot -> enable BT -> disable BT -> BT enable failure
>> if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
>>
>> The mentioned commit wrongly set quirk HCI_QUIRK_NON_PERSISTENT_SETUP
>> within qca_serdev_probe() for this case and cause this serious issue.
>>
>> Fixed by reverting the mentioned commit for QCA_QCA6390.
>>
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Reported-by: Wren Turkal <[email protected]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <[email protected]>
>> Tested-by: Wren Turkal <[email protected]>
>> ---
>> drivers/bluetooth/hci_qca.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..0934e74112a6 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2357,7 +2357,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>> if (IS_ERR(qcadev->bt_en)) {
>> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> power_ctrl_enabled = false;
>> - }
>> + } else if (!qcadev->bt_en && qcadev->btsoc_type == QCA_QCA6390)
>> + power_ctrl_enabled = false;
>>
>
> Please use kernel coding style, so {}.
>
i will correct based on your suggest even if "checkpatch.pl --strict"
don't give any warnings for present code style.
> Please run scripts/checkpatch.pl and fix reported warnings. Then please
> run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
> Some warnings can be ignored, especially from --strict run, but the code
> here looks like it needs a fix. Feel free to get in touch if the warning
> is not clear.
> the only warnings is that
"WARNING: Reported-by: should be immediately followed by Closes:"
we don't need to handle this warning for this case.
> Best regards,
> Krzysztof
>
>
On 4/20/2024 7:07 PM, Krzysztof Kozlowski wrote:
> On 20/04/2024 00:03, Zijun Hu wrote:
>> From: Zijun Hu <[email protected]>
>>
>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>> serdev") will cause below regression issue:
>>
>> BT can't be enabled after below steps:
>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
>> if BT reset pin is not configured within DT|ACPI for QCA_QCA6390.
>
> Please mention if QCA6390 requires reset pin, according to datasheet or
> some hardware guideline.
>
will update commit message.
>>
>> Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
>> once BT was ever enabled.
>>
>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>> Reported-by: Wren Turkal <[email protected]>
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Signed-off-by: Zijun Hu <[email protected]>
>> Tested-by: Wren Turkal <[email protected]>
>> ---
>> drivers/bluetooth/hci_qca.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 0934e74112a6..3f5173f1180b 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2438,13 +2438,12 @@ static void qca_serdev_shutdown(struct device *dev)
>> struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>> struct hci_uart *hu = &qcadev->serdev_hu;
>> struct hci_dev *hdev = hu->hdev;
>> - struct qca_data *qca = hu->priv;
>> const u8 ibs_wake_cmd[] = { 0xFD };
>> const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>
>> if (qcadev->btsoc_type == QCA_QCA6390) {
>> - if (test_bit(QCA_BT_OFF, &qca->flags) ||
>> - !test_bit(HCI_RUNNING, &hdev->flags))
>> + if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) ||
>> + hci_dev_test_flag(hdev, HCI_SETUP))
>> return;
>
> I am sorry, but why do we need to perform shutdown procedure now if
> device is off?
>
this is shutdown of serdev and not hdev's shutdown.
> I raised questions about this and I still don't understand. Not much got
> better comparing to previous version. Actually, I have no clue what
> changed. Where is the changelog?
>
my reply at below link will help you
https://lore.kernel.org/all/[email protected]/
>
> Best regards,
> Krzysztof
>
This patch series are to fix below 2 regression issues for QCA controllers
1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
the links for these issues are shown below:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Changes:
V3 -> V4: Correct code stype and commit message
V2 -> V3: Wrong patch sets are sent
V1 -> V2: Remove debugging logs
Zijun Hu (2):
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
then warm reboot
drivers/bluetooth/hci_qca.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
2.7.4
From: Zijun Hu <[email protected]>
Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev") will cause below regression issue:
BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA_QCA6390.
Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
once BT was ever enabled.
Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
Reported-by: Wren Turkal <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
drivers/bluetooth/hci_qca.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 4079254fb1c8..fc027da98297 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2439,13 +2439,12 @@ static void qca_serdev_shutdown(struct device *dev)
struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
struct hci_uart *hu = &qcadev->serdev_hu;
struct hci_dev *hdev = hu->hdev;
- struct qca_data *qca = hu->priv;
const u8 ibs_wake_cmd[] = { 0xFD };
const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
if (qcadev->btsoc_type == QCA_QCA6390) {
- if (test_bit(QCA_BT_OFF, &qca->flags) ||
- !test_bit(HCI_RUNNING, &hdev->flags))
+ if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) ||
+ hci_dev_test_flag(hdev, HCI_SETUP))
return;
serdev_device_write_flush(serdev);
--
2.7.4
Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
with gpiod_get_optional()") will cause below serious regression issue:
BT can't be enabled any more after below steps:
cold boot -> enable BT -> disable BT -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA_QCA6390.
The mentioned commit wrongly set quirk HCI_QUIRK_NON_PERSISTENT_SETUP
within qca_serdev_probe() for this case and cause this serious issue.
Fixed by reverting the mentioned commit for QCA_QCA6390.
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reported-by: Wren Turkal <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
drivers/bluetooth/hci_qca.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..4079254fb1c8 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2357,6 +2357,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
if (IS_ERR(qcadev->bt_en)) {
dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
power_ctrl_enabled = false;
+ } else if (!qcadev->bt_en && qcadev->btsoc_type == QCA_QCA6390) {
+ power_ctrl_enabled = false;
}
qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
--
2.7.4
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=846372
---Test result---
Test Summary:
CheckPatch FAIL 1.49 seconds
GitLint FAIL 0.79 seconds
SubjectPrefix PASS 0.23 seconds
BuildKernel PASS 29.58 seconds
CheckAllWarning PASS 32.85 seconds
CheckSparse PASS 38.06 seconds
CheckSmatch FAIL 34.71 seconds
BuildKernel32 PASS 28.63 seconds
TestRunnerSetup PASS 516.90 seconds
TestRunner_l2cap-tester PASS 18.30 seconds
TestRunner_iso-tester PASS 28.64 seconds
TestRunner_bnep-tester PASS 4.65 seconds
TestRunner_mgmt-tester PASS 109.46 seconds
TestRunner_rfcomm-tester PASS 7.21 seconds
TestRunner_sco-tester PASS 14.88 seconds
TestRunner_ioctl-tester PASS 7.57 seconds
TestRunner_mesh-tester PASS 5.74 seconds
TestRunner_smp-tester PASS 6.74 seconds
TestRunner_userchan-tester PASS 4.82 seconds
IncrementalBuild PASS 32.49 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v4,1/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#112:
Reported-by: Wren Turkal <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
total: 0 errors, 1 warnings, 8 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/src/13637223.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v4,1/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
17: B1 Line exceeds max length (139>80): "Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
[v4,2/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable then warm reboot
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (93>80): "[v4,2/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable then warm reboot"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
---
Regards,
Linux Bluetooth
On 4/20/24 3:06 PM, Zijun Hu wrote:
> This patch series are to fix below 2 regression issues for QCA controllers
> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
@Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
to ensure they fix the issues I reported?
> the links for these issues are shown below:
> https://bugzilla.kernel.org/show_bug.cgi?id=218726
> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>
> Changes:
> V3 -> V4: Correct code stype and commit message
> V2 -> V3: Wrong patch sets are sent
> V1 -> V2: Remove debugging logs
>
> Zijun Hu (2):
> Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
> Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
> then warm reboot
>
> drivers/bluetooth/hci_qca.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
--
You're more amazing than you think!
On 4/21/2024 3:44 PM, Wren Turkal wrote:
> On 4/20/24 3:06 PM, Zijun Hu wrote:
>> This patch series are to fix below 2 regression issues for QCA
>> controllers
>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>
> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
> to ensure they fix the issues I reported?
>
Hi Wren,
for QCA6390. this updated patch sets is the same as the patch sets you
ever tested.
sure. if you would like to test this one.
>> the links for these issues are shown below:
>> https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>
>> Changes:
>> V3 -> V4: Correct code stype and commit message
>> V2 -> V3: Wrong patch sets are sent
>> V1 -> V2: Remove debugging logs
>>
>> Zijun Hu (2):
>> Â Â Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
>> Â Â Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
>> Â Â Â Â then warm reboot
>>
>> Â drivers/bluetooth/hci_qca.c | 7 ++++---
>> Â 1 file changed, 4 insertions(+), 3 deletions(-)
>
>
On 21/04/2024 00:06, Zijun Hu wrote:
> This patch series are to fix below 2 regression issues for QCA controllers
> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>
> the links for these issues are shown below:
> https://bugzilla.kernel.org/show_bug.cgi?id=218726
> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>
> Changes:
> V3 -> V4: Correct code stype and commit message
> V2 -> V3: Wrong patch sets are sent
Didn't you got comment not to attach your postings to some other
threads? Each posting is a separate thread.
Best regards,
Krzysztof
On 21/04/2024 09:44, Wren Turkal wrote:
> On 4/20/24 3:06 PM, Zijun Hu wrote:
>> This patch series are to fix below 2 regression issues for QCA controllers
>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>
> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
> to ensure they fix the issues I reported?
>
I look forward to someone testing these on other hardware, not yours. On
the hardware where the original issues were happening leading to this
changes, e.g. RB5.
Anyway, the problem here is poor explanation of the problem which did
not improve in v3 and v4. Instead I receive explanations like:
"this is shutdown of serdev and not hdev's shutdown."
Not related...
"now. you understood why your merged change as shown link of 4) have
problems and introduced our discussed issue, right?"
No. I did not understand and I feel I am wasting here time.
Code could be correct, could be wrong. Especially second patch looks
suspicious. But the way Zijun Hu explains it and the way Zijun Hu
responds is not helping at all.
Sorry, with such replies to review, it is not worth my time.
Best regards,
Krzysztof
On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote:
> On 21/04/2024 09:44, Wren Turkal wrote:
>> On 4/20/24 3:06 PM, Zijun Hu wrote:
>>> This patch series are to fix below 2 regression issues for QCA controllers
>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>
>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
>> to ensure they fix the issues I reported?
>>
>
> I look forward to someone testing these on other hardware, not yours. On
> the hardware where the original issues were happening leading to this
> changes, e.g. RB5.
>
> Anyway, the problem here is poor explanation of the problem which did
> not improve in v3 and v4. Instead I receive explanations like:
>
> "this is shutdown of serdev and not hdev's shutdown."
> Not related...
>
this is the reply for secondary issue. i believe i have given much
explain for my fix for the 2nd issue as shown by below links.
let me add a bit more explanation within the ending "For the 2nd issue"
section, supposed you known much for generic flag
HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the
quirk. also supposed you see commit history to find why
qca_serdev_shutdown() was introduced for QCA6390.
https://lore.kernel.org/all/[email protected]/
> "now. you understood why your merged change as shown link of 4) have
> problems and introduced our discussed issue, right?"
>
this is the reply for the first issue as shown by below link. it almost
have the same description as the following "For 1st issue:" section.
i believe it have clear illustration why the commit have bugs.
https://lore.kernel.org/all/[email protected]/
> No. I did not understand and I feel I am wasting here time.
> > Code could be correct, could be wrong. Especially second patch looks
> suspicious. But the way Zijun Hu explains it and the way Zijun Hu
> responds is not helping at all.
>
> Sorry, with such replies to review, it is not worth my time.
>
> Best regards,
> Krzysztof
>
Hi luiz,marcel
it is time for me to request you give comments for our discussion
and for my fixes, Let me explain the 1st issue then 2nd one.
For 1st issue:
1) the following commit will cause serious regression issue for QCA
controllers, and it has been merged with linus's mainline kernel.
Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
with gpiod_get_optional()").
2) the regression issue is described by [PATCH v4 1/2] commit message
as following:
BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
failure if property enable-gpios is not configured within DT|ACPI for
QCA_QCA6390.
i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted.
3) let me explain the bug point for commit mentioned by 1), its
commit message and bug change applet are shown below.
The optional variants for the gpiod_get() family of functions return
NULL if the GPIO in question is not associated with this device. They
return ERR_PTR() on any other error. NULL descriptors are graciously
handled by GPIOLIB and can be safely passed to any of the GPIO consumer
interfaces as they will return 0 and act as if the function succeeded.
If one is using the optional variant, then there's no point in checking
for NULL.
qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
GPIOD_OUT_LOW);
- if (IS_ERR_OR_NULL(qcadev->bt_en)) {
+ if (IS_ERR(qcadev->bt_en)) {
dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
power_ctrl_enabled = false;
}
3.1) we only need to discuss how to handle case "qcadev->bt_en ==
NULL" since this is only difference between the commit and BT original
design.
3.2) BT original design are agree with the point of above commit
message that case "qcadev->bt_en == NULL" should not be treated as
error, so BT original design does not do error return for the case and
use dev_warn() instead of dev_err() to give.
3.3) the commit misunderstands BT original design and wrongly think
BT original design take "qcadev->bt_en == NULL" as error case,
so change the following flag power_ctrl_enabled set logic and cause
discussed issue.
For the 2nd issue:
1) the following commit will cause below regression issue for QCA_QCA6390.
Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev")
2) the regression issue is described by [PATCH v4 2/2] commit message
as following:
BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
failure if property enable-gpios is not configured within DT|ACPI for
QCA_QCA6390.
3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
it should not and also never get chance to be invoked even if BT is
disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
to reset controller during warm reset phase of above 2) steps.
On 4/21/24 5:14 PM, quic_zijuhu wrote:
> On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote:
>> On 21/04/2024 09:44, Wren Turkal wrote:
>>> On 4/20/24 3:06 PM, Zijun Hu wrote:
>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>
>>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
>>> to ensure they fix the issues I reported?
>>>
>>
>> I look forward to someone testing these on other hardware, not yours. On
>> the hardware where the original issues were happening leading to this
>> changes, e.g. RB5.
>>
>> Anyway, the problem here is poor explanation of the problem which did
>> not improve in v3 and v4. Instead I receive explanations like:
>>
>> "this is shutdown of serdev and not hdev's shutdown."
>> Not related...
>>
> this is the reply for secondary issue. i believe i have given much
> explain for my fix for the 2nd issue as shown by below links.
> let me add a bit more explanation within the ending "For the 2nd issue"
> section, supposed you known much for generic flag
> HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the
> quirk. also supposed you see commit history to find why
> qca_serdev_shutdown() was introduced for QCA6390.
> https://lore.kernel.org/all/[email protected]/
>> "now. you understood why your merged change as shown link of 4) have
>> problems and introduced our discussed issue, right?"
>>
> this is the reply for the first issue as shown by below link. it almost
> have the same description as the following "For 1st issue:" section.
> i believe it have clear illustration why the commit have bugs.
> https://lore.kernel.org/all/[email protected]/
>> No. I did not understand and I feel I am wasting here time.
>>> Code could be correct, could be wrong. Especially second patch looks
>> suspicious. But the way Zijun Hu explains it and the way Zijun Hu
>> responds is not helping at all.
>>
>> Sorry, with such replies to review, it is not worth my time.
>>
>> Best regards,
>> Krzysztof
>>
> Hi luiz,marcel
>
> it is time for me to request you give comments for our discussion
> and for my fixes, Let me explain the 1st issue then 2nd one.
>
> For 1st issue:
> 1) the following commit will cause serious regression issue for QCA
> controllers, and it has been merged with linus's mainline kernel.
>
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()").
As the user who originally reported thes issue, I can confirm this. I
was introduced to this regression because I use Fedora Rawhide on my
laptop, which builds and pushes kernels based on mainline very regularly.
Here is my description of the regression: After the reverted change, the
BT hardware in my laptop (qca6390) will only work after a cold boot when
the hardware has only be enabled once by the driver. Once the hardware
is enabled, the process of disabling/re-enabling fails. Also, the
hardware cannot be enabled after a warm boot of the laptop.
Among other things, this makes logging into KDE Plasma break my
bluetooth mouse. The cause of this breakage appears to be that Plasma
disables/re-enables bluetooth hardware upon login.
GNOME operates slightly less badly in that bluetooth stays enabled.
However, if I manually disable the bluetooth via the ui or by restarting
the bluetooth service with systemctl, the mouse fails in the same way as
happens with Plasma.
Once the bluetooth has failed, the only way to fix is a cold boot and
only enable the hardware once. I cannot remove the modules (btqca,
hci_uart, and bluetooth) and re-modprobe them to fix it. I can't restart
the bluetooth service. I can't do both of those things. I haven't found
any way to re-enable the hardware beyond cold boot with bluetooth
service enabled.
If I disable the bluetooth service and cold boot the laptop, there also
appears to be some kind of race condition as not enabling bluetooth
service very soon after loading the hci_uart and btqca modules during
boot puts the system in a state where I can never enable bluetooth. I do
not know what causes this specifically, but my theory is that not
starting the bluetooth service immediately puts the driver in a similar
state as when the service is started immediately. Maybe some kind of
lazy initialization that is forced to happen more quickly when the
bluetooth service is enabled?
Any way, this reversion by itself (which I manually did after a
discussion with Zijun before getting his test patches applying to my
kernel for test). However, this reversion did not get the hardware
working after a warm boot.
> 2) the regression issue is described by [PATCH v4 1/2] commit message
> as following:
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
> failure if property enable-gpios is not configured within DT|ACPI for
> QCA_QCA6390.
> i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted.
I can confirm this. Without this change (and with the #1 change), I can
cold boot the laptop and disable/re-enable the hardware as many times as
I want. However, warm booting will not allow the hardware to work. I
believe that a similar problem existed before the 6.8 kernel (if memory
serves), as I had been having issues of this sort for some time. I was
able to reproduce a similar issue as far back as 5.19. I tested that and
every intervening release until 6.8.0. I did not realize that the warm
boot problem was separate from the enable/disable issue until working
with Zijun.
> 3) let me explain the bug point for commit mentioned by 1), its
> commit message and bug change applet are shown below.
>
> The optional variants for the gpiod_get() family of functions return
> NULL if the GPIO in question is not associated with this device. They
> return ERR_PTR() on any other error. NULL descriptors are graciously
> handled by GPIOLIB and can be safely passed to any of the GPIO consumer
> interfaces as they will return 0 and act as if the function succeeded.
> If one is using the optional variant, then there's no point in checking
> for NULL.
>
> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> GPIOD_OUT_LOW);
> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
> + if (IS_ERR(qcadev->bt_en)) {
> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> power_ctrl_enabled = false;
> }
> 3.1) we only need to discuss how to handle case "qcadev->bt_en ==
> NULL" since this is only difference between the commit and BT original
> design.
> 3.2) BT original design are agree with the point of above commit
> message that case "qcadev->bt_en == NULL" should not be treated as
> error, so BT original design does not do error return for the case and
> use dev_warn() instead of dev_err() to give.
> 3.3) the commit misunderstands BT original design and wrongly think
> BT original design take "qcadev->bt_en == NULL" as error case,
> so change the following flag power_ctrl_enabled set logic and cause
> discussed issue.
>
> For the 2nd issue:
> 1) the following commit will cause below regression issue for QCA_QCA6390.
> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev")
>
> 2) the regression issue is described by [PATCH v4 2/2] commit message
> as following:
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
> failure if property enable-gpios is not configured within DT|ACPI for
> QCA_QCA6390.
>
> 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
> it should not and also never get chance to be invoked even if BT is
> disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
> to reset controller during warm reset phase of above 2) steps.
It was Zijun who realized that #1 and #2 these were two separate but
related issues. He really dug in and found the problem and produced test
patches. It was impressive, and he should be given credit for finding
that these were the issues so quickly.
The only reason I'm involved here is that I am squeaky wheel that
happened to be running Rawhide and got hurt by the kernel. I am a
glorified beta tester who got unlucky, and I was hoping the find help in
the kernel community. Zijun stepped up.
The only other thing that I am wondering about this patch set is if
Zijun or some other party should be listed as the maintainer of the
btqca module and hci_qca.c and btqca.* files so that they can be found
more easily with the get_maintainer.pl script.
wt
--
You're more amazing than you think!
On 22/04/2024 02:14, quic_zijuhu wrote:
> On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote:
>> On 21/04/2024 09:44, Wren Turkal wrote:
>>> On 4/20/24 3:06 PM, Zijun Hu wrote:
>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>
>>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
>>> to ensure they fix the issues I reported?
>>>
>>
>> I look forward to someone testing these on other hardware, not yours. On
>> the hardware where the original issues were happening leading to this
>> changes, e.g. RB5.
>>
>> Anyway, the problem here is poor explanation of the problem which did
>> not improve in v3 and v4. Instead I receive explanations like:
>>
>> "this is shutdown of serdev and not hdev's shutdown."
>> Not related...
>>
> this is the reply for secondary issue. i believe i have given much
> explain for my fix for the 2nd issue as shown by below links.
No, you did not.
> let me add a bit more explanation within the ending "For the 2nd issue"
> section, supposed you known much for generic flag
> HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the
> quirk. also supposed you see commit history to find why
> qca_serdev_shutdown() was introduced for QCA6390.
> https://lore.kernel.org/all/[email protected]/
You did not answer my questions.
Let's quote:
"i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and
HCI_SETUP is generic flag."
Srsly, what is such answer?
>> "now. you understood why your merged change as shown link of 4) have
>> problems and introduced our discussed issue, right?"
>>
> this is the reply for the first issue as shown by below link. it almost
> have the same description as the following "For 1st issue:" section.
> i believe it have clear illustration why the commit have bugs.
> https://lore.kernel.org/all/[email protected]/
>> No. I did not understand and I feel I am wasting here time.
>>> Code could be correct, could be wrong. Especially second patch looks
>> suspicious. But the way Zijun Hu explains it and the way Zijun Hu
>> responds is not helping at all.
>>
>> Sorry, with such replies to review, it is not worth my time.
>>
>> Best regards,
>> Krzysztof
>>
> Hi luiz,marcel
>
> it is time for me to request you give comments for our discussion
> and for my fixes, Let me explain the 1st issue then 2nd one.
You keep pushing and pushing even though I stated my remarks.
>
> For 1st issue:
> 1) the following commit will cause serious regression issue for QCA
> controllers, and it has been merged with linus's mainline kernel.
>
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()").
>
> 2) the regression issue is described by [PATCH v4 1/2] commit message
> as following:
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
> failure if property enable-gpios is not configured within DT|ACPI for
> QCA_QCA6390.
> i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted.
>
> 3) let me explain the bug point for commit mentioned by 1), its
> commit message and bug change applet are shown below.
>
> The optional variants for the gpiod_get() family of functions return
> NULL if the GPIO in question is not associated with this device. They
> return ERR_PTR() on any other error. NULL descriptors are graciously
> handled by GPIOLIB and can be safely passed to any of the GPIO consumer
> interfaces as they will return 0 and act as if the function succeeded.
> If one is using the optional variant, then there's no point in checking
> for NULL.
>
> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> GPIOD_OUT_LOW);
> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
> + if (IS_ERR(qcadev->bt_en)) {
> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> power_ctrl_enabled = false;
> }
> 3.1) we only need to discuss how to handle case "qcadev->bt_en ==
> NULL" since this is only difference between the commit and BT original
> design.
> 3.2) BT original design are agree with the point of above commit
> message that case "qcadev->bt_en == NULL" should not be treated as
> error, so BT original design does not do error return for the case and
> use dev_warn() instead of dev_err() to give.
> 3.3) the commit misunderstands BT original design and wrongly think
> BT original design take "qcadev->bt_en == NULL" as error case,
> so change the following flag power_ctrl_enabled set logic and cause
> discussed issue.
>
> For the 2nd issue:
> 1) the following commit will cause below regression issue for QCA_QCA6390.
> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev")
>
> 2) the regression issue is described by [PATCH v4 2/2] commit message
> as following:
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
> failure if property enable-gpios is not configured within DT|ACPI for
> QCA_QCA6390.
You did not address original issue of crash during shutdown and did not
clarify my questions.
>
> 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
> it should not and also never get chance to be invoked even if BT is
> disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
> to reset controller during warm reset phase of above 2) steps.
Anyway, any explanation providing background how you are fixing this
issue while keeping *previous problem fixed* is useful but should be
provided in commit msg. I asked about this two or three times.
BTW, provide here exact kernel version you tested this patches with.
Also the exact hardware.
Best regards,
Krzysztof
Hi Krzysztof,
could you list questions i need to explain within commit message based
on current v4 patch sets ?
let me send v5 patch sets with updated commit messages.
On 4/22/2024 1:52 PM, Krzysztof Kozlowski wrote:
> On 22/04/2024 02:14, quic_zijuhu wrote:
>> On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote:
>>> On 21/04/2024 09:44, Wren Turkal wrote:
>>>> On 4/20/24 3:06 PM, Zijun Hu wrote:
>>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>>
>>>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
>>>> to ensure they fix the issues I reported?
>>>>
>>>
>>> I look forward to someone testing these on other hardware, not yours. On
>>> the hardware where the original issues were happening leading to this
>>> changes, e.g. RB5.
>>>
>>> Anyway, the problem here is poor explanation of the problem which did
>>> not improve in v3 and v4. Instead I receive explanations like:
>>>
>>> "this is shutdown of serdev and not hdev's shutdown."
>>> Not related...
>>>
>> this is the reply for secondary issue. i believe i have given much
>> explain for my fix for the 2nd issue as shown by below links.
>
> No, you did not.
>
>> let me add a bit more explanation within the ending "For the 2nd issue"
>> section, supposed you known much for generic flag
>> HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the
>> quirk. also supposed you see commit history to find why
>> qca_serdev_shutdown() was introduced for QCA6390.
>> https://lore.kernel.org/all/[email protected]/
>
> You did not answer my questions.
>
> Let's quote:
>
> "i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and
> HCI_SETUP is generic flag."
>
> Srsly, what is such answer?
>
>
>
>
>
>>> "now. you understood why your merged change as shown link of 4) have
>>> problems and introduced our discussed issue, right?"
>>>
>> this is the reply for the first issue as shown by below link. it almost
>> have the same description as the following "For 1st issue:" section.
>> i believe it have clear illustration why the commit have bugs.
>> https://lore.kernel.org/all/[email protected]/
>>> No. I did not understand and I feel I am wasting here time.
>>>> Code could be correct, could be wrong. Especially second patch looks
>>> suspicious. But the way Zijun Hu explains it and the way Zijun Hu
>>> responds is not helping at all.
>>>
>>> Sorry, with such replies to review, it is not worth my time.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> Hi luiz,marcel
>>
>> it is time for me to request you give comments for our discussion
>> and for my fixes, Let me explain the 1st issue then 2nd one.
>
> You keep pushing and pushing even though I stated my remarks.
>
>
>>
>> For 1st issue:
>> 1) the following commit will cause serious regression issue for QCA
>> controllers, and it has been merged with linus's mainline kernel.
>>
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()").
>>
>> 2) the regression issue is described by [PATCH v4 1/2] commit message
>> as following:
>> BT can't be enabled after below steps:
>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
>> failure if property enable-gpios is not configured within DT|ACPI for
>> QCA_QCA6390.
>> i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted.
>>
>> 3) let me explain the bug point for commit mentioned by 1), its
>> commit message and bug change applet are shown below.
>>
>> The optional variants for the gpiod_get() family of functions return
>> NULL if the GPIO in question is not associated with this device. They
>> return ERR_PTR() on any other error. NULL descriptors are graciously
>> handled by GPIOLIB and can be safely passed to any of the GPIO consumer
>> interfaces as they will return 0 and act as if the function succeeded.
>> If one is using the optional variant, then there's no point in checking
>> for NULL.
>>
>> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>> GPIOD_OUT_LOW);
>> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>> + if (IS_ERR(qcadev->bt_en)) {
>> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> power_ctrl_enabled = false;
>> }
>> 3.1) we only need to discuss how to handle case "qcadev->bt_en ==
>> NULL" since this is only difference between the commit and BT original
>> design.
>> 3.2) BT original design are agree with the point of above commit
>> message that case "qcadev->bt_en == NULL" should not be treated as
>> error, so BT original design does not do error return for the case and
>> use dev_warn() instead of dev_err() to give.
>> 3.3) the commit misunderstands BT original design and wrongly think
>> BT original design take "qcadev->bt_en == NULL" as error case,
>> so change the following flag power_ctrl_enabled set logic and cause
>> discussed issue.
>>
>> For the 2nd issue:
>> 1) the following commit will cause below regression issue for QCA_QCA6390.
>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>> serdev")
>>
>> 2) the regression issue is described by [PATCH v4 2/2] commit message
>> as following:
>> BT can't be enabled after below steps:
>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
>> failure if property enable-gpios is not configured within DT|ACPI for
>> QCA_QCA6390.
>
> You did not address original issue of crash during shutdown and did not
> clarify my questions.
>
>>
>> 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
>> it should not and also never get chance to be invoked even if BT is
>> disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
>> to reset controller during warm reset phase of above 2) steps.
>
> Anyway, any explanation providing background how you are fixing this
> issue while keeping *previous problem fixed* is useful but should be
> provided in commit msg. I asked about this two or three times.
>
> BTW, provide here exact kernel version you tested this patches with.
> Also the exact hardware.
>
>
> Best regards,
> Krzysztof
>
On 22/04/2024 08:00, quic_zijuhu wrote:
> Hi Krzysztof,
>
> could you list questions i need to explain within commit message based
> on current v4 patch sets ?
>
> let me send v5 patch sets with updated commit messages.
NAK, no.
Stop sending new versions. You ignored several feedbacks already and my
question from that email.
Best regards,
Krzysztof
On 20/04/2024 13:01, Krzysztof Kozlowski wrote:
> On 20/04/2024 00:03, Zijun Hu wrote:
>> This patch series are to fix below 2 regression issues for QCA controllers
>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>
>> the links for these issues are shown below:
>> https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> https://lore.kernel.org/linux-bluetooth/ea20
>
>
> Provide changelog, either in cover letter or in individual patches under
> ---.
>
> Do not attach (thread) your patchsets to some other threads (unrelated
> or older versions). This buries them deep in the mailbox and might
> interfere with applying entire sets.
>
How did you implement these two feedbacks?
v4 and v5 are still bad.
Best regards,
Krzysztof
This patch series are to fix below 2 regression issues for QCA_QCA6390
1) BT can't be enabled any more after below steps:
cold boot -> enable BT -> disable BT -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA_QCA6390.
2) BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA_QCA6390.
Changes:
V4 -> V5: Correct commit message
V3 -> V4: Correct code stype and commit message
V1 -> V3: Remove debugging logs
Zijun Hu (2):
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390
Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable
then warm reboot
drivers/bluetooth/hci_qca.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
2.7.4
On 4/22/2024 3:44 PM, Krzysztof Kozlowski wrote:
> On 20/04/2024 13:01, Krzysztof Kozlowski wrote:
>> On 20/04/2024 00:03, Zijun Hu wrote:
>>> This patch series are to fix below 2 regression issues for QCA controllers
>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>
>>> the links for these issues are shown below:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>> https://lore.kernel.org/linux-bluetooth/ea20
>>
>>
>> Provide changelog, either in cover letter or in individual patches under
>> ---.
>>
>> Do not attach (thread) your patchsets to some other threads (unrelated
>> or older versions). This buries them deep in the mailbox and might
>> interfere with applying entire sets.
>>
>
sorry, not notice this.
in order to send vN patch sets.
do i need to --in-reply-to v(N-1) cover letter or v0 cover-letter ?
> How did you implement these two feedbacks?
>
actually. i don't understand that two feedbacks so ask above questions.
> v4 and v5 are still bad.
>
> Best regards,
> Krzysztof
>
On 22/04/2024 10:07, quic_zijuhu wrote:
> On 4/22/2024 3:44 PM, Krzysztof Kozlowski wrote:
>> On 20/04/2024 13:01, Krzysztof Kozlowski wrote:
>>> On 20/04/2024 00:03, Zijun Hu wrote:
>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>>
>>>> the links for these issues are shown below:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> https://lore.kernel.org/linux-bluetooth/ea20
>>>
>>>
>>> Provide changelog, either in cover letter or in individual patches under
>>> ---.
>>>
>>> Do not attach (thread) your patchsets to some other threads (unrelated
>>> or older versions). This buries them deep in the mailbox and might
>>> interfere with applying entire sets.
>>>
>>
> sorry, not notice this.
> in order to send vN patch sets.
> do i need to --in-reply-to v(N-1) cover letter or v0 cover-letter ?
No. b4 or git send-email handle everything correctly. Read go/upstream
before posting. If you ask such question, I doubt that you read it.
Eventually get someone experienced to help you with this.
>
>> How did you implement these two feedbacks?
>>
> actually. i don't understand that two feedbacks so ask above questions.
You did not ask questions. You ignored that feedback and kept sending
patches, pushing for your point of view and responding to review with
unrelated sentences.
Best regards,
Krzysztof
On 4/22/2024 4:11 PM, Krzysztof Kozlowski wrote:
> On 22/04/2024 10:07, quic_zijuhu wrote:
>> On 4/22/2024 3:44 PM, Krzysztof Kozlowski wrote:
>>> On 20/04/2024 13:01, Krzysztof Kozlowski wrote:
>>>> On 20/04/2024 00:03, Zijun Hu wrote:
>>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>>>
>>>>> the links for these issues are shown below:
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>> https://lore.kernel.org/linux-bluetooth/ea20
>>>>
>>>>
>>>> Provide changelog, either in cover letter or in individual patches under
>>>> ---.
>>>>
>>>> Do not attach (thread) your patchsets to some other threads (unrelated
>>>> or older versions). This buries them deep in the mailbox and might
>>>> interfere with applying entire sets.
>>>>
>>>
>> sorry, not notice this.
>> in order to send vN patch sets.
>> do i need to --in-reply-to v(N-1) cover letter or v0 cover-letter ?
>
> No. b4 or git send-email handle everything correctly. Read go/upstream
> before posting. If you ask such question, I doubt that you read it.
> Eventually get someone experienced to help you with this.
>
actually. i have read go/upstream many times. but b4 is hard for me to
use. so i always use git send-email to send patches and use thunderbird
to reply mails. we get someone to help.
>>
>>> How did you implement these two feedbacks?
>>>
>> actually. i don't understand that two feedbacks so ask above questions.
>
> You did not ask questions. You ignored that feedback and kept sending
> patches, pushing for your point of view and responding to review with
> unrelated sentences.
>
i now understood your feedbacks and concerns about format.
> Best regards,
> Krzysztof
>
On Mon, 22 Apr 2024 at 07:21, Wren Turkal <[email protected]> wrote:
>
> As the user who originally reported thes issue, I can confirm this. I
> was introduced to this regression because I use Fedora Rawhide on my
> laptop, which builds and pushes kernels based on mainline very regularly.
>
I don't doubt my patch could have caused a regression.
> Here is my description of the regression: After the reverted change, the
> BT hardware in my laptop (qca6390) will only work after a cold boot when
> the hardware has only be enabled once by the driver. Once the hardware
> is enabled, the process of disabling/re-enabling fails. Also, the
> hardware cannot be enabled after a warm boot of the laptop.
>
> Among other things, this makes logging into KDE Plasma break my
> bluetooth mouse. The cause of this breakage appears to be that Plasma
> disables/re-enables bluetooth hardware upon login.
>
> GNOME operates slightly less badly in that bluetooth stays enabled.
> However, if I manually disable the bluetooth via the ui or by restarting
> the bluetooth service with systemctl, the mouse fails in the same way as
> happens with Plasma.
>
> Once the bluetooth has failed, the only way to fix is a cold boot and
> only enable the hardware once. I cannot remove the modules (btqca,
> hci_uart, and bluetooth) and re-modprobe them to fix it. I can't restart
> the bluetooth service. I can't do both of those things. I haven't found
> any way to re-enable the hardware beyond cold boot with bluetooth
> service enabled.
>
> If I disable the bluetooth service and cold boot the laptop, there also
> appears to be some kind of race condition as not enabling bluetooth
> service very soon after loading the hci_uart and btqca modules during
> boot puts the system in a state where I can never enable bluetooth. I do
> not know what causes this specifically, but my theory is that not
> starting the bluetooth service immediately puts the driver in a similar
> state as when the service is started immediately. Maybe some kind of
> lazy initialization that is forced to happen more quickly when the
> bluetooth service is enabled?
>
> Any way, this reversion by itself (which I manually did after a
> discussion with Zijun before getting his test patches applying to my
> kernel for test). However, this reversion did not get the hardware
> working after a warm boot.
>
This all sounds plausible. However just reverting this patch is a
waste of time as checking IS_ERR_OR_NULL() on the return value of
gpiod_get_optional() and continuing on error is wrong as I explained
several times under Ziju's emails already. I provided a suggestion:
bail out on error returned from gpiod_get_optional() even if the
driver could technically continue in some cases. I don't want to have
to argue this anymore.
Bart
On 4/22/2024 1:52 PM, Krzysztof Kozlowski wrote:
> On 22/04/2024 02:14, quic_zijuhu wrote:
>> On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote:
>>> On 21/04/2024 09:44, Wren Turkal wrote:
>>>> On 4/20/24 3:06 PM, Zijun Hu wrote:
>>>>> This patch series are to fix below 2 regression issues for QCA controllers
>>>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390
>>>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390
>>>>
>>>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these
>>>> to ensure they fix the issues I reported?
>>>>
>>>
>>> I look forward to someone testing these on other hardware, not yours. On
>>> the hardware where the original issues were happening leading to this
>>> changes, e.g. RB5.
>>>
>>> Anyway, the problem here is poor explanation of the problem which did
>>> not improve in v3 and v4. Instead I receive explanations like:
>>>
>>> "this is shutdown of serdev and not hdev's shutdown."
>>> Not related...
>>>
>> this is the reply for secondary issue. i believe i have given much
>> explain for my fix for the 2nd issue as shown by below links.
>
> No, you did not.
>
>> let me add a bit more explanation within the ending "For the 2nd issue"
>> section, supposed you known much for generic flag
>> HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the
>> quirk. also supposed you see commit history to find why
>> qca_serdev_shutdown() was introduced for QCA6390.
>> https://lore.kernel.org/all/[email protected]/
>
> You did not answer my questions.
>
> Let's quote:
>
> "i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and
> HCI_SETUP is generic flag."
>
> Srsly, what is such answer?
>
>
i reviewed my reply. i have explained to you why my change fix both this
issue and the issue your commit fixed.
so i don't think it is meaningful to explain why your wrong condition
are changed by me.
>
>
>
>>> "now. you understood why your merged change as shown link of 4) have
>>> problems and introduced our discussed issue, right?"
>>>
>> this is the reply for the first issue as shown by below link. it almost
>> have the same description as the following "For 1st issue:" section.
>> i believe it have clear illustration why the commit have bugs.
>> https://lore.kernel.org/all/[email protected]/
>>> No. I did not understand and I feel I am wasting here time.
>>>> Code could be correct, could be wrong. Especially second patch looks
>>> suspicious. But the way Zijun Hu explains it and the way Zijun Hu
>>> responds is not helping at all.
>>>
>>> Sorry, with such replies to review, it is not worth my time.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> Hi luiz,marcel
>>
>> it is time for me to request you give comments for our discussion
>> and for my fixes, Let me explain the 1st issue then 2nd one.
>
> You keep pushing and pushing even though I stated my remarks.
>
>
>>
>> For 1st issue:
>> 1) the following commit will cause serious regression issue for QCA
>> controllers, and it has been merged with linus's mainline kernel.
>>
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()").
>>
>> 2) the regression issue is described by [PATCH v4 1/2] commit message
>> as following:
>> BT can't be enabled after below steps:
>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
>> failure if property enable-gpios is not configured within DT|ACPI for
>> QCA_QCA6390.
>> i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted.
>>
>> 3) let me explain the bug point for commit mentioned by 1), its
>> commit message and bug change applet are shown below.
>>
>> The optional variants for the gpiod_get() family of functions return
>> NULL if the GPIO in question is not associated with this device. They
>> return ERR_PTR() on any other error. NULL descriptors are graciously
>> handled by GPIOLIB and can be safely passed to any of the GPIO consumer
>> interfaces as they will return 0 and act as if the function succeeded.
>> If one is using the optional variant, then there's no point in checking
>> for NULL.
>>
>> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>> GPIOD_OUT_LOW);
>> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>> + if (IS_ERR(qcadev->bt_en)) {
>> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> power_ctrl_enabled = false;
>> }
>> 3.1) we only need to discuss how to handle case "qcadev->bt_en ==
>> NULL" since this is only difference between the commit and BT original
>> design.
>> 3.2) BT original design are agree with the point of above commit
>> message that case "qcadev->bt_en == NULL" should not be treated as
>> error, so BT original design does not do error return for the case and
>> use dev_warn() instead of dev_err() to give.
>> 3.3) the commit misunderstands BT original design and wrongly think
>> BT original design take "qcadev->bt_en == NULL" as error case,
>> so change the following flag power_ctrl_enabled set logic and cause
>> discussed issue.
>>
>> For the 2nd issue:
>> 1) the following commit will cause below regression issue for QCA_QCA6390.
>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>> serdev")
>>
>> 2) the regression issue is described by [PATCH v4 2/2] commit message
>> as following:
>> BT can't be enabled after below steps:
>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable
>> failure if property enable-gpios is not configured within DT|ACPI for
>> QCA_QCA6390.
>
> You did not address original issue of crash during shutdown and did not
> clarify my questions.
>
as i statemented. my fix have fixed both this issue and the original
crash issue. don't need to talk about others.
>>
>> 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
>> it should not and also never get chance to be invoked even if BT is
>> disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
>> to reset controller during warm reset phase of above 2) steps.
>
> Anyway, any explanation providing background how you are fixing this
> issue while keeping *previous problem fixed* is useful but should be
> provided in commit msg. I asked about this two or three times.
>
> BTW, provide here exact kernel version you tested this patches with.
> Also the exact hardware.
>
there are almost no commit with tag Tested-by also provide exact kernel
version. for one type bt controller. different h/w has different config.
important is that this issue is fixed in reported H/W and don't cause
issue for other issue.
let us stop here and wait for other comments.
i have given too much explanations for my change of only total 7 lines.
>
> Best regards,
> Krzysztof
>
On 4/22/24 1:51 AM, Bartosz Golaszewski wrote:
> On Mon, 22 Apr 2024 at 07:21, Wren Turkal <[email protected]> wrote:
>>
>> As the user who originally reported thes issue, I can confirm this. I
>> was introduced to this regression because I use Fedora Rawhide on my
>> laptop, which builds and pushes kernels based on mainline very regularly.
>>
>
> I don't doubt my patch could have caused a regression.
>
>> Here is my description of the regression: After the reverted change, the
>> BT hardware in my laptop (qca6390) will only work after a cold boot when
>> the hardware has only be enabled once by the driver. Once the hardware
>> is enabled, the process of disabling/re-enabling fails. Also, the
>> hardware cannot be enabled after a warm boot of the laptop.
>>
>> Among other things, this makes logging into KDE Plasma break my
>> bluetooth mouse. The cause of this breakage appears to be that Plasma
>> disables/re-enables bluetooth hardware upon login.
>>
>> GNOME operates slightly less badly in that bluetooth stays enabled.
>> However, if I manually disable the bluetooth via the ui or by restarting
>> the bluetooth service with systemctl, the mouse fails in the same way as
>> happens with Plasma.
>>
>> Once the bluetooth has failed, the only way to fix is a cold boot and
>> only enable the hardware once. I cannot remove the modules (btqca,
>> hci_uart, and bluetooth) and re-modprobe them to fix it. I can't restart
>> the bluetooth service. I can't do both of those things. I haven't found
>> any way to re-enable the hardware beyond cold boot with bluetooth
>> service enabled.
>>
>> If I disable the bluetooth service and cold boot the laptop, there also
>> appears to be some kind of race condition as not enabling bluetooth
>> service very soon after loading the hci_uart and btqca modules during
>> boot puts the system in a state where I can never enable bluetooth. I do
>> not know what causes this specifically, but my theory is that not
>> starting the bluetooth service immediately puts the driver in a similar
>> state as when the service is started immediately. Maybe some kind of
>> lazy initialization that is forced to happen more quickly when the
>> bluetooth service is enabled?
>>
>> Any way, this reversion by itself (which I manually did after a
>> discussion with Zijun before getting his test patches applying to my
>> kernel for test). However, this reversion did not get the hardware
>> working after a warm boot.
>>
>
> This all sounds plausible. However just reverting this patch is a
> waste of time as checking IS_ERR_OR_NULL() on the return value of
> gpiod_get_optional() and continuing on error is wrong as I explained
> several times under Ziju's emails already. I provided a suggestion:
> bail out on error returned from gpiod_get_optional() even if the
> driver could technically continue in some cases. I don't want to have
> to argue this anymore.
I'm not trying to argue. I am trying to find a path forward as a
concerned user. I am also trying to figure out if there is any way I can
help resolve this. I am not a kernel developer, but I would really like
to contribute in some way, if possible.
>
> Bart
--
You're more amazing than you think!
On 22/04/2024 12:05, quic_zijuhu wrote:
>>> 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown()
>>> it should not and also never get chance to be invoked even if BT is
>>> disabled at above 2) step. qca_serdev_shutdown() need to send the VSC
>>> to reset controller during warm reset phase of above 2) steps.
>>
>> Anyway, any explanation providing background how you are fixing this
>> issue while keeping *previous problem fixed* is useful but should be
>> provided in commit msg. I asked about this two or three times.
>>
>> BTW, provide here exact kernel version you tested this patches with.
>> Also the exact hardware.
>>
> there are almost no commit with tag Tested-by also provide exact kernel
?!?
So this was not tested at all by you on mainline kernel and you push
downstream patch? That's how shall we understand this?
> version. for one type bt controller. different h/w has different config.
> important is that this issue is fixed in reported H/W and don't cause
> issue for other issue.
Amount of pushback from your side and ignoring questions raised during
review is way too much.
>
> let us stop here and wait for other comments.
So why do you push again in v5?
Best regards,
Krzysztof
On Mon, 22 Apr 2024 at 12:42, Wren Turkal <[email protected]> wrote:
>
> On 4/22/24 1:51 AM, Bartosz Golaszewski wrote:
> >
> > This all sounds plausible. However just reverting this patch is a
> > waste of time as checking IS_ERR_OR_NULL() on the return value of
> > gpiod_get_optional() and continuing on error is wrong as I explained
> > several times under Ziju's emails already. I provided a suggestion:
> > bail out on error returned from gpiod_get_optional() even if the
> > driver could technically continue in some cases. I don't want to have
> > to argue this anymore.
>
> I'm not trying to argue. I am trying to find a path forward as a
> concerned user. I am also trying to figure out if there is any way I can
> help resolve this. I am not a kernel developer, but I would really like
> to contribute in some way, if possible.
>
Can you test the patch[1] I just sent?
Bart
[1] https://lore.kernel.org/linux-bluetooth/[email protected]/
On 4/22/24 6:02 AM, Bartosz Golaszewski wrote:
> Can you test the patch[1] I just sent?
I am doing this now. Just to be clear, I am testing the patch I found in
the thread with subject "[PATCH] Bluetooth: qca: set power_ctrl_enabled
on NULL returned by gpiod_get_optional()". If that isn't the one you're
referring to, please let me know.
I will reply back to that patch after testing.
wt
--
You're more amazing than you think!
V6 and V7 patch serial discussion link are shown below:
V6 patch serials discussion link is here:
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#r0a177b0db7f7185ecec9274460fdf8d369d5c255
V7 patch serials discussion link is here:
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#t
--
2.7.4