Received-SPF: pass (google.com: domain of linux-kernel+bounces-153319-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d9908e10-66d6-4737-b366-80cbc9df1801@quicinc.com>
Date: Mon, 22 Apr 2024 20:06:06 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to
 enter EDL
To: Manivannan Sadhasivam <mani@kernel.org>
CC: Mayank Rana <quic_mrana@quicinc.com>, <quic_jhugo@quicinc.com>,
        <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <quic_cang@quicinc.com>
References: <1713170945-44640-1-git-send-email-quic_qianyu@quicinc.com>
 <1713170945-44640-4-git-send-email-quic_qianyu@quicinc.com>
 <17d94b91-137c-409c-8af3-f32f1af2eb71@quicinc.com>
 <4b684db2-d384-404a-9c54-60d79ac7cf9f@quicinc.com>
 <adb9ab3d-0fd2-4afe-96d7-573b1822e0c3@quicinc.com>
 <ab27b383-dcbf-4337-b3ea-da91763e834a@quicinc.com>
 <a5ea5263-8acb-48dd-a4e1-bc48a9bdf791@quicinc.com>
 <20240422080837.GC9775@thinkpad>
Content-Language: en-US
From: Qiang Yu <quic_qianyu@quicinc.com>
In-Reply-To: <20240422080837.GC9775@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 4/22/2024 4:08 PM, Manivannan Sadhasivam wrote:
> On Wed, Apr 17, 2024 at 12:41:25PM +0800, Qiang Yu wrote:
>
> [...]
>
>>>>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>> Don't we need error handling part here i.e. calling
>>>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>>> Hi Mayank
>>>>>
>>>>> After soc_reset, device will reboot to EDL mode and MHI state
>>>>> will be SYSERR. So host will fail to suspend
>>>>> anyway. The "error handling" we need here is actually to enter
>>>>> EDL mode, this will be done by SYSERR irq.
>>>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to
>>>>> balance mhi_cntrl->runtime_get() and
>>>>> mhi_device_get_sync().
>>>>>
>>>>> Thanks,
>>>>> Qiang
>>>> I am saying is there possibility that mhi_get_channel_doorbell()
>>>> returns error ?
>>>> If yes, then why don't we need error handling as part of it. you are
>>>> exiting if this API return error without doing anything.
>>> I think here mhi_get_channel_doorbell will not return error. But I still
>>> add a error checking because it invoked mhi_read_reg, which is a must
>>> check
>>> API. For modem mhi controller, this API eventually does a memory read.
>>> This memory read will return a normal value if link is up and all f's if
>>> link
>>> is bad.
>>>
>>> Thanks,
>>> Qiang
>> Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
>> replace the getting chdb operation by invoking mhi_read_reg as Mani
>> commented.
>> In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
>> additionnal error handling.
>>
>> I'm not very sure the reason but perhaps if mhi_read_reg returns error (I
>> don't
>> know which controller will provide a memory read callback returning errors),
> Take a look at AIC100 driver: drivers/accel/qaic/mhi_controller.c
>
>> most
>> probably something is wrong in PCIe, which is not predictable by MHI and we
>> can
>> not add err handling every time invoking mhi_read_reg. But we have a timer
>> to
>> do health_check in pci_generic.c. If link is down, we will do
>> pci_function_reset
>> to try to reovery.
>>
> Right, but the MHI stack is designed to be bus agnostic. So if we happen to use
> it with other busses like USB, I2C etc... then read APIs may fail.
>
> Even with PCIe, read transaction may return all 1 response and MHI needs to
> treat it as an error condition. But sadly, both pci_generic and ath controllers
> are not checking for invalid read value. But those need to be fixed.
>
> Regarding Mayank's query, you should do error cleanups if
> mhi_get_channel_doorbell() API fails.
>
> - Mani
Hi Mani, Mayank

Checked drivers/accel/qaic/mhi_controller.c, the mhi_read_reg
callback of AIC100 does return -EIO if it reads out all'f.

Considering that pci_generic controller also needs to check for
invalid read value which is not fixed though. It's better to invoke
mhi_cntrl->runtime_put() and mhi_device_put() as error cleanups if
mhi_get_channel_doorbell returns error here.

Let me address all your comments and prepare next version patch.

Thanks,
Qiang
>