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@xxxxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxxx>
>>>
>>> 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
>
