[PATCH] usb: otg-fsm: Cancel HNP polling work when not used
Stephen Boyd
stephen.boyd at linaro.org
Wed Jun 29 19:02:32 PDT 2016
Quoting Peter Chen (2016-06-29 02:43:47)
> On Mon, Jun 27, 2016 at 06:18:27PM -0700, Stephen Boyd wrote:
> It introduces circular locking after applying it, otg_statemachine calls
> otg_leave_state, and otg_leave_state calls otg_statemachine again due
> to flush work, see below dump:
>
> I suggest moving initialization/flush hnp polling work to chipidea driver.
>
> [ 183.086987] ======================================================
> [ 183.093183] [ INFO: possible circular locking dependency detected ]
> [ 183.099471] 4.7.0-rc4-00012-gf1f333f-dirty #856 Not tainted
> [ 183.105059] -------------------------------------------------------
> [ 183.111341] kworker/0:2/114 is trying to acquire lock:
> [ 183.116492] (&ci->fsm.lock){+.+.+.}, at: [<806118dc>]
> otg_statemachine+0x20/0x470
> [ 183.124199]
> [ 183.124199] but task is already holding lock:
> [ 183.130052] ((&(&fsm->hnp_polling_work)->work)){+.+...}, at:
> [<80140368>] process_one_work+0x128/0x418
> [ 183.139568]
> [ 183.139568] which lock already depends on the new lock.
> [ 183.139568]
> [ 183.147765]
> [ 183.147765] the existing dependency chain (in reverse order) is:
> [ 183.155265]
> -> #1 ((&(&fsm->hnp_polling_work)->work)){+.+...}:
> [ 183.161371] [<8013e97c>] flush_work+0x44/0x234
> [ 183.166469] [<801411a8>] __cancel_work_timer+0x98/0x1c8
> [ 183.172347] [<80141304>] cancel_delayed_work_sync+0x14/0x18
> [ 183.178570] [<80610ef8>] otg_set_state+0x290/0xc54
> [ 183.184011] [<806119d0>] otg_statemachine+0x114/0x470
> [ 183.189712] [<8060a590>] ci_otg_fsm_work+0x40/0x190
> [ 183.195239] [<806054d0>] ci_otg_work+0xcc/0x1e4
> [ 183.200430] [<801403d4>] process_one_work+0x194/0x418
> [ 183.206136] [<8014068c>] worker_thread+0x34/0x4fc
> [ 183.211489] [<80146d08>] kthread+0xdc/0xf8
> [ 183.216238] [<80108ab0>] ret_from_fork+0x14/0x24
> [ 183.221514]
> -> #0 (&ci->fsm.lock){+.+.+.}:
> [ 183.225880] [<8016ff94>] lock_acquire+0x78/0x98
> [ 183.231062] [<80947c18>] mutex_lock_nested+0x54/0x3ec
> [ 183.236773] [<806118dc>] otg_statemachine+0x20/0x470
> [ 183.242388] [<80611df4>] otg_hnp_polling_work+0xc8/0x1a4
> [ 183.248352] [<801403d4>] process_one_work+0x194/0x418
> [ 183.254055] [<8014068c>] worker_thread+0x34/0x4fc
> [ 183.259409] [<80146d08>] kthread+0xdc/0xf8
> [ 183.264154] [<80108ab0>] ret_from_fork+0x14/0x24
> [ 183.269424]
> [ 183.269424] other info that might help us debug this:
> [ 183.269424]
> [ 183.277451] Possible unsafe locking scenario:
> [ 183.277451]
> [ 183.283389] CPU0 CPU1
> [ 183.287931] ---- ----
> [ 183.292473] lock((&(&fsm->hnp_polling_work)->work));
> [ 183.297665] lock(&ci->fsm.lock);
> [ 183.303639]
> lock((&(&fsm->hnp_polling_work)->work));
> [ 183.311347] lock(&ci->fsm.lock);
> [ 183.314801]
Hm.. perhaps we should do cancel_delayed_work() then and not require any
sync? That would require some locking in the polling worker, but that
looks racy anyway as it runs in parallel to the state machine so it
probably needs to lock with the FSM. Completely untested patch as I'm
going home from work now.
----8<----
diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c
index 73eec8c12235..6f4b7f0c81ff 100644
--- a/drivers/usb/common/usb-otg-fsm.c
+++ b/drivers/usb/common/usb-otg-fsm.c
@@ -70,7 +70,11 @@ static void otg_stop_hnp_polling(struct otg_fsm *fsm)
if (!fsm->host_req_flag)
return;
- cancel_delayed_work_sync(&fsm->hnp_polling_work);
+ /*
+ * We don't call cancel_delayed_work_sync() here because the
+ * worker is synchronized to this function via the fsm lock.
+ */
+ cancel_delayed_work(&fsm->hnp_polling_work);
}
/* Called when leaving a state. Do state clean up jobs here */
@@ -136,23 +140,27 @@ static void otg_leave_state(struct otg_fsm *fsm, enum usb_otg_state old_state)
}
}
+static int __otg_statemachine(struct otg_fsm *fsm);
+
static void otg_hnp_polling_work(struct work_struct *work)
{
struct otg_fsm *fsm = container_of(to_delayed_work(work),
struct otg_fsm, hnp_polling_work);
struct usb_device *udev;
- enum usb_otg_state state = fsm->otg->state;
+ enum usb_otg_state state;
u8 flag;
int retval;
+ mutex_lock(&fsm->lock);
+ state = fsm->otg->state;
if (state != OTG_STATE_A_HOST && state != OTG_STATE_B_HOST)
- return;
+ goto unlock;
udev = usb_hub_find_child(fsm->otg->host->root_hub, 1);
if (!udev) {
dev_err(fsm->otg->host->controller,
"no usb dev connected, can't start HNP polling\n");
- return;
+ goto unlock;
}
*fsm->host_req_flag = 0;
@@ -168,7 +176,7 @@ static void otg_hnp_polling_work(struct work_struct *work)
USB_CTRL_GET_TIMEOUT);
if (retval != 1) {
dev_err(&udev->dev, "Get one byte OTG status failed\n");
- return;
+ goto unlock;
}
flag = *fsm->host_req_flag;
@@ -176,10 +184,10 @@ static void otg_hnp_polling_work(struct work_struct *work)
/* Continue HNP polling */
schedule_delayed_work(&fsm->hnp_polling_work,
msecs_to_jiffies(T_HOST_REQ_POLL));
- return;
+ goto unlock;
} else if (flag != HOST_REQUEST_FLAG) {
dev_err(&udev->dev, "host request flag %d is invalid\n", flag);
- return;
+ goto unlock;
}
/* Host request flag is set */
@@ -200,7 +208,9 @@ static void otg_hnp_polling_work(struct work_struct *work)
fsm->b_bus_req = 0;
}
- otg_statemachine(fsm);
+ __otg_statemachine(fsm);
+unlock:
+ mutex_unlock(&fsm->lock);
}
static void otg_start_hnp_polling(struct otg_fsm *fsm)
@@ -340,12 +350,10 @@ static int otg_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
}
/* State change judgement */
-int otg_statemachine(struct otg_fsm *fsm)
+static int __otg_statemachine(struct otg_fsm *fsm)
{
enum usb_otg_state state;
- mutex_lock(&fsm->lock);
-
state = fsm->otg->state;
fsm->state_changed = 0;
/* State machine state change judgement */
@@ -458,9 +466,16 @@ int otg_statemachine(struct otg_fsm *fsm)
default:
break;
}
- mutex_unlock(&fsm->lock);
VDBG("quit statemachine, changed = %d\n", fsm->state_changed);
return fsm->state_changed;
}
+
+int otg_statemachine(struct otg_fsm *fsm)
+{
+ int ret;
+ mutex_lock(&fsm->lock);
+ ret = __otg_statemachine(fsm);
+ mutex_unlock(&fsm->lock);
+}
EXPORT_SYMBOL_GPL(otg_statemachine);
More information about the linux-arm-kernel
mailing list