[PATCH v7 0/2] Fix two BT regression issues for QCA6390

[PATCH v7 0/2] Fix two BT regression issues for QCA6390

[Zijun Hu]

This patch series are to fix below 2 BT regression issues for 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 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 QCA6390.

Fix solution has been verified by the reported device Dell XPS 13 9310
laptop over below bluetooth-next tree commit as the last bugzilla comment
commit 6abf9dd26bb1 ("Bluetooth: qca: Fix triggering coredump
implementation").

These two issues were initially reported at below link:
https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
then reported to bugzilla as shown by below link:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
the previous discussion link is listed below
https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/

Zijun Hu (2):
  Bluetooth: qca: Fix BT enable failure for QCA6390
  Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then
    warm reboot

 drivers/bluetooth/hci_qca.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Zijun Hu]

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 QCA6390.

The commit wrongly changes flag @power_ctrl_enabled set logic for this
case as shown by its below code applet and causes this serious issue.
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;
  }

Fixed by reverting the mentioned commit for QCA6390.

Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Cc: stable@vge.kernel.org
Reported-by: Wren Turkal <wt@penguintechs.org>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
Tested-by: Wren Turkal <wt@penguintechs.org>
---
Changes:
V6 -> V7: Add stable tag
V3 -> V6: Correct code stype and title and commit message
V1 -> V3: Don't revert the whole wrong commit but focus on impacted device 

 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

[PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot

[Zijun Hu]

From: Zijun Hu <zijuhu@qti.qualcomm.com>

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 QCA6390.

The commit is to fix a use-after-free issue within qca_serdev_shutdown()
during reboot, but also introduces this regression issue regarding above
steps since the VSC is not sent to reset controller during warm reboot.

Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
once BT was ever enabled, and the use-after-free issue is also be fixed
by this change since serdev is still opened when send to serdev.

Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
Cc: stable@vge.kernel.org
Reported-by: Wren Turkal <wt@penguintechs.org>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Signed-off-by: Zijun Hu <zijuhu@qti.qualcomm.com>
Tested-by: Wren Turkal <wt@penguintechs.org>
---
Changes:
V6 -> V7: Add stable tag
V3 -> V6: Correct title and commit message
V1 -> V3: Remove debugging logs

 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

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Krzysztof Kozlowski]

On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for QCA6390.
> 
> The commit wrongly changes flag @power_ctrl_enabled set logic for this
> case as shown by its below code applet and causes this serious issue.
> 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;
>   }
> 
> Fixed by reverting the mentioned commit for QCA6390.
> 
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Cc: stable@vge.kernel.org
> Reported-by: Wren Turkal <wt@penguintechs.org>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> Tested-by: Wren Turkal <wt@penguintechs.org>
> ---
> Changes:
> V6 -> V7: Add stable tag

Stop sending multiple pathchsets per day. I already asked you to first
finish discussion and then send new version. You again start sending
something while previous discussion is going.

Best regards,
Krzysztof

