This patch series are to fix below 2 BT regression issues for QCA6390
1) 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.
2) BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA6390.
Fix solution has been verified by the reported device Dell XPS 13 9310
laptop over below bluetooth-next tree commit as the last bugzilla comment
commit 6abf9dd26bb1 ("Bluetooth: qca: Fix triggering coredump
implementation").
These two issues were initially reported at below link:
https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
then reported to bugzilla as shown by below link:
https://bugzilla.kernel.org/show_bug.cgi?id=218726
the previous discussion link is listed below
https://lore.kernel.org/linux-bluetooth/[email protected]/
Zijun Hu (2):
Bluetooth: qca: Fix BT enable failure for QCA6390
Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then
warm reboot
drivers/bluetooth/hci_qca.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
2.7.4
From: Zijun Hu <[email protected]>
Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev") will cause below regression issue:
BT can't be enabled after below steps:
cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
if property enable-gpios is not configured within DT|ACPI for QCA6390.
The commit is to fix a use-after-free issue within qca_serdev_shutdown()
during reboot, but also introduces this regression issue regarding above
steps since the VSC is not sent to reset controller during warm reboot.
Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
once BT was ever enabled, and the use-after-free issue is also be fixed
by this change since serdev is still opened when send to serdev.
Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
Cc: [email protected]
Reported-by: Wren Turkal <[email protected]>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
Changes:
V6 -> V7: Add stable tag
V3 -> V6: Correct title and commit message
V1 -> V3: Remove debugging logs
drivers/bluetooth/hci_qca.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 4079254fb1c8..fc027da98297 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2439,13 +2439,12 @@ static void qca_serdev_shutdown(struct device *dev)
struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
struct hci_uart *hu = &qcadev->serdev_hu;
struct hci_dev *hdev = hu->hdev;
- struct qca_data *qca = hu->priv;
const u8 ibs_wake_cmd[] = { 0xFD };
const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
if (qcadev->btsoc_type == QCA_QCA6390) {
- if (test_bit(QCA_BT_OFF, &qca->flags) ||
- !test_bit(HCI_RUNNING, &hdev->flags))
+ if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks) ||
+ hci_dev_test_flag(hdev, HCI_SETUP))
return;
serdev_device_write_flush(serdev);
--
2.7.4
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: [email protected]
Reported-by: Wren Turkal <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
Signed-off-by: Zijun Hu <[email protected]>
Tested-by: Wren Turkal <[email protected]>
---
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(+)
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);
--
2.7.4
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: [email protected]
> Reported-by: Wren Turkal <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> Signed-off-by: Zijun Hu <[email protected]>
> Tested-by: Wren Turkal <[email protected]>
> ---
> 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
On Wed, Apr 24, 2024 at 12:26:47PM +0800, Zijun Hu wrote:
> From: Zijun Hu <[email protected]>
>
> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev") will cause below regression issue:
>
> BT can't be enabled after below steps:
> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
> if property enable-gpios is not configured within DT|ACPI for QCA6390.
>
> The commit is to fix a use-after-free issue within qca_serdev_shutdown()
> during reboot, but also introduces this regression issue regarding above
> steps since the VSC is not sent to reset controller during warm reboot.
>
> Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
> once BT was ever enabled, and the use-after-free issue is also be fixed
> by this change since serdev is still opened when send to serdev.
>
> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
> Cc: [email protected]
That is not a valid email address :(
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 <[email protected]>
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/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
32: B2 Line has trailing whitespace: "V1 -> V3: Don't revert the whole wrong commit but focus on impacted device "
[v7,2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (89>80): "[v7,2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
---
Regards,
Linux Bluetooth
On 4/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: [email protected]
>> Reported-by: Wren Turkal <[email protected]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <[email protected]>
>> Tested-by: Wren Turkal <[email protected]>
>> ---
>> 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
>
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.
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: [email protected]
>>> Reported-by: Wren Turkal <[email protected]>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>> Link: https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>> Signed-off-by: Zijun Hu <[email protected]>
>>> Tested-by: Wren Turkal <[email protected]>
>>> ---
>>> 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
--
You're more amazing than you think!
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: [email protected]
>>>> Reported-by: Wren Turkal <[email protected]>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> Link:
>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>> Tested-by: Wren Turkal <[email protected]>
>>>> ---
>>>> 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
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: [email protected]
>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>> Link:
>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>> ---
>>>>> 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
>
--
You're more amazing than you think!
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: [email protected]
>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Link:
>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>> ---
>>>>>> 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
>>
>
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: [email protected]
>>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>>> ---
>>>>>>> Changes:
>>>>>>> V6 -> V7: Add stable tag
>>>>>>
>>>>>> Stop sending multiple pathchsets per day. I already asked you to first
>>>>>> finish discussion and then send new version. You again start sending
>>>>>> something while previous discussion is going.
>>>>>> you concern is wrong and i am sure it don't block me sending new patch
>>>>> sets to solve other issue. so i send this v7.
>>>>>
>>>>> i have give reply for Bartosz' patch.
>>>>>
>>>>> i hop you as DTS expert to notice my concern about DTS in the reply.
>>>>
>>>> Are you saying here (1) that you identified a problem in the DTs that
>>>> you hope Krzysztof notices or (2) that you want Krzysztof to notice how
>>>> your description of way that DT declares the gpio as required affects
>>>> your proposed change. As a native American English speaker, I am finding
>>>> your text hard to follow.
>>>>
>>> 1) is my purpose. i have given my concern about DTS for Bartosz' patch
>>> and hope DTS expert notice the concern.
>>>
>>> my change don't have any such concern about DTS usage. that is why i
>>> changed my fix from original reverting the whole wrong commit to now
>>> focusing on QCA6390.
>>
>> Let me try to parse this. If #1 is the correct interpretation, does that
>> mean that the DTs are wrong and need to be changed? Do you expect K to
>> do that since he's the "DTS expert"?
>>
> for your 1) question, NO
> for your 2) question, need DTS expert notice or suggest how to handle
> case that a DTS property is marked as required but not be configed by user.
>
>>>> I think you are saying #2.
>>>>
>>>> I just want to make sure I am following the discussion here.
>>>>
>>>> wt
>>>
>>
>
Hi Krzysztof, bartosz.
do you have any concern for this patch serials?
On 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: [email protected]
>>>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>> Link:
>>>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
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: [email protected]
>>>>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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
>
>
On Wed, 24 Apr 2024 at 11:40, quic_zijuhu <[email protected]> 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: [email protected]
> >>>>>>> Reported-by: Wren Turkal <[email protected]>
> >>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >>>>>>> Link:
> >>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> >>>>>>> Signed-off-by: Zijun Hu <[email protected]>
> >>>>>>> Tested-by: Wren Turkal <[email protected]>
> >>>>>>> ---
> >>>>>>> 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
On 4/24/2024 6:34 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 11:40, quic_zijuhu <[email protected]> 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: [email protected]
>>>>>>>>> Reported-by: Wren Turkal <[email protected]>
>>>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>>>>> Link:
>>>>>>>>> https://lore.kernel.org/linux-bluetooth/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>>>>>>>>> Signed-off-by: Zijun Hu <[email protected]>
>>>>>>>>> Tested-by: Wren Turkal <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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
On Tuesday, April 23, 2024 9:34:43 PM PDT Greg KH wrote:
> On Wed, Apr 24, 2024 at 12:26:47PM +0800, Zijun Hu wrote:
> > From: Zijun Hu <[email protected]>
> >
> > Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> > serdev") will cause below regression issue:
> >
> > BT can't be enabled after below steps:
> > cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
> > if property enable-gpios is not configured within DT|ACPI for QCA6390.
> >
> > The commit is to fix a use-after-free issue within qca_serdev_shutdown()
> > during reboot, but also introduces this regression issue regarding above
> > steps since the VSC is not sent to reset controller during warm reboot.
> >
> > Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
> > once BT was ever enabled, and the use-after-free issue is also be fixed
> > by this change since serdev is still opened when send to serdev.
> >
> > Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> > serdev") Cc: [email protected]
>
> That is not a valid email address :(
@ziljun, can you please post this patch in a new patch series by itself? The
warm boot problem is fixed by this patch, and I think it would be better to
have a thread dedicated to this fix so that it doesn't get lost.
FWIW, I think that this may be the correct technical change. I would like to
try to help get this patch landed, and I think a clean thread for only this
change would help.
Just so everyone is aware, I am a concerned user of the hardware, and not a
kernel dev myself. I have tested this change. This patch on top of Linus'
mainline does allow qca6390 to work after warm reboot on my laptop.
wt
--
You're more amazing than you think!