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
