Hi Greg,
> On Mon, Aug 28, 2023 at 05:30:34PM +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.
>
> What do you mean by "other modules" here?
The PTN5110 chip communicates with SoC via i2c bus. If i2c controller
behaves abnormally, regmap_read/write() will fail and return a negative
value. So the "other modules" mainly mean i2c controller.
>
> > 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>
> > ---
> > drivers/usb/typec/tcpm/tcpci.c | 36 +++++++++++++++++++++++++---------
> > 1 file changed, 27 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index 0ee3e6e29bb1..698d00b7fce9 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -657,21 +657,30 @@ 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) {
> > + dev_err(tcpci->dev, "ALERT read failed\n");
>
> You are printing something in an irq handler, are you sure you want to
> do that? What happens if this is very noisy?
This dev_err() may need to be removed. A log may be helpful to locate
issue. I just notice max_tcpci_irq() is doing the same thing:
359 ret = max_tcpci_read16(chip, TCPC_ALERT, &status);
360 if (ret < 0) {
361 dev_err(chip->dev, "ALERT read failed\n");
362 return ret;
363 }
>
> > + 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 +696,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;
> > /*
> > * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
> > * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is
> > @@ -699,18 +710,25 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> > else
> > payload_cnt = 0;
> >
> > - tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> > + ret = tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> > + if (ret < 0)
> > + return ret;
> > msg.header = cpu_to_le16(header);
> >
> > if (WARN_ON(payload_cnt > sizeof(msg.payload)))
> > payload_cnt = sizeof(msg.payload);
> >
> > - if (payload_cnt > 0)
> > - regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> > + if (payload_cnt > 0) {
> > + ret = regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> > &msg.payload, payload_cnt);
> > + if (ret < 0)
> > + return ret;
> > + }
> >
> > /* Read complete, clear RX status alert bit */
> > - tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> > + ret = tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> > + if (ret < 0)
> > + return ret;
>
> For all of these, if an error happens, you are not able to unwind from
> the previous actions that the irq handler took.
The previous actions are necessary since they are real states. If an
error happens, the tcpm will also take over it. Normally, if the first
place to read ALERT register succeed, the following read/write() will
also success.
>
> Are you sure this is ok? How was this tested?
I just tested tcpci_read16(tcpci, TCPC_ALERT, &status). I have enabled
tcpci wake interrupt. When system had waked up from tcpci interrupt,
tcpci_read16() sometimes failed to get ALERT register (i2c controller is
abnormal in this period), then I got below log:
# cat /sys/kernel/debug/usb/tcpm-2-0050/log
[ 80.696202] PD TX complete, status: 1
[ 80.696226] PD TX complete, status: 1
[ 80.696256] PD TX complete, status: 1
[ 80.696280] PD TX complete, status: 1
[ 80.696310] PD TX complete, status: 1
...
[ 80.699982] PD RX, header: 0x8000 [1]
[ 80.699987] Unchunked extended messages unsupported
[ 80.699991] PD TX, header: 0x1b0
[ 80.700034] PD TX complete, status: 1
[ 80.700063] PD TX complete, status: 1
[ 80.700089] PD TX complete, status: 1
[ 80.700112] PD TX complete, status: 1
...
[ 80.705056] PD TX complete, status: 1
[ 80.705075] PD TX complete, status: 1
[ 80.706364] CC1: 2 -> 0, CC2: 0 -> 0 [state SRC_READY, polarity 0, disconnected]
[ 80.706373] state change SRC_READY -> SNK_UNATTACHED [rev3 NONE_AMS]
[ 80.708250] Start toggling
[ 80.709560] VBUS off
[ 80.709564] VBUS VSAFE0V
[ 80.709900] VBUS off
[ 80.709902] VBUS VSAFE0V
Although i2c bus was recovered at the end, tcpci had still transferred wrong state
to tcpm. After I add check code for read/write(), the behavior is normal.
Thanks,
Xu Yang
>
> thanks,
>
> greg k-h
