linux-kernel - Re: [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll

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] [day] [month] [year] [list]

Message-ID: <Z6WtgqH5CaKTfaDX@cae.in-ulm.de>
Date: Fri, 7 Feb 2025 07:51:46 +0100
From: "Christian A. Ehrhardt" <lk@...e.de>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Fedor Pchelkin <boddah8794@...il.com>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Benson Leung <bleung@...omium.org>,
	Jameson Thies <jthies@...gle.com>,
	Saranya Gopal <saranya.gopal@...el.com>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, Mark Pearson <mpearson@...ebb.ca>,
	stable@...r.kernel.org
Subject: Re: [PATCH RFC 1/2] acpi: typec: ucsi: Introduce a ->poll_cci method


Hi,

On Fri, Feb 07, 2025 at 02:38:03AM +0200, Dmitry Baryshkov wrote:
> On Thu, 6 Feb 2025 at 20:43, Fedor Pchelkin <boddah8794@...il.com> wrote:
> >
> > From: "Christian A. Ehrhardt" <lk@...e.de>
> >
> > For the ACPI backend of UCSI the UCSI "registers" are just a memory copy
> > of the register values in an opregion. The ACPI implementation in the
> > BIOS ensures that the opregion contents are synced to the embedded
> > controller and it ensures that the registers (in particular CCI) are
> > synced back to the opregion on notifications. While there is an ACPI call
> > that syncs the actual registers to the opregion there is rarely a need to
> > do this and on some ACPI implementations it actually breaks in various
> > interesting ways.
> >
> > The only reason to force a sync from the embedded controller is to poll
> > CCI while notifications are disabled. Only the ucsi core knows if this
> > is the case and guessing based on the current command is suboptimal, i.e.
> > leading to the following spurious assertion splat:
> >
> > WARNING: CPU: 3 PID: 76 at drivers/usb/typec/ucsi/ucsi.c:1388 ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> > CPU: 3 UID: 0 PID: 76 Comm: kworker/3:0 Not tainted 6.12.11-200.fc41.x86_64 #1
> > Hardware name: LENOVO 21D0/LNVNB161216, BIOS J6CN45WW 03/17/2023
> > Workqueue: events_long ucsi_init_work [typec_ucsi]
> > RIP: 0010:ucsi_reset_ppm+0x1b4/0x1c0 [typec_ucsi]
> > Call Trace:
> >  <TASK>
> >  ucsi_init_work+0x3c/0xac0 [typec_ucsi]
> >  process_one_work+0x179/0x330
> >  worker_thread+0x252/0x390
> >  kthread+0xd2/0x100
> >  ret_from_fork+0x34/0x50
> >  ret_from_fork_asm+0x1a/0x30
> >  </TASK>
> >
> > Thus introduce a ->poll_cci() method that works like ->read_cci() with an
> > additional forced sync and document that this should be used when polling
> > with notifications disabled. For all other backends that presumably don't
> > have this issue use the same implementation for both methods.
> 
> Should the ucsi_init() also use ->poll_cci instead of ->read_cci?
> Although it's executed with notifications enabled, it looks as if it
> might need the additional resync.

I don't think it should be neccessary. The command completion event
for the ucsi_send_command just above should have synced already and
anything that happens after that ought to generate an event.

Best regards,
Christian