linux-kernel - Re: [PATCH v4 09/18] usb: dwc3: qcom: Override VBUS when using gpio_usb

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: <2bd67925-14cf-5851-14a2-c51a065fac6c@linaro.org>
Date:   Fri, 7 Feb 2020 10:36:26 +0000
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Jack Pham <jackp@...eaurora.org>
Cc:     linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
        gregkh@...uxfoundation.org, balbi@...nel.org,
        bjorn.andersson@...aro.org, linux-kernel@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Manu Gautam <mgautam@...eaurora.org>
Subject: Re: [PATCH v4 09/18] usb: dwc3: qcom: Override VBUS when using
 gpio_usb_connector

On 07/02/2020 08:07, Jack Pham wrote:
> Hi Bryan,
> 
> On Fri, Feb 07, 2020 at 01:58:58AM +0000, Bryan O'Donoghue wrote:
>> Using the gpio_usb_connector driver also means that we are not supplying
>> VBUS via the SoC but by an external PMIC directly.
>>
>> This patch searches for a gpio_usb_connector as a child node of the core
>> DWC3 block and if found switches on the VBUS over-ride, leaving it up to
>> the role-switching code in gpio-usb-connector to switch off and on VBUS.
>   
> <snip>
> 
>>   static int dwc3_qcom_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node	*np = pdev->dev.of_node;
>> @@ -557,7 +572,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	struct dwc3_qcom	*qcom;
>>   	struct resource		*res, *parent_res = NULL;
>>   	int			ret, i;
>> -	bool			ignore_pipe_clk;
>> +	bool			ignore_pipe_clk, gpio_usb_conn;
>>   
>>   	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
>>   	if (!qcom)
>> @@ -649,9 +664,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>> +	gpio_usb_conn = dwc3_qcom_find_gpio_usb_connector(qcom->dwc3);
>>   
>> -	/* enable vbus override for device mode */
>> -	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
>> +	/* enable vbus override for device mode or GPIO USB connector mode */
>> +	if (qcom->mode == USB_DR_MODE_PERIPHERAL || gpio_usb_conn)
>>   		dwc3_qcom_vbus_overrride_enable(qcom, true);
> 
> This doesn't seem right. It looks like you are doing the vbus_override
> only once on probe() and keeping it that way regardless of the dynamic
> state of the connector, i.e. even after VBUS is physically removed
> and/or ID pin is low.
> 

Hmm, I don't see anything much in the documentation that flags why we 
want or need to toggle this.

>>   	/* register extcon to override sw_vbus on Vbus change later */
> 
> As suggested by this comment, if you look at the extcon handling, it
> intercepts the VBUS state toggling in dwc3_qcom_vbus_notifier() and
> calls vbus_override() accordingly. That way it should only be true when
> the role==USB_ROLE_DEVICE and disabled otherwise (USB_ROLE_HOST/NONE).
> 
> To me the gpio-b connector + usb-role-switch is attempting to be an
> alternative to extcon. But to correctly mimic the vbus_override()
> behavior I think we need a way to intercept when the connector child
> driver calls usb_role_switch_set_role() to the dwc3 device, but somehow
> be able to do it from up here in the parent/glue layer. Unfortunately I
> don't have a good idea of how to do that, short of shoehorning an
> "upcall" notification from drd.c to the glue, something I don't think
> Felipe would be a fan of.
> 
> Could the usb_role_switch class somehow be enhanced to support chaining
> multiple "consumers" to support this case? Such that when the gpio-b
> driver calls set_role() it could get handled both by drd.c and
> dwc3-qcom.c?

It is probably necessary eventually, but, per my reading of the 
documents and working with the hardware, I couldn't justify the 
additional work.

However if you think this patchset needs the toggle, I can look into 
getting the indicator to toggle here too.

We'd need to add some sort of linked list of notifiers to the role 
switching logic and toggle them in order.

Similar to what is done in extcon now for the various notifer hooks.

---
bod