On 4/24/24 4:35 PM, quic_zijuhu wrote:
On 4/25/2024 6:17 AM, Wren Turkal wrote:
On 4/24/24 6:53 AM, Bartosz Golaszewski wrote:
On Wed, 24 Apr 2024 at 15:26, Wren Turkal <wt@xxxxxxxxxxxxxxxx> wrote:
On 4/24/24 6:12 AM, quic_zijuhu wrote:
On 4/24/2024 8:27 PM, Bartosz Golaszewski wrote:
On Wed, Apr 24, 2024 at 2:24 PM Wren Turkal <wt@xxxxxxxxxxxxxxxx>
wrote:
That's OK, we have the first part right. Let's now see if we
can reuse
patch 2/2 from Zijun.
I'm compiling it right now. Be back soon.
Well I doubt it's correct as it removed Krzysztof's fix which looks
right. If I were to guess I'd say we need some mix of both.
Patch 2/2 remove K's fix? I thought only 1/2 did that.
To be specific, I have applied your patch and Zijun's 2/2 only.
No, patch 1/2 from Zijun reverted my changes. Patch 2/2 removes
Krzysztof's changes and replaces them with a different if else. This
patch is a better alternative to Zijun's patch 1/2. For 2/2, I'll let
Krzysztof handle it.
do you really realize what do you talk about?
do you really listen what do @Wren says?
he says that my patch 2/2 is right based on several verification
results.
she, not he
BTW, my 2/2 fix don't have anything about DTS usage.
I think the problem with your 2/2 patch is that it removes the
conditional bailing if the device is shutdown or not open.
Maybe this patch instead?
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 2f7ae38d85eb..fcac44ae7898 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2456,6 +2456,10 @@ static void qca_serdev_shutdown(struct device
*dev)
!test_bit(HCI_RUNNING, &hdev->flags))
return;
+ if (test_bit(HCI_QUIRK_NON_PERSISTENT_SETUP,
&hdev->quirks) ||
+ hci_dev_test_flag(hdev, HCI_SETUP))
+ return;
+
serdev_device_write_flush(serdev);
ret = serdev_device_write_buf(serdev, ibs_wake_cmd,
sizeof(ibs_wake_cmd));
he maybe be a DTS expert but not BT from his present fix history for
bluetooth system.
Did you test it? Does it work? If so, please consider sending it
upstream for review.
You can keep Zijun's authorship but add your own SoB tag at the end
and mention what you did. Something like this:
[V7 2/2] as shown by below link is the formal fix.
https://patchwork.kernel.org/project/bluetooth/patch/1713932807-19619-3-git-send-email-quic_zijuhu@xxxxxxxxxxx/
this fix logic was introduced from the very beginning when i saw your
issue description as shown by below link
https://lore.kernel.org/all/1713095825-4954-3-git-send-email-quic_zijuhu@xxxxxxxxxxx/#t
This is not quite fully correct. You did introduce that logic. However,
you also removed the other conditional. That is what K.K. was objecting
to. I literally copied your logic without deleting K.K.'s logic. My
specific question is does that make my change incorrect in some way.
[Wren: kept Krzysztof's fix]
Signed-off-by: Wren...
Bartosz
@Bartosz, I have tested this, and it works functionally for my setup. I
cannot detect a difference between this and Zijun's logic when I compile
a kernel with this patch.
@Zijun, I think you have objections to this patch. I would like to make
sure I hear your concern. Can you please take through it like I'm a 5
year old who barely knows C? In retrospect, I guess that I would be a
pretty precocious 5 year old. LOL.
In all seriousness, @Zijun, I really appreciate the work you did on
this. I would like to understand why you assert that removing the logic
of Krzysztof is appropriate. Again, I am not a kernel developer, so this
stuff is really outside my wheelhouse. Having said that, by my reading,
which may very well be worng, it seems like you are just adding another
case that is orthogonal to K's conditions. I'd like to truly understand
you position to know if the patch I am suggesting is somehow harmful.
This is an earnest question. I really want to respect your expertise
here, and I really want you to know how much I appreciate your work.
you maybe see all replies of [2/2] patch for this issue within below
link. i believe you will understand it. the bottom of the link includes
all reply history.
https://lore.kernel.org/all/fe1a0e3b-3408-4a33-90e9-d4ffcfc7a99b@xxxxxxxxxxx/
I read through the discussion again.
Can you please define VSC? I'm not sure what that means.
With regard to your change's logic.
First a kernel dev newbie question, does "hci_dev_test_flag(hdev,
HCI_SETUP)" test whether the hardware has ever been setup? The rest of
my discussions assumes that is the case.
So, basically, the logic in your change is something like this:
if dev_must_be_setup_every_time_opened || dev_is_in_setup_state
bail out on shutdown logic
If I am getting this correctly, you're saying that any device that must
be setup is already by definition not needing setup and should not have
shutdown logic run on it.
Okay, so question: Is it possible for a device to need setup (both not
have the quirk and "hci_dev_test_flag(hdev, HCI_SETUP)" to have BT off
or not be running?
Assuming the answer to that question is, "It is not possible." How do we
know that? I think that may be what K.K. is saying is not obvious.
If the code ever gets into that state, shouldn't we at least log that
the state is not an expected state?
I am now worried that allowing an unexpected state through could result
in a unintended logic executing. How do I know I don't need to worry
about that?
wt
--
You're more amazing than you think!
