On 4/19/2024 6:38 AM, Krzysztof Kozlowski wrote:
> On 19/04/2024 00:05, quic_zijuhu wrote:
>> On 4/19/2024 4:58 AM, Krzysztof Kozlowski wrote:
>>> On 18/04/2024 22:34, quic_zijuhu wrote:
>>>> On 4/19/2024 12:48 AM, Krzysztof Kozlowski wrote:
>>>>> On 18/04/2024 16:06, Zijun Hu wrote:
>>>>>> From: Zijun Hu <zijuhu@xxxxxxxxxxxxxxxx>
>>>>>>
>>>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>>>> serdev") will cause regression issue for QCA_QCA6390 if BT reset pin is
>>>>>> not configured by DT or ACPI, the regression issue is that BT can't be
>>>>>> enabled after disable then warm reboot, fixed by correcting conditions
>>>>>> for sending the VSC reset controller within qca_serdev_shutdown().
>>>>>
>>>>> I have trouble understanding what is the bug. Can you rephrase it?
>>>>>
>>>> Think about below sequences:
>>>> cold reboot(power off then power on) -> Enable BT -> Disable BT -> Warm reboot -> Enable BT again ...
>>>> BT is failed to be enabled after warm reboot.
>>>
>>> Still not. Please get someone to help you rephrase it. One long sentence
>>> is not good approach. Describe the problem, how it can be reproduced and
>>> then come with brief explanation how you fixed it (because it is not
>>> obvious to me).
>>>
>> thanks for your suggestions. will optimize commit message based on your suggestions.
>>>>>>
>>>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>>>> Reported-by: Wren Turkal <wt@xxxxxxxxxxxxxxxx>
>>>>>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>>>>>> Signed-off-by: Zijun Hu <zijuhu@xxxxxxxxxxxxxxxx>
>>>>>> Tested-by: Wren Turkal <wt@xxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/bluetooth/hci_qca.c | 12 +++++++++---
>>>>>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>>>> index 160175a23a49..2a47a60ecc25 100644
>>>>>> --- a/drivers/bluetooth/hci_qca.c
>>>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>>>> @@ -2437,15 +2437,21 @@ 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)) {
>>>>>> + BT_INFO("QCA do not need to send EDL_RESET_REQ");
>>>>>> return;
>>>>>> + }
>>>>>> +
>>>>>> + if (hci_dev_test_flag(hdev, HCI_SETUP)) {
>>>>>
>>>>> Commit msg does not help me at all to understand why you are changing
>>>>> the test bits.
>>>> it is test bits not changing bits.
>>>
>>> Previously we tested bits for BT off and HCI running. Now not, so you
>>> changed the logic. Maybe it is correct, maybe not, I don't understand why.
>>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is set, it means we can and need to do initialization
>> by calling hdev->setup() for every BT enable, so we don't need to send VSC to reset controller
>> here.
>>
>> if HCI_QUIRK_NON_PERSISTENT_SETUP is cleared. it means we only can or need to do initialization
>> for the first BT enable operation. if HCI_SETUP is set, that means we don't do any BT enable yet
>> and the initialization operations are never performed. so we also don't need to send VSC any more.
>>
>> otherwise, we need to send VSC to reset controller since initialization have been Done regardless of
>> BT state. for this case the serdev have still be opened. so it also don't meet the crash the 272970be3dab
>> fixed.
>
> Please read again what I request here: your change is not obvious and is
> not explained in commit msg.
>
>
i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and HCI_SETUP is generic flag.
>
>
> Best regards,
> Krzysztof
>
