[PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[Zijun Hu]

This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.

Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
with gpiod_get_optional()") will cause serious regression issue for
several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
enabled any more once BT is disabled if BT reset pin is not configured
by DT or ACPI.

if BT reset pin is not configured, devm_gpiod_get_optional() will return
NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
reverted commit SET the quirk since NULL is not a error case, and cause
qca_setup() call failure triggered by the 2nd and later BT enable
operations since there are no available BT reset pin to clear BT firmware
downloaded by the 1st enable operation, fixed by reverting the commit.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/bluetooth/hci_qca.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..160175a23a49 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
-		if (IS_ERR(qcadev->bt_en) &&
+		if (IS_ERR_OR_NULL(qcadev->bt_en) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
@@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 
 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
-		if (IS_ERR(qcadev->sw_ctrl) &&
+		if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
 		     data->soc_type == QCA_WCN7850))
@@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
 	default:
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
-		if (IS_ERR(qcadev->bt_en)) {
+		if (IS_ERR_OR_NULL(qcadev->bt_en)) {
 			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
 			power_ctrl_enabled = false;
 		}
-- 
2.7.4

RE: Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2580 bytes --]

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=845417

---Test result---

Test Summary:
CheckPatch                    PASS      0.64 seconds
GitLint                       PASS      0.30 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      29.80 seconds
CheckAllWarning               PASS      33.20 seconds
CheckSparse                   PASS      38.40 seconds
CheckSmatch                   FAIL      35.17 seconds
BuildKernel32                 PASS      28.95 seconds
TestRunnerSetup               PASS      525.36 seconds
TestRunner_l2cap-tester       PASS      18.53 seconds
TestRunner_iso-tester         PASS      33.68 seconds
TestRunner_bnep-tester        PASS      4.76 seconds
TestRunner_mgmt-tester        FAIL      110.69 seconds
TestRunner_rfcomm-tester      PASS      7.38 seconds
TestRunner_sco-tester         PASS      11.07 seconds
TestRunner_ioctl-tester       PASS      7.61 seconds
TestRunner_mesh-tester        PASS      6.73 seconds
TestRunner_smp-tester         PASS      6.70 seconds
TestRunner_userchan-tester    PASS      4.85 seconds
IncrementalBuild              PASS      27.83 seconds

Details
##############################
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
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Add Device 7 (AL is full)               Failed       0.194 seconds


---
Regards,
Linux Bluetooth

Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[Bartosz Golaszewski]

On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>
> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.
>
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()") will cause serious regression issue for
> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
> enabled any more once BT is disabled if BT reset pin is not configured
> by DT or ACPI.
>
> if BT reset pin is not configured, devm_gpiod_get_optional() will return
> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
> reverted commit SET the quirk since NULL is not a error case, and cause
> qca_setup() call failure triggered by the 2nd and later BT enable
> operations since there are no available BT reset pin to clear BT firmware
> downloaded by the 1st enable operation, fixed by reverting the commit.
>

Then you just go back to bad usage of the GPIO API. Please see my
suggestion below.

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> ---
>  drivers/bluetooth/hci_qca.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..160175a23a49 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>
>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>                                                GPIOD_OUT_LOW);
> -               if (IS_ERR(qcadev->bt_en) &&
> +               if (IS_ERR_OR_NULL(qcadev->bt_en) &&
>                     (data->soc_type == QCA_WCN6750 ||
>                      data->soc_type == QCA_WCN6855)) {
>                         dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>
>                 qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>                                                GPIOD_IN);
> -               if (IS_ERR(qcadev->sw_ctrl) &&
> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
>                     (data->soc_type == QCA_WCN6750 ||
>                      data->soc_type == QCA_WCN6855 ||
>                      data->soc_type == QCA_WCN7850))
> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>         default:
>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>                                                GPIOD_OUT_LOW);
> -               if (IS_ERR(qcadev->bt_en)) {
> +               if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>                         dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>                         power_ctrl_enabled = false;

Can you just move this out of this block? Or just simply check the
presence of the GPIO descriptor in the if block setting the quirk bit?
Warning on a missing *optional* GPIO is wrong. It's not an unexpected
situation, it's normal.

Bartosz

>                 }
> --
> 2.7.4
>

Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[quic_zijuhu]

On 4/18/2024 2:59 AM, Bartosz Golaszewski wrote:
> On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>
>> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.
>>
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()") will cause serious regression issue for
>> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
>> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
>> enabled any more once BT is disabled if BT reset pin is not configured
>> by DT or ACPI.
>>
>> if BT reset pin is not configured, devm_gpiod_get_optional() will return
>> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
>> reverted commit SET the quirk since NULL is not a error case, and cause
>> qca_setup() call failure triggered by the 2nd and later BT enable
>> operations since there are no available BT reset pin to clear BT firmware
>> downloaded by the 1st enable operation, fixed by reverting the commit.
>>
> 
> Then you just go back to bad usage of the GPIO API. Please see my
> suggestion below.
> 
we need to treat below tow cases equivalently.
1) BT reset pin is NOT configured, devm_gpiod_get_optional() return NULL.
2) BT reset pin is configured, but failed to be got, devm_gpiod_get_optional() return ERROR.

