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

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 <[e-mail]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Signed-off-by: Zijun Hu <[e-mail]>
Tested-by: Wren Turkal <[e-mail]>
---
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

[e-mail] 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 <[e-mail]>
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/[e-mail]/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

Greg KH April 24, 2024, 3:21 a.m. UTC | #2

On Wed, Apr 24, 2024 at 08:46:41AM +0800, 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 <[e-mail]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <[e-mail]>
> Tested-by: Wren Turkal <[e-mail]>
> ---
> 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 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Krzysztof Kozlowski April 24, 2024, 4:06 a.m. UTC | #3

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 <[e-mail]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <[e-mail]>
> Tested-by: Wren Turkal <[e-mail]>

No, Bartosz' patch should go.

Best regards,
Krzysztof

quic_zijuhu April 24, 2024, 4:07 a.m. UTC | #4

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 <[e-mail]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <[e-mail]>
>> Tested-by: Wren Turkal <[e-mail]>
> 
> No, Bartosz' patch should go.
> 
what is Bartosz' patch.

> Best regards,
> Krzysztof
>

Krzysztof Kozlowski April 24, 2024, 4:10 a.m. UTC | #5

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 <[e-mail]>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>> Signed-off-by: Zijun Hu <[e-mail]>
>>> Tested-by: Wren Turkal <[e-mail]>
>>
>> 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

quic_zijuhu April 24, 2024, 4:18 a.m. UTC | #6

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 <[e-mail]>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>> Tested-by: Wren Turkal <[e-mail]>
>>>
>>> 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.
if below link is Bartosz' patch
https://patchwork.kernel.org/project/bluetooth/patch/[e-mail]/


do you really code review for Bartosz' patch before give your
Reviewed-by: Krzysztof Kozlowski <[e-mail]>

> Best regards,
> Krzysztof
>

Krzysztof Kozlowski April 24, 2024, 4:31 a.m. UTC | #7

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 <[e-mail]>
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>
>>>> 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

quic_zijuhu April 24, 2024, 4:33 a.m. UTC | #8

On 4/24/2024 12: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 <[e-mail]>
>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>
>>>>> 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.
> let me give comments for this change now.
> Best regards,
> Krzysztof
>

Wren Turkal April 24, 2024, 5:04 a.m. UTC | #9

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 <[e-mail]>
>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>
>>>>> 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
>

quic_zijuhu April 24, 2024, 5:33 a.m. UTC | #10

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 <[e-mail]>
>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>>
>>>>>> 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.
>>
>> Best regards,
>> Krzysztof
>>
>

Luiz Augusto von Dentz April 24, 2024, 1:49 p.m. UTC | #11

Hi Wren,

On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[e-mail]> 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 <[e-mail]>
> >>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> >>>>>> Signed-off-by: Zijun Hu <[e-mail]>
> >>>>>> Tested-by: Wren Turkal <[e-mail]>
> >>>>>
> >>>>> 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.

> >
> > Best regards,
> > Krzysztof
> >
>
> --
> You're more amazing than you think!

quic_zijuhu April 24, 2024, 1:51 p.m. UTC | #12

On 4/24/2024 9:49 PM, Luiz Augusto von Dentz wrote:
> Hi Wren,
> 
> On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[e-mail]> 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 <[e-mail]>
>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>>>
>>>>>>> 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.
> 
thank you, we will start to fix this issue for other product customer
have reported to us with different fix.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> --
>> You're more amazing than you think!
> 
> 
>

Bartosz Golaszewski April 24, 2024, 1:52 p.m. UTC | #13

On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Wren,
>
> On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[e-mail]> 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 <[e-mail]>
> > >>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> > >>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> > >>>>>> Signed-off-by: Zijun Hu <[e-mail]>
> > >>>>>> Tested-by: Wren Turkal <[e-mail]>
> > >>>>>
> > >>>>> 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=[e-mail]/T/#mbf94795476d21df0a24441470ce02def9d2970a7

quic_zijuhu April 24, 2024, 1:53 p.m. UTC | #14

On 4/24/2024 9:52 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
> <[e-mail]> wrote:
>>
>> Hi Wren,
>>
>> On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[e-mail]> 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 <[e-mail]>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>>>>
>>>>>>>> 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.
>
why is it wrong ?

> Bart
> 
> [1] https://lore.kernel.org/linux-bluetooth/CAMRc=[e-mail]/T/#mbf94795476d21df0a24441470ce02def9d2970a7

Luiz Augusto von Dentz April 24, 2024, 1:56 p.m. UTC | #15

Hi Quic_zijuhu,

On Wed, Apr 24, 2024 at 1:33 AM quic_zijuhu <[e-mail]> 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 <[e-mail]>
> >>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >>>>>>> Link:
> >>>>>>> https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> >>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
> >>>>>>> Tested-by: Wren Turkal <[e-mail]>
> >>>>>>
> >>>>>> 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.

> >>
> >> Best regards,
> >> Krzysztof
> >>
> >
>

Bartosz Golaszewski April 24, 2024, 2 p.m. UTC | #16

On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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().

Bart

quic_zijuhu April 24, 2024, 2:01 p.m. UTC | #17

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 <[e-mail]> 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 <[e-mail]>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>>>>
>>>>>>>> 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 | #18

Hi Bartosz,

On Wed, Apr 24, 2024 at 9:52 AM Bartosz Golaszewski
<[e-mail]> wrote:
>
> On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Wren,
> >
> > On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[e-mail]> 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 <[e-mail]>
> > > >>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> > > >>>>>> Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> > > >>>>>> Signed-off-by: Zijun Hu <[e-mail]>
> > > >>>>>> Tested-by: Wren Turkal <[e-mail]>
> > > >>>>>
> > > >>>>> 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=[e-mail]/T/#mbf94795476d21df0a24441470ce02def9d2970a7

