Messages in this thread | | | Date | Mon, 22 Apr 2024 20:06:06 +0800 | Subject | Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL | From | Qiang Yu <> |
| |
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 >
| |