Re: [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot

[Greg KH]

On Wed, Apr 24, 2024 at 12:26:47PM +0800, Zijun Hu wrote:
> From: Zijun Hu <zijuhu@qti.qualcomm.com>
> 
> 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 QCA6390.
> 
> The commit is to fix a use-after-free issue within qca_serdev_shutdown()
> during reboot, but also introduces this regression issue regarding above
> steps since the VSC is not sent to reset controller during warm reboot.
> 
> Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
> once BT was ever enabled, and the use-after-free issue is also be fixed
> by this change since serdev is still opened when send to serdev.
> 
> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
> Cc: stable@vge.kernel.org

That is not a valid email address :(

RE: Fix two BT regression issues for QCA6390

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4675 bytes --]

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=847290

---Test result---

Test Summary:
CheckPatch                    FAIL      1.59 seconds
GitLint                       FAIL      0.79 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      30.46 seconds
CheckAllWarning               PASS      32.43 seconds
CheckSparse                   PASS      38.38 seconds
CheckSmatch                   FAIL      34.52 seconds
BuildKernel32                 PASS      28.60 seconds
TestRunnerSetup               PASS      514.95 seconds
TestRunner_l2cap-tester       PASS      18.26 seconds
TestRunner_iso-tester         PASS      28.93 seconds
TestRunner_bnep-tester        PASS      4.66 seconds
TestRunner_mgmt-tester        PASS      124.62 seconds
TestRunner_rfcomm-tester      PASS      7.11 seconds
TestRunner_sco-tester         PASS      10.80 seconds
TestRunner_ioctl-tester       PASS      7.55 seconds
TestRunner_mesh-tester        PASS      5.70 seconds
TestRunner_smp-tester         PASS      6.72 seconds
TestRunner_userchan-tester    PASS      4.82 seconds
IncrementalBuild              PASS      33.01 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v7,1/2] Bluetooth: qca: Fix BT enable failure for QCA6390
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#121: 
Reported-by: Wren Turkal <wt@penguintechs.org>
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/13641176.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:
[v7,1/2] Bluetooth: qca: Fix BT enable failure for 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
16: B3 Line contains hard tab characters (\t): "  	dev_warn(&serdev->dev, "failed to acquire enable gpio\n");"
17: B3 Line contains hard tab characters (\t): "	power_ctrl_enabled = false;"
25: B1 Line exceeds max length (139>80): "Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
32: B2 Line has trailing whitespace: "V1 -> V3: Don't revert the whole wrong commit but focus on impacted device "
[v7,2/2] Bluetooth: qca: Fix BT enable failure for 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 (89>80): "[v7,2/2] Bluetooth: qca: Fix BT enable failure for 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

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for QCA6390.
>>
>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>> case as shown by its below code applet and causes this serious issue.
>> 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;
>>   }
>>
>> Fixed by reverting the mentioned commit for QCA6390.
>>
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Cc: stable@vge.kernel.org
>> Reported-by: Wren Turkal <wt@penguintechs.org>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> Tested-by: Wren Turkal <wt@penguintechs.org>
>> ---
>> Changes:
>> V6 -> V7: Add stable tag
> 
> Stop sending multiple pathchsets per day. I already asked you to first
> finish discussion and then send new version. You again start sending
> something while previous discussion is going.
>you concern is wrong and i am sure it don't block me sending new patch
sets to solve other issue. so i send this v7.

i have give reply for Bartosz' patch.

i hop you as DTS expert to notice my concern about DTS in the reply.

> Best regards,
> Krzysztof
> 

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 1:02 PM, quic_zijuhu wrote:
>> you concern is wrong and i am sure it don't block me sending new patch
> sets to solve other issue. so i send this v7.
> 
> i have give reply for Bartosz' patch.
> 
> i hop you as DTS expert to notice my concern about DTS in the reply.

sorry to correct a typo error.

Hi Krzysztof.
you concern is wrong and i am sure it don't block me sending new patch
sets to solve other issue. so i send this v7.

i have give reply for Bartosz' patch.

i hop you as DTS expert to notice my concern about DTS in the reply.

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Wren Turkal]

On 4/23/24 10:02 PM, quic_zijuhu wrote:
> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for QCA6390.
>>>
>>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>>> case as shown by its below code applet and causes this serious issue.
>>> 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;
>>>    }
>>>
>>> Fixed by reverting the mentioned commit for QCA6390.
>>>
>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>> Cc: stable@vge.kernel.org
>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>> ---
>>> Changes:
>>> V6 -> V7: Add stable tag
>>
>> Stop sending multiple pathchsets per day. I already asked you to first
>> finish discussion and then send new version. You again start sending
>> something while previous discussion is going.
>> you concern is wrong and i am sure it don't block me sending new patch
> sets to solve other issue. so i send this v7.
> 
> i have give reply for Bartosz' patch.
> 
> i hop you as DTS expert to notice my concern about DTS in the reply.