@Wren Turkal How did you test this, what patches did you have?

quic_zijuhu April 24, 2024, 2:08 p.m. UTC | #19

On 4/24/2024 10:00 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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

Luiz Augusto von Dentz April 24, 2024, 2:08 p.m. UTC | #20

Hi Bartosz,

On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
<[e-mail]> wrote:
>
> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/

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

quic_zijuhu April 24, 2024, 2:12 p.m. UTC | #21

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
> <[e-mail]> wrote:
>>
>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
> 
> 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 | #22

On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Bartosz,
>
> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> <[e-mail]> wrote:
> >
> > On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
>
> 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

quic_zijuhu April 24, 2024, 2:25 p.m. UTC | #23

On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> <[e-mail]> wrote:
>>
>> Hi Bartosz,
>>
>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>> <[e-mail]> wrote:
>>>
>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
>>
>> 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

Bartosz Golaszewski April 24, 2024, 2:41 p.m. UTC | #24

On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[e-mail]> wrote:
>
> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> >> <[e-mail]> wrote:
> >>>
> >>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
> >>
> >> 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

Krzysztof Kozlowski April 24, 2024, 2:44 p.m. UTC | #25

On 24/04/2024 16:41, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[e-mail]> wrote:
>>
>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>> <[e-mail]> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
>>>>
>>>> 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.

Yeah, I given up. It suck way too much of my time and effort. I will
just review Bartosz' patch for completeness.

Best regards,
Krzysztof

quic_zijuhu April 24, 2024, 2:46 p.m. UTC | #26

On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[e-mail]> wrote:
>>
>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>> <[e-mail]> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
>>>>
>>>> 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.
> 
how about GPIO maintainer? it is your change about GPIOs causes serious
regression issue.

i maybe send many mails. but dos it have any relevant my change's rightness.

do you find anything my change have wrong usage about GPIO about the
case i care about?

> Bartosz

Luiz Augusto von Dentz April 24, 2024, 3:31 p.m. UTC | #27

Hi Quic_zijuhu,

On Wed, Apr 24, 2024 at 10:46 AM quic_zijuhu <[e-mail]> wrote:
>
> On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[e-mail]> wrote:
> >>
> >> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> >>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> >>> <[e-mail]> wrote:
> >>>>
> >>>> Hi Bartosz,
> >>>>
> >>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> >>>> <[e-mail]> wrote:
> >>>>>
> >>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
> >>>>
> >>>> 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.
> >
> how about GPIO maintainer? it is your change about GPIOs causes serious
> regression issue.
>
> i maybe send many mails. but dos it have any relevant my change's rightness.

Well you are not making it any better if you are still claiming the
maintainer doesn't know what doing when you should really be thanking
him for looking into this, now perhaps his changes doesn't address a
particular problem you are trying to solve nevertheless it is worth
incorporating his changes in your set and then have yours on top
without reverting his changes? Can you do that or there is something
fundamentally broken with that.

Everyone here probably have their own assignments, so you are getting
sort of _free_ consultancy, so please instead continue disputing what
we are suggesting at least try to incorporate the suggested changes,
we want to have it fixed properly so we don't have to keep receiving
the same changes over and over again which is a waste of everyone's
time, including yours.

quic_zijuhu April 24, 2024, 3:42 p.m. UTC | #28

On 4/24/2024 11:31 PM, Luiz Augusto von Dentz wrote:
> Hi Quic_zijuhu,
> 
> On Wed, Apr 24, 2024 at 10:46 AM quic_zijuhu <[e-mail]> wrote:
>>
>> On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[e-mail]> wrote:
>>>>
>>>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>>>> <[e-mail]> wrote:
>>>>>>
>>>>>> Hi Bartosz,
>>>>>>
>>>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[e-mail]> 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/[e-mail]/
>>>>>>
>>>>>> 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.
>>>
>> how about GPIO maintainer? it is your change about GPIOs causes serious
>> regression issue.
>>
>> i maybe send many mails. but dos it have any relevant my change's rightness.
> 
> Well you are not making it any better if you are still claiming the
> maintainer doesn't know what doing when you should really be thanking
> him for looking into this, now perhaps his changes doesn't address a
> particular problem you are trying to solve nevertheless it is worth
> incorporating his changes in your set and then have yours on top
> without reverting his changes? Can you do that or there is something
> fundamentally broken with that.
> 
> Everyone here probably have their own assignments, so you are getting
> sort of _free_ consultancy, so please instead continue disputing what
> we are suggesting at least try to incorporate the suggested changes,
> we want to have it fixed properly so we don't have to keep receiving
> the same changes over and over again which is a waste of everyone's
> time, including yours.
> 

sorry for my offense.

suggest merge my change.

Elliot Berman April 25, 2024, 5:37 p.m. UTC | #29

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 <[e-mail]>
> >>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >>>>>>> Link:
> >>>>>>> https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> >>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
> >>>>>>> Tested-by: Wren Turkal <[e-mail]>
> >>>>>>
> >>>>>> 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.

Thanks,
Elliot

quic_zijuhu April 26, 2024, 2:45 a.m. UTC | #30

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 <[e-mail]>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/[e-mail]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <[e-mail]>
>>>>>>>>> Tested-by: Wren Turkal <[e-mail]>
>>>>>>>>
>>>>>>>> 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
>

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