Hi Heikki & Guenter,
> On 9/18/23 03:26, Heikki Krogerus wrote:
> > On Thu, Sep 14, 2023 at 08:11:57PM +0800, Xu Yang wrote:
> >> The return value from tcpci/regmap_read/write() must be checked to get
> >> rid of the bad influence of other modules. This will add check code for
> >> all of the rest read/write() callbacks and will show error when failed
> >> to get ALERT register.
> >>
> >> Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >>
> >> ---
> >> Changes in v2:
> >> - remove printing code
> >> ---
> >> drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++---------
> >> 1 file changed, 25 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> >> index 0ee3e6e29bb1..8ccc2d1a8ffc 100644
> >> --- a/drivers/usb/typec/tcpm/tcpci.c
> >> +++ b/drivers/usb/typec/tcpm/tcpci.c
> >> @@ -657,21 +657,28 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >> int ret;
> >> unsigned int raw;
> >>
> >> - tcpci_read16(tcpci, TCPC_ALERT, &status);
> >> + ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >> /*
> >> * Clear alert status for everything except RX_STATUS, which shouldn't
> >> * be cleared until we have successfully retrieved message.
> >> */
> >> - if (status & ~TCPC_ALERT_RX_STATUS)
> >> - tcpci_write16(tcpci, TCPC_ALERT,
> >> + if (status & ~TCPC_ALERT_RX_STATUS) {
> >> + ret = tcpci_write16(tcpci, TCPC_ALERT,
> >> status & ~TCPC_ALERT_RX_STATUS);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >>
> >> if (status & TCPC_ALERT_CC_STATUS)
> >> tcpm_cc_change(tcpci->port);
> >>
> >> if (status & TCPC_ALERT_POWER_STATUS) {
> >> - regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> >> + ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> >> + if (ret < 0)
> >> + return ret;
> >> /*
> >> * If power status mask has been reset, then the TCPC
> >> * has reset.
> >> @@ -687,7 +694,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >> unsigned int cnt, payload_cnt;
> >> u16 header;
> >>
> >> - regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> >> + ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> >> + if (ret < 0)
> >> + return ret;
> >
> > I think you still need to clear TCPC_ALERT_RX_STATUS in this case.
> > Guenter, can you check this?
> >
>
> If reading from or writing to the status register failed, we are pretty
> much messed up anyway, so I don't think it really matters.
>
> I think the more severe problem is that this is an interrupt handler,
> which either handles the interrupt or it doesn't. It does not have the
Yes, I agree.
> option of returning an error (negative error code).
Normally speaking, it means the device has successfully handled the
interrupt event if an interrupt handler return IRQ_HANDLED, and the
interrupt event doesn't belong to this device or not be handled if the
interrupt handler return IRQ_NONE. However, the irq core will regard
an negative error code as IRQ_NONE as far as I know. And when IRQ_NONE
or a an negative error code is returned, irq core will judge if the irq
is bad or if there is need to disable this irq. Therefore, the irq handler
returns an negative error code should be ok if it's threaded.
I just do the same thing as max_tcpci_irq() in tcpci_maxim_core.c.
>
> The submitter will have to decide what to do in the error case: Was
> the interrupt handled or not ? I have no real answer to that question; all
> answers seem wrong to me. I would tend to returning that the interrupt
> was not handled. Most likely that would cause the handler to be called
> again because the interrupt condition is not cleared. If this happens
> repeatedly, the interrupt might end up being disabled, which would probably
> be the best possible outcome.
When error occurs, I just know the tcpci irq handler should not continue
to handle events since read/write() is not successful. Otherwise, we will
see unexpected behaviors. If the error doesn't disappear in time, then irq
core will disable the irq as usual. If the hw or i2c adapter driver has
been recovered from a short fault state, then the tcpci driver will work as
usual too.
Thanks,
Xu Yang
>
> Guenter
