Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Manivannan Sadhasivam <mani@kernel.org>]

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

-- 
மணிவண்ணன் சதாசிவம்