FOR above two cases, hci_qca driver doesn't get available BT reset pin for H/W reset.
so we use IS_ERR_OR_NULL() instead of IS_ERR().

is it not right ?


>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>  drivers/bluetooth/hci_qca.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..160175a23a49 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>
>>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>                                                GPIOD_OUT_LOW);
>> -               if (IS_ERR(qcadev->bt_en) &&
>> +               if (IS_ERR_OR_NULL(qcadev->bt_en) &&
>>                     (data->soc_type == QCA_WCN6750 ||
>>                      data->soc_type == QCA_WCN6855)) {
>>                         dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>
>>                 qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>>                                                GPIOD_IN);
>> -               if (IS_ERR(qcadev->sw_ctrl) &&
>> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
>>                     (data->soc_type == QCA_WCN6750 ||
>>                      data->soc_type == QCA_WCN6855 ||
>>                      data->soc_type == QCA_WCN7850))
>> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>         default:
>>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>                                                GPIOD_OUT_LOW);
>> -               if (IS_ERR(qcadev->bt_en)) {
>> +               if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>>                         dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>                         power_ctrl_enabled = false;
> 
> Can you just move this out of this block? Or just simply check the
> presence of the GPIO descriptor in the if block setting the quirk bit?
> Warning on a missing *optional* GPIO is wrong. It's not an unexpected
> situation, it's normal.
> 
your concern is that the prompt message is printed by wrong way or the prompt message is not precise.
is it right ?

> Bartosz
> 
>>                 }
>> --
>> 2.7.4
>>

Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[Bartosz Golaszewski]