Are you saying here (1) that you identified a problem in the DTs that 
you hope Krzysztof notices or (2) that you want Krzysztof to notice how 
your description of way that DT declares the gpio as required affects 
your proposed change. As a native American English speaker, I am finding 
your text hard to follow.

I think you are saying #2.

I just want to make sure I am following the discussion here.

wt
-- 
You're more amazing than you think!

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 1:37 PM, Wren Turkal wrote:
> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for QCA6390.
>>>>
>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>>>> case as shown by its below code applet and causes this serious issue.
>>>> 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;
>>>>    }
>>>>
>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>
>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>> with gpiod_get_optional()")
>>>> Cc: stable@vge.kernel.org
>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> Link:
>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>> ---
>>>> Changes:
>>>> V6 -> V7: Add stable tag
>>>
>>> Stop sending multiple pathchsets per day. I already asked you to first
>>> finish discussion and then send new version. You again start sending
>>> something while previous discussion is going.
>>> you concern is wrong and i am sure it don't block me sending new patch
>> sets to solve other issue. so i send this v7.
>>
>> i have give reply for Bartosz' patch.
>>
>> i hop you as DTS expert to notice my concern about DTS in the reply.
> 
> Are you saying here (1) that you identified a problem in the DTs that
> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
> your description of way that DT declares the gpio as required affects
> your proposed change. As a native American English speaker, I am finding
> your text hard to follow.
> 
1) is my purpose. i have given my concern about DTS for Bartosz' patch
and hope DTS expert notice the concern.

my change don't have any such concern about DTS usage. that is why i
changed my fix from original reverting the whole wrong commit to now
focusing on QCA6390.

> I think you are saying #2.
> 
> I just want to make sure I am following the discussion here.
> 
> wt

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Wren Turkal]

On 4/23/24 10:46 PM, quic_zijuhu wrote:
> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for QCA6390.
>>>>>
>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>>>>> case as shown by its below code applet and causes this serious issue.
>>>>> 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;
>>>>>     }
>>>>>
>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>
>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>> with gpiod_get_optional()")
>>>>> Cc: stable@vge.kernel.org
>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>> Link:
>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>> ---
>>>>> Changes:
>>>>> V6 -> V7: Add stable tag
>>>>
>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>> finish discussion and then send new version. You again start sending
>>>> something while previous discussion is going.
>>>> you concern is wrong and i am sure it don't block me sending new patch
>>> sets to solve other issue. so i send this v7.
>>>
>>> i have give reply for Bartosz' patch.
>>>
>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>
>> Are you saying here (1) that you identified a problem in the DTs that
>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>> your description of way that DT declares the gpio as required affects
>> your proposed change. As a native American English speaker, I am finding
>> your text hard to follow.
>>
> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
> and hope DTS expert notice the concern.
> 
> my change don't have any such concern about DTS usage. that is why i
> changed my fix from original reverting the whole wrong commit to now
> focusing on QCA6390.

Let me try to parse this. If #1 is the correct interpretation, does that 
mean that the DTs are wrong and need to be changed? Do you expect K to 
do that since he's the "DTS expert"?

>> I think you are saying #2.
>>
>> I just want to make sure I am following the discussion here.
>>
>> wt
> 

-- 
You're more amazing than you think!

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 1:49 PM, Wren Turkal wrote:
> On 4/23/24 10:46 PM, quic_zijuhu wrote:
>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
>>>>>> QCA6390.
>>>>>>
>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
>>>>>> this
>>>>>> case as shown by its below code applet and causes this serious issue.
>>>>>> 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;
>>>>>>     }
>>>>>>
>>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>>
>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>>> with gpiod_get_optional()")
>>>>>> Cc: stable@vge.kernel.org
>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Link:
>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>> ---
>>>>>> Changes:
>>>>>> V6 -> V7: Add stable tag
>>>>>
>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>> finish discussion and then send new version. You again start sending
>>>>> something while previous discussion is going.
>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>> sets to solve other issue. so i send this v7.
>>>>
>>>> i have give reply for Bartosz' patch.
>>>>
>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>
>>> Are you saying here (1) that you identified a problem in the DTs that
>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>> your description of way that DT declares the gpio as required affects
>>> your proposed change. As a native American English speaker, I am finding
>>> your text hard to follow.
>>>
>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>> and hope DTS expert notice the concern.
>>
>> my change don't have any such concern about DTS usage. that is why i
>> changed my fix from original reverting the whole wrong commit to now
>> focusing on QCA6390.
> 
> Let me try to parse this. If #1 is the correct interpretation, does that
> mean that the DTs are wrong and need to be changed? Do you expect K to
> do that since he's the "DTS expert"?
> 
for your 1) question, NO
for your 2) question, need DTS expert notice or suggest how to handle
case that a DTS property is marked as required but not be configed by user.

