linux-kernel - Re: [PATCH v2 2/4] usb: dwc3: qcom: Implement glue callbacks to facilitate runtime suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <98f9936b-3833-4350-8249-169784be1e53@oss.qualcomm.com>
Date: Wed, 6 Aug 2025 05:48:09 +0530
From: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] usb: dwc3: qcom: Implement glue callbacks to
 facilitate runtime suspend



On 8/6/2025 4:48 AM, Thinh Nguyen wrote:

[...]

>>>>>>>>>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>>>>>>>>>> index ca7e1c02773a..d40b52e2ba01 100644
>>>>>>>>>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>>>>>>>>>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>>>>>>>>>> @@ -89,6 +89,12 @@ struct dwc3_qcom {
>>>>>>>>>>       	bool			pm_suspended;
>>>>>>>>>>       	struct icc_path		*icc_path_ddr;
>>>>>>>>>>       	struct icc_path		*icc_path_apps;
>>>>>>>>>> +
>>>>>>>>>> +	/*
>>>>>>>>>> +	 * Current role changes via usb_role_switch_set_role callback protected
>>>>>>>>>> +	 * internally by mutex lock.
>>>>>>>>>> +	 */
>>>>>>>>>> +	enum usb_role		current_role;
>>>>>>>>>
>>>>>>>>> Can we just track the current role through dwc3 core instead of an
>>>>>>>>> addition field in the glue?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Core caches only mode. We need ROLE NONE to identify cable connect. So
>>>>>>>> adding that in glue to keep track.
>>>>>>>>
>>>>>>>
>>>>>>> The controller is always either host or device, not somewhere in
>>>>>>> between. You're using ROLE_NONE to indicate connection, which is a
>>>>>>> separate state.
>>>>>>>
>>>>>>
>>>>>> Yes, but there is no flag that indicates that in dwc structure today. Also
>>>>>> since only dwc3-qcom needs it at the moment, I added that role info in glue
>>>>>> layer.
>>>>>
>>>>> How are you using ROLE NONE? Do you send a role-switch call to "none" to
>>>>> indicate disconnect? Let's not do that. Currently the dwc3 driver would
>>>>> switch back to the default mode if "none" is selected, but this is not
>>>>> well defined and implementation specific. It can be no-op and would not
>>>>> violate the interface.
>>>>>
>>>>> The role-switch interface should only be used for role-switching and not
>>>>> connection/disconnection.
>>>>>
>>>>>>
>>>>>>> I feel this should be tracked separately for clarity. The dwc3 also
>>>>>>> tracks the connection state, can we use that?
>>>>>>>
>>>>>>
>>>>>> Are you referring to the "connected" flag in DWC structure ? I see that it
>>>>>> indicates connection status only in gadget mode.
>>>>>>
>>>>>
>>>>> Yes, that flag is only for gadget.
>>>>>
>>>>> Can you provide more info of the setup? Is there a type-c controller or
>>>>> phy that can detect attach/deattach? Or it only propagates to the usb
>>>>> controller?
>>>>
>>>> My response didn't show up on lore since the mail client I used before sent
>>>> the message in HTML format. So resending my response again.
>>>>
>>>> Hi Thinh,
>>>>
>>>>    Yes this is for cases where role switching is present (either with a Type-C
>>>> controller, USB-conn-gpio, or a glink based role-switch).
>>>>
>>>>    Actually the requirement is as follows:
>>>>    1. When in device mode, if we receive a cable disconnect, we need to clear
>>>> OTG_VBUS_VALID bit of QSCRATCH register in glue address space.
>>>>    2. When cable is connected in device mode, we need to set the
>>>> OTG_VBUS_VALID bit of QSCRATCH register in glue address space.
>>>>    3. When the runstop is set back after a suspend rotuine, then we need to
>>>> set  OTG_VBUS_VALID bit of QSCRATCH register in glueaddress space.
>>>>
>>>>    To take care of all the 3 scenarios above, the set_role and run_stop
>>>> notifiers have been added.
>>>>
>>>>    The role info propagates only from core to glue. It is not sent to the phy.
>>>>
>>>
>>> When does ROLE NONE occur? Did you have the type-c driver set the role
>>> switch to "none" indicate disconnect?
>>>
>>> The vbus-valid is only for gadget, and you only care about the
>>> OTG_VBUS_VALID right? Can we just use the dwc3->connected flag? Just
>>> make sure that it's cleared on role-switch, which should be the case
>>> because we always perform soft-disconnect on gadget unbind, and make
>>> sure to set vbus-valid on pullup or gadget binding. Is there some
>>> scenarios that dwc->connected does not cover?
>>>
>>
>> Hi Thinh,
>>
>>   In case of just a cable disconnect in device mode (and default dr mode is
>> peripheral only), there would be no role switch. In that scenario, connected
>> flag would stay "true" even after removing cable. In that case, we can
>> generate disconnect interrupt only by clearing this VBUS_VALID bit and
>> inturn the suspend will succeed. So wanted to use notification from set_role
>> which would cover all cases:
>> 1. cable disconnect/cable connect
>> 2. Role switch from host->device and device->host
>>
> 
> Ok. Thanks for the explanation. How everything tied together seems
> awkward: The connector does a role-switch to "none" to trigger a
> role-switch in dwc3, which then triggers a callback to dwc3-qcom to
> clear vbus_valid, which then allows the controller to see a disconnect
> event in dwc3. Is that right?
> 

Yes, that is right.

> The connector driver should have a separate interface for
> attach/deattach in addition to role-switch. But I don't think type-c
> controller, USB-conn-gpio, and glink share the same way to handle
> connection/disconnection.
> 

I think although ROLE_NONE is for selecting default mode, it still means 
the same in USB-conn-gpio / ucsi-glink and hd3ss3220 (not sure about 
typec-controller).

> Let's keep what you proposed, and keep in mind that ROLE NONE is for
> selecting the default mode and not necessarily mean disconnection.
> 

Sure. Will do the following in v3:
1. Instead of adding host notifiers, will add 
"pm_runtime_use_autosuspend" in xhci-plat.c
2. Fix up coding issues pointed by you in patch 1/4.

Let me know if this sounds fine.

Regards,
Krishna,