linux-kernel - Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190508162211.GA5495@roeck-us.net>
Date:   Wed, 8 May 2019 09:22:11 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Angus Ainslie <angus@...ea.ca>
Cc:     angus.ainslie@...i.sm,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Guenter Roeck <groeck7@...il.com>, linux-imx@....com
Subject: Re: [PATCH v2 1/1] usb: typec: tcpci: Clear the fault status register

On Wed, May 08, 2019 at 07:48:43AM -0600, Angus Ainslie wrote:
> Hi Guenter
> 
> On 2019-05-07 23:18, Guenter Roeck wrote:
> >On 5/7/19 7:49 PM, Angus Ainslie wrote:
> >>On 2019-05-07 20:03, Guenter Roeck wrote:
> >>>On 5/7/19 5:27 PM, Angus Ainslie (Purism) wrote:
> >>>>If the fault status register doesn't get cleared then
> >>>>the ptn5110 interrupt gets stuck on. As the fault register gets
> >>>>set everytime the ptn5110 powers on the interrupt is always stuck.
> >>>>
> >>>>Fixes: fault status register stuck
> >>>>Signed-off-by: Angus Ainslie (Purism) <angus@...ea.ca>
> >>>>---
> >>>>  drivers/usb/typec/tcpm/tcpci.c | 11 +++++++++++
> >>>>  1 file changed, 11 insertions(+)
> >>>>
> >>>>diff --git a/drivers/usb/typec/tcpm/tcpci.c
> >>>>b/drivers/usb/typec/tcpm/tcpci.c
> >>>>index c1f7073a56de..a5746657b190 100644
> >>>>--- a/drivers/usb/typec/tcpm/tcpci.c
> >>>>+++ b/drivers/usb/typec/tcpm/tcpci.c
> >>>>@@ -463,6 +463,17 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
> >>>>      else if (status & TCPC_ALERT_TX_FAILED)
> >>>>          tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
> >>>>  +    if (status & TCPC_ALERT_FAULT) {
> >>>
> >>>Wait - the driver doesn't set TCPC_ALERT_FAULT in the alert mask
> >>>register. How can the chip report it if fault alerts are not enabled ?
> >>
> >>Well that I didn't check. But I know this code gets executed so
> >>something must be turning it on.
> >>
> >>Also if I don't clear it I get an unlimited number of interrupts.
> >>
> >>>What am I missing here ?
> >>
> >>Can the power on fault be masked ?
> >>
> >
> >There is a TCPC_ALERT_FAULT mask bit, so I would think so.
> >Can you dump register contents in the irq function and at the end of
> >tcpci_init() ?
> >
> 
> Ok so this seems to be related to imx8mq errata e7805:
> 
> I2C: When the I2C clock speed is configured for 400 kHz, the SCL low period
> violates the I2C spec of
> 1.3 uS min
> 
> The work around suggested by NXP is to set the clock to 384 kHz so that is
> what I did and this is the output:
> 
> [    4.091512] device: 'tcpm-source-psy-0-0052': device_add
> [    4.091581] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [    4.091596] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [    4.094774] tcpci 0-0052: ALERT MASK 0x7f
> [    4.107869] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [    4.107935] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
> [    4.110994] tcpci 0-0052: ALERT MASK 0x7f
> [    4.115511] tcpci 0-0052: FAULT ALERT status 0x80
> [    4.126332] tcpci 0-0052: ALERT MASK 0x7f
> [    4.130784] tcpci 0-0052: FAULT ALERT status 0x0
> 
> The first "ALERT MASK" is in the init function immediately after setting
> 
>         reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
>                 TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
>                 TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
>         if (tcpci->controls_vbus)
>                 reg |= TCPC_ALERT_POWER_STATUS;
>         ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> 
> 
> So it looks like the register is correct but the fault interrupt still
> fires. At 200 kHz I get the following output.
> 
> [    4.136845] device: 'tcpm-source-psy-0-0052': device_add
> [    4.136943] PM: Adding info for No Bus:tcpm-source-psy-0-0052
> [    4.136966] device: 'tcpm-source-psy-0-0052': dev_uevent: class uevent()
> returned -11
> [    4.178510] tcpci 0-0052: ALERT MASK 0x7f
> [    4.217197] driver: 'tcpci': driver_bound: bound to device '0-0052'
> [    4.217371] bus: 'i2c': really_probe: bound device 0-0052 to driver tcpci
> 
> So this is what is expected no fault interrupt.
> 
> Maybe errata e7805 needs an update.
> 
> Sorry for the noise.
> 

Let's not jump to conclusions; I don't think this is noise. It is more
likely that the i2c problem uncovers a race condition in tcpci_init().

In tcpci_init(), we first clear TCPC_ALERT by writing 0xffff into it.
Subsequently, we set TCPC_ALERT_MASK. I suspect what may happen is 
that the chip has FAULT_ALERT enabled, and that a fault was logged.
We don't clear the FAULT_STATUS register in tcpci_init(), thus
FAULT_ALERT is immediately set again, before we clear the FAULT_ALERT
mask bit.

The standard says that the alert pin shall not be set if the respective
interrupt is masked, but maybe the chip doesn't follow that. Either case,
the standard does say that masked alerts are still reported in the status
registers, so it is not surprising that the fault status is reported.

What we should probably do in tcpci_init() is to change the register
initialization order, and to clear the fault status register.

- Set TCPC_ALERT_MASK
- Set FAULT_STATUS_MASK (to 0)
- Clear TCPC_FAULT_STATUS
- Clear TCPC_ALERT

I suspect that will fix your problem.

Another question is if TCPC_ALERT_FAULT (together with appropriate
FAULT_STATUS_MASK bits) should be enabled, and if faults should be
logged. But that would be a separate patch or patch series.

Thanks,
Guenter