>>> I think you are saying #2.
>>>
>>> I just want to make sure I am following the discussion here.
>>>
>>> wt
>>
> 

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 2:01 PM, quic_zijuhu wrote:
> On 4/24/2024 1:49 PM, Wren Turkal wrote:
>> On 4/23/24 10:46 PM, quic_zijuhu wrote:
>>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
>>>>>>> QCA6390.
>>>>>>>
>>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
>>>>>>> this
>>>>>>> case as shown by its below code applet and causes this serious issue.
>>>>>>> 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;
>>>>>>>     }
>>>>>>>
>>>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>>>
>>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>>>> with gpiod_get_optional()")
>>>>>>> Cc: stable@vge.kernel.org
>>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>>> ---
>>>>>>> Changes:
>>>>>>> V6 -> V7: Add stable tag
>>>>>>
>>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>>> finish discussion and then send new version. You again start sending
>>>>>> something while previous discussion is going.
>>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>>> sets to solve other issue. so i send this v7.
>>>>>
>>>>> i have give reply for Bartosz' patch.
>>>>>
>>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>>
>>>> Are you saying here (1) that you identified a problem in the DTs that
>>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>>> your description of way that DT declares the gpio as required affects
>>>> your proposed change. As a native American English speaker, I am finding
>>>> your text hard to follow.
>>>>
>>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>>> and hope DTS expert notice the concern.
>>>
>>> my change don't have any such concern about DTS usage. that is why i
>>> changed my fix from original reverting the whole wrong commit to now
>>> focusing on QCA6390.
>>
>> Let me try to parse this. If #1 is the correct interpretation, does that
>> mean that the DTs are wrong and need to be changed? Do you expect K to
>> do that since he's the "DTS expert"?
>>
> for your 1) question, NO
> for your 2) question, need DTS expert notice or suggest how to handle
> case that a DTS property is marked as required but not be configed by user.
> 
>>>> I think you are saying #2.
>>>>
>>>> I just want to make sure I am following the discussion here.
>>>>
>>>> wt
>>>
>>
> 
Hi Krzysztof, bartosz.

do you have any concern for this patch serials?

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Krzysztof Kozlowski]

