Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3856-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <9ac11453-b7cf-43f3-8e46-f96e41ef190d@quicinc.com>
Date: Mon, 22 Apr 2024 20:59:27 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 2/2] Bluetooth: qca: Fix BT enable failure for
 QCA_QCA6390 after disable then warm reboot
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <luiz.dentz@gmail.com>, <luiz.von.dentz@intel.com>,
        <marcel@holtmann.org>
CC: <linux-bluetooth@vger.kernel.org>, <bartosz.golaszewski@linaro.org>,
        <wt@penguintechs.org>, <regressions@lists.linux.dev>
References: <1713650800-29741-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713771497-5733-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713771497-5733-3-git-send-email-quic_zijuhu@quicinc.com>
 <f744a2ba-f842-454a-a04f-76207f0db9bf@linaro.org>
 <930736f3-696c-4076-93fb-d879691bac66@quicinc.com>
 <01b0f63e-55f0-4396-bb8b-521bbe9b060e@linaro.org>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <01b0f63e-55f0-4396-bb8b-521bbe9b060e@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/22/2024 8:22 PM, Krzysztof Kozlowski wrote:
> On 22/04/2024 13:25, quic_zijuhu wrote:
>> On 4/22/2024 3:42 PM, Krzysztof Kozlowski wrote:
>>> On 22/04/2024 09:38, Zijun Hu wrote:
>>>> From: Zijun Hu <zijuhu@qti.qualcomm.com>
>>>>
>>>> Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
>>>> serdev") will cause below regression issue:
>>>>
>>>> BT can't be enabled after below steps:
>>>> cold boot -> enable BT -> disable BT -> warm reboot -> BT enable failure
>>>> if property enable-gpios is not configured within DT|ACPI for QCA_QCA6390.
>>>>
>>>> The commit is to fix a use-after-free issue within qca_serdev_shutdown()
>>>> during reboot, but also introduces this new issue regarding above steps
>>>> since the VSC is not sent to reset controller during warm reboot.
>>>>
>>>> Fixed by sending the VSC to reset controller within qca_serdev_shutdown()
>>>> once BT was ever enabled, and the use-after-free issue is also be fixed
>>>> by this change since serdev is still opened when send to serdev.
>>>>
>>>> Fixes: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed serdev")
>>>> Reported-by: Wren Turkal <wt@penguintechs.org>
>>>
>>> NAK. This is way too much. Previous discussion is going, I asked there
>>> questions and before any answers happen, you keep sending new version.
>>> This leads to previous discussion gone/missed.
>>>
>>> You ignored several questions and feedbacks.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> i believe my commit message explains WHAT/WHY/HOW about this issue.
>>
>> 1) the qca_serdev_shutdown() was introduced by my below commit
>> Commit 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure
>> after warm reboot")
>>
>> 2) then Krzysztof's below commit was made to fix use-after-free issue
>> but also causes discussed regression issue.
>> Commit 0b7015132878 ("Bluetooth: btusb: mediatek: add MediaTek
>> devcoredump support")
>>
>> 3) my fix will solve both this issue and the use-after-free issue.
> 
> I had to keep reminding you about answering to question multiple times.
> So one more time:
> 
>> You did not address original issue of crash during shutdown and did
> not clarify my questions.
> 
let me explain here. original crash your commit fixed should only
happens with machine for which property enable-gpios is configured,
which also results in quirk HCI_QUIRK_NON_PERSISTENT_SETUP is set. for
this case. for this case, serdev has been closed when
qca_serdev_shutdown() write the VSC to serdev, so cause the crash.
your commit fix this issue for this cause but cause regression for the
case my commit message described.

>> Anyway, any explanation providing background how you are fixing this
> issue while keeping *previous problem fixed* is useful but should be
> provided in commit msg. I asked about this two or three times.
> 
i have provided it in this v5 commit.
>> BTW, provide here exact kernel version you tested this patches with.
> Also the exact hardware.
> 
we can provide the kernel version in the bugzilla, as explained above,
for the same soc_type, different machine have different config.
different config have different result.
> Best regards,
> Krzysztof
>