On 2024/6/6 20:54, Lk Sii wrote:
>
>
> On 2024/6/5 15:14, Krzysztof Kozlowski wrote:
>> On 05/06/2024 03:49, Lk Sii wrote:
>>>
>>>
>>> On 2024/6/4 23:18, Krzysztof Kozlowski wrote:
>>>> On 04/06/2024 16:25, Lk Sii wrote:
>>>>>
>>>>>
>>>>> On 2024/5/22 00:02, Krzysztof Kozlowski wrote:
>>>>>> On 16/05/2024 15:31, Zijun Hu wrote:
>>>>>>> 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 QCA6390.
>>>>>>>
>>>>>>> The commit is to fix a use-after-free issue within qca_serdev_shutdown()
>>>>>>> by adding condition to avoid the serdev is flushed or wrote after closed
>>>>>>> but also introduces this regression 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 fixed by
>>>>>>> this change since the serdev is still opened before it is flushed or wrote.
>>>>>>>
>>>>>>> Verified by the reported machine Dell XPS 13 9310 laptop over below two
>>>>>>> kernel commits:
>>>>>>
>>>>>> I don't understand how does it solve my question. I asked you: on which
>>>>>> hardware did you, not the reporter, test?
>>>>>> It seems Zijun did NOT perform any tests obviously.
>>>>> All these tests were performed by reporter Wren with her machine
>>>>> "Dell XPS 13 9310 laptop".
>>>>
>>>> Wren != Zijun.
>>>>
>>>>>
>>>>> From previous discussion, it seems she have tested this change
>>>>> several times with positive results over different trees with her
>>>>> machine. i noticed she given you reply for your questions within
>>>>> below v1 discussion link as following:
>>>>>
>>>>> Here are v1 discussion link.
>>>>> https://lore.kernel.org/linux-bluetooth/d553edef-c1a4-4d52-a892-715549d31ebe@xxxxxxx/T/#m7371df555fd58ba215d0da63055134126a43c460
>>>>>
>>>>> Here are Krzysztof's questions.
>>>>> "I asked already *two times*:
>>>>> 1. On which kernel did you test it?
>>>>> 2. On which hardware did you test it?"
>>>>>
>>>>> Here are Wren's reply for Krzysztof's questions
>>>>> "I thought I had already chimed in with this information. I am using a
>>>>> Dell XPS 13 9310. It's the only hardware I have access to. I can say
>>>>> that the fix seems to work as advertised in that it fixes the warm boot
>>>>> issue I have been experiencing."
>>>>
>>>> I asked Zijun, not Wren. I believe all this is tested or done by
>>>> Qualcomm on some other kernel, so that's my question.
>>>>
>>> Zijun is the only guy from Qualcomm who ever joined our discussion,
>>> he ever said he belongs to Bluetooth team, so let us suppose the term
>>> "Qualcomm" you mentioned above is Zijun.
>>>
>>> from discussion history. in fact, ALL these tests were performed by
>>> reporter Wren instead of Zijun, and there are also NOT Zijun's Tested-by
>>> tag, so what you believe above is wrong in my opinion.
>>
>> Patch author is supposed to test the code. Are you implying that
>> Qualcomm Bluetooth team cannot test the patch on any of Qualcomm
>> Bluetooth devices?
>>
> i guess Zijun did not test the patch on himself based on below reasons:
> 1) the patch has been tested by reporter with report's machine.
> 2) perhaps, Zijun is confident about his patch based on his experience.
> 3) perhaps, it is difficult for Zijun to find a suitable machine to
> perform tests, and test machines must have QCA6390 *embedded* and use
> Bluez solution.
>
>>>
>>> Only Zijun and reporter were involved during those early debugging days,
>>> Zijun shared changes for reporter to verify with reporter's machine,
>>> then Zijun posted his fixes after debugging and verification were done.
>>>
>>>> That's important because Wren did not test particular scenarios, like
>>>> PREEMPT_RT or RB5 hardware, but Zijun is claiming problems are solved.
>>>> Maybe indeed solved, but if takes one month and still not answer which
>>>> kernel you are using, then I am sure: this was nowhere tested by Zijun
>>>> on the hardware and on the kernel the Qualcomm wants it to be.
>>>>
>>>>>
>>>>>>> commit e00fc2700a3f ("Bluetooth: btusb: Fix triggering coredump
>>>>>>> implementation for QCA") of bluetooth-next tree.
>>>>>>> commit b23d98d46d28 ("Bluetooth: btusb: Fix triggering coredump
>>>>>>> implementation for QCA") of linus mainline tree.
>>>>>>
>>>>>> ? Same commit with different hashes? No, it looks like you are working
>>>>>> on some downstream tree with cherry picks.
>>>>>>
>>>>> From Zijun's commit message, for the same commit, it seems
>>>>> bluetooth-next tree has different hashes as linus tree.
>>>>> not sure if this scenario is normal during some time window.
>>>>>> No, test it on mainline and answer finally, after *five* tries, which
>>>>>> kernel and which hardware did you use for testing this.
>>>>>>
>>>>>>
>>>>> it seems there are two issues mentioned with Zijun's commit message.
>>>>> regression issue A: BT enable failure after warm reboot.
>>>>> issue B: use-after-free issue, namely, kernel crash.
>>>>>
>>>>> @Krzysztof
>>>>> which issue to test based on your concerns with mainline tree?
>>>>
>>>> No one tested this on non-laptop platform. Wren did not, which is fine.
>>>> Qualcomm should, but since they avoid any talks about it for so long
>>>> (plus pushy comments during review, re-spinning v1 suggesting entire
>>>> discussion is gone), I do not trust their statements at all.
>>>>
>>>
>>> For issue A:
>>> reporter's tests are enough in my opinion.
>>> Zijun ever said that "he known the root cause and this fix logic was
>>> introduced from the very beginning when he saw reporter's issue
>>> description" by below link:
>>> https://lore.kernel.org/lkml/1d0878e0-d138-4de2-86b8-326ab9ebde3f@xxxxxxxxxxx/
>>>
>>>> So really, did anything test it on any Qualcomm embedded platform?
>>>> Anyone tested the actual race visible with PREEMPT_RT?
>>>> For issue B, it was originally fixed and verified by you,
>>> it is obvious for the root cause and current fix solution after
>>> our discussion.
>>>
>>> luzi also ever tried to ask you if you have a chance to verify issue B
>>> with your machine for this change.
>>
>> I tried, but my setup is incomplete since ~half a year and will remain
>> probably for another short time, depending on ongoing work on power
>> sequencing. Therefore I cannot test whether anything improves or
>> deteriorates regarding this patch.
>>
>>>
>>>> Why Zijun cannot provide answer on which kernel was it tested? Why the
>>>> hardware cannot be mentioned?
>>>>
>>> i believe zijun never perform any tests for these two issues as
>>> explained above.
>>
>> yeah, and that was worrying me.
>>
> Only RB5 has QCA6390 *embedded* among DTS of mainline kernel, but we
> can't have a RB5 to test.
>
> Don't worry about due to below points:
> 1) Reporter have tested it with her machine
> 2) issue B and relevant fix is obvious after discussion.
>
I believe we have had too much discussion for this simple change.
@Krzysztof
do you have any other concerns?
thanks
>> Best regards,
>> Krzysztof
>
