Re: [PATCH v5 2/6] usb: typec: Add support for Parade PS8830 Type-C Retimer

[Bjorn Andersson]

On Wed, Dec 04, 2024 at 05:24:54PM +0100, Johan Hovold wrote:
> On Tue, Nov 12, 2024 at 07:01:11PM +0200, Abel Vesa wrote:
> > The Parade PS8830 is a USB4, DisplayPort and Thunderbolt 4 retimer,
> > controlled over I2C. It usually sits between a USB/DisplayPort PHY
> > and the Type-C connector, and provides orientation and altmode handling.
> > 
> > The boards that use this retimer are the ones featuring the Qualcomm
> > Snapdragon X Elite SoCs.
> 
> > +static int ps883x_sw_set(struct typec_switch_dev *sw,
> > +			 enum typec_orientation orientation)
> > +{
> > +	struct ps883x_retimer *retimer = typec_switch_get_drvdata(sw);
> > +	int ret = 0;
> > +
> > +	ret = typec_switch_set(retimer->typec_switch, orientation);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mutex_lock(&retimer->lock);
> > +
> > +	if (retimer->orientation != orientation) {
> > +		retimer->orientation = orientation;
> > +
> > +		ret = ps883x_set(retimer);
> > +	}
> > +
> > +	mutex_unlock(&retimer->lock);
> > +
> > +	return ret;
> > +}
> 
> This seems to indicate a bigger problem, but I see this function called
> during early resume while the i2c controller is suspended:
> 
> [   54.213900] ------------[ cut here ]------------
> [   54.213942] i2c i2c-2: Transfer while suspended
> [   54.214125] WARNING: CPU: 0 PID: 126 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0x874/0x968 [i2c_core]
> ...
> [   54.214833] CPU: 0 UID: 0 PID: 126 Comm: kworker/0:2 Not tainted 6.13.0-rc1 #11
> [   54.214844] Hardware name: Qualcomm CRD, BIOS 6.0.231221.BOOT.MXF.2.4-00348.1-HAMOA-1 12/21/2023
> [   54.214852] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> ...
> [   54.215090] Call trace:
> [   54.215097]  __i2c_transfer+0x874/0x968 [i2c_core] (P)
> [   54.215112]  __i2c_transfer+0x874/0x968 [i2c_core] (L)
> [   54.215126]  i2c_transfer+0x94/0xf0 [i2c_core]
> [   54.215140]  i2c_transfer_buffer_flags+0x5c/0x90 [i2c_core]
> [   54.215153]  regmap_i2c_write+0x20/0x58 [regmap_i2c]
> [   54.215166]  _regmap_raw_write_impl+0x740/0x894
> [   54.215184]  _regmap_bus_raw_write+0x60/0x7c
> [   54.215192]  _regmap_write+0x60/0x1b4
> [   54.215200]  regmap_write+0x4c/0x78
> [   54.215207]  ps883x_set+0xb0/0x10c [ps883x]
> [   54.215219]  ps883x_sw_set+0x74/0x98 [ps883x]
> [   54.215227]  typec_switch_set+0x58/0x90 [typec]
> [   54.215248]  pmic_glink_altmode_worker+0x3c/0x23c [pmic_glink_altmode]
> [   54.215257]  process_one_work+0x20c/0x610
> [   54.215274]  worker_thread+0x23c/0x378
> [   54.215283]  kthread+0x124/0x128
> [   54.215291]  ret_from_fork+0x10/0x20
> [   54.215303] irq event stamp: 28140
> [   54.215309] hardirqs last  enabled at (28139): [<ffffd15e3bc2a434>] __up_console_sem+0x6c/0x80
> [   54.215325] hardirqs last disabled at (28140): [<ffffd15e3c596aa4>] el1_dbg+0x24/0x8c
> [   54.215341] softirqs last  enabled at (28120): [<ffffd15e3bb9b82c>] handle_softirqs+0x4c4/0x4dc
> [   54.215355] softirqs last disabled at (27961): [<ffffd15e3bb501ec>] __do_softirq+0x14/0x20
> [   54.215363] ---[ end trace 0000000000000000 ]---
> [   54.216889] Enabling non-boot CPUs ...
> 
> This can be reproduced on the CRD (or T14s) by disconnecting, for
> example, a mass storage device while the laptop is suspended.
> 

I wonder if this is because drivers/rpmsg/qcom_glink_smem.c line 309
registers the GLINK interrupt as IRQF_NO_SUSPEND as a remnant from being
used for rpm communication...

This is no longer needed (glink/rpm code path is now different), but
iirc the cleanup got stuck in the question of dealing with wakeup
capabilities (and priorities).

Regards,
Bjorn