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
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 <[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:
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
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")
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:
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
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 <[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/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/[email protected]/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f"
30: B2 Line has trailing whitespace: "V1 -> V3: Don't revert the whole wrong commit but focus on impacted device "
[v6,2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (89>80): "[v6,2/2] Bluetooth: qca: Fix BT enable failure for QCA6390 after disable then warm reboot"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
---
Regards,
Linux Bluetooth
On 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 <[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:
> 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
On Wed, Apr 24, 2024 at 08:46:42AM +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")
> 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:
> 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
>
>
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
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 <[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]>
No, Bartosz' patch should go.
Best regards,
Krzysztof
On 24/04/2024 06:07, quic_zijuhu wrote:
> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote:
>> On 24/04/2024 02:46, Zijun Hu wrote:
>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>> with gpiod_get_optional()") will cause below serious regression issue:
>>>
>>> BT can't be enabled any more after below steps:
>>> cold boot -> enable BT -> disable BT -> BT enable failure
>>> if property enable-gpios is not configured within DT|ACPI for QCA6390.
>>>
>>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>>> case as shown by its below code applet and causes this serious issue.
>>> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>> GPIOD_OUT_LOW);
>>> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>>> + if (IS_ERR(qcadev->bt_en)) {
>>> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>> power_ctrl_enabled = false;
>>> }
>>>
>>> Fixed by reverting the mentioned commit for QCA6390.
>>>
>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>> Reported-by: Wren Turkal <[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]>
>>
>> No, Bartosz' patch should go.
>>
> what is Bartosz' patch.
Srsly? You were Cc'ed on it. How many upstream patches on upstream
mailing lists do you receive that you lost track of them?
Best regards,
Krzysztof
On 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 <[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]>
>
> No, Bartosz' patch should go.
>
what is Bartosz' patch.
> Best regards,
> Krzysztof
>
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 <[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]>
>>>
>>> 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/[email protected]/
do you really code review for Bartosz' patch before give your
Reviewed-by: Krzysztof Kozlowski <[email protected]>
> Best regards,
> Krzysztof
>
On 24/04/2024 06:18, quic_zijuhu wrote:
> On 4/24/2024 12:10 PM, Krzysztof Kozlowski wrote:
>> On 24/04/2024 06:07, quic_zijuhu wrote:
>>> On 4/24/2024 12:06 PM, Krzysztof Kozlowski wrote:
>>>> On 24/04/2024 02:46, Zijun Hu wrote:
>>>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>>> with gpiod_get_optional()") will cause below serious regression issue:
>>>>>
>>>>> BT can't be enabled any more after below steps:
>>>>> cold boot -> enable BT -> disable BT -> BT enable failure
>>>>> if property enable-gpios is not configured within DT|ACPI for QCA6390.
>>>>>
>>>>> The commit wrongly changes flag @power_ctrl_enabled set logic for this
>>>>> case as shown by its below code applet and causes this serious issue.
>>>>> qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>> GPIOD_OUT_LOW);
>>>>> - if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>>>>> + if (IS_ERR(qcadev->bt_en)) {
>>>>> dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>> power_ctrl_enabled = false;
>>>>> }
>>>>>
>>>>> Fixed by reverting the mentioned commit for QCA6390.
>>>>>
>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>>> Reported-by: Wren Turkal <[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]>
>>>>
>>>> No, Bartosz' patch should go.
>>>>
>>> what is Bartosz' patch.
>>
>> Srsly? You were Cc'ed on it. How many upstream patches on upstream
>> mailing lists do you receive that you lost track of them?
>>
> Bartosz' patch have basic serious mistook and logic error and have no
> any help for QCA6390, and it is not suitable regarding DTS usage.
Really? Why you did not respond with comments then? Considering how
imprecise and vague you are in describing real issues, I have doubts in
your judgment here as well.
Best regards,
Krzysztof
On 4/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 <[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]>
>>>>>
>>>>> 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
>
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 <[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]>
>>>>>
>>>>> 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
>
--
You're more amazing than you think!
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 <[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]>
>>>>>>
>>>>>> 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
>>
>
Hi Wren,
On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[email protected]> 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 <[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]>
> >>>>>
> >>>>> 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!
--
Luiz Augusto von Dentz
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 <[email protected]> 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 <[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]>
>>>>>>>
>>>>>>> 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!
>
>
>
On 4/24/2024 9:52 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
> <[email protected]> wrote:
>>
>> Hi Wren,
>>
>> On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[email protected]> 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 <[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]>
>>>>>>>>
>>>>>>>> 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=MfJ1v3pAB+Wvu1ahJAUvDfk3OsN5nieA-EYgTXPwMzqyg@mail.gmail.com/T/#mbf94795476d21df0a24441470ce02def9d2970a7
On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Wren,
>
> On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[email protected]> 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 <[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]>
> > >>>>>
> > >>>>> No, Bartosz' patch should go.
> > >>>>>
> > >>>> what is Bartosz' patch.
> > >>>
> > >>> Srsly? You were Cc'ed on it. How many upstream patches on upstream
> > >>> mailing lists do you receive that you lost track of them?
> > >>>
> > >> Bartosz' patch have basic serious mistook and logic error and have no
> > >> any help for QCA6390, and it is not suitable regarding DTS usage.
> > >
> > > Really? Why you did not respond with comments then? Considering how
> > > imprecise and vague you are in describing real issues, I have doubts in
> > > your judgment here as well.
> >
> > Please slow down here. Zijun's patch works and Bartosz's patch does not.
> > I don't think Zijun means any ill intent. I am replying to Bartosz's
> > patch right now.
>
> Ok, that is great feedback, so I might be picking up the Zijun v7 set
> if we don't find any major problems with it.
>
Luiz,
Please consider my alternative[1] also tested by Wren. Zijun's usage
of GPIO API is wrong.
Bart
[1] https://lore.kernel.org/linux-bluetooth/CAMRc=MfJ1v3pAB+Wvu1ahJAUvDfk3OsN5nieA-EYgTXPwMzqyg@mail.gmail.com/T/#mbf94795476d21df0a24441470ce02def9d2970a7
On 4/24/2024 9:56 PM, Luiz Augusto von Dentz wrote:
> Hi Quic_zijuhu,
>
> On Wed, Apr 24, 2024 at 1:33 AM quic_zijuhu <[email protected]> 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 <[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]>
>>>>>>>>
>>>>>>>> No, Bartosz' patch should go.
>>>>>>>>
>>>>>>> what is Bartosz' patch.
>>>>>>
>>>>>> Srsly? You were Cc'ed on it. How many upstream patches on upstream
>>>>>> mailing lists do you receive that you lost track of them?
>>>>>>
>>>>> Bartosz' patch have basic serious mistook and logic error and have no
>>>>> any help for QCA6390, and it is not suitable regarding DTS usage.
>>>>
>>>> Really? Why you did not respond with comments then? Considering how
>>>> imprecise and vague you are in describing real issues, I have doubts in
>>>> your judgment here as well.
>>>
>>> Please slow down here. Zijun's patch works and Bartosz's patch does not.
>>> I don't think Zijun means any ill intent. I am replying to Bartosz's
>>> patch right now.
>>>
>> this is reporter's latest verification results. actually i don't have
>> much time for kernel upstream. i really hope my fix is able to merged
>> ASAP, it will help us to solve other possible impacted QCA controllers.
>
> Well I really hope we get some more support upstream because things
> don't look quite clean right now and it should be a lesson that you
> guys need to spend more time reviewing what goes upstream otherwise
> things escalate since there isn't much documentation about your
> hardware we can rely on.
>
thanks for your reminder. i will push company setup bluez team for QCA
BT driver.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>
>
>
Hi Bartosz,
On Wed, Apr 24, 2024 at 9:52 AM Bartosz Golaszewski
<[email protected]> wrote:
>
> On Wed, 24 Apr 2024 at 15:49, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Wren,
> >
> > On Wed, Apr 24, 2024 at 1:04 AM Wren Turkal <[email protected]> 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 <[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]>
> > > >>>>>
> > > >>>>> No, Bartosz' patch should go.
> > > >>>>>
> > > >>>> what is Bartosz' patch.
> > > >>>
> > > >>> Srsly? You were Cc'ed on it. How many upstream patches on upstream
> > > >>> mailing lists do you receive that you lost track of them?
> > > >>>
> > > >> Bartosz' patch have basic serious mistook and logic error and have no
> > > >> any help for QCA6390, and it is not suitable regarding DTS usage.
> > > >
> > > > Really? Why you did not respond with comments then? Considering how
> > > > imprecise and vague you are in describing real issues, I have doubts in
> > > > your judgment here as well.
> > >
> > > Please slow down here. Zijun's patch works and Bartosz's patch does not.
> > > I don't think Zijun means any ill intent. I am replying to Bartosz's
> > > patch right now.
> >
> > Ok, that is great feedback, so I might be picking up the Zijun v7 set
> > if we don't find any major problems with it.
> >
>
> Luiz,
>
> Please consider my alternative[1] also tested by Wren. Zijun's usage
> of GPIO API is wrong.
>
> Bart
>
> [1] https://lore.kernel.org/linux-bluetooth/CAMRc=MfJ1v3pAB+Wvu1ahJAUvDfk3OsN5nieA-EYgTXPwMzqyg@mail.gmail.com/T/#mbf94795476d21df0a24441470ce02def9d2970a7
@Wren Turkal How did you test this, what patches did you have?
--
Luiz Augusto von Dentz
Hi Quic_zijuhu,
On Wed, Apr 24, 2024 at 1:33 AM quic_zijuhu <[email protected]> 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 <[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]>
> >>>>>>
> >>>>>> 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
> >>
> >
>
--
Luiz Augusto von Dentz
On 4/24/2024 10:00 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> wrote:
>>
>>>>>
>>>>> Please slow down here. Zijun's patch works and Bartosz's patch does not.
>>>>> I don't think Zijun means any ill intent. I am replying to Bartosz's
>>>>> patch right now.
>>>>
>>>> Ok, that is great feedback, so I might be picking up the Zijun v7 set
>>>> if we don't find any major problems with it.
>>>>
>>>
>>> Luiz,
>>>
>>> Please consider my alternative[1] also tested by Wren. Zijun's usage
>>> of GPIO API is wrong.
>>>
>> why is it wrong ?
>>
>
> I have already told you that at least three times. But whatever, let
> me repeat again: gpiod_get_optional() returns NULL if the given GPIO
> is not assigned to the device in question OR a pointer to a valid GPIO
> descriptor. Anything else returned by it is an error and the driver
> must abort probe().
>
notice that i talked many times for you. the only different between my
fix and present kernel code is that how to handle NULL case.
for QCA6390. the GPIO is not marked as required by DTS binding spec.
so, we don't need to take the case the gpio is not configured(return
NULL) as error.
i don't need to care about how to handle gpiod_get_optional() returning
error case since my change don't change current handle logic for it. i
currently only care about the issue reported.
> Bart
On 4/24/2024 10:08 PM, Luiz Augusto von Dentz wrote:
> Hi Bartosz,
>
> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> <[email protected]> wrote:
>>
>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>
> I guess that could go in but it would really help if you guys could
> work together so we don't have more competing solutions.
>
>>
>> Bart
>
>
>
On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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
On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Bartosz,
>
> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> <[email protected]> wrote:
> >
> > On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>
> I guess that could go in but it would really help if you guys could
> work together so we don't have more competing solutions.
>
These threads with their 7 patch versions from Zijun within 2 days or
so have become very chaotic. Let me summarize: there are two
regressions: one caused by my commit 6845667146a2 ("Bluetooth:
hci_qca: Fix NULL vs IS_ERR_OR_NULL check in qca_serdev_probe") and a
second caused by Krzysztof's commit 272970be3dab ("Bluetooth: hci_qca:
Fix driver shutdown on closed serdev"). The patch I linked here is how
I propose to fix my regression only. These fixes don't seem to
conflict with one another.
We (Krzysztof and I) have provided feedback to Zijun but he refused to
address it and instead kept on resending his patches every couple
hours. Zijun's patch 1/2 proposed to revert my commit 6845667146a2. I
disagreed and proposed a way forward by fixing the regression. This
fix was incorrect as pointed out by Wren, so I submitted v2 which
works.
Bartosz
On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> <[email protected]> wrote:
>>
>> Hi Bartosz,
>>
>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>> <[email protected]> wrote:
>>>
>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>>
>> 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
Hi Bartosz,
On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
<[email protected]> wrote:
>
> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
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
--
Luiz Augusto von Dentz
On 24/04/2024 16:41, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[email protected]> wrote:
>>
>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>> <[email protected]> wrote:
>>>>>
>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>>>>
>>>> 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
On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[email protected]> wrote:
>
> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> >>
> >> Hi Bartosz,
> >>
> >> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> >> <[email protected]> wrote:
> >>>
> >>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
> >>
> >> I guess that could go in but it would really help if you guys could
> >> work together so we don't have more competing solutions.
> >>
> >
> > These threads with their 7 patch versions from Zijun within 2 days or
> > so have become very chaotic. Let me summarize: there are two
> > regressions: one caused by my commit 6845667146a2 ("Bluetooth:
> > hci_qca: Fix NULL vs IS_ERR_OR_NULL check in qca_serdev_probe") and a
> > second caused by Krzysztof's commit 272970be3dab ("Bluetooth: hci_qca:
> > Fix driver shutdown on closed serdev"). The patch I linked here is how
> > I propose to fix my regression only. These fixes don't seem to
> > conflict with one another.
> >
> it is not conflict issue, from my perspective, you fix are wrong.
> do you see my patch change log?
>
> > We (Krzysztof and I) have provided feedback to Zijun but he refused to
> > address it and instead kept on resending his patches every couple
> > hours. Zijun's patch 1/2 proposed to revert my commit 6845667146a2. I
> > disagreed and proposed a way forward by fixing the regression. This
> > fix was incorrect as pointed out by Wren, so I submitted v2 which
> > works.
> >
> v2 is not right from my point as i commented with your solution.
>
> you don't answer my questions commented within your solution.
>
> what is your question i don't answer?
>
> > Bartosz
>
Luiz,
This is an example of how Zijun will borrow any attempt at meaningful
communication under a heap of incomprehensible emails. Krzysztof has
already given up and I think I will stop too now. As the GPIO
maintainer I suggest you take my fix for this regression. I can't make
you though and I've already wasted way too much time on it. Your call.
Bartosz
On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[email protected]> wrote:
>>
>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>> <[email protected]> wrote:
>>>>
>>>> Hi Bartosz,
>>>>
>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>> <[email protected]> wrote:
>>>>>
>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>>>>
>>>> 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
Hi Quic_zijuhu,
On Wed, Apr 24, 2024 at 10:46 AM quic_zijuhu <[email protected]> wrote:
>
> On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[email protected]> wrote:
> >>
> >> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
> >>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
> >>> <[email protected]> wrote:
> >>>>
> >>>> Hi Bartosz,
> >>>>
> >>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
> >>>> <[email protected]> wrote:
> >>>>>
> >>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
> >>>>
> >>>> 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.
--
Luiz Augusto von Dentz
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 <[email protected]> wrote:
>>
>> On 4/24/2024 10:41 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 16:25, quic_zijuhu <[email protected]> wrote:
>>>>
>>>> On 4/24/2024 10:19 PM, Bartosz Golaszewski wrote:
>>>>> On Wed, 24 Apr 2024 at 16:08, Luiz Augusto von Dentz
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> Hi Bartosz,
>>>>>>
>>>>>> On Wed, Apr 24, 2024 at 10:00 AM Bartosz Golaszewski
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On Wed, 24 Apr 2024 at 15:53, quic_zijuhu <[email protected]> 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/[email protected]/
>>>>>>
>>>>>> 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.
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 <[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]>
> >>>>>>
> >>>>>> 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
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 <[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]>
>>>>>>>>
>>>>>>>> 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
>