On 24/04/2024 11:39, quic_zijuhu wrote:
> On 4/24/2024 2:01 PM, quic_zijuhu wrote:
>> On 4/24/2024 1:49 PM, Wren Turkal wrote:
>>> On 4/23/24 10:46 PM, quic_zijuhu wrote:
>>>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>>>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
>>>>>>>> QCA6390.
>>>>>>>>
>>>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
>>>>>>>> this
>>>>>>>> case as shown by its below code applet and causes this serious issue.
>>>>>>>> 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;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>>>>
>>>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>>>>> with gpiod_get_optional()")
>>>>>>>> Cc: stable@vge.kernel.org
>>>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>> Link:
>>>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>> ---
>>>>>>>> Changes:
>>>>>>>> V6 -> V7: Add stable tag
>>>>>>>
>>>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>>>> finish discussion and then send new version. You again start sending
>>>>>>> something while previous discussion is going.
>>>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>>>> sets to solve other issue. so i send this v7.
>>>>>>
>>>>>> i have give reply for Bartosz' patch.
>>>>>>
>>>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>>>
>>>>> Are you saying here (1) that you identified a problem in the DTs that
>>>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>>>> your description of way that DT declares the gpio as required affects
>>>>> your proposed change. As a native American English speaker, I am finding
>>>>> your text hard to follow.
>>>>>
>>>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>>>> and hope DTS expert notice the concern.
>>>>
>>>> my change don't have any such concern about DTS usage. that is why i
>>>> changed my fix from original reverting the whole wrong commit to now
>>>> focusing on QCA6390.
>>>
>>> Let me try to parse this. If #1 is the correct interpretation, does that
>>> mean that the DTs are wrong and need to be changed? Do you expect K to
>>> do that since he's the "DTS expert"?
>>>
>> for your 1) question, NO
>> for your 2) question, need DTS expert notice or suggest how to handle
>> case that a DTS property is marked as required but not be configed by user.
>>
>>>>> I think you are saying #2.
>>>>>
>>>>> I just want to make sure I am following the discussion here.
>>>>>
>>>>> wt
>>>>
>>>
>>
> Hi Krzysztof, bartosz.
> 
> do you have any concern for this patch serials?

What? Amount of noise coming with this constant ping and dispersed
discussions is really annoying.

Best regards,
Krzysztof

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 5:48 PM, Krzysztof Kozlowski wrote:
> On 24/04/2024 11:39, quic_zijuhu wrote:
>> On 4/24/2024 2:01 PM, quic_zijuhu wrote:
>>> On 4/24/2024 1:49 PM, Wren Turkal wrote:
>>>> On 4/23/24 10:46 PM, quic_zijuhu wrote:
>>>>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>>>>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>>>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
>>>>>>>>> QCA6390.
>>>>>>>>>
>>>>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
>>>>>>>>> this
>>>>>>>>> case as shown by its below code applet and causes this serious issue.
>>>>>>>>> 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;
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>>>>>
>>>>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>>>>>> with gpiod_get_optional()")
>>>>>>>>> Cc: stable@vge.kernel.org
>>>>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>>> ---
>>>>>>>>> Changes:
>>>>>>>>> V6 -> V7: Add stable tag
>>>>>>>>
>>>>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>>>>> finish discussion and then send new version. You again start sending
>>>>>>>> something while previous discussion is going.
>>>>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>>>>> sets to solve other issue. so i send this v7.
>>>>>>>
>>>>>>> i have give reply for Bartosz' patch.
>>>>>>>
>>>>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>>>>
>>>>>> Are you saying here (1) that you identified a problem in the DTs that
>>>>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>>>>> your description of way that DT declares the gpio as required affects
>>>>>> your proposed change. As a native American English speaker, I am finding
>>>>>> your text hard to follow.
>>>>>>
>>>>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>>>>> and hope DTS expert notice the concern.
>>>>>
>>>>> my change don't have any such concern about DTS usage. that is why i
>>>>> changed my fix from original reverting the whole wrong commit to now
>>>>> focusing on QCA6390.
>>>>
>>>> Let me try to parse this. If #1 is the correct interpretation, does that
>>>> mean that the DTs are wrong and need to be changed? Do you expect K to
>>>> do that since he's the "DTS expert"?
>>>>
>>> for your 1) question, NO
>>> for your 2) question, need DTS expert notice or suggest how to handle
>>> case that a DTS property is marked as required but not be configed by user.
>>>
>>>>>> I think you are saying #2.
>>>>>>
>>>>>> I just want to make sure I am following the discussion here.
>>>>>>
>>>>>> wt
>>>>>
>>>>
>>>
>> Hi Krzysztof, bartosz.
>>
>> do you have any concern for this patch serials?
> 
> What? Amount of noise coming with this constant ping and dispersed
> discussions is really annoying.
> 
sorry for ping you. i really hope this fix can be merged ASAP since the
wrong commit have been drifted into kernel mainline, i have talked about
this fix with you and bartosz too much for this only total 7 line
change. i really hope got your code review results, accept or rejected
or neutral?

