| 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
| ||
|
Date: Wed, 8 Jul 2020 18:00:07 +0200
From: Amelie DELAUNAY <amelie.delaunay@...com>
To: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
CC: Alexandre TORGUE <alexandre.torgue@...com>,
"balbi@...nel.org" <balbi@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
Fabrice GASNIER <fabrice.gasnier@...com>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"hminas@...opsys.com" <hminas@...opsys.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>
Subject: Re: [PATCH 1/3] usb: dwc2: override PHY input signals with usb role
switch support
Hi Martin,
On 7/7/20 8:55 PM, Martin Blumenstingl wrote:
> Hi Amelie,
>
> On Tue, Jul 7, 2020 at 6:13 PM Amelie DELAUNAY <amelie.delaunay@...com> wrote:
>>
>> Hi Martin,
>>
>> On 7/4/20 7:42 PM, Martin Blumenstingl wrote:
>>> Hello Amelie,
>>>
>>> thank you for this patch - I am hoping that it will help us on Amlogic
>>> Meson8, Meson8b, Meson8m2 and GXBB SoCs as well.
>>> On these SoCs the ID detection is performed by the PHY IP and needs to
>>> be polled.
>>> I think usb_role_switch is the perfect framework for this on dwc2 side.
>>> For the PHY driver I'm going to implement the cable state using the
>>> extcon framework and then having a new usb-conn-extcon driver. This is
>>> just to give you an overview why I'm interested in this.
>>>
>>
>> I'm wondering, why use extcon framework and not the usb role switch API
>> ? This patch on dwc2 is tested on STM32MP157C-DK2 board with STUSB160x
>> Type-C controller driver recently pushed with usb role switch. You can
>> have a look here https://lore.kernel.org/patchwork/patch/1256238/.
> one of the boards that I'm working on is for example the Odroid-C1. It
> has a Micro-USB port and there's no Type-C controller present.
>
> in the next few days I'll try to send my idea as RFC, but this is the
> .dts I've come up with so far:
> &usb0 {
> dr_mode = "otg";
> usb-role-switch;
>
> connector {
> compatible = "extcon-usb-b-connector", "usb-b-connector";
> type = "micro";
> extcon = <&usb0_phy>;
> vbus-supply = <&usb_vbus>;
> };
> };
>
> I did this for two reasons:
> 1. I think the PHY is not a connector and thus it's driver shouldn't
> implement any connector specific logic (managing VBUS)
> 2. without the connector there would be a circular dependency: the USB
> controller needs the PHY to initialize but the PHY would need the USB
> controller so it can manage the role switch
>
> (or in other words: the connector replaces the Type-C controller in this case)
>
>>> [...]
>>>> +static int dwc2_drd_role_sw_set(struct usb_role_switch *sw, enum usb_role role)
>>>> +{
>>>> + struct dwc2_hsotg *hsotg = usb_role_switch_get_drvdata(sw);
>>>> + unsigned long flags;
>>>> +
>>>> + /* Skip session not in line with dr_mode */
>>>> + if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
>>>> + (role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
>>>> + return -EINVAL;
>>>> +
>>>> + /* Skip session if core is in test mode */
>>>> + if (role == USB_ROLE_NONE && hsotg->test_mode) {
>>>> + dev_dbg(hsotg->dev, "Core is in test mode\n");
>>>> + return -EBUSY;
>>>> + }
>>>> +
>>>> + spin_lock_irqsave(&hsotg->lock, flags);
>>> due to this spin_lock_irqsave() ...
>>>
>>>> + if (role == USB_ROLE_HOST) {
>>>> + if (dwc2_ovr_avalid(hsotg, true))
>>>> + goto unlock;
>>>> +
>>>> + if (hsotg->dr_mode == USB_DR_MODE_OTG)
>>>> + /*
>>>> + * This will raise a Connector ID Status Change
>>>> + * Interrupt - connID A
>>>> + */
>>>> + dwc2_force_mode(hsotg, true);
>>> ... we cannot sleep in here. the call flow is:
>>> dwc2_drd_role_sw_set
>>> spin_lock_irqsave
>>> dwc2_force_mode
>>> dwc2_wait_for_mode
>>> usleep_range
>>>
>>
>> In fact, with the avalid or bvalid overriding + the debounce filter
>> bypass, GINTSTS_CURMOD is already in the expected mode, so that we exit
>> the loop directly, without running into usleep_range.
> on my Amlogic SoC this is not the case:
> The kernel complains because of that usleep_range from within the
> spinlock context
>
> Please let me know if/how I can help debug this.
>
Could you please test with:
static int dwc2_drd_role_sw_set(struct device *dev, enum usb_role role)
{
struct dwc2_hsotg *hsotg = dev_get_drvdata(dev);
unsigned long flags;
int already = 0;
/* Skip session not in line with dr_mode */
if ((role == USB_ROLE_DEVICE && hsotg->dr_mode == USB_DR_MODE_HOST) ||
(role == USB_ROLE_HOST && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL))
return -EINVAL;
/* Skip session if core is in test mode */
if (role == USB_ROLE_NONE && hsotg->test_mode) {
dev_dbg(hsotg->dev, "Core is in test mode\n");
return -EBUSY;
}
spin_lock_irqsave(&hsotg->lock, flags);
if (role == USB_ROLE_HOST) {
already = dwc2_ovr_avalid(hsotg, true);
} else if (role == USB_ROLE_DEVICE) {
already = dwc2_ovr_bvalid(hsotg, true);
/* This clear DCTL.SFTDISCON bit */
dwc2_hsotg_core_connect(hsotg);
} else {
if (dwc2_is_device_mode(hsotg)) {
if (!dwc2_ovr_bvalid(hsotg, false))
/* This set DCTL.SFTDISCON bit */
dwc2_hsotg_core_disconnect(hsotg);
} else {
dwc2_ovr_avalid(hsotg, false);
}
}
spin_unlock_irqrestore(&hsotg->lock, flags);
if (!already &&
role != USB_ROLE_NONE && hsotg->dr_mode == USB_DR_MODE_OTG)
/* This will raise a Connector ID Status Change Interrupt */
dwc2_force_mode(hsotg, role == USB_ROLE_HOST);
dev_dbg(hsotg->dev, "%s-session valid\n",
role == USB_ROLE_NONE ? "No" :
role == USB_ROLE_HOST ? "A" : "B");
return 0;
}
dwc2_force_mode is called outside the spin_lock_irqsave so the kernel
should not complain. I've tested on my setup and the behavior seems the
same.
Regards,
Amelie
Powered by blists - more mailing lists