Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3850-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <96702ddc-ef5c-499c-abd4-95f89eb3aeba@quicinc.com>
Date: Mon, 22 Apr 2024 18:05:52 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 0/2] Fix two regression issues for QCA controllers
To: Krzysztof Kozlowski <krzk@kernel.org>, Wren Turkal <wt@penguintechs.org>,
        <luiz.dentz@gmail.com>, <luiz.von.dentz@intel.com>,
        <marcel@holtmann.org>
CC: <linux-bluetooth@vger.kernel.org>, <bartosz.golaszewski@linaro.org>,
        <regressions@lists.linux.dev>
References: <1713564212-21725-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713650800-29741-1-git-send-email-quic_zijuhu@quicinc.com>
 <369512e2-f091-4370-bce5-9ac32178dc4d@penguintechs.org>
 <a6ae69ad-b602-4cfc-9f76-8932f357d30b@kernel.org>
 <c0f7036d-c129-400b-85a2-1414ec3d2622@quicinc.com>
 <2bd1f1bf-f867-4430-8ce5-c691485665e1@kernel.org>
Content-Language: en-US
From: quic_zijuhu <quic_zijuhu@quicinc.com>
In-Reply-To: <2bd1f1bf-f867-4430-8ce5-c691485665e1@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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@quicinc.com/
> 
> 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?
> 
> 
i reviewed my reply. i have explained to you why my change fix both this
issue and the issue your commit fixed.

so i don't think it is meaningful to explain why your wrong condition
are changed by me.
> 
> 
> 
>>> "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@linaro.org/
>>> 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.
> 
as i statemented. my fix have fixed both this issue and the original
crash issue. don't need to talk about others.
>>
>> 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.
> 
there are almost no commit with tag Tested-by also provide exact kernel
version. for one type bt controller. different h/w has different config.
important is that this issue is fixed in reported H/W and don't cause
issue for other issue.

let us stop here and wait for other comments.

i have given too much explanations for my change of only total 7 lines.
> 
> Best regards,
> Krzysztof
>