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.
Best regards,
Krzysztof
