* [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
@ 2021-11-18 9:23 Xu Yang
2021-11-18 13:18 ` Heikki Krogerus
0 siblings, 1 reply; 6+ messages in thread
From: Xu Yang @ 2021-11-18 9:23 UTC (permalink / raw)
To: linux, heikki.krogerus; +Cc: gregkh, linux-usb, jun.li, linux-imx
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
2021-11-18 9:23 [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer Xu Yang
@ 2021-11-18 13:18 ` Heikki Krogerus
2021-11-18 14:38 ` Guenter Roeck
2021-11-18 15:00 ` [EXT] " Xu Yang
0 siblings, 2 replies; 6+ messages in thread
From: Heikki Krogerus @ 2021-11-18 13:18 UTC (permalink / raw)
To: Xu Yang; +Cc: linux, gregkh, linux-usb, jun.li, linux-imx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
2021-11-18 13:18 ` Heikki Krogerus
@ 2021-11-18 14:38 ` Guenter Roeck
2021-11-18 15:00 ` [EXT] " Xu Yang
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2021-11-18 14:38 UTC (permalink / raw)
To: Heikki Krogerus, Xu Yang; +Cc: gregkh, linux-usb, jun.li, linux-imx
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,
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
2021-11-18 13:18 ` Heikki Krogerus
2021-11-18 14:38 ` Guenter Roeck
@ 2021-11-18 15:00 ` Xu Yang
2021-11-19 9:25 ` Heikki Krogerus
1 sibling, 1 reply; 6+ messages in thread
From: Xu Yang @ 2021-11-18 15:00 UTC (permalink / raw)
To: Heikki Krogerus; +Cc: linux, gregkh, linux-usb, Jun Li, dl-linux-imx
Hi,
> -----Original Message-----
> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Sent: Thursday, November 18, 2021 9:18 PM
> To: Xu Yang <xu.yang_2@nxp.com>
> Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; linux-
> usb@vger.kernel.org; Jun Li <jun.li@nxp.com>; dl-linux-imx <linux-
> imx@nxp.com>
> Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but
> leave a pending timer
>
> Caution: EXT Email
>
> 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 think we should not take the port lock before kthread_destroy_worker() since a deadlock might occur. Considering a work is pending and tcpm_unregister_port is called at this time, the worker needs to flush all the works after taking the port lock in tcpm_unregister_port(). However, the work can't take the port lock anymore.
Xu Yang
>
> > 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
2021-11-18 15:00 ` [EXT] " Xu Yang
@ 2021-11-19 9:25 ` Heikki Krogerus
0 siblings, 0 replies; 6+ messages in thread
From: Heikki Krogerus @ 2021-11-19 9:25 UTC (permalink / raw)
To: Xu Yang; +Cc: linux, gregkh, linux-usb, Jun Li, dl-linux-imx
On Thu, Nov 18, 2021 at 03:00:16PM +0000, Xu Yang wrote:
> Hi,
>
> > -----Original Message-----
> > From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Sent: Thursday, November 18, 2021 9:18 PM
> > To: Xu Yang <xu.yang_2@nxp.com>
> > Cc: linux@roeck-us.net; gregkh@linuxfoundation.org; linux-
> > usb@vger.kernel.org; Jun Li <jun.li@nxp.com>; dl-linux-imx <linux-
> > imx@nxp.com>
> > Subject: [EXT] Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but
> > leave a pending timer
> >
> > Caution: EXT Email
> >
> > 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 think we should not take the port lock before kthread_destroy_worker() since
> a deadlock might occur. Considering a work is pending and tcpm_unregister_port
> is called at this time, the worker needs to flush all the works after taking
> the port lock in tcpm_unregister_port(). However, the work can't take the port
> lock anymore.
The point is that you create a race with that code. If the port lock
is not useful in this case, there needs to be something else.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer
@ 2021-11-22 5:14 Xu Yang
0 siblings, 0 replies; 6+ messages in thread
From: Xu Yang @ 2021-11-22 5:14 UTC (permalink / raw)
To: Guenter Roeck, Heikki Krogerus; +Cc: gregkh, linux-usb, Jun Li, dl-linux-imx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-22 5:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 9:23 [PATCH] usb: typec: tcpm: fix tcpm unregister port but leave a pending timer Xu Yang
2021-11-18 13:18 ` Heikki Krogerus
2021-11-18 14:38 ` Guenter Roeck
2021-11-18 15:00 ` [EXT] " Xu Yang
2021-11-19 9:25 ` Heikki Krogerus
2021-11-22 5:14 Xu Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).