| Message ID | 20250417111502.140474-1-oneukum@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | USB: initialize struct otg_fsm earlier | expand |
On 25-04-17 13:14:54, Oliver Neukum wrote: > 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. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> Add Dmitry. I am okay for this change, and see what's the Dmitry's response. Peter > --- > drivers/usb/common/usb-otg-fsm.c | 7 +------ > drivers/usb/phy/phy-fsl-usb.c | 1 + > include/linux/usb/otg-fsm.h | 2 +- > 3 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c > index e11803225775..a22d536ccdf8 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 40ac68e52cee..7f0fdba689de 100644 > --- a/drivers/usb/phy/phy-fsl-usb.c > +++ b/drivers/usb/phy/phy-fsl-usb.c > @@ -845,6 +845,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..26cb7e84cd50 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; > }; > > @@ -308,5 +307,6 @@ static inline int otg_start_gadget(struct otg_fsm *fsm, int on) > } > > int otg_statemachine(struct otg_fsm *fsm); > +void otg_hnp_polling_work(struct work_struct *work); > > #endif /* __LINUX_USB_OTG_FSM_H */ > -- > 2.49.0 >
21.04.2025 04:45, Peter Chen (CIX) пишет: > On 25-04-17 13:14:54, Oliver Neukum wrote: >> 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. >> >> Signed-off-by: Oliver Neukum <oneukum@suse.com> > > Add Dmitry. > > I am okay for this change, and see what's the Dmitry's response. Thanks for notifying me > Peter >> --- >> drivers/usb/common/usb-otg-fsm.c | 7 +------ >> drivers/usb/phy/phy-fsl-usb.c | 1 + >> include/linux/usb/otg-fsm.h | 2 +- >> 3 files changed, 3 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c >> index e11803225775..a22d536ccdf8 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 40ac68e52cee..7f0fdba689de 100644 >> --- a/drivers/usb/phy/phy-fsl-usb.c >> +++ b/drivers/usb/phy/phy-fsl-usb.c >> @@ -845,6 +845,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; The original problem was fixed for the ChipIdea driver in the common USB code, while this phy-fsl-usb is the Freeescale USB driver that has nothing to do with the ChipIdea and the common code, AFAICT. Hence this patch should be wrong. I suggest not to change the original logic.
On 25-04-21 11:15:37, Dmitry Osipenko wrote: > 21.04.2025 04:45, Peter Chen (CIX) пишет: > > On 25-04-17 13:14:54, Oliver Neukum wrote: > >> 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. > >> > >> Signed-off-by: Oliver Neukum <oneukum@suse.com> > > > > Add Dmitry. > > > > I am okay for this change, and see what's the Dmitry's response. > > Thanks for notifying me > > > Peter > >> --- > >> drivers/usb/common/usb-otg-fsm.c | 7 +------ > >> drivers/usb/phy/phy-fsl-usb.c | 1 + > >> include/linux/usb/otg-fsm.h | 2 +- > >> 3 files changed, 3 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c > >> index e11803225775..a22d536ccdf8 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 40ac68e52cee..7f0fdba689de 100644 > >> --- a/drivers/usb/phy/phy-fsl-usb.c > >> +++ b/drivers/usb/phy/phy-fsl-usb.c > >> @@ -845,6 +845,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; > > The original problem was fixed for the ChipIdea driver in the common USB > code, while this phy-fsl-usb is the Freeescale USB driver that has > nothing to do with the ChipIdea and the common code, AFAICT. Hence this > patch should be wrong. I suggest not to change the original logic. > Thanks for confirming it. I did not check the user for OTG FSM carefully since there are no active users long time. I have checked that the phy-fsl-usb has not used hnp polling, and the fsm->host_req_flag is not allocated. The chipidea driver is the only user for hnp polling, so this patch is not needed. By the way, I just curious that are there any products in market still use OTG FSM?
22.04.2025 04:23, Peter Chen (CIX) пишет: > On 25-04-21 11:15:37, Dmitry Osipenko wrote: >> 21.04.2025 04:45, Peter Chen (CIX) пишет: >>> On 25-04-17 13:14:54, Oliver Neukum wrote: >>>> 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. >>>> >>>> Signed-off-by: Oliver Neukum <oneukum@suse.com> >>> >>> Add Dmitry. >>> >>> I am okay for this change, and see what's the Dmitry's response. >> >> Thanks for notifying me >> >>> Peter >>>> --- >>>> drivers/usb/common/usb-otg-fsm.c | 7 +------ >>>> drivers/usb/phy/phy-fsl-usb.c | 1 + >>>> include/linux/usb/otg-fsm.h | 2 +- >>>> 3 files changed, 3 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c >>>> index e11803225775..a22d536ccdf8 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 40ac68e52cee..7f0fdba689de 100644 >>>> --- a/drivers/usb/phy/phy-fsl-usb.c >>>> +++ b/drivers/usb/phy/phy-fsl-usb.c >>>> @@ -845,6 +845,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; >> >> The original problem was fixed for the ChipIdea driver in the common USB >> code, while this phy-fsl-usb is the Freeescale USB driver that has >> nothing to do with the ChipIdea and the common code, AFAICT. Hence this >> patch should be wrong. I suggest not to change the original logic. >> > > Thanks for confirming it. I did not check the user for OTG FSM > carefully since there are no active users long time. > > I have checked that the phy-fsl-usb has not used hnp polling, > and the fsm->host_req_flag is not allocated. The chipidea driver is > the only user for hnp polling, so this patch is not needed. > . > By the way, I just curious that are there any products in market still > use OTG FSM? There are older devices supported by mainline kernel that can make use of OTG FSM, but these devices likely are relevant to hobbyists only. Otherwise, I'm not aware of products actively using OTG FSM.
diff --git a/drivers/usb/common/usb-otg-fsm.c b/drivers/usb/common/usb-otg-fsm.c index e11803225775..a22d536ccdf8 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 40ac68e52cee..7f0fdba689de 100644 --- a/drivers/usb/phy/phy-fsl-usb.c +++ b/drivers/usb/phy/phy-fsl-usb.c @@ -845,6 +845,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..26cb7e84cd50 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; }; @@ -308,5 +307,6 @@ static inline int otg_start_gadget(struct otg_fsm *fsm, int on) } int otg_statemachine(struct otg_fsm *fsm); +void otg_hnp_polling_work(struct work_struct *work); #endif /* __LINUX_USB_OTG_FSM_H */
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. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/usb/common/usb-otg-fsm.c | 7 +------ drivers/usb/phy/phy-fsl-usb.c | 1 + include/linux/usb/otg-fsm.h | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-)