[PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Abel Vesa]

The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
When the system is suspended (s2idle), the fact that the worker can be
scheduled to run while devices are still suspended provesto be a problem
when a Type-C retimer, switch or mux that is controlled over a bus like
I2C, because the I2C controller is suspended.

This has been proven to be the case on the X Elite boards where such
retimers (ParadeTech PS8830) are used in order to handle Type-C
orientation and altmode configuration. The following warning is thrown:

[   35.134876] i2c i2c-4: Transfer while suspended
[   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
[   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
[   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[   35.455242] Call trace:
[   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
[   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
[   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
[   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
[   35.478659]  _regmap_raw_write_impl+0x780/0x944
[   35.483401]  _regmap_bus_raw_write+0x60/0x7c
[   35.487848]  _regmap_write+0x134/0x184
[   35.491773]  regmap_write+0x54/0x78
[   35.495418]  ps883x_set+0x58/0xec [ps883x]
[   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
[   35.504223]  typec_switch_set+0x48/0x74 [typec]
[   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
[   35.515712]  process_scheduled_works+0x1a0/0x2d0
[   35.520525]  worker_thread+0x2a8/0x3c8
[   35.524449]  kthread+0xfc/0x184
[   35.527749]  ret_from_fork+0x10/0x20

The solution here is to schedule the altmode worker on the system_freezable_wq
instead of the system_wq. This will result in the altmode worker not being
scheduled to run until the devices are resumed first, which will give the
controllers like I2C a chance to resume before the transfer is requested.

Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
Cc: stable@vger.kernel.org    # 6.3
Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
 drivers/soc/qcom/pmic_glink_altmode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index bd06ce16180411059e9efb14d9aeccda27744280..bde129aa7d90a39becaa720376c0539bcaa492fb 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -295,7 +295,7 @@ static void pmic_glink_altmode_sc8180xp_notify(struct pmic_glink_altmode *altmod
 	alt_port->mode = mode;
 	alt_port->hpd_state = hpd_state;
 	alt_port->hpd_irq = hpd_irq;
-	schedule_work(&alt_port->work);
+	queue_work(system_freezable_wq, &alt_port->work);
 }
 
 #define SC8280XP_DPAM_MASK	0x3f
@@ -338,7 +338,7 @@ static void pmic_glink_altmode_sc8280xp_notify(struct pmic_glink_altmode *altmod
 	alt_port->mode = mode;
 	alt_port->hpd_state = hpd_state;
 	alt_port->hpd_irq = hpd_irq;
-	schedule_work(&alt_port->work);
+	queue_work(system_freezable_wq, &alt_port->work);
 }
 
 static void pmic_glink_altmode_callback(const void *data, size_t len, void *priv)

---
base-commit: 2b88851f583d3c4e40bcd40cfe1965241ec229dd
change-id: 20250110-soc-qcom-pmic-glink-fix-device-access-on-worker-while-suspended-af54c5e43ed6

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Caleb Connolly]

On 10/01/2025 16:29, Abel Vesa wrote:
> The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> When the system is suspended (s2idle), the fact that the worker can be
> scheduled to run while devices are still suspended provesto be a problem
> when a Type-C retimer, switch or mux that is controlled over a bus like
> I2C, because the I2C controller is suspended.
> 
> This has been proven to be the case on the X Elite boards where such
> retimers (ParadeTech PS8830) are used in order to handle Type-C
> orientation and altmode configuration. The following warning is thrown:
> 
> [   35.134876] i2c i2c-4: Transfer while suspended
> [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   35.455242] Call trace:
> [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> [   35.487848]  _regmap_write+0x134/0x184
> [   35.491773]  regmap_write+0x54/0x78
> [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> [   35.520525]  worker_thread+0x2a8/0x3c8
> [   35.524449]  kthread+0xfc/0x184
> [   35.527749]  ret_from_fork+0x10/0x20
> 
> The solution here is to schedule the altmode worker on the system_freezable_wq
> instead of the system_wq. This will result in the altmode worker not being
> scheduled to run until the devices are resumed first, which will give the
> controllers like I2C a chance to resume before the transfer is requested.

Nice job figuring this out, it was definitely a confusing one :D
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: stable@vger.kernel.org    # 6.3
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

Reviewed-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/soc/qcom/pmic_glink_altmode.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index bd06ce16180411059e9efb14d9aeccda27744280..bde129aa7d90a39becaa720376c0539bcaa492fb 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -295,7 +295,7 @@ static void pmic_glink_altmode_sc8180xp_notify(struct pmic_glink_altmode *altmod
>  	alt_port->mode = mode;
>  	alt_port->hpd_state = hpd_state;
>  	alt_port->hpd_irq = hpd_irq;
> -	schedule_work(&alt_port->work);
> +	queue_work(system_freezable_wq, &alt_port->work);
>  }
>  
>  #define SC8280XP_DPAM_MASK	0x3f
> @@ -338,7 +338,7 @@ static void pmic_glink_altmode_sc8280xp_notify(struct pmic_glink_altmode *altmod
>  	alt_port->mode = mode;
>  	alt_port->hpd_state = hpd_state;
>  	alt_port->hpd_irq = hpd_irq;
> -	schedule_work(&alt_port->work);
> +	queue_work(system_freezable_wq, &alt_port->work);
>  }
>  
>  static void pmic_glink_altmode_callback(const void *data, size_t len, void *priv)
> 
> ---
> base-commit: 2b88851f583d3c4e40bcd40cfe1965241ec229dd
> change-id: 20250110-soc-qcom-pmic-glink-fix-device-access-on-worker-while-suspended-af54c5e43ed6
> 
> Best regards,

-- 
// Caleb (they/them)

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Krzysztof Kozlowski]

On 10/01/2025 16:29, Abel Vesa wrote:
> The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> When the system is suspended (s2idle), the fact that the worker can be
> scheduled to run while devices are still suspended provesto be a problem
> when a Type-C retimer, switch or mux that is controlled over a bus like
> I2C, because the I2C controller is suspended.
> 
> This has been proven to be the case on the X Elite boards where such
> retimers (ParadeTech PS8830) are used in order to handle Type-C
> orientation and altmode configuration. The following warning is thrown:
> 
> [   35.134876] i2c i2c-4: Transfer while suspended
> [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   35.455242] Call trace:
> [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> [   35.487848]  _regmap_write+0x134/0x184
> [   35.491773]  regmap_write+0x54/0x78
> [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> [   35.520525]  worker_thread+0x2a8/0x3c8
> [   35.524449]  kthread+0xfc/0x184
> [   35.527749]  ret_from_fork+0x10/0x20
> 
> The solution here is to schedule the altmode worker on the system_freezable_wq
> instead of the system_wq. This will result in the altmode worker not being
> scheduled to run until the devices are resumed first, which will give the
> controllers like I2C a chance to resume before the transfer is requested.
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: stable@vger.kernel.org    # 6.3
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

This is an incomplete fix, I think. You fix one case but several other
possibilities are still there:

1. Maybe the driver just lacks proper suspend/resume handling?
I assume all this happens during system suspend, so what certainty you
have that your second work - pmic_glink_altmode_pdr_notify() - is not
executed as well?

2. Follow up: all other drivers and all other future use cases will be
affected as well. Basically what this patch is admitting is that driver
can be executed anytime, even during suspend, so each call of
pmic_glink_send() has to be audited. Now and in the future, because what
stops some developer of adding one more path calling pmic_glink_send(),
which also turns out to be executed during suspend?

3. So qcom_battmgr.c is buggy as well?

4. ucsi_glink.c? I don't see handling suspend, either...

Maybe the entire problem is how pmic glink was designed: not as proper
bus driver which handles both child-parent relationship and system suspend.
Best regards,
Krzysztof

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Bjorn Andersson]

On Sat, Jan 11, 2025 at 04:35:09PM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2025 16:29, Abel Vesa wrote:
> > The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> > When the system is suspended (s2idle), the fact that the worker can be
> > scheduled to run while devices are still suspended provesto be a problem
> > when a Type-C retimer, switch or mux that is controlled over a bus like
> > I2C, because the I2C controller is suspended.
> > 
> > This has been proven to be the case on the X Elite boards where such
> > retimers (ParadeTech PS8830) are used in order to handle Type-C
> > orientation and altmode configuration. The following warning is thrown:
> > 
> > [   35.134876] i2c i2c-4: Transfer while suspended
> > [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> > [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> > [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> > [   35.455242] Call trace:
> > [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> > [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> > [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> > [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> > [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> > [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> > [   35.487848]  _regmap_write+0x134/0x184
> > [   35.491773]  regmap_write+0x54/0x78
> > [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> > [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> > [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> > [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> > [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> > [   35.520525]  worker_thread+0x2a8/0x3c8
> > [   35.524449]  kthread+0xfc/0x184
> > [   35.527749]  ret_from_fork+0x10/0x20
> > 
> > The solution here is to schedule the altmode worker on the system_freezable_wq
> > instead of the system_wq. This will result in the altmode worker not being
> > scheduled to run until the devices are resumed first, which will give the
> > controllers like I2C a chance to resume before the transfer is requested.
> > 
> > Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> > Cc: stable@vger.kernel.org    # 6.3
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> 
> This is an incomplete fix, I think. You fix one case but several other
> possibilities are still there:
> 

I agree, this whacks only one mole, but it's reasonable to expect that
there are more hidden here.

> 1. Maybe the driver just lacks proper suspend/resume handling?
> I assume all this happens during system suspend, so what certainty you
> have that your second work - pmic_glink_altmode_pdr_notify() - is not
> executed as well?
> 
> 2. Follow up: all other drivers and all other future use cases will be
> affected as well. Basically what this patch is admitting is that driver
> can be executed anytime, even during suspend, so each call of
> pmic_glink_send() has to be audited. Now and in the future, because what
> stops some developer of adding one more path calling pmic_glink_send(),
> which also turns out to be executed during suspend?
> 
> 3. So qcom_battmgr.c is buggy as well?
> 
> 4. ucsi_glink.c? I don't see handling suspend, either...
> 
> Maybe the entire problem is how pmic glink was designed: not as proper
> bus driver which handles both child-parent relationship and system suspend.

The underlying problem is that GLINK register its interrupt as
IRQF_NO_SUSPEND (for historical reasons) and as such incoming messages
will be delivered in late suspend and early resume. In this specific
case, a specific message is handled by pmic_glink_altmode_callback(), by
invoking schedule_work() which in this case happens to schedule
pmic_glink_altmode_worker before we've resumed the I2C controller. 

I presume with your suggestion about a pmic_glink bus driver we'd come
up with some mechanism for pmic_glink to defer these messages until
resume has happened?

As you suggest, I too suspect that we have more of these hidden in other
rpmsg client drivers.


In the discussions leading up to this patch we agreed that a better
solution would be to change GLINK (SMEM) to not deliver messages when
the system is suspended. But as this has an impact on how GLINK may or
may not wake up the system, Abel's fix is a reasonable stop-gap solution
while we work that out.

That said, this backstory, the description of the actual underlying
problem, the planned longevity (shortgevity?) of this fix are missing
from the commit message. As written, we could expect a good Samaritan to
come in and replicate this fix across all those other use cases,
contrary to the agreed plans.


@Abel, can you please make sure that your commit message captures those
aspects as well?

Regards,
Bjorn

> Best regards,
> Krzysztof
> 

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Krzysztof Kozlowski]

On 11/01/2025 20:22, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:35:09PM +0100, Krzysztof Kozlowski wrote:
>> On 10/01/2025 16:29, Abel Vesa wrote:
>>> The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
>>> When the system is suspended (s2idle), the fact that the worker can be
>>> scheduled to run while devices are still suspended provesto be a problem
>>> when a Type-C retimer, switch or mux that is controlled over a bus like
>>> I2C, because the I2C controller is suspended.
>>>
>>> This has been proven to be the case on the X Elite boards where such
>>> retimers (ParadeTech PS8830) are used in order to handle Type-C
>>> orientation and altmode configuration. The following warning is thrown:
>>>
>>> [   35.134876] i2c i2c-4: Transfer while suspended
>>> [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
>>> [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
>>> [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
>>> [   35.455242] Call trace:
>>> [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
>>> [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
>>> [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
>>> [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
>>> [   35.478659]  _regmap_raw_write_impl+0x780/0x944
>>> [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
>>> [   35.487848]  _regmap_write+0x134/0x184
>>> [   35.491773]  regmap_write+0x54/0x78
>>> [   35.495418]  ps883x_set+0x58/0xec [ps883x]
>>> [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
>>> [   35.504223]  typec_switch_set+0x48/0x74 [typec]
>>> [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
>>> [   35.515712]  process_scheduled_works+0x1a0/0x2d0
>>> [   35.520525]  worker_thread+0x2a8/0x3c8
>>> [   35.524449]  kthread+0xfc/0x184
>>> [   35.527749]  ret_from_fork+0x10/0x20
>>>
>>> The solution here is to schedule the altmode worker on the system_freezable_wq
>>> instead of the system_wq. This will result in the altmode worker not being
>>> scheduled to run until the devices are resumed first, which will give the
>>> controllers like I2C a chance to resume before the transfer is requested.
>>>
>>> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
>>> Cc: stable@vger.kernel.org    # 6.3
>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>
>> This is an incomplete fix, I think. You fix one case but several other
>> possibilities are still there:
>>
> 
> I agree, this whacks only one mole, but it's reasonable to expect that
> there are more hidden here.
> 
>> 1. Maybe the driver just lacks proper suspend/resume handling?
>> I assume all this happens during system suspend, so what certainty you
>> have that your second work - pmic_glink_altmode_pdr_notify() - is not
>> executed as well?
>>
>> 2. Follow up: all other drivers and all other future use cases will be
>> affected as well. Basically what this patch is admitting is that driver
>> can be executed anytime, even during suspend, so each call of
>> pmic_glink_send() has to be audited. Now and in the future, because what
>> stops some developer of adding one more path calling pmic_glink_send(),
>> which also turns out to be executed during suspend?
>>
>> 3. So qcom_battmgr.c is buggy as well?
>>
>> 4. ucsi_glink.c? I don't see handling suspend, either...
>>
>> Maybe the entire problem is how pmic glink was designed: not as proper
>> bus driver which handles both child-parent relationship and system suspend.
> 
> The underlying problem is that GLINK register its interrupt as
> IRQF_NO_SUSPEND (for historical reasons) and as such incoming messages
> will be delivered in late suspend and early resume. In this specific
> case, a specific message is handled by pmic_glink_altmode_callback(), by
> invoking schedule_work() which in this case happens to schedule
> pmic_glink_altmode_worker before we've resumed the I2C controller. 
> 
> I presume with your suggestion about a pmic_glink bus driver we'd come
> up with some mechanism for pmic_glink to defer these messages until
> resume has happened?

Yes. Regardless whether we call pmic_glink a bus or some sort of
transport mechanism, the users of its interface should not be called
after they are suspended.

How to do it? Maybe this can be achieved by dropping IRQF_NO_SUSPEND
flag, maybe I2C method of i2c_mark_adapter_suspended()? I2C devices (not
controllers) disable IRQ in their suspend callback.

> 
> As you suggest, I too suspect that we have more of these hidden in other
> rpmsg client drivers.

Whatever is chosen here, the driver should not receive calls from
pmic_glink or whatever other core-part after it gets suspended.

> 
> 
> In the discussions leading up to this patch we agreed that a better
> solution would be to change GLINK (SMEM) to not deliver messages when
> the system is suspended. But as this has an impact on how GLINK may or

Yep

> may not wake up the system, Abel's fix is a reasonable stop-gap solution
> while we work that out.

Then at least let's store a TODO note, that this is not complete/finished.

> 
> That said, this backstory, the description of the actual underlying
> problem, the planned longevity (shortgevity?) of this fix are missing
> from the commit message. As written, we could expect a good Samaritan to
> come in and replicate this fix across all those other use cases,
> contrary to the agreed plans.
> 
> 
> @Abel, can you please make sure that your commit message captures those
> aspects as well?
Best regards,
Krzysztof

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Abel Vesa]

On 25-01-11 13:22:12, Bjorn Andersson wrote:
> On Sat, Jan 11, 2025 at 04:35:09PM +0100, Krzysztof Kozlowski wrote:
> > On 10/01/2025 16:29, Abel Vesa wrote:
> > > The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> > > When the system is suspended (s2idle), the fact that the worker can be
> > > scheduled to run while devices are still suspended provesto be a problem
> > > when a Type-C retimer, switch or mux that is controlled over a bus like
> > > I2C, because the I2C controller is suspended.
> > > 
> > > This has been proven to be the case on the X Elite boards where such
> > > retimers (ParadeTech PS8830) are used in order to handle Type-C
> > > orientation and altmode configuration. The following warning is thrown:
> > > 
> > > [   35.134876] i2c i2c-4: Transfer while suspended
> > > [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> > > [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> > > [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> > > [   35.455242] Call trace:
> > > [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> > > [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> > > [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> > > [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> > > [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> > > [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> > > [   35.487848]  _regmap_write+0x134/0x184
> > > [   35.491773]  regmap_write+0x54/0x78
> > > [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> > > [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> > > [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> > > [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> > > [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> > > [   35.520525]  worker_thread+0x2a8/0x3c8
> > > [   35.524449]  kthread+0xfc/0x184
> > > [   35.527749]  ret_from_fork+0x10/0x20
> > > 
> > > The solution here is to schedule the altmode worker on the system_freezable_wq
> > > instead of the system_wq. This will result in the altmode worker not being
> > > scheduled to run until the devices are resumed first, which will give the
> > > controllers like I2C a chance to resume before the transfer is requested.
> > > 
> > > Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> > > Cc: stable@vger.kernel.org    # 6.3
> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > 
> > This is an incomplete fix, I think. You fix one case but several other
> > possibilities are still there:
> > 
> 
> I agree, this whacks only one mole, but it's reasonable to expect that
> there are more hidden here.
> 
> > 1. Maybe the driver just lacks proper suspend/resume handling?
> > I assume all this happens during system suspend, so what certainty you
> > have that your second work - pmic_glink_altmode_pdr_notify() - is not
> > executed as well?
> > 
> > 2. Follow up: all other drivers and all other future use cases will be
> > affected as well. Basically what this patch is admitting is that driver
> > can be executed anytime, even during suspend, so each call of
> > pmic_glink_send() has to be audited. Now and in the future, because what
> > stops some developer of adding one more path calling pmic_glink_send(),
> > which also turns out to be executed during suspend?
> > 
> > 3. So qcom_battmgr.c is buggy as well?
> > 
> > 4. ucsi_glink.c? I don't see handling suspend, either...
> > 
> > Maybe the entire problem is how pmic glink was designed: not as proper
> > bus driver which handles both child-parent relationship and system suspend.
> 
> The underlying problem is that GLINK register its interrupt as
> IRQF_NO_SUSPEND (for historical reasons) and as such incoming messages
> will be delivered in late suspend and early resume. In this specific
> case, a specific message is handled by pmic_glink_altmode_callback(), by
> invoking schedule_work() which in this case happens to schedule
> pmic_glink_altmode_worker before we've resumed the I2C controller. 
> 
> I presume with your suggestion about a pmic_glink bus driver we'd come
> up with some mechanism for pmic_glink to defer these messages until
> resume has happened?

So is the suggestion here to rework the entire pmic_glink into a bus
driver? (I like the sound of that)

I'd assume the new bus driver will have to queue the messages until it
has resumed, which is fine.

But still doesn't solve the fact that we can't filter out when to
wake-up or not. What am I missing here?

> 
> As you suggest, I too suspect that we have more of these hidden in other
> rpmsg client drivers.
> 
> 
> In the discussions leading up to this patch we agreed that a better
> solution would be to change GLINK (SMEM) to not deliver messages when
> the system is suspended. But as this has an impact on how GLINK may or
> may not wake up the system, Abel's fix is a reasonable stop-gap solution
> while we work that out.
> 
> That said, this backstory, the description of the actual underlying
> problem, the planned longevity (shortgevity?) of this fix are missing
> from the commit message. As written, we could expect a good Samaritan to
> come in and replicate this fix across all those other use cases,
> contrary to the agreed plans.
> 
> 
> @Abel, can you please make sure that your commit message captures those
> aspects as well?

Sure, will through the backstory and the reason this is a temporary fix
with the hope that this is all going to be reworked soon.

> 
> Regards,
> Bjorn

Thanks for reviewing,

Abel

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Krzysztof Kozlowski]

On 29/01/2025 09:46, Abel Vesa wrote:
>>
>>> 1. Maybe the driver just lacks proper suspend/resume handling?
>>> I assume all this happens during system suspend, so what certainty you
>>> have that your second work - pmic_glink_altmode_pdr_notify() - is not
>>> executed as well?
>>>
>>> 2. Follow up: all other drivers and all other future use cases will be
>>> affected as well. Basically what this patch is admitting is that driver
>>> can be executed anytime, even during suspend, so each call of
>>> pmic_glink_send() has to be audited. Now and in the future, because what
>>> stops some developer of adding one more path calling pmic_glink_send(),
>>> which also turns out to be executed during suspend?
>>>
>>> 3. So qcom_battmgr.c is buggy as well?
>>>
>>> 4. ucsi_glink.c? I don't see handling suspend, either...
>>>
>>> Maybe the entire problem is how pmic glink was designed: not as proper
>>> bus driver which handles both child-parent relationship and system suspend.
>>
>> The underlying problem is that GLINK register its interrupt as
>> IRQF_NO_SUSPEND (for historical reasons) and as such incoming messages
>> will be delivered in late suspend and early resume. In this specific
>> case, a specific message is handled by pmic_glink_altmode_callback(), by
>> invoking schedule_work() which in this case happens to schedule
>> pmic_glink_altmode_worker before we've resumed the I2C controller. 
>>
>> I presume with your suggestion about a pmic_glink bus driver we'd come
>> up with some mechanism for pmic_glink to defer these messages until
>> resume has happened?
> 
> So is the suggestion here to rework the entire pmic_glink into a bus
> driver? (I like the sound of that)
> 
> I'd assume the new bus driver will have to queue the messages until it
> has resumed, which is fine.

Queue or just disable interrupts/notifications to clients.

> 
> But still doesn't solve the fact that we can't filter out when to
> wake-up or not. What am I missing here?

I think this was not the concern in my email. I was only wondering about
the design flaw that we allow pmic glink to send notifications to
children anytime. And that's not how the bus-like driver should be written.



Best regards,
Krzysztof

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Caleb Connolly]

On 1/29/25 09:46, Abel Vesa wrote:
> On 25-01-11 13:22:12, Bjorn Andersson wrote:
>> On Sat, Jan 11, 2025 at 04:35:09PM +0100, Krzysztof Kozlowski wrote:
>>> On 10/01/2025 16:29, Abel Vesa wrote:
>>>> The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
>>>> When the system is suspended (s2idle), the fact that the worker can be
>>>> scheduled to run while devices are still suspended provesto be a problem
>>>> when a Type-C retimer, switch or mux that is controlled over a bus like
>>>> I2C, because the I2C controller is suspended.
>>>>
>>>> This has been proven to be the case on the X Elite boards where such
>>>> retimers (ParadeTech PS8830) are used in order to handle Type-C
>>>> orientation and altmode configuration. The following warning is thrown:
>>>>
>>>> [   35.134876] i2c i2c-4: Transfer while suspended
>>>> [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
>>>> [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
>>>> [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
>>>> [   35.455242] Call trace:
>>>> [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
>>>> [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
>>>> [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
>>>> [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
>>>> [   35.478659]  _regmap_raw_write_impl+0x780/0x944
>>>> [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
>>>> [   35.487848]  _regmap_write+0x134/0x184
>>>> [   35.491773]  regmap_write+0x54/0x78
>>>> [   35.495418]  ps883x_set+0x58/0xec [ps883x]
>>>> [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
>>>> [   35.504223]  typec_switch_set+0x48/0x74 [typec]
>>>> [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
>>>> [   35.515712]  process_scheduled_works+0x1a0/0x2d0
>>>> [   35.520525]  worker_thread+0x2a8/0x3c8
>>>> [   35.524449]  kthread+0xfc/0x184
>>>> [   35.527749]  ret_from_fork+0x10/0x20
>>>>
>>>> The solution here is to schedule the altmode worker on the system_freezable_wq
>>>> instead of the system_wq. This will result in the altmode worker not being
>>>> scheduled to run until the devices are resumed first, which will give the
>>>> controllers like I2C a chance to resume before the transfer is requested.
>>>>
>>>> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
>>>> Cc: stable@vger.kernel.org    # 6.3
>>>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
>>>
>>> This is an incomplete fix, I think. You fix one case but several other
>>> possibilities are still there:
>>>
>>
>> I agree, this whacks only one mole, but it's reasonable to expect that
>> there are more hidden here.
>>
>>> 1. Maybe the driver just lacks proper suspend/resume handling?
>>> I assume all this happens during system suspend, so what certainty you
>>> have that your second work - pmic_glink_altmode_pdr_notify() - is not
>>> executed as well?
>>>
>>> 2. Follow up: all other drivers and all other future use cases will be
>>> affected as well. Basically what this patch is admitting is that driver
>>> can be executed anytime, even during suspend, so each call of
>>> pmic_glink_send() has to be audited. Now and in the future, because what
>>> stops some developer of adding one more path calling pmic_glink_send(),
>>> which also turns out to be executed during suspend?
>>>
>>> 3. So qcom_battmgr.c is buggy as well?
>>>
>>> 4. ucsi_glink.c? I don't see handling suspend, either...
>>>
>>> Maybe the entire problem is how pmic glink was designed: not as proper
>>> bus driver which handles both child-parent relationship and system suspend.
>>
>> The underlying problem is that GLINK register its interrupt as
>> IRQF_NO_SUSPEND (for historical reasons) and as such incoming messages
>> will be delivered in late suspend and early resume. In this specific
>> case, a specific message is handled by pmic_glink_altmode_callback(), by
>> invoking schedule_work() which in this case happens to schedule
>> pmic_glink_altmode_worker before we've resumed the I2C controller.
>>
>> I presume with your suggestion about a pmic_glink bus driver we'd come
>> up with some mechanism for pmic_glink to defer these messages until
>> resume has happened?
> 
> So is the suggestion here to rework the entire pmic_glink into a bus
> driver? (I like the sound of that)
> 
> I'd assume the new bus driver will have to queue the messages until it
> has resumed, which is fine.
> 
> But still doesn't solve the fact that we can't filter out when to
> wake-up or not. What am I missing here?

Some kind of mechanism for devices on the GLINK bus to either:

* Have a callback which gets called even in suspend to "peek" messages 
and either drop them, defer until resume, or trigger a wakeup. This 
could call all the way into battmgr which could determine if a power 
cable was just plugged in for example.

* More ideally, if the message format is consistent/simple enough they 
could just tell the GLINK bus to wake up if certain bytes have certain 
values in a message, not sure if the firmware is nice enough to make 
this viable though.

> 
>>
>> As you suggest, I too suspect that we have more of these hidden in other
>> rpmsg client drivers.
>>
>>
>> In the discussions leading up to this patch we agreed that a better
>> solution would be to change GLINK (SMEM) to not deliver messages when
>> the system is suspended. But as this has an impact on how GLINK may or
>> may not wake up the system, Abel's fix is a reasonable stop-gap solution
>> while we work that out.
>>
>> That said, this backstory, the description of the actual underlying
>> problem, the planned longevity (shortgevity?) of this fix are missing
>> from the commit message. As written, we could expect a good Samaritan to
>> come in and replicate this fix across all those other use cases,
>> contrary to the agreed plans.
>>
>>
>> @Abel, can you please make sure that your commit message captures those
>> aspects as well?
> 
> Sure, will through the backstory and the reason this is a temporary fix
> with the hope that this is all going to be reworked soon.
> 
>>
>> Regards,
>> Bjorn
> 
> Thanks for reviewing,
> 
> Abel

-- 
Caleb (they/them)

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Johan Hovold]

On Fri, Jan 10, 2025 at 05:29:51PM +0200, Abel Vesa wrote:
> The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> When the system is suspended (s2idle), the fact that the worker can be
> scheduled to run while devices are still suspended provesto be a problem
> when a Type-C retimer, switch or mux that is controlled over a bus like
> I2C, because the I2C controller is suspended.
> 
> This has been proven to be the case on the X Elite boards where such
> retimers (ParadeTech PS8830) are used in order to handle Type-C
> orientation and altmode configuration. The following warning is thrown:
> 
> [   35.134876] i2c i2c-4: Transfer while suspended
> [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> [   35.455242] Call trace:
> [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> [   35.487848]  _regmap_write+0x134/0x184
> [   35.491773]  regmap_write+0x54/0x78
> [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> [   35.520525]  worker_thread+0x2a8/0x3c8
> [   35.524449]  kthread+0xfc/0x184
> [   35.527749]  ret_from_fork+0x10/0x20
> 
> The solution here is to schedule the altmode worker on the system_freezable_wq
> instead of the system_wq. This will result in the altmode worker not being
> scheduled to run until the devices are resumed first, which will give the
> controllers like I2C a chance to resume before the transfer is requested.
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: stable@vger.kernel.org    # 6.3
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>

You forgot to include:

Reported-by: Johan Hovold <johan+linaro@kernel.org>	
Link: https://lore.kernel.org/lkml/Z1CCVjEZMQ6hJ-wK@hovoldconsulting.com/

Johan

Re: [PATCH RFC] soc: qcom: pmic_glink: Fix device access from worker during suspend

[Abel Vesa]

On 25-01-13 13:16:53, Johan Hovold wrote:
> On Fri, Jan 10, 2025 at 05:29:51PM +0200, Abel Vesa wrote:
> > The pmic_glink_altmode_worker() currently gets scheduled on the system_wq.
> > When the system is suspended (s2idle), the fact that the worker can be
> > scheduled to run while devices are still suspended provesto be a problem
> > when a Type-C retimer, switch or mux that is controlled over a bus like
> > I2C, because the I2C controller is suspended.
> > 
> > This has been proven to be the case on the X Elite boards where such
> > retimers (ParadeTech PS8830) are used in order to handle Type-C
> > orientation and altmode configuration. The following warning is thrown:
> > 
> > [   35.134876] i2c i2c-4: Transfer while suspended
> > [   35.143865] WARNING: CPU: 0 PID: 99 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xb4/0x57c [i2c_core]
> > [   35.352879] Workqueue: events pmic_glink_altmode_worker [pmic_glink_altmode]
> > [   35.360179] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> > [   35.455242] Call trace:
> > [   35.457826]  __i2c_transfer+0xb4/0x57c [i2c_core] (P)
> > [   35.463086]  i2c_transfer+0x98/0xf0 [i2c_core]
> > [   35.467713]  i2c_transfer_buffer_flags+0x54/0x88 [i2c_core]
> > [   35.473502]  regmap_i2c_write+0x20/0x48 [regmap_i2c]
> > [   35.478659]  _regmap_raw_write_impl+0x780/0x944
> > [   35.483401]  _regmap_bus_raw_write+0x60/0x7c
> > [   35.487848]  _regmap_write+0x134/0x184
> > [   35.491773]  regmap_write+0x54/0x78
> > [   35.495418]  ps883x_set+0x58/0xec [ps883x]
> > [   35.499688]  ps883x_sw_set+0x60/0x84 [ps883x]
> > [   35.504223]  typec_switch_set+0x48/0x74 [typec]
> > [   35.508952]  pmic_glink_altmode_worker+0x44/0x1fc [pmic_glink_altmode]
> > [   35.515712]  process_scheduled_works+0x1a0/0x2d0
> > [   35.520525]  worker_thread+0x2a8/0x3c8
> > [   35.524449]  kthread+0xfc/0x184
> > [   35.527749]  ret_from_fork+0x10/0x20
> > 
> > The solution here is to schedule the altmode worker on the system_freezable_wq
> > instead of the system_wq. This will result in the altmode worker not being
> > scheduled to run until the devices are resumed first, which will give the
> > controllers like I2C a chance to resume before the transfer is requested.
> > 
> > Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> > Cc: stable@vger.kernel.org    # 6.3
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> 
> You forgot to include:
> 
> Reported-by: Johan Hovold <johan+linaro@kernel.org>	
> Link: https://lore.kernel.org/lkml/Z1CCVjEZMQ6hJ-wK@hovoldconsulting.com/

Oups, sorry. Will fix in the next version.

> 
> Johan

Thanks for reviewing.

Abel