linux-kernel - Re: [PATCH RFC] usb: typec: ucsi: Schedule connector worker on freezable workqueue

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: <dp5ohy5piss7osycibuke7kwnfl5elnmc6gfvqgfh6mzzrldla@sru3ylzrtuea>
Date: Wed, 5 Feb 2025 19:31:31 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Abel Vesa <abel.vesa@...aro.org>
Cc: Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, Caleb Connolly <caleb.connolly@...aro.org>, 
	Johan Hovold <johan@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>, 
	linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] usb: typec: ucsi: Schedule connector worker on
 freezable workqueue

On Wed, Feb 05, 2025 at 01:45:05PM +0200, Abel Vesa wrote:
> Currently, the UCSI connector worker is scheduled on the non-freezable
> system workqueue. During system suspend, on a plug/unplug event, the
> worker can run before the devices have actually resumed. The UCSI
> instances can implement operations that might need to do some HW accesses
> while the devices are still suspended.
> 
> Scheduling the USCI connector worker on the freezable system workqueue
> instead will ensure the devices are resumed by the time the worker is
> scheduled to run.
> 
> Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> ---
> Sending this as an RFC since I'm not really sure if this should be done
> in the ucsi generic implementation or in the pmic glink UCSI instance.

Can we really fix PMIC GLINK instead, please? Previously you were saying
that it for some reason has to be woken up earlier.

> 
> For context, on some Qualcomm Snapdragon X Elite laptops, there are some
> i2c interfaced USB Type-C retimers (ParadeTech PS8830) that need to be
> configured on each plug/unplug event. Since the i2c controller is
> suspended when the UCSI connector worker gets scheduled, it results in
> the following:
> 
> [   70.036669] i2c i2c-4: Transfer while suspended
> [   70.036802] WARNING: CPU: 0 PID: 819 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> [   70.036945] CPU: 0 UID: 0 PID: 819 Comm: kworker/0:4 Tainted: G        W          6.13.0+ #84
> [   70.036949] Tainted: [W]=WARN
> [   70.036950] Hardware name: LENOVO 21N10007UK/21N10007UK, BIOS N42ET85W (2.15 ) 11/22/2024
> [   70.036952] Workqueue: events ucsi_handle_connector_change [typec_ucsi]
> [   70.036959] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   70.036961] pc : __i2c_transfer+0xb4/0x57c [i2c_core]
> [   70.036963] lr : __i2c_transfer+0xb0/0x57c [i2c_core]
> [   70.036964] sp : ffff800082ffba90
> [   70.036966] x29: ffff800082ffba90 x28: 0000000000000000 x27: ffff1cc400dba0a0
> [   70.036969] x26: 0000000000000000 x25: ffff1cc4034bd500 x24: ffff1cc400010005
> [   70.036970] x23: 0000000000000000 x22: ffff1cc400dba0a1 x21: 0000000000000001
> [   70.036972] x20: ffff1cc4011ab0f0 x19: ffff1cc4011ab160 x18: 000000000009eb8a
> [   70.036974] x17: 00000005bf44b304 x16: 00000000000000cc x15: 0000000000000004
> [   70.036976] x14: ffffde65ecd46798 x13: 0000000000000fff x12: 0000000000000003
> [   70.036978] x11: ffff3e658f1f7000 x10: 00000000ffffffff x9 : 340eced73efb4000
> [   70.036980] x8 : 340eced73efb4000 x7 : 656c696877207265 x6 : 66736e617254203a
> [   70.036982] x5 : ffffde65ece89084 x4 : ffffde65c9120093 x3 : 0000000000000000
> [   70.036984] x2 : ffff800082ffb854 x1 : 00000000000000c0 x0 : 00000000ffffff94
> [   70.036987] Call trace:
> [   70.036989]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> [   70.036994]  i2c_transfer+0x98/0xf0 [i2c_core]
> [   70.036995]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> [   70.036997]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> [   70.037001]  _regmap_raw_write_impl+0x780/0x944
> [   70.037012]  _regmap_bus_raw_write+0x60/0x7c
> [   70.037014]  _regmap_write+0x134/0x184
> [   70.037016]  regmap_write+0x54/0x78
> [   70.037018]  ps883x_set+0x58/0xec [ps883x]
> [   70.037021]  ps883x_sw_set+0x60/0x84 [ps883x]
> [   70.037022]  typec_switch_set+0x48/0x74 [typec]
> [   70.037026]  typec_set_orientation+0x24/0x6c [typec]
> [   70.037027]  pmic_glink_ucsi_connector_status+0x30/0x7c [ucsi_glink]
> [   70.037032]  ucsi_handle_connector_change+0x98/0x614 [typec_ucsi]
> [   70.037034]  process_scheduled_works+0x1a0/0x2d0
> [   70.037045]  worker_thread+0x2a8/0x3c8
> [   70.037046]  kthread+0xfc/0x184
> [   70.037048]  ret_from_fork+0x10/0x20
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index fcf499cc9458c0d12015a7e36e5f1ac448c3a431..8c6081e0cd6155a59ca733070cd93e6b79398b3e 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1307,7 +1307,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  	}
>  
>  	if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
> -		schedule_work(&con->work);
> +		queue_work(system_freezable_wq, &con->work);
>  }
>  EXPORT_SYMBOL_GPL(ucsi_connector_change);
>  
> 
> ---
> base-commit: 00f3246adeeacbda0bd0b303604e46eb59c32e6e
> change-id: 20250205-ucsi-schedule-conn-worker-on-freezable-wq-cff0a880b08b
> 
> Best regards,
> -- 
> Abel Vesa <abel.vesa@...aro.org>
> 

-- 
With best wishes
Dmitry