> Best regards,
> Krzysztof
> 
> 

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[Bartosz Golaszewski]

On Wed, 24 Apr 2024 at 11:40, quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>
> On 4/24/2024 2:01 PM, quic_zijuhu wrote:
> > On 4/24/2024 1:49 PM, Wren Turkal wrote:
> >> On 4/23/24 10:46 PM, quic_zijuhu wrote:
> >>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
> >>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
> >>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
> >>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
> >>>>>>> QCA6390.
> >>>>>>>
> >>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
> >>>>>>> this
> >>>>>>> case as shown by its below code applet and causes this serious issue.
> >>>>>>> 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;
> >>>>>>>     }
> >>>>>>>
> >>>>>>> Fixed by reverting the mentioned commit for QCA6390.
> >>>>>>>
> >>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> >>>>>>> with gpiod_get_optional()")
> >>>>>>> Cc: stable@vge.kernel.org
> >>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
> >>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >>>>>>> Link:
> >>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> >>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
> >>>>>>> ---
> >>>>>>> Changes:
> >>>>>>> V6 -> V7: Add stable tag
> >>>>>>
> >>>>>> Stop sending multiple pathchsets per day. I already asked you to first
> >>>>>> finish discussion and then send new version. You again start sending
> >>>>>> something while previous discussion is going.
> >>>>>> you concern is wrong and i am sure it don't block me sending new patch
> >>>>> sets to solve other issue. so i send this v7.
> >>>>>
> >>>>> i have give reply for Bartosz' patch.
> >>>>>
> >>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
> >>>>
> >>>> Are you saying here (1) that you identified a problem in the DTs that
> >>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
> >>>> your description of way that DT declares the gpio as required affects
> >>>> your proposed change. As a native American English speaker, I am finding
> >>>> your text hard to follow.
> >>>>
> >>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
> >>> and hope DTS expert notice the concern.
> >>>
> >>> my change don't have any such concern about DTS usage. that is why i
> >>> changed my fix from original reverting the whole wrong commit to now
> >>> focusing on QCA6390.
> >>
> >> Let me try to parse this. If #1 is the correct interpretation, does that
> >> mean that the DTs are wrong and need to be changed? Do you expect K to
> >> do that since he's the "DTS expert"?
> >>
> > for your 1) question, NO
> > for your 2) question, need DTS expert notice or suggest how to handle
> > case that a DTS property is marked as required but not be configed by user.
> >
> >>>> I think you are saying #2.
> >>>>
> >>>> I just want to make sure I am following the discussion here.
> >>>>
> >>>> wt
> >>>
> >>
> >
> Hi Krzysztof, bartosz.
>
> do you have any concern for this patch serials?
>

Yes, we have voiced a number of concerns. Please see the fifty
previous emails in several chaotic threads.

I will stop responding to you now, at least for some time. I
understand that there's a regression. I will work with Wren to address
it. Hopefully we can get it fixed soon. However I feel like I'm
wasting my time trying to get to you and I have more things on my
plate right now.

Thanks,
Bartosz

Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

[quic_zijuhu]