On Thu, 18 Apr 2024 at 01:44, quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>
> On 4/18/2024 2:59 AM, Bartosz Golaszewski wrote:
> > On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> wrote:
> >>
> >> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.
> >>
> >> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> >> with gpiod_get_optional()") will cause serious regression issue for
> >> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
> >> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
> >> enabled any more once BT is disabled if BT reset pin is not configured
> >> by DT or ACPI.
> >>
> >> if BT reset pin is not configured, devm_gpiod_get_optional() will return
> >> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
> >> reverted commit SET the quirk since NULL is not a error case, and cause
> >> qca_setup() call failure triggered by the 2nd and later BT enable
> >> operations since there are no available BT reset pin to clear BT firmware
> >> downloaded by the 1st enable operation, fixed by reverting the commit.
> >>
> >
> > Then you just go back to bad usage of the GPIO API. Please see my
> > suggestion below.
> >
> we need to treat below tow cases equivalently.
> 1) BT reset pin is NOT configured, devm_gpiod_get_optional() return NULL.
> 2) BT reset pin is configured, but failed to be got, devm_gpiod_get_optional() return ERROR.
>
> FOR above two cases, hci_qca driver doesn't get available BT reset pin for H/W reset.
> so we use IS_ERR_OR_NULL() instead of IS_ERR().
>
> is it not right ?
>
>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
> >> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
> >> ---
> >>  drivers/bluetooth/hci_qca.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> >> index 92fa20f5ac7d..160175a23a49 100644
> >> --- a/drivers/bluetooth/hci_qca.c
> >> +++ b/drivers/bluetooth/hci_qca.c
> >> @@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>
> >>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >>                                                GPIOD_OUT_LOW);
> >> -               if (IS_ERR(qcadev->bt_en) &&
> >> +               if (IS_ERR_OR_NULL(qcadev->bt_en) &&
> >>                     (data->soc_type == QCA_WCN6750 ||
> >>                      data->soc_type == QCA_WCN6855)) {
> >>                         dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> >> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>
> >>                 qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
> >>                                                GPIOD_IN);
> >> -               if (IS_ERR(qcadev->sw_ctrl) &&
> >> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
> >>                     (data->soc_type == QCA_WCN6750 ||
> >>                      data->soc_type == QCA_WCN6855 ||
> >>                      data->soc_type == QCA_WCN7850))
> >> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>         default:
> >>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >>                                                GPIOD_OUT_LOW);
> >> -               if (IS_ERR(qcadev->bt_en)) {
> >> +               if (IS_ERR_OR_NULL(qcadev->bt_en)) {
> >>                         dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> >>                         power_ctrl_enabled = false;
> >
> > Can you just move this out of this block? Or just simply check the
> > presence of the GPIO descriptor in the if block setting the quirk bit?
> > Warning on a missing *optional* GPIO is wrong. It's not an unexpected
> > situation, it's normal.
> >
> your concern is that the prompt message is printed by wrong way or the prompt message is not precise.
> is it right ?
>

Actually now that you're saying this: if gpiod_get_optional() returns
IS_ERR() then we should bail-out and return dev_err_probe(), not
proceed. What if GPIOLIB returns EPROBE_DEFER? Only NULL is an
acceptable return value from gpiod_get_optional().

Bart

> > Bartosz
> >
> >>                 }
> >> --
> >> 2.7.4
> >>
>

Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[quic_zijuhu]

On 4/18/2024 8:01 AM, Bartosz Golaszewski wrote:
> On Thu, 18 Apr 2024 at 01:44, quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>>
>> On 4/18/2024 2:59 AM, Bartosz Golaszewski wrote:
>>> On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>>>
>>>> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.
>>>>
>>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>> with gpiod_get_optional()") will cause serious regression issue for
>>>> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
>>>> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
>>>> enabled any more once BT is disabled if BT reset pin is not configured
>>>> by DT or ACPI.
>>>>
>>>> if BT reset pin is not configured, devm_gpiod_get_optional() will return
>>>> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
>>>> reverted commit SET the quirk since NULL is not a error case, and cause
>>>> qca_setup() call failure triggered by the 2nd and later BT enable
>>>> operations since there are no available BT reset pin to clear BT firmware
>>>> downloaded by the 1st enable operation, fixed by reverting the commit.
>>>>
>>>
>>> Then you just go back to bad usage of the GPIO API. Please see my
>>> suggestion below.
>>>
>> we need to treat below tow cases equivalently.
>> 1) BT reset pin is NOT configured, devm_gpiod_get_optional() return NULL.
>> 2) BT reset pin is configured, but failed to be got, devm_gpiod_get_optional() return ERROR.
>>
>> FOR above two cases, hci_qca driver doesn't get available BT reset pin for H/W reset.
>> so we use IS_ERR_OR_NULL() instead of IS_ERR().
>>
>> is it not right ?
>>
>>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>>>> ---
>>>>  drivers/bluetooth/hci_qca.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index 92fa20f5ac7d..160175a23a49 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>
>>>>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>                                                GPIOD_OUT_LOW);
>>>> -               if (IS_ERR(qcadev->bt_en) &&
>>>> +               if (IS_ERR_OR_NULL(qcadev->bt_en) &&
>>>>                     (data->soc_type == QCA_WCN6750 ||
>>>>                      data->soc_type == QCA_WCN6855)) {
>>>>                         dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>>>> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>
>>>>                 qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>>>>                                                GPIOD_IN);
>>>> -               if (IS_ERR(qcadev->sw_ctrl) &&
>>>> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
>>>>                     (data->soc_type == QCA_WCN6750 ||
>>>>                      data->soc_type == QCA_WCN6855 ||
>>>>                      data->soc_type == QCA_WCN7850))
>>>> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>         default:
>>>>                 qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>                                                GPIOD_OUT_LOW);
>>>> -               if (IS_ERR(qcadev->bt_en)) {
>>>> +               if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>>>>                         dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>                         power_ctrl_enabled = false;
>>>
>>> Can you just move this out of this block? Or just simply check the
>>> presence of the GPIO descriptor in the if block setting the quirk bit?
>>> Warning on a missing *optional* GPIO is wrong. It's not an unexpected
>>> situation, it's normal.
>>>
>> your concern is that the prompt message is printed by wrong way or the prompt message is not precise.
>> is it right ?
>>
> 
> Actually now that you're saying this: if gpiod_get_optional() returns
> IS_ERR() then we should bail-out and return dev_err_probe(), not
> proceed. What if GPIOLIB returns EPROBE_DEFER? Only NULL is an
> acceptable return value from gpiod_get_optional().
> 
1)  "you're saying this" or "i am saying this" ? is it a typo error?
2)  BT still is able to work by clearing the quirk even if getting preferred H/W reset way failure, why need to bail-out and return dev_err_probe() ?
3)  are you sumbit the change because you meet any functionality issues?
4)  do you meet the case that EPROBE_DEFER is returned ?, are your change is to fix the EPROBE_DEFER case ? if so, your fix is not right.
> Bart
> 
>>> Bartosz
>>>
>>>>                 }
>>>> --
>>>> 2.7.4
>>>>
>>

Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[Wren Turkal]

