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@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.
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
