| Message ID | 20180420080613.38371-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Heikki, On 20-04-18 10:06, Heikki Krogerus wrote: > This will add an array of known USB Type-C Port devices that > will be used as a blacklist for enabling userspace-control, > and remove the PMIC ACPI HID which was used for the same > purpose. > > It turns out that on some CHT based platforms the X-Powers > PMIC is handled in firmware. The ACPI HID for it is > therefore not usable for determining the port type. The > driver now searches for known USB Type-C port devices > instead. > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Hi Hans, > > So it seems that we can't rely on the PMIC. This is my proposal for a > fix. I'm in practice just using blacklist instead of whitelist for > identifying the port type. > > Let me know if it's OK to you. I'm afraid that this is not going to work, almost all CHT devices (and some BYT devices to) define an INT33FE device, here is a quick grep on a directory where I store dsdt files from various CHT and BYT devices: [hans@shalem ~]$ ls /home/archive/hwinfo | wc -l 64 [hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l 36 So 36 out of 64 devices define an INT33FE device and at least half of the devices there are BYT. The INT33FE device is described as a "XPOWER Battery Device", so I'm not sure why you are checking for this to determine if the Type-C connector is controlled by firmware. I have a bunch of CHT devices with a Type-C connector: GPD win: -------- This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller, pi3usb30532. Which is all driven by the tcpm code. Chuwi Hi8 Pro and Chuwi Vi8 Plus: --------------------------------- These use a Type-C connector as a glorified micro-usb connector, the Hi8 Pro supports Superspeed using a hardwired asmedia switch to deal with upright / upside-down insertion. The Vi8 Plus is USB2 only. Host/device mode is determine by treating the Cc pin as an id-pin (it has a gpio connected which is either low or high depending on the Cc being pulled up/down). These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger detection there is no PD and no 3A / 1.5A / 0.5A detection based on the Cc pull up resistor, resulting in charging at 0.5 A when using a Type-C charger which does not do BC1.2 on its USB2 lines. As said these really treat the Type-C connector as a micro-sub connector, so we need userspace control to be able to switch to host mode when using an otg charging-hub (so that the user can still use devices attached to the hub while charging). Asus T100HA and Lenovo Ideapad Miix 320: ---------------------------------------- These have an USB host only Type-C connector, which does do Superspeed (likely contain a fixed switch to deal with upright / upside-down insertion). These cannot charge at all through the Type-C connector (they do turn of the 5v boost when used in device mode), theoretically the port may work in device mode (depending on which lanes they used), but the device-mode USB controller is disabled by the BIOS with no option to change it. I also have 5 different CHT devices with a micro-usb connector: -Cube iWork8 Air -Jumper Ezpad mini 3 -Pipo W2s -Teclast X80 pro -Lenovo Ideapad Miix 310 All of which use an AXP288 PMIC and can do charging, host and device mode through their micro-usb connector with the exception of the Lenovo Ideapad Miix 310 where the micro-usb is host mode only (like the Type-C on the 320) and there is a separate power-barrel connector for charging. All these define an INT33FE device, although at least on the Cube and Lenovo ones the INT33FE's device _STA returns 0, so your check should not match, but on others the _STA for the INT33FE device does return 0x0f. TL;DR: Checking the INT33FE device to determine if a Type-C connector is present is not a good way to handle this. Can you describe the device on which you are seeing userspace control being enabled even though it should not be enabled and maybe mail me an ACPI dump for it? Regards, Hans > > > Thanks, > > --- > .../usb/roles/intel-xhci-usb-role-switch.c | 40 +++++++++++-------- > 1 file changed, 24 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c > index de72eedb762e..1696d1f25769 100644 > --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c > +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c > @@ -38,19 +38,26 @@ struct intel_xhci_usb_data { > void __iomem *base; > }; > > -struct intel_xhci_acpi_match { > - const char *hid; > - int hrv; > +static const struct acpi_device_id typec_port_devices[] = { > + { "PNP0CA0", 0 }, /* UCSI */ > + { "INT33FE", 0 }, /* CHT USB Type-C PHY */ > + { }, > }; > > -/* > - * ACPI IDs for PMICs which do not support separate data and power role > - * detection (USB ACA detection for micro USB OTG), we allow userspace to > - * change the role manually on these. > - */ > -static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = { > - { "INT33F4", 3 }, /* X-Powers AXP288 PMIC */ > -}; > +static acpi_status intel_xhci_userspace_ctrl(acpi_handle handle, u32 level, > + void *ctx, void **ret) > +{ > + struct acpi_device *adev; > + > + if (acpi_bus_get_device(handle, &adev)) > + return AE_OK; > + > + /* Don't allow userspace-control with Type-C ports */ > + if (!acpi_match_device_ids(adev, typec_port_devices)) > + return AE_ACCESS; > + > + return AE_OK; > +} > > static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) > { > @@ -137,7 +144,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct intel_xhci_usb_data *data; > struct resource *res; > - int i; > + acpi_status status; > > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > @@ -148,10 +155,11 @@ static int intel_xhci_usb_probe(struct platform_device *pdev) > if (!data->base) > return -ENOMEM; > > - for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++) > - if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1", > - allow_userspace_ctrl_ids[i].hrv)) > - sw_desc.allow_userspace_control = true; > + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, > + ACPI_UINT32_MAX, intel_xhci_userspace_ctrl, > + NULL, NULL, NULL); > + if (ACPI_SUCCESS(status)) > + sw_desc.allow_userspace_control = true; > > platform_set_drvdata(pdev, data); > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 20, 2018 at 11:16:05AM +0200, Hans de Goede wrote: > Hi Heikki, > > On 20-04-18 10:06, Heikki Krogerus wrote: > > This will add an array of known USB Type-C Port devices that > > will be used as a blacklist for enabling userspace-control, > > and remove the PMIC ACPI HID which was used for the same > > purpose. > > > > It turns out that on some CHT based platforms the X-Powers > > PMIC is handled in firmware. The ACPI HID for it is > > therefore not usable for determining the port type. The > > driver now searches for known USB Type-C port devices > > instead. > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > --- > > Hi Hans, > > > > So it seems that we can't rely on the PMIC. This is my proposal for a > > fix. I'm in practice just using blacklist instead of whitelist for > > identifying the port type. > > > > Let me know if it's OK to you. > > I'm afraid that this is not going to work, almost all CHT devices > (and some BYT devices to) define an INT33FE device, here is a quick grep > on a directory where I store dsdt files from various CHT and BYT devices: > > [hans@shalem ~]$ ls /home/archive/hwinfo | wc -l > 64 > [hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l > 36 > > So 36 out of 64 devices define an INT33FE device and at least half > of the devices there are BYT. That does not tell you anything. You need to check the status of the device to know if it's there or not: % grep . /sys/bus/acpi/devices/INT33FE*/status > The INT33FE device is described as a "XPOWER Battery Device", so I'm > not sure why you are checking for this to determine if the Type-C > connector is controlled by firmware. Well, INT33F4 has DOS Device Name set to "XPOWER PMIC Controller". It does not give any more hints regarding the USB connector. I only used INT33FE quite simply because it is the HID used in the driver that populates the device for fusb302. But I can see now that _HID INT33FE is in practice used as the identifier for a "type" of devices instead of identifier for specific IP. That to be honest is pretty scary. _CID could have been used for that purpose, but every IP should have its own _HID. It's of course too late to complain about that. INT33FE does not indeed seem like usable for this case. > I have a bunch of CHT devices with a Type-C connector: > > GPD win: > -------- > This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller, > pi3usb30532. Which is all driven by the tcpm code. > > Chuwi Hi8 Pro and Chuwi Vi8 Plus: > --------------------------------- > These use a Type-C connector as a glorified micro-usb connector, > the Hi8 Pro supports Superspeed using a hardwired asmedia switch > to deal with upright / upside-down insertion. The Vi8 Plus is > USB2 only. > > Host/device mode is determine by treating the Cc pin as an id-pin > (it has a gpio connected which is either low or high depending > on the Cc being pulled up/down). > > These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger > detection there is no PD and no 3A / 1.5A / 0.5A detection based > on the Cc pull up resistor, resulting in charging at 0.5 A > when using a Type-C charger which does not do BC1.2 on its > USB2 lines. > > As said these really treat the Type-C connector as a micro-sub > connector, so we need userspace control to be able to switch > to host mode when using an otg charging-hub (so that the user can > still use devices attached to the hub while charging). > > Asus T100HA and Lenovo Ideapad Miix 320: > ---------------------------------------- > These have an USB host only Type-C connector, which does > do Superspeed (likely contain a fixed switch to deal with upright / > upside-down insertion). > > These cannot charge at all through the Type-C connector (they > do turn of the 5v boost when used in device mode), theoretically > the port may work in device mode (depending on which lanes they > used), but the device-mode USB controller is disabled by the BIOS > with no option to change it. In these cases we don't really need the mux. Actually, we probable should not even register the driver for it in these cases. > I also have 5 different CHT devices with a micro-usb connector: > -Cube iWork8 Air > -Jumper Ezpad mini 3 > -Pipo W2s > -Teclast X80 pro > -Lenovo Ideapad Miix 310 > > All of which use an AXP288 PMIC and can do charging, host and > device mode through their micro-usb connector with the > exception of the Lenovo Ideapad Miix 310 where the micro-usb > is host mode only (like the Type-C on the 320) and there is > a separate power-barrel connector for charging. > > All these define an INT33FE device, although at least on > the Cube and Lenovo ones the INT33FE's device _STA returns 0, > so your check should not match, but on others the _STA for > the INT33FE device does return 0x0f. > > TL;DR: Checking the INT33FE device to determine if a Type-C > connector is present is not a good way to handle this. Fair enough. > Can you describe the device on which you are seeing > userspace control being enabled even though it should not be > enabled and maybe mail me an ACPI dump for it? The other way arround. The user-space control is not enabled when it should be. It's somekind of an automotive platform. I don't have the ACPI dump, nor access to one, but I can ask for it. Right now I just know that INT33F4 is not enabled on that platform, but user space control is needed. Braswells use CHT xHCI PCI ID AFAIK, so I checked the ACPI tables I have for them. There was no node for INT33F4 at all. It could be that they are using a Braswell based board. In any case, it seems to be really difficult to reliable determine the cases where we need the userspace control. Perhaps we should just enable the control always with this mux driver after all? It's not ideal when the port is Type-C port, but I don't think there is any real risk of, for example, the user being able to break his/her board with it at least. Cheers,
Hi, On 04/20/2018 04:35 PM, Heikki Krogerus wrote: > On Fri, Apr 20, 2018 at 11:16:05AM +0200, Hans de Goede wrote: >> Hi Heikki, >> >> On 20-04-18 10:06, Heikki Krogerus wrote: >>> This will add an array of known USB Type-C Port devices that >>> will be used as a blacklist for enabling userspace-control, >>> and remove the PMIC ACPI HID which was used for the same >>> purpose. >>> >>> It turns out that on some CHT based platforms the X-Powers >>> PMIC is handled in firmware. The ACPI HID for it is >>> therefore not usable for determining the port type. The >>> driver now searches for known USB Type-C port devices >>> instead. >>> >>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> >>> --- >>> Hi Hans, >>> >>> So it seems that we can't rely on the PMIC. This is my proposal for a >>> fix. I'm in practice just using blacklist instead of whitelist for >>> identifying the port type. >>> >>> Let me know if it's OK to you. >> >> I'm afraid that this is not going to work, almost all CHT devices >> (and some BYT devices to) define an INT33FE device, here is a quick grep >> on a directory where I store dsdt files from various CHT and BYT devices: >> >> [hans@shalem ~]$ ls /home/archive/hwinfo | wc -l >> 64 >> [hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l >> 36 >> >> So 36 out of 64 devices define an INT33FE device and at least half >> of the devices there are BYT. > > That does not tell you anything. You need to check the status of the > device to know if it's there or not: > > % grep . /sys/bus/acpi/devices/INT33FE*/status > >> The INT33FE device is described as a "XPOWER Battery Device", so I'm >> not sure why you are checking for this to determine if the Type-C >> connector is controlled by firmware. > > Well, INT33F4 has DOS Device Name set to "XPOWER PMIC Controller". It > does not give any more hints regarding the USB connector. INT33F4 is the AXP288 PMIC controller and in general boards with that PMIC do not have Type-C functionality, as described in my list of devices some devices do have a Type-C connector, but they use it as a glorified micro-usb connector. > I only used INT33FE quite simply because it is the HID used in the > driver that populates the device for fusb302. INT33FE is some weird messed up PMIC co-device used for battery monitoring combined with a proprietary opregion which is registered by the INT33FE driver. The driver registering the fusb302 device has the following checks to make sure the INT33FE it is dealing with is paired with a Whiskey Cove PMIC: status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp); if (ACPI_FAILURE(status)) { dev_err(dev, "Error getting PTYPE\n"); return -ENODEV; } /* * The same ACPI HID is used for different configurations check PTYP * to ensure that we are dealing with the expected config. */ if (ptyp != EXPECTED_PTYPE) return -ENODEV; /* Check presence of INT34D3 (hardware-rev 3) expected for ptype == 4 */ if (!acpi_dev_present("INT34D3", "1", 3)) { dev_err(dev, "Error PTYPE == %d, but no INT34D3 device\n", EXPECTED_PTYPE); return -ENODEV; } At least for CHT there seems to be a 1:1 relationship between which PMIC is used and if Type-C functionality is available. > > But I can see now that _HID INT33FE is in practice used as the > identifier for a "type" of devices instead of identifier for specific > IP. That to be honest is pretty scary. _CID could have been used for > that purpose, but every IP should have its own _HID. It's of course > too late to complain about that. > > INT33FE does not indeed seem like usable for this case. > >> I have a bunch of CHT devices with a Type-C connector: >> >> GPD win: >> -------- >> This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller, >> pi3usb30532. Which is all driven by the tcpm code. >> >> Chuwi Hi8 Pro and Chuwi Vi8 Plus: >> --------------------------------- >> These use a Type-C connector as a glorified micro-usb connector, >> the Hi8 Pro supports Superspeed using a hardwired asmedia switch >> to deal with upright / upside-down insertion. The Vi8 Plus is >> USB2 only. >> >> Host/device mode is determine by treating the Cc pin as an id-pin >> (it has a gpio connected which is either low or high depending >> on the Cc being pulled up/down). >> >> These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger >> detection there is no PD and no 3A / 1.5A / 0.5A detection based >> on the Cc pull up resistor, resulting in charging at 0.5 A >> when using a Type-C charger which does not do BC1.2 on its >> USB2 lines. >> >> As said these really treat the Type-C connector as a micro-sub >> connector, so we need userspace control to be able to switch >> to host mode when using an otg charging-hub (so that the user can >> still use devices attached to the hub while charging). >> >> Asus T100HA and Lenovo Ideapad Miix 320: >> ---------------------------------------- >> These have an USB host only Type-C connector, which does >> do Superspeed (likely contain a fixed switch to deal with upright / >> upside-down insertion). >> >> These cannot charge at all through the Type-C connector (they >> do turn of the 5v boost when used in device mode), theoretically >> the port may work in device mode (depending on which lanes they >> used), but the device-mode USB controller is disabled by the BIOS >> with no option to change it. > > In these cases we don't really need the mux. Actually, we probable > should not even register the driver for it in these cases. > >> I also have 5 different CHT devices with a micro-usb connector: >> -Cube iWork8 Air >> -Jumper Ezpad mini 3 >> -Pipo W2s >> -Teclast X80 pro >> -Lenovo Ideapad Miix 310 >> >> All of which use an AXP288 PMIC and can do charging, host and >> device mode through their micro-usb connector with the >> exception of the Lenovo Ideapad Miix 310 where the micro-usb >> is host mode only (like the Type-C on the 320) and there is >> a separate power-barrel connector for charging. >> >> All these define an INT33FE device, although at least on >> the Cube and Lenovo ones the INT33FE's device _STA returns 0, >> so your check should not match, but on others the _STA for >> the INT33FE device does return 0x0f. >> >> TL;DR: Checking the INT33FE device to determine if a Type-C >> connector is present is not a good way to handle this. > > Fair enough. > >> Can you describe the device on which you are seeing >> userspace control being enabled even though it should not be >> enabled and maybe mail me an ACPI dump for it? > > The other way arround. The user-space control is not enabled when it > should be. > > It's somekind of an automotive platform. I don't have the ACPI dump, > nor access to one, but I can ask for it. Right now I just know that > INT33F4 is not enabled on that platform, but user space control is > needed. So what PMIC are they using ? Is is a CHT platform? I've done all my work on CHT platforms so the current whitelist is PMIC based and CHT only really. > Braswells use CHT xHCI PCI ID AFAIK, so I checked the ACPI tables I > have for them. There was no node for INT33F4 at all. It could be that > they are using a Braswell based board. > > In any case, it seems to be really difficult to reliable determine the > cases where we need the userspace control. Perhaps we should just > enable the control always with this mux driver after all? It's not > ideal when the port is Type-C port, but I don't think there is any > real risk of, for example, the user being able to break his/her board > with it at least. Always enabling user control of the mux is fine with me. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c b/drivers/usb/roles/intel-xhci-usb-role-switch.c index de72eedb762e..1696d1f25769 100644 --- a/drivers/usb/roles/intel-xhci-usb-role-switch.c +++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c @@ -38,19 +38,26 @@ struct intel_xhci_usb_data { void __iomem *base; }; -struct intel_xhci_acpi_match { - const char *hid; - int hrv; +static const struct acpi_device_id typec_port_devices[] = { + { "PNP0CA0", 0 }, /* UCSI */ + { "INT33FE", 0 }, /* CHT USB Type-C PHY */ + { }, }; -/* - * ACPI IDs for PMICs which do not support separate data and power role - * detection (USB ACA detection for micro USB OTG), we allow userspace to - * change the role manually on these. - */ -static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = { - { "INT33F4", 3 }, /* X-Powers AXP288 PMIC */ -}; +static acpi_status intel_xhci_userspace_ctrl(acpi_handle handle, u32 level, + void *ctx, void **ret) +{ + struct acpi_device *adev; + + if (acpi_bus_get_device(handle, &adev)) + return AE_OK; + + /* Don't allow userspace-control with Type-C ports */ + if (!acpi_match_device_ids(adev, typec_port_devices)) + return AE_ACCESS; + + return AE_OK; +} static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role) { @@ -137,7 +144,7 @@ static int intel_xhci_usb_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct intel_xhci_usb_data *data; struct resource *res; - int i; + acpi_status status; data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) @@ -148,10 +155,11 @@ static int intel_xhci_usb_probe(struct platform_device *pdev) if (!data->base) return -ENOMEM; - for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++) - if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1", - allow_userspace_ctrl_ids[i].hrv)) - sw_desc.allow_userspace_control = true; + status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, + ACPI_UINT32_MAX, intel_xhci_userspace_ctrl, + NULL, NULL, NULL); + if (ACPI_SUCCESS(status)) + sw_desc.allow_userspace_control = true; platform_set_drvdata(pdev, data);
This will add an array of known USB Type-C Port devices that will be used as a blacklist for enabling userspace-control, and remove the PMIC ACPI HID which was used for the same purpose. It turns out that on some CHT based platforms the X-Powers PMIC is handled in firmware. The ACPI HID for it is therefore not usable for determining the port type. The driver now searches for known USB Type-C port devices instead. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- Hi Hans, So it seems that we can't rely on the PMIC. This is my proposal for a fix. I'm in practice just using blacklist instead of whitelist for identifying the port type. Let me know if it's OK to you. Thanks, --- .../usb/roles/intel-xhci-usb-role-switch.c | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-)