diff mbox series

[v6,1/2] Bluetooth: qca: Fix BT enable failure for QCA6390

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

Commit Message

quic_zijuhu April 24, 2024, 12:46 a.m. UTC
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(+)

Comments

bluez.test.bot@gmail.com April 24, 2024, 2:40 a.m. UTC | #1
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
Krzysztof Kozlowski April 24, 2024, 4:06 a.m. UTC | #2
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
Krzysztof Kozlowski April 24, 2024, 4:10 a.m. UTC | #3
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
Krzysztof Kozlowski April 24, 2024, 4:31 a.m. UTC | #4
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
Wren Turkal April 24, 2024, 5:04 a.m. UTC | #5
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
>
Bartosz Golaszewski April 24, 2024, 1:52 p.m. UTC | #6
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
quic_zijuhu April 24, 2024, 2:01 p.m. UTC | #7
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
>>>>
>>>
>>
> 
>
Luiz Augusto von Dentz April 24, 2024, 2:02 p.m. UTC | #8
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?
quic_zijuhu April 24, 2024, 2:08 p.m. UTC | #9
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
quic_zijuhu April 24, 2024, 2:12 p.m. UTC | #10
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
> 
> 
>
Bartosz Golaszewski April 24, 2024, 2:19 p.m. UTC | #11
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
Bartosz Golaszewski April 24, 2024, 2:41 p.m. UTC | #12
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
quic_zijuhu April 26, 2024, 2:45 a.m. UTC | #13
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 mbox series

Patch

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);