On 4/17/24 11:59 AM, Bartosz Golaszewski wrote:
> On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> wrote:
>>
>> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8.
>>
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()") will cause serious regression issue for
>> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME,
>> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
>> enabled any more once BT is disabled if BT reset pin is not configured
>> by DT or ACPI.
>>
>> if BT reset pin is not configured, devm_gpiod_get_optional() will return
>> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
>> reverted commit SET the quirk since NULL is not a error case, and cause
>> qca_setup() call failure triggered by the 2nd and later BT enable
>> operations since there are no available BT reset pin to clear BT firmware
>> downloaded by the 1st enable operation, fixed by reverting the commit.
>>
> 
> Then you just go back to bad usage of the GPIO API. Please see my
> suggestion below.

This revert fixes all the issues I have seen with QCA6390 hardware 
recently. I use a bluetooth mouse, and the hardware hasn't been working 
well for sometime, but really fully broke sometime during the last few 
months. It made my laptop unusable without shutting it down every time I 
wanted to just put it to sleep.

I frankly would prefer the bad usage to live another dev cycle if that's 
what it takes to find a proper solution that doesn't re-break my hardware.

>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>   drivers/bluetooth/hci_qca.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..160175a23a49 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2323,7 +2323,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>
>>                  qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>                                                 GPIOD_OUT_LOW);
>> -               if (IS_ERR(qcadev->bt_en) &&
>> +               if (IS_ERR_OR_NULL(qcadev->bt_en) &&
>>                      (data->soc_type == QCA_WCN6750 ||
>>                       data->soc_type == QCA_WCN6855)) {
>>                          dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>
>>                  qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>>                                                 GPIOD_IN);
>> -               if (IS_ERR(qcadev->sw_ctrl) &&
>> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
>>                      (data->soc_type == QCA_WCN6750 ||
>>                       data->soc_type == QCA_WCN6855 ||
>>                       data->soc_type == QCA_WCN7850))
>> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>          default:
>>                  qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>                                                 GPIOD_OUT_LOW);
>> -               if (IS_ERR(qcadev->bt_en)) {
>> +               if (IS_ERR_OR_NULL(qcadev->bt_en)) {
>>                          dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>                          power_ctrl_enabled = false;
> 
> Can you just move this out of this block? Or just simply check the
> presence of the GPIO descriptor in the if block setting the quirk bit?
> Warning on a missing *optional* GPIO is wrong. It's not an unexpected
> situation, it's normal.

Do you have a proposed patch? I am happy to help test it if that helps 
get to a conclusion.

> 
> Bartosz
> 
>>                  }
>> --
>> 2.7.4
>>

-- 
You're more amazing than you think!