diff mbox series

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

Message ID 1713932807-19619-2-git-send-email-quic_zijuhu@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Fix two BT regression issues for QCA6390 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning 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.
tedd_an/GitLint fail 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 "
tedd_an/SubjectPrefix success Gitlint PASS
tedd_an/BuildKernel success BuildKernel PASS
tedd_an/CheckAllWarning success CheckAllWarning PASS
tedd_an/CheckSparse success CheckSparse PASS
tedd_an/CheckSmatch fail CheckSparse: FAIL: 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
tedd_an/BuildKernel32 success BuildKernel32 PASS
tedd_an/TestRunnerSetup success TestRunnerSetup PASS
tedd_an/TestRunner_l2cap-tester success TestRunner PASS
tedd_an/TestRunner_iso-tester success TestRunner PASS
tedd_an/TestRunner_bnep-tester success TestRunner PASS
tedd_an/TestRunner_mgmt-tester success TestRunner PASS
tedd_an/TestRunner_rfcomm-tester success TestRunner PASS
tedd_an/TestRunner_sco-tester success TestRunner PASS
tedd_an/TestRunner_ioctl-tester success TestRunner PASS
tedd_an/TestRunner_mesh-tester success TestRunner PASS
tedd_an/TestRunner_smp-tester success TestRunner PASS
tedd_an/TestRunner_userchan-tester success TestRunner PASS
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

quic_zijuhu April 24, 2024, 4:26 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()")
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(+)

Comments

Krzysztof Kozlowski April 24, 2024, 4:30 a.m. UTC | #1
On 24/04/2024 06:26, Zijun Hu wrote:
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()") will cause below serious regression issue:
> 
> BT can't be enabled any more after below steps:
> cold boot -> enable BT -> disable BT -> BT enable failure
> if property enable-gpios is not configured within DT|ACPI for QCA6390.
> 
> The commit wrongly changes flag @power_ctrl_enabled set logic for this
> case as shown by its below code applet and causes this serious issue.
> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>                                                GPIOD_OUT_LOW);
> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
> + if (IS_ERR(qcadev->bt_en)) {
>   	dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> 	power_ctrl_enabled = false;
>   }
> 
> Fixed by reverting the mentioned commit for QCA6390.
> 
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Cc: stable@vge.kernel.org
> Reported-by: Wren Turkal <wt@penguintechs.org>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> Tested-by: Wren Turkal <wt@penguintechs.org>
> ---
> Changes:
> V6 -> V7: Add stable tag

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

Best regards,
Krzysztof
bluez.test.bot@gmail.com April 24, 2024, 4:56 a.m. UTC | #2
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
quic_zijuhu April 24, 2024, 5:02 a.m. UTC | #3
On 4/24/2024 12:30 PM, Krzysztof Kozlowski wrote:
> On 24/04/2024 06:26, Zijun Hu wrote:
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()") will cause below serious regression issue:
>>
>> BT can't be enabled any more after below steps:
>> cold boot -> enable BT -> disable BT -> BT enable failure
>> if property enable-gpios is not configured within DT|ACPI for QCA6390.
>>
>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>> case as shown by its below code applet and causes this serious issue.
>> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>                                                GPIOD_OUT_LOW);
>> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>> + if (IS_ERR(qcadev->bt_en)) {
>>   	dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> 	power_ctrl_enabled = false;
>>   }
>>
>> Fixed by reverting the mentioned commit for QCA6390.
>>
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Cc: stable@vge.kernel.org
>> Reported-by: Wren Turkal <wt@penguintechs.org>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> Tested-by: Wren Turkal <wt@penguintechs.org>
>> ---
>> Changes:
>> V6 -> V7: Add stable tag
> 
> Stop sending multiple pathchsets per day. I already asked you to first
> finish discussion and then send new version. You again start sending
> something while previous discussion is going.
>you concern is wrong and i am sure it don't block me sending new patch
sets to solve other issue. so i send this v7.

i have give reply for Bartosz' patch.

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

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

sorry to correct a typo error.

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

i have give reply for Bartosz' patch.

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

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

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

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

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

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

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

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

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

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

> Thanks,
> Bartosz
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);