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

[PATCH V2] 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 until
timeout even though tcpm port is removed. Once the hrtimer timeout, many
strange kernel dumps 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>

---
Changes in V2:
 -Introduced a flag "port->registered" to avoid race.
---
 drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 6010b9901126..59d4fa2443f2 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -324,6 +324,7 @@ struct tcpm_port {
 
 	bool attached;
 	bool connected;
+	bool registered;
 	bool pd_supported;
 	enum typec_port_type port_type;
 
@@ -6291,7 +6292,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->registered)
+		kthread_queue_work(port->wq, &port->state_machine);
 	return HRTIMER_NORESTART;
 }
 
@@ -6299,7 +6301,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->registered)
+		kthread_queue_work(port->wq, &port->vdm_state_machine);
 	return HRTIMER_NORESTART;
 }
 
@@ -6307,7 +6310,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->registered)
+		kthread_queue_work(port->wq, &port->enable_frs);
 	return HRTIMER_NORESTART;
 }
 
@@ -6315,7 +6319,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->registered)
+		kthread_queue_work(port->wq, &port->send_discover_work);
 	return HRTIMER_NORESTART;
 }
 
@@ -6403,6 +6408,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
 	typec_port_register_altmodes(port->typec_port,
 				     &tcpm_altmode_ops, port,
 				     port->port_altmode, ALTMODE_DISCOVERY_MAX);
+	port->registered = true;
 
 	mutex_lock(&port->lock);
 	tcpm_init(port);
@@ -6424,6 +6430,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
 {
 	int i;
 
+	port->registered = false;
+	kthread_destroy_worker(port->wq);
+
 	hrtimer_cancel(&port->send_discover_timer);
 	hrtimer_cancel(&port->enable_frs_timer);
 	hrtimer_cancel(&port->vdm_state_machine_timer);
@@ -6435,7 +6444,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

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

[Heikki Krogerus]

On Thu, Dec 09, 2021 at 06:15:07PM +0800, Xu Yang wrote:
> 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 until
> timeout even though tcpm port is removed. Once the hrtimer timeout, many
> strange kernel dumps 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>

Guenter should take a look at this.

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Changes in V2:
>  -Introduced a flag "port->registered" to avoid race.
> ---
>  drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 6010b9901126..59d4fa2443f2 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -324,6 +324,7 @@ struct tcpm_port {
>  
>  	bool attached;
>  	bool connected;
> +	bool registered;
>  	bool pd_supported;
>  	enum typec_port_type port_type;
>  
> @@ -6291,7 +6292,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->registered)
> +		kthread_queue_work(port->wq, &port->state_machine);
>  	return HRTIMER_NORESTART;
>  }
>  
> @@ -6299,7 +6301,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->registered)
> +		kthread_queue_work(port->wq, &port->vdm_state_machine);
>  	return HRTIMER_NORESTART;
>  }
>  
> @@ -6307,7 +6310,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->registered)
> +		kthread_queue_work(port->wq, &port->enable_frs);
>  	return HRTIMER_NORESTART;
>  }
>  
> @@ -6315,7 +6319,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->registered)
> +		kthread_queue_work(port->wq, &port->send_discover_work);
>  	return HRTIMER_NORESTART;
>  }
>  
> @@ -6403,6 +6408,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>  	typec_port_register_altmodes(port->typec_port,
>  				     &tcpm_altmode_ops, port,
>  				     port->port_altmode, ALTMODE_DISCOVERY_MAX);
> +	port->registered = true;
>  
>  	mutex_lock(&port->lock);
>  	tcpm_init(port);
> @@ -6424,6 +6430,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
>  {
>  	int i;
>  
> +	port->registered = false;
> +	kthread_destroy_worker(port->wq);
> +
>  	hrtimer_cancel(&port->send_discover_timer);
>  	hrtimer_cancel(&port->enable_frs_timer);
>  	hrtimer_cancel(&port->vdm_state_machine_timer);
> @@ -6435,7 +6444,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

-- 
heikki

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

[Guenter Roeck]

On 12/9/21 2:15 AM, Xu Yang wrote:
> 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 until
> timeout even though tcpm port is removed. Once the hrtimer timeout, many
> strange kernel dumps 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>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> 
> ---
> Changes in V2:
>   -Introduced a flag "port->registered" to avoid race.
> ---
>   drivers/usb/typec/tcpm/tcpm.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 6010b9901126..59d4fa2443f2 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -324,6 +324,7 @@ struct tcpm_port {
>   
>   	bool attached;
>   	bool connected;
> +	bool registered;
>   	bool pd_supported;
>   	enum typec_port_type port_type;
>   
> @@ -6291,7 +6292,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->registered)
> +		kthread_queue_work(port->wq, &port->state_machine);
>   	return HRTIMER_NORESTART;
>   }
>   
> @@ -6299,7 +6301,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->registered)
> +		kthread_queue_work(port->wq, &port->vdm_state_machine);
>   	return HRTIMER_NORESTART;
>   }
>   
> @@ -6307,7 +6310,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->registered)
> +		kthread_queue_work(port->wq, &port->enable_frs);
>   	return HRTIMER_NORESTART;
>   }
>   
> @@ -6315,7 +6319,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->registered)
> +		kthread_queue_work(port->wq, &port->send_discover_work);
>   	return HRTIMER_NORESTART;
>   }
>   
> @@ -6403,6 +6408,7 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>   	typec_port_register_altmodes(port->typec_port,
>   				     &tcpm_altmode_ops, port,
>   				     port->port_altmode, ALTMODE_DISCOVERY_MAX);
> +	port->registered = true;
>   
>   	mutex_lock(&port->lock);
>   	tcpm_init(port);
> @@ -6424,6 +6430,9 @@ void tcpm_unregister_port(struct tcpm_port *port)
>   {
>   	int i;
>   
> +	port->registered = false;
> +	kthread_destroy_worker(port->wq);
> +
>   	hrtimer_cancel(&port->send_discover_timer);
>   	hrtimer_cancel(&port->enable_frs_timer);
>   	hrtimer_cancel(&port->vdm_state_machine_timer);
> @@ -6435,7 +6444,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);
>   
>