linux-kernel - Re: [RFC PATCH 0/7] usb: typec: ucsi: Polling the alt modes and PDOs

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: <0bf4d8fc6d64ac553a319b8c5af49a3d7705842d.camel@redhat.com>
Date:   Wed, 09 Jun 2021 19:39:41 +0200
From:   Benjamin Berg <bberg@...hat.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/7] usb: typec: ucsi: Polling the alt modes and PDOs

On Wed, 2021-06-09 at 15:56 +0300, Heikki Krogerus wrote:
> On Wed, Jun 09, 2021 at 03:18:04PM +0300, Heikki Krogerus wrote:
> > 
> > I'm trying to get a confirmation on my suspecion that we do always
> > actually get an event from the EC firmware, but we just end up
> > filtering it out in this case because we are too slow in the driver.
> > I
> > have an idea what could be done about that, but I need to test if
> > that
> > really is the case.
> > 
> > I'll prepare a new version out of this entire series.
> 
> Actually, it's easier if you could just test this attached patch on
> top of this series. It makes sure the every single event is
> considered. I'm sorry about the hassle.

No worries! I probably should have included some more information
earlier (i.e. enabling the debug print for spurious events).

With the patch, I am seeing the following on plug

   kworker/u16:1-6847    [002] ....  1375.485010: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:2-6848    [006] ....  1375.561811: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:2-6848    [007] ....  1375.634275: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:2-6848    [003] ....  1375.743161: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1

and unplug

   kworker/u16:1-6847    [005] ....  1394.062501: ucsi_connector_change: port1 status: change=4804, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
   kworker/u16:1-6847    [005] ....  1394.161612: ucsi_connector_change: port1 status: change=4000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
   kworker/u16:1-6847    [005] ....  1394.251503: ucsi_connector_change: port1 status: change=4000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0

where all but the first event are spurious events. I believe that in
the above spurious event with the change to opmode=3, the PPM should be
reporting change=0004 (i.e. UCSI_CONSTAT_POWER_OPMODE_CHANGE).

Occasionally I also see the following on plug. Note the non-spurious
event with change=0040 (UCSI_CONSTAT_POWER_LEVEL_CHANGE) right before
the event where opmode changes.

  kworker/u16:11-2201    [001] ....  3240.124431: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:3-7469    [003] ....  3240.222799: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:3-7469    [003] ....  3240.325946: ucsi_connector_change: port1 status: change=0040, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:3-7469    [003] ....  3240.423503: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:3-7469    [003] ....  3240.861986: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
   kworker/u16:3-7469    [007] ....  3240.999048: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1

My thought when I first ran into the issue was that the PPM simply
resets the change bitfield on ACK, effectively discarding any changes
that happened after the last GET_CONNECTOR_STATUS call. I believed to
have confirmed this by inserting an msleep in between.
However, I have to admit that I have never really looked for
alternative explanations.

Benjamin

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)