| Message ID | 1713919602-5812-2-git-send-email-quic_zijuhu@quicinc.com |
|---|---|
| State | New |
| 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=847233
---Test result---
Test Summary:
CheckPatch FAIL 1.47 seconds
GitLint FAIL 0.73 seconds
SubjectPrefix PASS 0.18 seconds
BuildKernel PASS 30.08 seconds
CheckAllWarning PASS 32.38 seconds
CheckSparse PASS 37.95 seconds
CheckSmatch FAIL 35.95 seconds
BuildKernel32 PASS 28.92 seconds
TestRunnerSetup PASS 520.94 seconds
TestRunner_l2cap-tester PASS 20.19 seconds
TestRunner_iso-tester PASS 30.42 seconds
TestRunner_bnep-tester PASS 4.68 seconds
TestRunner_mgmt-tester PASS 116.79 seconds
TestRunner_rfcomm-tester PASS 7.24 seconds
TestRunner_sco-tester PASS 14.86 seconds
TestRunner_ioctl-tester PASS 7.60 seconds
TestRunner_mesh-tester PASS 5.75 seconds
TestRunner_smp-tester PASS 6.64 seconds
TestRunner_userchan-tester PASS 4.81 seconds
IncrementalBuild PASS 33.01 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v6,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
#119:
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/13640915.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:
[v6,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;"
24: B1 Line exceeds max length (139>80): "Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
30: B2 Line has trailing whitespace: "V1 -> V3: Don't revert the whole wrong commit but focus on impacted device "
[v6,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): "[v6,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....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
---
Regards,
Linux Bluetooth
On 24/04/2024 02:46, 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()") > 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> No, Bartosz' patch should go. Best regards, Krzysztof
On 24/04/2024 06:07, quic_zijuhu wrote: > On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: >> On 24/04/2024 02:46, 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()") >>> 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> >> >> No, Bartosz' patch should go. >> > what is Bartosz' patch. Srsly? You were Cc'ed on it. How many upstream patches on upstream mailing lists do you receive that you lost track of them? Best regards, Krzysztof
On 24/04/2024 06:18, quic_zijuhu wrote: > On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: >> On 24/04/2024 06:07, quic_zijuhu wrote: >>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: >>>> On 24/04/2024 02:46, 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()") >>>>> 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> >>>> >>>> No, Bartosz' patch should go. >>>> >>> what is Bartosz' patch. >> >> Srsly? You were Cc'ed on it. How many upstream patches on upstream >> mailing lists do you receive that you lost track of them? >> > Bartosz' patch have basic serious mistook and logic error and have no > any help for QCA6390, and it is not suitable regarding DTS usage. Really? Why you did not respond with comments then? Considering how imprecise and vague you are in describing real issues, I have doubts in your judgment here as well. Best regards, Krzysztof
On 4/23/24 9:31 PM, Krzysztof Kozlowski wrote: > On 24/04/2024 06:18, quic_zijuhu wrote: >> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: >>> On 24/04/2024 06:07, quic_zijuhu wrote: >>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: >>>>> On 24/04/2024 02:46, 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()") >>>>>> 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> >>>>> >>>>> No, Bartosz' patch should go. >>>>> >>>> what is Bartosz' patch. >>> >>> Srsly? You were Cc'ed on it. How many upstream patches on upstream >>> mailing lists do you receive that you lost track of them? >>> >> Bartosz' patch have basic serious mistook and logic error and have no >> any help for QCA6390, and it is not suitable regarding DTS usage. > > Really? Why you did not respond with comments then? Considering how > imprecise and vague you are in describing real issues, I have doubts in > your judgment here as well. Please slow down here. Zijun's patch works and Bartosz's patch does not. I don't think Zijun means any ill intent. I am replying to Bartosz's patch right now. > > Best regards, > Krzysztof >
On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Wren, > > On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <wt@penguintechs.org> wrote: > > > > On 4/23/24 9:31 PM, Krzysztof Kozlowski wrote: > > > On 24/04/2024 06:18, quic_zijuhu wrote: > > >> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: > > >>> On 24/04/2024 06:07, quic_zijuhu wrote: > > >>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: > > >>>>> On 24/04/2024 02:46, 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()") > > >>>>>> 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> > > >>>>> > > >>>>> No, Bartosz' patch should go. > > >>>>> > > >>>> what is Bartosz' patch. > > >>> > > >>> Srsly? You were Cc'ed on it. How many upstream patches on upstream > > >>> mailing lists do you receive that you lost track of them? > > >>> > > >> Bartosz' patch have basic serious mistook and logic error and have no > > >> any help for QCA6390, and it is not suitable regarding DTS usage. > > > > > > Really? Why you did not respond with comments then? Considering how > > > imprecise and vague you are in describing real issues, I have doubts in > > > your judgment here as well. > > > > Please slow down here. Zijun's patch works and Bartosz's patch does not. > > I don't think Zijun means any ill intent. I am replying to Bartosz's > > patch right now. > > Ok, that is great feedback, so I might be picking up the Zijun v7 set > if we don't find any major problems with it. > Luiz, Please consider my alternative[1] also tested by Wren. Zijun's usage of GPIO API is wrong. Bart [1] https://lore.kernel.org/linux-bluetooth/CAMRc=MfJ1v3pAB+Wvu1ahJAUvDfk3OsN5nieA-EYgTXPwMzqyg@mail.gmail.com/T/#mbf94795476d21df0a24441470ce02def9d2970a7
On 4/24/2024 9:56 PM, Luiz Augusto von Dentz wrote: > Hi Quic_zijuhu, > > On Wed, Apr 24, 2024 at 1:33 AM quic_zijuhu <quic_zijuhu@quicinc.com> wrote: >> >> On 4/24/2024 1:04 PM, Wren Turkal wrote: >>> On 4/23/24 9:31 PM, Krzysztof Kozlowski wrote: >>>> On 24/04/2024 06:18, quic_zijuhu wrote: >>>>> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: >>>>>> On 24/04/2024 06:07, quic_zijuhu wrote: >>>>>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: >>>>>>>> On 24/04/2024 02:46, 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()") >>>>>>>>> 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> >>>>>>>> >>>>>>>> No, Bartosz' patch should go. >>>>>>>> >>>>>>> what is Bartosz' patch. >>>>>> >>>>>> Srsly? You were Cc'ed on it. How many upstream patches on upstream >>>>>> mailing lists do you receive that you lost track of them? >>>>>> >>>>> Bartosz' patch have basic serious mistook and logic error and have no >>>>> any help for QCA6390, and it is not suitable regarding DTS usage. >>>> >>>> Really? Why you did not respond with comments then? Considering how >>>> imprecise and vague you are in describing real issues, I have doubts in >>>> your judgment here as well. >>> >>> Please slow down here. Zijun's patch works and Bartosz's patch does not. >>> I don't think Zijun means any ill intent. I am replying to Bartosz's >>> patch right now. >>> >> this is reporter's latest verification results. actually i don't have >> much time for kernel upstream. i really hope my fix is able to merged >> ASAP, it will help us to solve other possible impacted QCA controllers. > > Well I really hope we get some more support upstream because things > don't look quite clean right now and it should be a lesson that you > guys need to spend more time reviewing what goes upstream otherwise > things escalate since there isn't much documentation about your > hardware we can rely on. > thanks for your reminder. i will push company setup bluez team for QCA BT driver. >>>> >>>> Best regards, >>>> Krzysztof >>>> >>> >> > >
Hi Bartosz, On Wed, Apr 24, 2024 at 9:52 AM Bartosz Golaszewski <bartosz.golaszewski@linaro.org> wrote: > > On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Wren, > > > > On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <wt@penguintechs.org> wrote: > > > > > > On 4/23/24 9:31 PM, Krzysztof Kozlowski wrote: > > > > On 24/04/2024 06:18, quic_zijuhu wrote: > > > >> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: > > > >>> On 24/04/2024 06:07, quic_zijuhu wrote: > > > >>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: > > > >>>>> On 24/04/2024 02:46, 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()") > > > >>>>>> 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> > > > >>>>> > > > >>>>> No, Bartosz' patch should go. > > > >>>>> > > > >>>> what is Bartosz' patch. > > > >>> > > > >>> Srsly? You were Cc'ed on it. How many upstream patches on upstream > > > >>> mailing lists do you receive that you lost track of them? > > > >>> > > > >> Bartosz' patch have basic serious mistook and logic error and have no > > > >> any help for QCA6390, and it is not suitable regarding DTS usage. > > > > > > > > Really? Why you did not respond with comments then? Considering how > > > > imprecise and vague you are in describing real issues, I have doubts in > > > > your judgment here as well. > > > > > > Please slow down here. Zijun's patch works and Bartosz's patch does not. > > > I don't think Zijun means any ill intent. I am replying to Bartosz's > > > patch right now. > > > > Ok, that is great feedback, so I might be picking up the Zijun v7 set > > if we don't find any major problems with it. > > > > Luiz, > > Please consider my alternative[1] also tested by Wren. Zijun's usage > of GPIO API is wrong. > > Bart > > [1] https://lore.kernel.org/linux-bluetooth/CAMRc=MfJ1v3pAB+Wvu1ahJAUvDfk3OsN5nieA-EYgTXPwMzqyg@mail.gmail.com/T/#mbf94795476d21df0a24441470ce02def9d2970a7 @Wren Turkal How did you test this, what patches did you have?
On 4/24/2024 10:00 PM, Bartosz Golaszewski wrote: > On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <quic_zijuhu@quicinc.com> wrote: >> >>>>> >>>>> Please slow down here. Zijun's patch works and Bartosz's patch does not. >>>>> I don't think Zijun means any ill intent. I am replying to Bartosz's >>>>> patch right now. >>>> >>>> Ok, that is great feedback, so I might be picking up the Zijun v7 set >>>> if we don't find any major problems with it. >>>> >>> >>> Luiz, >>> >>> Please consider my alternative[1] also tested by Wren. Zijun's usage >>> of GPIO API is wrong. >>> >> why is it wrong ? >> > > I have already told you that at least three times. But whatever, let > me repeat again: gpiod_get_optional() returns NULL if the given GPIO > is not assigned to the device in question OR a pointer to a valid GPIO > descriptor. Anything else returned by it is an error and the driver > must abort probe(). > notice that i talked many times for you. the only different between my fix and present kernel code is that how to handle NULL case. for QCA6390. the GPIO is not marked as required by DTS binding spec. so, we don't need to take the case the gpio is not configured(return NULL) as error. i don't need to care about how to handle gpiod_get_optional() returning error case since my change don't change current handle logic for it. i currently only care about the issue reported. > Bart
On 4/24/2024 10:08 PM, Luiz Augusto von Dentz wrote: > Hi Bartosz, > > On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski > <bartosz.golaszewski@linaro.org> wrote: >> >> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <quic_zijuhu@quicinc.com> wrote: >>> >>>>>> >>>>>> Please slow down here. Zijun's patch works and Bartosz's patch does not. >>>>>> I don't think Zijun means any ill intent. I am replying to Bartosz's >>>>>> patch right now. >>>>> >>>>> Ok, that is great feedback, so I might be picking up the Zijun v7 set >>>>> if we don't find any major problems with it. >>>>> >>>> >>>> Luiz, >>>> >>>> Please consider my alternative[1] also tested by Wren. Zijun's usage >>>> of GPIO API is wrong. >>>> >>> why is it wrong ? >>> >> >> I have already told you that at least three times. But whatever, let >> me repeat again: gpiod_get_optional() returns NULL if the given GPIO >> is not assigned to the device in question OR a pointer to a valid GPIO >> descriptor. Anything else returned by it is an error and the driver >> must abort probe(). > > Ok, but there are other fixes on top of it: > as i commented with Bartosz's solution, it maybe break lunched product's BT functionality for his solution. > https://patchwork.kernel.org/project/bluetooth/patch/1713932807-19619-3-git-send-email-quic_zijuhu@quicinc.com/ > > I guess that could go in but it would really help if you guys could > work together so we don't have more competing solutions. > >> >> Bart > > >
On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Bartosz, > > On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski > <bartosz.golaszewski@linaro.org> wrote: > > > > On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <quic_zijuhu@quicinc.com> wrote: > > > > > > >>> > > > >>> Please slow down here. Zijun's patch works and Bartosz's patch does not. > > > >>> I don't think Zijun means any ill intent. I am replying to Bartosz's > > > >>> patch right now. > > > >> > > > >> Ok, that is great feedback, so I might be picking up the Zijun v7 set > > > >> if we don't find any major problems with it. > > > >> > > > > > > > > Luiz, > > > > > > > > Please consider my alternative[1] also tested by Wren. Zijun's usage > > > > of GPIO API is wrong. > > > > > > > why is it wrong ? > > > > > > > I have already told you that at least three times. But whatever, let > > me repeat again: gpiod_get_optional() returns NULL if the given GPIO > > is not assigned to the device in question OR a pointer to a valid GPIO > > descriptor. Anything else returned by it is an error and the driver > > must abort probe(). > > Ok, but there are other fixes on top of it: > > https://patchwork.kernel.org/project/bluetooth/patch/1713932807-19619-3-git-send-email-quic_zijuhu@quicinc.com/ > > I guess that could go in but it would really help if you guys could > work together so we don't have more competing solutions. > These threads with their 7 patch versions from Zijun within 2 days or so have become very chaotic. Let me summarize: there are two regressions: one caused by my commit 6845667146a2 ("Bluetooth: hci_qca: Fix NULL vs IS_ERR_OR_NULL check in qca_serdev_probe") and a second caused by Krzysztof's commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev"). The patch I linked here is how I propose to fix my regression only. These fixes don't seem to conflict with one another. We (Krzysztof and I) have provided feedback to Zijun but he refused to address it and instead kept on resending his patches every couple hours. Zijun's patch 1/2 proposed to revert my commit 6845667146a2. I disagreed and proposed a way forward by fixing the regression. This fix was incorrect as pointed out by Wren, so I submitted v2 which works. Bartosz
On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <quic_zijuhu@quicinc.com> wrote: > > On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote: > > On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > >> > >> Hi Bartosz, > >> > >> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski > >> <bartosz.golaszewski@linaro.org> wrote: > >>> > >>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <quic_zijuhu@quicinc.com> wrote: > >>>> > >>>>>>> > >>>>>>> Please slow down here. Zijun's patch works and Bartosz's patch does not. > >>>>>>> I don't think Zijun means any ill intent. I am replying to Bartosz's > >>>>>>> patch right now. > >>>>>> > >>>>>> Ok, that is great feedback, so I might be picking up the Zijun v7 set > >>>>>> if we don't find any major problems with it. > >>>>>> > >>>>> > >>>>> Luiz, > >>>>> > >>>>> Please consider my alternative[1] also tested by Wren. Zijun's usage > >>>>> of GPIO API is wrong. > >>>>> > >>>> why is it wrong ? > >>>> > >>> > >>> I have already told you that at least three times. But whatever, let > >>> me repeat again: gpiod_get_optional() returns NULL if the given GPIO > >>> is not assigned to the device in question OR a pointer to a valid GPIO > >>> descriptor. Anything else returned by it is an error and the driver > >>> must abort probe(). > >> > >> Ok, but there are other fixes on top of it: > >> > >> https://patchwork.kernel.org/project/bluetooth/patch/1713932807-19619-3-git-send-email-quic_zijuhu@quicinc.com/ > >> > >> I guess that could go in but it would really help if you guys could > >> work together so we don't have more competing solutions. > >> > > > > These threads with their 7 patch versions from Zijun within 2 days or > > so have become very chaotic. Let me summarize: there are two > > regressions: one caused by my commit 6845667146a2 ("Bluetooth: > > hci_qca: Fix NULL vs IS_ERR_OR_NULL check in qca_serdev_probe") and a > > second caused by Krzysztof's commit 272970be3dab ("Bluetooth: hci_qca: > > Fix driver shutdown on closed serdev"). The patch I linked here is how > > I propose to fix my regression only. These fixes don't seem to > > conflict with one another. > > > it is not conflict issue, from my perspective, you fix are wrong. > do you see my patch change log? > > > We (Krzysztof and I) have provided feedback to Zijun but he refused to > > address it and instead kept on resending his patches every couple > > hours. Zijun's patch 1/2 proposed to revert my commit 6845667146a2. I > > disagreed and proposed a way forward by fixing the regression. This > > fix was incorrect as pointed out by Wren, so I submitted v2 which > > works. > > > v2 is not right from my point as i commented with your solution. > > you don't answer my questions commented within your solution. > > what is your question i don't answer? > > > Bartosz > Luiz, This is an example of how Zijun will borrow any attempt at meaningful communication under a heap of incomprehensible emails. Krzysztof has already given up and I think I will stop too now. As the GPIO maintainer I suggest you take my fix for this regression. I can't make you though and I've already wasted way too much time on it. Your call. Bartosz
On 4/26/2024 1:37 AM, Elliot Berman wrote: > Hi Zijun, > > On Wed, Apr 24, 2024 at 01:33:50PM +0800, quic_zijuhu wrote: >> On 4/24/2024 1:04 PM, Wren Turkal wrote: >>> On 4/23/24 9:31 PM, Krzysztof Kozlowski wrote: >>>> On 24/04/2024 06:18, quic_zijuhu wrote: >>>>> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote: >>>>>> On 24/04/2024 06:07, quic_zijuhu wrote: >>>>>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote: >>>>>>>> On 24/04/2024 02:46, 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()") >>>>>>>>> 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> >>>>>>>> >>>>>>>> No, Bartosz' patch should go. >>>>>>>> >>>>>>> what is Bartosz' patch. >>>>>> >>>>>> Srsly? You were Cc'ed on it. How many upstream patches on upstream >>>>>> mailing lists do you receive that you lost track of them? >>>>>> >>>>> Bartosz' patch have basic serious mistook and logic error and have no >>>>> any help for QCA6390, and it is not suitable regarding DTS usage. >>>> >>>> Really? Why you did not respond with comments then? Considering how >>>> imprecise and vague you are in describing real issues, I have doubts in >>>> your judgment here as well. >>> >>> Please slow down here. Zijun's patch works and Bartosz's patch does not. >>> I don't think Zijun means any ill intent. I am replying to Bartosz's >>> patch right now. >>> >> this is reporter's latest verification results. actually i don't have >> much time for kernel upstream. i really hope my fix is able to merged >> ASAP, it will help us to solve other possible impacted QCA controllers. > > I saw you were planning to slow down for a minute. When you're ready to > work on these patches again, let's get them reviewed internally first. > Please check go/upstream for some helpful hints. > thank you @Elliot for your reminder. i will learn go/upstream again before next upstream, and i will request for internal code review before do external upstream for further upstream. thank you Elliot again. > Thanks, > Elliot >
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()") 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: 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(+)