On 24-12-05 17:05:17, Bjorn Andersson wrote:
> 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...
Yes. Seems like dropping the flag fixes this.
>
> 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).
I'll send a patch to drop the flag and we then can debate there if it's
the right thing to do w.r.t. wakeup.
>
> Regards,
> Bjorn
