On 22/04/2024 02:14, quic_zijuhu wrote: > On 4/22/2024 2:41 AM, Krzysztof Kozlowski wrote: >> On 21/04/2024 09:44, Wren Turkal wrote: >>> On 4/20/24 3:06 PM, Zijun Hu wrote: >>>> This patch series are to fix below 2 regression issues for QCA controllers >>>> 1) BT can't be enabled once BT was ever enabled for QCA_QCA6390 >>>> 2) BT can't be enabled after disable then warm reboot for QCA_QCA6390 >>> >>> @Zijun @Krzysztof and @Bartosz Would it be helpful for me to test these >>> to ensure they fix the issues I reported? >>> >> >> I look forward to someone testing these on other hardware, not yours. On >> the hardware where the original issues were happening leading to this >> changes, e.g. RB5. >> >> Anyway, the problem here is poor explanation of the problem which did >> not improve in v3 and v4. Instead I receive explanations like: >> >> "this is shutdown of serdev and not hdev's shutdown." >> Not related... >> > this is the reply for secondary issue. i believe i have given much > explain for my fix for the 2nd issue as shown by below links. No, you did not. > let me add a bit more explanation within the ending "For the 2nd issue" > section, supposed you known much for generic flag > HCI_QUIRK_NON_PERSISTENT_SETUP, otherwise, see header comment for the > quirk. also supposed you see commit history to find why > qca_serdev_shutdown() was introduced for QCA6390. > https://lore.kernel.org/all/fe1a0e3b-3408-4a33-90e9-d4ffcfc7a99b@xxxxxxxxxxx/ You did not answer my questions. Let's quote: "i don't explain much since these HCI_QUIRK_NON_PERSISTENT_SETUP and HCI_SETUP is generic flag." Srsly, what is such answer? >> "now. you understood why your merged change as shown link of 4) have >> problems and introduced our discussed issue, right?" >> > this is the reply for the first issue as shown by below link. it almost > have the same description as the following "For 1st issue:" section. > i believe it have clear illustration why the commit have bugs. > https://lore.kernel.org/all/2166fc66-9340-4e8c-8662-17a19a7d8ce6@xxxxxxxxxx/ >> No. I did not understand and I feel I am wasting here time. >>> Code could be correct, could be wrong. Especially second patch looks >> suspicious. But the way Zijun Hu explains it and the way Zijun Hu >> responds is not helping at all. >> >> Sorry, with such replies to review, it is not worth my time. >> >> Best regards, >> Krzysztof >> > Hi luiz,marcel > > it is time for me to request you give comments for our discussion > and for my fixes, Let me explain the 1st issue then 2nd one. You keep pushing and pushing even though I stated my remarks. > > For 1st issue: > 1) the following commit will cause serious regression issue for QCA > controllers, and it has been merged with linus's mainline kernel. > > Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() > with gpiod_get_optional()"). > > 2) the regression issue is described by [PATCH v4 1/2] commit message > as following: > 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. > i will verify and confirm if QCA_QCA2066 and QCA_ROME also are impacted. > > 3) let me explain the bug point for commit mentioned by 1), its > commit message and bug change applet are shown below. > > The optional variants for the gpiod_get() family of functions return > NULL if the GPIO in question is not associated with this device. They > return ERR_PTR() on any other error. NULL descriptors are graciously > handled by GPIOLIB and can be safely passed to any of the GPIO consumer > interfaces as they will return 0 and act as if the function succeeded. > If one is using the optional variant, then there's no point in checking > for NULL. > > qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", > GPIOD_OUT_LOW); > - if (IS_ERR_OR_NULL(qcadev->bt_en)) { > + if (IS_ERR(qcadev->bt_en)) { > dev_warn(&serdev->dev, "failed to acquire enable gpio\n"); > power_ctrl_enabled = false; > } > 3.1) we only need to discuss how to handle case "qcadev->bt_en == > NULL" since this is only difference between the commit and BT original > design. > 3.2) BT original design are agree with the point of above commit > message that case "qcadev->bt_en == NULL" should not be treated as > error, so BT original design does not do error return for the case and > use dev_warn() instead of dev_err() to give. > 3.3) the commit misunderstands BT original design and wrongly think > BT original design take "qcadev->bt_en == NULL" as error case, > so change the following flag power_ctrl_enabled set logic and cause > discussed issue. > > For the 2nd issue: > 1) the following commit will cause below regression issue for QCA_QCA6390. > Commit 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed > serdev") > > 2) the regression issue is described by [PATCH v4 2/2] commit message > as following: > 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. You did not address original issue of crash during shutdown and did not clarify my questions. > > 3) qca_serdev_shutdown() is serdev's shutdown and not hdev's shutdown() > it should not and also never get chance to be invoked even if BT is > disabled at above 2) step. qca_serdev_shutdown() need to send the VSC > to reset controller during warm reset phase of above 2) steps. 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. BTW, provide here exact kernel version you tested this patches with. Also the exact hardware. Best regards, Krzysztof
