Cleaning up bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list corruption")

Commit Message

Oliver Neukum Aug. 29, 2022, 1:10 p.m. UTC

Hi,

I am looking at that patch and I am afraid, while it does the job
it is quite unclean. In effect you introduce a flag you set, but
never clear. That is just a kludge. It really tells you that your
setup of data structures is misplaced and you should just do it earlier.

Could you test the attached patch?

	Regards
		Oliver

Comments

Dmitry Osipenko Sept. 1, 2022, 11:14 p.m. UTC | #1

29.08.2022 16:10, Oliver Neukum пишет:
> Hi,
> 
> I am looking at that patch and I am afraid, while it does the job
> it is quite unclean. In effect you introduce a flag you set, but
> never clear. That is just a kludge. It really tells you that your
> setup of data structures is misplaced and you should just do it earlier.
> 
> Could you test the attached patch?
> 
> 	Regards
> 		Oliver

That certainly won't work because phy-fsl has nothing to do with the
original problem. You'll need to check carefully every USB controller
driver. It's just not very worthwhile to do, IMO.

Patch

From a11e0684f338f6cf003eb5dfb562d91da1866cc8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <[e-mail]>
Date: Mon, 29 Aug 2022 14:42:17 +0200
Subject: [PATCH] initialize struct otg_fsm earlier

The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list
corruption") in effect hid an issue with intialization.
In effect it replaces the racy continous reinitialization
of fsm->hnp_polling_work with a delayed one-time
initialization.

This just makes no sense. As a single initialization
is sufficient, the clean solution is just to do it once
and do it early enough.

Fixes: bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list corruption")
Signed-off-by: Oliver Neukum <[e-mail]>
---
 drivers/usb/common/usb-otg-fsm.c | 7 +------
 drivers/usb/phy/phy-fsl-usb.c    | 1 +
 include/linux/usb/otg-fsm.h      | 1 -
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c
index 0697fde51d00..0aa2eb7396ce 100644
--- a/drivers/usb/common/usb-otg-fsm.c
+++ b/drivers/usb/common/usb-otg-fsm.c
@@ -117,7 +117,7 @@  static void otg_leave_state(struct otg_fsm *fsm, enum usb_otg_state old_state)
 	}
 }
 
-static void otg_hnp_polling_work(struct work_struct *work)
+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);
@@ -193,11 +193,6 @@  static void otg_start_hnp_polling(struct otg_fsm *fsm)
 	if (!fsm->host_req_flag)
 		return;
 
-	if (!fsm->hnp_work_inited) {
-		INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work);
-		fsm->hnp_work_inited = true;
-	}
-
 	schedule_delayed_work(&fsm->hnp_polling_work,
 					msecs_to_jiffies(T_HOST_REQ_POLL));
 }
diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index 972704262b02..c3bac7eefe82 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -844,6 +844,7 @@  int usb_otg_start(struct platform_device *pdev)
 
 	/* Initialize the state machine structure with default values */
 	SET_OTG_STATE(otg_trans, OTG_STATE_UNDEFINED);
+	INIT_DELAYED_WORK(&fsm->hnp_polling_work, otg_hnp_polling_work);
 	fsm->otg = p_otg->phy.otg;
 
 	/* We don't require predefined MEM/IRQ resource index */
diff --git a/include/linux/usb/otg-fsm.h b/include/linux/usb/otg-fsm.h
index 6135d076c53d..cc0bc4edf227 100644
--- a/include/linux/usb/otg-fsm.h
+++ b/include/linux/usb/otg-fsm.h
@@ -183,7 +183,6 @@  struct otg_fsm {
 	struct mutex lock;
 	u8 *host_req_flag;
 	struct delayed_work hnp_polling_work;
-	bool hnp_work_inited;
 	bool state_changed;
 };
 
-- 
2.35.3