On 4/24/2024 6:34 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 11:40, quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>>
>> On 4/24/2024 2:01 PM, quic_zijuhu wrote:
>>> On 4/24/2024 1:49 PM, Wren Turkal wrote:
>>>> On 4/23/24 10:46 PM, quic_zijuhu wrote:
>>>>> On 4/24/2024 1:37 PM, Wren Turkal wrote:
>>>>>> On 4/23/24 10:02 PM, quic_zijuhu wrote:
>>>>>>> On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 24/04/2024 06:26, 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 property enable-gpios is not configured within DT|ACPI for
>>>>>>>>> QCA6390.
>>>>>>>>>
>>>>>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for
>>>>>>>>> this
>>>>>>>>> case as shown by its below code applet and causes this serious issue.
>>>>>>>>> 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;
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>>>>>
>>>>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>>>>>> with gpiod_get_optional()")
>>>>>>>>> Cc: stable@vge.kernel.org
>>>>>>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>>>>>>> Tested-by: Wren Turkal <wt@penguintechs.org>
>>>>>>>>> ---
>>>>>>>>> Changes:
>>>>>>>>> V6 -> V7: Add stable tag
>>>>>>>>
>>>>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>>>>> finish discussion and then send new version. You again start sending
>>>>>>>> something while previous discussion is going.
>>>>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>>>>> sets to solve other issue. so i send this v7.
>>>>>>>
>>>>>>> i have give reply for Bartosz' patch.
>>>>>>>
>>>>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>>>>
>>>>>> Are you saying here (1) that you identified a problem in the DTs that
>>>>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>>>>> your description of way that DT declares the gpio as required affects
>>>>>> your proposed change. As a native American English speaker, I am finding
>>>>>> your text hard to follow.
>>>>>>
>>>>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>>>>> and hope DTS expert notice the concern.
>>>>>
>>>>> my change don't have any such concern about DTS usage. that is why i
>>>>> changed my fix from original reverting the whole wrong commit to now
>>>>> focusing on QCA6390.
>>>>
>>>> Let me try to parse this. If #1 is the correct interpretation, does that
>>>> mean that the DTs are wrong and need to be changed? Do you expect K to
>>>> do that since he's the "DTS expert"?
>>>>
>>> for your 1) question, NO
>>> for your 2) question, need DTS expert notice or suggest how to handle
>>> case that a DTS property is marked as required but not be configed by user.
>>>
>>>>>> I think you are saying #2.
>>>>>>
>>>>>> I just want to make sure I am following the discussion here.
>>>>>>
>>>>>> wt
>>>>>
>>>>
>>>
>> Hi Krzysztof, bartosz.
>>
>> do you have any concern for this patch serials?
>>
> 
> Yes, we have voiced a number of concerns. Please see the fifty
> previous emails in several chaotic threads.
> 
> I will stop responding to you now, at least for some time. I
> understand that there's a regression. I will work with Wren to address
> it. Hopefully we can get it fixed soon. However I feel like I'm
> wasting my time trying to get to you and I have more things on my
> plate right now.
> 
okay, let us stop here now, i would like to summarize here.

1) it is me to co-work with reporter and solved his issue and verified
with PASS results

2) suppose you don't have any negative comments any more about my patch
serials.

3) there are no other working fixes until now when i write this summary

4) i, as a member of BT team of Qualcomm, will give comments about its
QCA BT driver changes as much as possible if i have time.

> Thanks,
> Bartosz

Re: [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot

[Wren Turkal]

On Tuesday, April 23, 2024 9:34:43 PM PDT Greg KH wrote:
> On Wed, Apr 24, 2024 at 12:26:47PM +0800, Zijun Hu wrote:
> > From: Zijun Hu <zijuhu@qti.qualcomm.com>
> > 
> > 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 QCA6390.
> > 
> > The commit is to fix a use-after-free issue within qca_serdev_shutdown()
> > during reboot, but also introduces this regression issue regarding above
> > steps since the VSC is not sent to reset controller during warm reboot.
> > 
> > Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
> > once BT was ever enabled, and the use-after-free issue is also be fixed
> > by this change since serdev is still opened when send to serdev.
> > 
> > Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> > serdev") Cc: stable@vge.kernel.org
> 
> That is not a valid email address :(

@ziljun, can you please post this patch in a new patch series by itself? The 
warm boot problem is fixed by this patch, and I think it would be better to 
have a thread dedicated to this fix so that it doesn't get lost.

FWIW, I think that this may be the correct technical change. I would like to 
try to help get this patch landed, and I think a clean thread for only this 
change would help.

Just so everyone is aware, I am a concerned user of the hardware, and not a 
kernel dev myself. I have tested this change. This patch on top of Linus' 
mainline does allow qca6390 to work after warm reboot on my laptop.

wt
-- 
You're more amazing than you think!