linux-kernel - Re: [PATCH v2 1/3] usb: typec: tcpm: Add callbacks to mitigate wakeups due to contaminant

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: <7bf33aaa-11d3-2ff2-8c32-70e11340a3a9@roeck-us.net>
Date:   Wed, 31 Aug 2022 10:51:23 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Badhri Jagan Sridharan <badhri@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Kyle Tso <kyletso@...gle.com>
Subject: Re: [PATCH v2 1/3] usb: typec: tcpm: Add callbacks to mitigate
 wakeups due to contaminant

On 8/31/22 07:45, Heikki Krogerus wrote:
> Hi Badhri,
> 
> On Tue, Aug 30, 2022 at 05:15:53PM -0700, Badhri Jagan Sridharan wrote:
>> On some of the TCPC implementations, when the Type-C port is exposed
>> to contaminants, such as water, TCPC stops toggling while reporting OPEN
>> either by the time TCPM reads CC pin status or during CC debounce
>> window. This causes TCPM to be stuck in TOGGLING state. If TCPM is made
>> to restart toggling, the behavior recurs causing redundant CPU wakeups
>> till the USB-C port is free of contaminant.
>>
>> [206199.287817] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>> [206199.640337] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>> [206199.985789] CC1: 0 -> 0, CC2: 0 -> 0 [state TOGGLING, polarity 0, disconnected]
>> ...
>>
>> To mitigate redundant TCPM wakeups, TCPCs which do have the needed hardware
>> can implement the check_contaminant callback which is invoked by TCPM
>> to evaluate for presence of contaminant. Lower level TCPC driver can
>> restart toggling through TCPM_PORT_CLEAN event when the driver detects
>> that USB-C port is free of contaminant. check_contaminant callback also passes
>> the disconnect_while_debounce flag which when true denotes that the CC pins
>> transitioned to OPEN state during the CC debounce window.
> 
> I'm a little bit concerned about the size of the state machine. I
> think this is a special case that at least in the beginning only the
> Maxim port controller can support, but it's still mixed into the
> "generic" state machine.
> 
> How about if we just add "run_state_machine" callback for the port
> controller drivers so they can handle this kind of special cases on
> their own - they can then also add custom states?
> 

Same concern here. I would very much prefer an approach as suggested below,
especially since the changes around the added disconnect_while_debounce flag
are extensive and difficult to verify.

Thanks,
Guenter

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 904c7b4ce2f0c..91c22945ba258 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -4858,9 +4858,11 @@ static void run_state_machine(struct tcpm_port *port)
>                  tcpm_set_state(port, port->pwr_role == TYPEC_SOURCE ? SRC_READY : SNK_READY, 0);
>                  break;
>          default:
> -               WARN(1, "Unexpected port state %d\n", port->state);
>                  break;
>          }
> +
> +       if (port->tcpc->run_state_machine)
> +               port->tcpc->run_state_machine(port->tcpc);
>   }
>   
>   static void tcpm_state_machine_work(struct kthread_work *work)
> 
> thanks,
>