* [PATCH v7 0/2] Fix two BT regression issues for QCA6390 @ 2024-04-24 4:26 Zijun Hu 2024-04-24 4:26 ` [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure " Zijun Hu 2024-04-24 4:26 ` [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot Zijun Hu 0 siblings, 2 replies; 18+ messages in thread From: Zijun Hu @ 2024-04-24 4:26 UTC (permalink / raw) To: luiz.dentz, luiz.von.dentz, marcel Cc: quic_zijuhu, linux-bluetooth, bartosz.golaszewski, krzysztof.kozlowski, wt, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 4:26 [PATCH v7 0/2] Fix two BT regression issues for QCA6390 Zijun Hu @ 2024-04-24 4:26 ` Zijun Hu 2024-04-24 4:30 ` Krzysztof Kozlowski 2024-04-24 4:56 ` Fix two BT regression issues " bluez.test.bot 2024-04-24 4:26 ` [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot Zijun Hu 1 sibling, 2 replies; 18+ messages in thread From: Zijun Hu @ 2024-04-24 4:26 UTC (permalink / raw) To: luiz.dentz, luiz.von.dentz, marcel Cc: quic_zijuhu, linux-bluetooth, bartosz.golaszewski, krzysztof.kozlowski, wt, regressions, kernel, gregkh, stable 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 4:26 ` [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure " Zijun Hu @ 2024-04-24 4:30 ` Krzysztof Kozlowski 2024-04-24 5:02 ` quic_zijuhu 2024-04-24 4:56 ` Fix two BT regression issues " bluez.test.bot 1 sibling, 1 reply; 18+ messages in thread From: Krzysztof Kozlowski @ 2024-04-24 4:30 UTC (permalink / raw) To: Zijun Hu, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, wt, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 4:30 ` Krzysztof Kozlowski @ 2024-04-24 5:02 ` quic_zijuhu 2024-04-24 5:05 ` quic_zijuhu 2024-04-24 5:37 ` Wren Turkal 0 siblings, 2 replies; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 5:02 UTC (permalink / raw) To: Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, wt, regressions, kernel, gregkh, stable 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 5:02 ` quic_zijuhu @ 2024-04-24 5:05 ` quic_zijuhu 2024-04-24 5:37 ` Wren Turkal 1 sibling, 0 replies; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 5:05 UTC (permalink / raw) To: Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, wt, regressions, kernel, gregkh, stable 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 5:02 ` quic_zijuhu 2024-04-24 5:05 ` quic_zijuhu @ 2024-04-24 5:37 ` Wren Turkal 2024-04-24 5:46 ` quic_zijuhu 1 sibling, 1 reply; 18+ messages in thread From: Wren Turkal @ 2024-04-24 5:37 UTC (permalink / raw) To: quic_zijuhu, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 5:37 ` Wren Turkal @ 2024-04-24 5:46 ` quic_zijuhu 2024-04-24 5:49 ` Wren Turkal 0 siblings, 1 reply; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 5:46 UTC (permalink / raw) To: Wren Turkal, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 5:46 ` quic_zijuhu @ 2024-04-24 5:49 ` Wren Turkal 2024-04-24 6:01 ` quic_zijuhu 0 siblings, 1 reply; 18+ messages in thread From: Wren Turkal @ 2024-04-24 5:49 UTC (permalink / raw) To: quic_zijuhu, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 5:49 ` Wren Turkal @ 2024-04-24 6:01 ` quic_zijuhu 2024-04-24 9:39 ` quic_zijuhu 0 siblings, 1 reply; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 6:01 UTC (permalink / raw) To: Wren Turkal, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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 >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 6:01 ` quic_zijuhu @ 2024-04-24 9:39 ` quic_zijuhu 2024-04-24 9:48 ` Krzysztof Kozlowski 2024-04-24 10:34 ` Bartosz Golaszewski 0 siblings, 2 replies; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 9:39 UTC (permalink / raw) To: Wren Turkal, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 9:39 ` quic_zijuhu @ 2024-04-24 9:48 ` Krzysztof Kozlowski 2024-04-24 9:54 ` quic_zijuhu 2024-04-24 10:34 ` Bartosz Golaszewski 1 sibling, 1 reply; 18+ messages in thread From: Krzysztof Kozlowski @ 2024-04-24 9:48 UTC (permalink / raw) To: quic_zijuhu, Wren Turkal, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 9:48 ` Krzysztof Kozlowski @ 2024-04-24 9:54 ` quic_zijuhu 0 siblings, 0 replies; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 9:54 UTC (permalink / raw) To: Krzysztof Kozlowski, Wren Turkal, luiz.dentz, luiz.von.dentz, marcel Cc: linux-bluetooth, bartosz.golaszewski, regressions, kernel, gregkh, stable 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 9:39 ` quic_zijuhu 2024-04-24 9:48 ` Krzysztof Kozlowski @ 2024-04-24 10:34 ` Bartosz Golaszewski 2024-04-24 11:35 ` quic_zijuhu 1 sibling, 1 reply; 18+ messages in thread From: Bartosz Golaszewski @ 2024-04-24 10:34 UTC (permalink / raw) To: quic_zijuhu Cc: Wren Turkal, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel, linux-bluetooth, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure for QCA6390 2024-04-24 10:34 ` Bartosz Golaszewski @ 2024-04-24 11:35 ` quic_zijuhu 0 siblings, 0 replies; 18+ messages in thread From: quic_zijuhu @ 2024-04-24 11:35 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Wren Turkal, Krzysztof Kozlowski, luiz.dentz, luiz.von.dentz, marcel, linux-bluetooth, regressions, kernel, gregkh, stable 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: Fix two BT regression issues for QCA6390 2024-04-24 4:26 ` [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure " Zijun Hu 2024-04-24 4:30 ` Krzysztof Kozlowski @ 2024-04-24 4:56 ` bluez.test.bot 1 sibling, 0 replies; 18+ messages in thread From: bluez.test.bot @ 2024-04-24 4:56 UTC (permalink / raw) To: linux-bluetooth, quic_zijuhu [-- 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot 2024-04-24 4:26 [PATCH v7 0/2] Fix two BT regression issues for QCA6390 Zijun Hu 2024-04-24 4:26 ` [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure " Zijun Hu @ 2024-04-24 4:26 ` Zijun Hu 2024-04-24 4:34 ` Greg KH 1 sibling, 1 reply; 18+ messages in thread From: Zijun Hu @ 2024-04-24 4:26 UTC (permalink / raw) To: luiz.dentz, luiz.von.dentz, marcel Cc: quic_zijuhu, linux-bluetooth, bartosz.golaszewski, krzysztof.kozlowski, wt, regressions, kernel, gregkh, stable 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot 2024-04-24 4:26 ` [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot Zijun Hu @ 2024-04-24 4:34 ` Greg KH 2024-04-26 20:42 ` Wren Turkal 0 siblings, 1 reply; 18+ messages in thread From: Greg KH @ 2024-04-24 4:34 UTC (permalink / raw) To: Zijun Hu Cc: luiz.dentz, luiz.von.dentz, marcel, linux-bluetooth, bartosz.golaszewski, krzysztof.kozlowski, wt, regressions, kernel, stable 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 :( ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot 2024-04-24 4:34 ` Greg KH @ 2024-04-26 20:42 ` Wren Turkal 0 siblings, 0 replies; 18+ messages in thread From: Wren Turkal @ 2024-04-26 20:42 UTC (permalink / raw) To: Zijun Hu, linux-bluetooth Cc: luiz.dentz, luiz.von.dentz, marcel, linux-bluetooth, bartosz.golaszewski, krzysztof.kozlowski, regressions, kernel, stable, Greg KH 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! ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-04-26 20:42 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-24 4:26 [PATCH v7 0/2] Fix two BT regression issues for QCA6390 Zijun Hu 2024-04-24 4:26 ` [PATCH v7 1/2] Bluetooth: qca: Fix BT enable failure " Zijun Hu 2024-04-24 4:30 ` Krzysztof Kozlowski 2024-04-24 5:02 ` quic_zijuhu 2024-04-24 5:05 ` quic_zijuhu 2024-04-24 5:37 ` Wren Turkal 2024-04-24 5:46 ` quic_zijuhu 2024-04-24 5:49 ` Wren Turkal 2024-04-24 6:01 ` quic_zijuhu 2024-04-24 9:39 ` quic_zijuhu 2024-04-24 9:48 ` Krzysztof Kozlowski 2024-04-24 9:54 ` quic_zijuhu 2024-04-24 10:34 ` Bartosz Golaszewski 2024-04-24 11:35 ` quic_zijuhu 2024-04-24 4:56 ` Fix two BT regression issues " bluez.test.bot 2024-04-24 4:26 ` [PATCH v7 2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot Zijun Hu 2024-04-24 4:34 ` Greg KH 2024-04-26 20:42 ` Wren Turkal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).