Re: [PATCH v1 2/2] Bluetooth: qca: Fix BT enable failure for QCA_QCA6390 after disable then warm reboot

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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:
>>>>> 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,

[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux