| Message ID | 20231113173125.23696-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | initialize struct otg_fsm earlier | expand |
On 23-11-13 18:31:20, Oliver Neukum wrote: > The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list > corruption") in effect hid an issue with intialization. Typo, hit an issue with initialization. > 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. The phy-fsl-usb is not the only user, there are other users, eg, chipidea. Peter > > Fixes: bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list corruption") > 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 | 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 79617bb0a70e..4553aaab3761 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.42.0 >
Hi, On 14.11.23 08:27, Peter Chen wrote: > On 23-11-13 18:31:20, Oliver Neukum wrote: >> The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list >> corruption") in effect hid an issue with intialization. > > Typo, hit an issue with initialization. No, it hides an issue. >> 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. > > The phy-fsl-usb is not the only user, there are other users, eg, > chipidea. Interesting. Are you saying that somebody calls otg_start_hnp_polling() without a prior call to usb_otg_start()? If so, how is struct otg_fsm initialized? Currently struct otg_fsm is partially initialized and that just makes no sense. Regards Oliver
On Tue, Nov 14, 2023 at 10:44:36AM +0100, Oliver Neukum wrote: > Hi, > > On 14.11.23 08:27, Peter Chen wrote: > > On 23-11-13 18:31:20, Oliver Neukum wrote: > > > The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list > > > corruption") in effect hid an issue with intialization. > > > > Typo, hit an issue with initialization. > > No, it hides an issue. The typo was in the last word of the line, not the fourth word. That is: s/intial/initial/, not s/hid/hit/. Peter had a typo in his correction. :-) Alan Stern
On 23-11-14 10:44:36, Oliver Neukum wrote: > Hi, > > On 14.11.23 08:27, Peter Chen wrote: > > On 23-11-13 18:31:20, Oliver Neukum wrote: > > > The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list > > > corruption") in effect hid an issue with intialization. > > > > Typo, hit an issue with initialization. > > No, it hides an issue. > > > > 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. > > > > The phy-fsl-usb is not the only user, there are other users, eg, > > chipidea. > > Interesting. > Are you saying that somebody calls otg_start_hnp_polling() without > a prior call to usb_otg_start()? Yes, phy-fsl-usb is the freescale implementation. Others may have different, see ci_hdrc_otg_fsm_init for detail. Peter > If so, how is struct otg_fsm initialized? > Currently struct otg_fsm is partially initialized and that just > makes no sense. > > Regards > Oliver
On 23-11-14 10:33:55, Alan Stern wrote: > On Tue, Nov 14, 2023 at 10:44:36AM +0100, Oliver Neukum wrote: > > Hi, > > > > On 14.11.23 08:27, Peter Chen wrote: > > > On 23-11-13 18:31:20, Oliver Neukum wrote: > > > > The earlier fix bf88fef0b6f1 ("usb: otg-fsm: Fix hrtimer list > > > > corruption") in effect hid an issue with intialization. > > > > > > Typo, hit an issue with initialization. > > > > No, it hides an issue. > > The typo was in the last word of the line, not the fourth word. That is: > > s/intial/initial/, not s/hid/hit/. > > Peter had a typo in his correction. :-) > Alan, thanks for pointing it.
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 79617bb0a70e..4553aaab3761 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; };
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 <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 | 1 - 3 files changed, 2 insertions(+), 7 deletions(-)