On 4/19/2024 1:00 AM, Bartosz Golaszewski wrote:
> On Thu, 18 Apr 2024 at 16:06, Zijun Hu <quic_zijuhu@xxxxxxxxxxx> 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.
>>
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Reported-by: Wren Turkal <wt@xxxxxxxxxxxxxxxx>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Link: https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@xxxxxxxxxxx/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
>> Signed-off-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
>> Tested-by: Wren Turkal <wt@xxxxxxxxxxxxxxxx>
>> ---
>> 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
>>
>
> I told you under your yesterday's submission that you should instead
> consider bailing out from probe() if gpiod_get_optional() returns an
> error as right now if it returns EPROBE_DEFER (enable-gpios is there
> but the controller is not up yet), you will act like the GPIO was not
> even specified.
>
> gpiod_get_optional() returns NULL if the GPIO property is not there or
> an error if anything else goes wrong. In the latter case, you should
> abort probe.
>
1) do you meet the case that EPROBE_DEFER is returned ?
2) does the reverted change solve above issue you mentioned?
3) does the reverted change solve any functionality issue you actually meet ?
4) BT still is able to work fine by clearing the quirk even if getting preferred H/W reset way failure, why do we need to bail-out and return dev_err_probe() ?
we will fix it by right way if EPROBE_DEFER is reported.
this change is to solve the issue mentioned by commit message regardless other issues.
it is not possible for every commit to fix any other potential issues as long as the fix
doesn't introduce new issue.
right ?
> Bart
