Re: [PATCH] USB: initialize struct otg_fsm earlier

[Dmitry Osipenko <digetx@gmail.com>]

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.