| Message ID | 20230914121158.2955900-1-xu.yang_2@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write() | expand |
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@nxp.com> > > --- > 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? > /* > * '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 +708,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; > > tcpm_pd_receive(tcpci->port, &msg); > } > -- > 2.34.1
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@nxp.com> >> >> --- >> 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 option of returning an error (negative error code). 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. Guenter
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@nxp.com> > >> > >> --- > >> 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
On 9/21/23 23:47, Xu Yang wrote: > 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@nxp.com> >>>> >>>> --- >>>> 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. > Unless that behavior is documented, I'd rather not depend on it. Regarding "because this other driver does it" ... telling police that you only drove faster than the speed limit because everyone does it isn't usually a well received argument. Guenter
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; /* * '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 +708,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; tcpm_pd_receive(tcpci->port, &msg); }
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@nxp.com> --- Changes in v2: - remove printing code --- drivers/usb/typec/tcpm/tcpci.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)