Hi Krzysztof, could you list questions i need to explain within commit message based on current v4 patch sets ? let me send v5 patch sets with updated commit messages. On 4/22/2024 1:52 PM, Krzysztof Kozlowski wrote: > 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 >
