| Message ID | 1713932807-19619-2-git-send-email-quic_zijuhu@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix two BT regression issues for QCA6390 | expand |
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
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
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 >
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?
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
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);
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(+)