Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

[Xu Yang]

Hi,

> -----Original Message-----
> 
> On 11/18/21 5:18 AM, Heikki Krogerus wrote:
> > Hi,
> >
> > On Thu, Nov 18, 2021 at 05:23:52PM +0800, Xu Yang wrote:
> >> @@ -6428,6 +6432,9 @@ void tcpm_unregister_port(struct tcpm_port
> *port)
> >>   {
> >>      int i;
> >
> > You need to take the port lock here, no?
> >
> >          mutex_lock(&port->lock);
> >
> >> +    kthread_destroy_worker(port->wq);
> >> +    port->wq = NULL;
> >
> >          mutex_unlock(&port->lock);
> >
> 
> I don't think the timer code runs under the lock, so that won't help.
> But, yes, the code is inherently racy. At the very least, port->wq
> would have to be set to NULL first, but even that would have to be
> synchronized. I wonder how other drivers handle that situation.
> 
> Guenter
> 

Actually, the race is unlikely to happen if the hrtimer's expire time is a
little bigger. But it's better to avoid this situation from happening. So it
may be necessary to use a flag to indicate to the hrtimer the port is going
to be unregistered and it's no need to queue work anymore.

The patch maybe like following:

struct tcpm_port  *tcpm_register_port(...)
{
	...
	port->registered = true;
	return;
}

static enum hrtimer_restart  state_machine_timer_handler(timer)
{
	if (port->registered)
		kthread_queue_work(port->wq, work);
	return HRTIMER_NORESTART;
}

void tcpm_unregister_port(port)
{
	port->registered = false;
	kthread_destroy_worker(port->wq);

	hrtimer_cancel(&timer);
	...
}

What about this idea? Or I will submit it as patch V2.

Xu Yang

Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

[Guenter Roeck]

On 11/18/21 5:18 AM, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 05:23:52PM +0800, Xu Yang wrote:
>> @@ -6428,6 +6432,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
>>   {
>>   	int i;
> 
> You need to take the port lock here, no?
> 
>          mutex_lock(&port->lock);
> 
>> +	kthread_destroy_worker(port->wq);
>> +	port->wq = NULL;
> 
>          mutex_unlock(&port->lock);
> 

I don't think the timer code runs under the lock, so that won't help.
But, yes, the code is inherently racy. At the very least, port->wq
would have to be set to NULL first, but even that would have to be
synchronized. I wonder how other drivers handle that situation.

Guenter

>>   	hrtimer_cancel(&port->send_discover_timer);
>>   	hrtimer_cancel(&port->enable_frs_timer);
>>   	hrtimer_cancel(&port->vdm_state_machine_timer);
>> @@ -6439,7 +6446,6 @@ void tcpm_unregister_port(struct tcpm_port *port)
>>   	typec_unregister_port(port->typec_port);
>>   	usb_role_switch_put(port->role_sw);
>>   	tcpm_debugfs_exit(port);
>> -	kthread_destroy_worker(port->wq);
>>   }
>>   EXPORT_SYMBOL_GPL(tcpm_unregister_port);
> 
> thanks,
> 

Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

[Heikki Krogerus]

Hi,

On Thu, Nov 18, 2021 at 05:23:52PM +0800, Xu Yang wrote:
> @@ -6428,6 +6432,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
>  {
>  	int i;

You need to take the port lock here, no?

        mutex_lock(&port->lock);

> +	kthread_destroy_worker(port->wq);
> +	port->wq = NULL;

        mutex_unlock(&port->lock);

>  	hrtimer_cancel(&port->send_discover_timer);
>  	hrtimer_cancel(&port->enable_frs_timer);
>  	hrtimer_cancel(&port->vdm_state_machine_timer);
> @@ -6439,7 +6446,6 @@ void tcpm_unregister_port(struct tcpm_port *port)
>  	typec_unregister_port(port->typec_port);
>  	usb_role_switch_put(port->role_sw);
>  	tcpm_debugfs_exit(port);
> -	kthread_destroy_worker(port->wq);
>  }
>  EXPORT_SYMBOL_GPL(tcpm_unregister_port);

thanks,

-- 
heikki

[PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer

[Xu Yang]

In current design, when the tcpm port is unregisterd, the kthread_worker
will be destroyed in the last step. Inside the kthread_destroy_worker(),
the worker will flush all the works and wait for them to end. However, if
one of the works calls hrtimer_start(), this hrtimer will be pending for
timeout even through tcpm port is removed. Once the hrtimer timeout, many
strange kernel dumps will appear.

Thus, we can first complete kthread_destroy_worker(), then cancel all the
hrtimers. This will guarantee that no hrtimer is pending at the end.

Fixes: 3ed8e1c2ac99 ("usb: typec: tcpm: Migrate workqueue to RT priority for processing events")
cc: <stable@vger.kernel.org>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/typec/tcpm/tcpm.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 7f2f3ff1b391..91136d71640f 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -6295,7 +6295,8 @@ static enum hrtimer_restart state_machine_timer_handler(struct hrtimer *timer)
 {
 	struct tcpm_port *port = container_of(timer, struct tcpm_port, state_machine_timer);
 
-	kthread_queue_work(port->wq, &port->state_machine);
+	if (port->wq)
+		kthread_queue_work(port->wq, &port->state_machine);
 	return HRTIMER_NORESTART;
 }
 
@@ -6303,7 +6304,8 @@ static enum hrtimer_restart vdm_state_machine_timer_handler(struct hrtimer *time
 {
 	struct tcpm_port *port = container_of(timer, struct tcpm_port, vdm_state_machine_timer);
 
-	kthread_queue_work(port->wq, &port->vdm_state_machine);
+	if (port->wq)
+		kthread_queue_work(port->wq, &port->vdm_state_machine);
 	return HRTIMER_NORESTART;
 }
 
@@ -6311,7 +6313,8 @@ static enum hrtimer_restart enable_frs_timer_handler(struct hrtimer *timer)
 {
 	struct tcpm_port *port = container_of(timer, struct tcpm_port, enable_frs_timer);
 
-	kthread_queue_work(port->wq, &port->enable_frs);
+	if (port->wq)
+		kthread_queue_work(port->wq, &port->enable_frs);
 	return HRTIMER_NORESTART;
 }
 
@@ -6319,7 +6322,8 @@ static enum hrtimer_restart send_discover_timer_handler(struct hrtimer *timer)
 {
 	struct tcpm_port *port = container_of(timer, struct tcpm_port, send_discover_timer);
 
-	kthread_queue_work(port->wq, &port->send_discover_work);
+	if (port->wq)
+		kthread_queue_work(port->wq, &port->send_discover_work);
 	return HRTIMER_NORESTART;
 }
 
@@ -6428,6 +6432,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
 {
 	int i;
 
+	kthread_destroy_worker(port->wq);
+	port->wq = NULL;
+
 	hrtimer_cancel(&port->send_discover_timer);
 	hrtimer_cancel(&port->enable_frs_timer);
 	hrtimer_cancel(&port->vdm_state_machine_timer);
@@ -6439,7 +6446,6 @@ void tcpm_unregister_port(struct tcpm_port *port)
 	typec_unregister_port(port->typec_port);
 	usb_role_switch_put(port->role_sw);
 	tcpm_debugfs_exit(port);
-	kthread_destroy_worker(port->wq);
 }
 EXPORT_SYMBOL_GPL(tcpm_unregister_port);
 
-- 
2.25.1