Re: [PATCH v4 0/2] Fix two regression issues for QCA controllers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux