| Message ID | 20240223140305.185182-1-mathias.nyman@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f3ac348e6e04501479fecf55250b25ff2092540b |
| Headers | show |
| Series | [v2] usb: usb-acpi: Set port connect type of not connectable ports correctly | expand |
Hi, On 2024-02-23 15:03, Mathias Nyman wrote: > Ports with _UPC (USB Port Capability) ACPI objects stating they are > "not connectable" are not wired to any connector or internal device. > They only exist inside the host controller. > > These ports may not have an ACPI _PLD (Physical Location of Device) > object. > > Rework the code so that _UPC is read even if _PLD does not exist, and > make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead > of "USB_PORT_CONNECT_TYPE_UNKNOWN". > > No bugs or known issues are reported due to possibly not parsing _UPC, > and thus leaving the port connect type as "unknown" instead of > "not used". Nice to have this fixed but no need to add it to stable > kernels, or urgency to get it upstream. With this patch (f3ac348e6e04501479fecf55250b25ff2092540b in next-20240307), my machine fails to boot. I was able to get some kernel console output from the pstore when compiling USB support as a module instead of built in. Reverting it on top of next-20240307 resolves the issue for me. Please tell me if there's anything else you need. Kind regards, Klara Modin > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> > --- > v1 -> v2 > - Commit message improvements > - send patch separately for easier picking to usb-next > > drivers/usb/core/usb-acpi.c | 46 ++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c > index a34b22537d7c..f250dfc3b14d 100644 > --- a/drivers/usb/core/usb-acpi.c > +++ b/drivers/usb/core/usb-acpi.c > @@ -142,12 +142,19 @@ int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable) > } > EXPORT_SYMBOL_GPL(usb_acpi_set_power_state); > > -static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, > - struct acpi_pld_info *pld) > +/* > + * Private to usb-acpi, all the core needs to know is that > + * port_dev->location is non-zero when it has been set by the firmware. > + */ > +#define USB_ACPI_LOCATION_VALID (1 << 31) > + > +static void > +usb_acpi_get_connect_type(struct usb_port *port_dev, acpi_handle *handle) > { > enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN; > struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > union acpi_object *upc = NULL; > + struct acpi_pld_info *pld; > acpi_status status; > > /* > @@ -158,6 +165,12 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, > * a usb device is directly hard-wired to the port. If no visible and > * no connectable, the port would be not used. > */ > + > + status = acpi_get_physical_device_location(handle, &pld); > + if (ACPI_SUCCESS(status) && pld) > + port_dev->location = USB_ACPI_LOCATION_VALID | > + pld->group_token << 8 | pld->group_position; > + > status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer); > if (ACPI_FAILURE(status)) > goto out; > @@ -166,25 +179,22 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, > if (!upc || (upc->type != ACPI_TYPE_PACKAGE) || upc->package.count != 4) > goto out; > > + /* UPC states port is connectable */ > if (upc->package.elements[0].integer.value) > - if (pld->user_visible) > + if (!pld) > + ; /* keep connect_type as unknown */ > + else if (pld->user_visible) > connect_type = USB_PORT_CONNECT_TYPE_HOT_PLUG; > else > connect_type = USB_PORT_CONNECT_TYPE_HARD_WIRED; > - else if (!pld->user_visible) > + else > connect_type = USB_PORT_NOT_USED; > out: > + port_dev->connect_type = connect_type; > kfree(upc); > - return connect_type; > + ACPI_FREE(pld); > } > > - > -/* > - * Private to usb-acpi, all the core needs to know is that > - * port_dev->location is non-zero when it has been set by the firmware. > - */ > -#define USB_ACPI_LOCATION_VALID (1 << 31) > - > static struct acpi_device * > usb_acpi_get_companion_for_port(struct usb_port *port_dev) > { > @@ -222,22 +232,12 @@ static struct acpi_device * > usb_acpi_find_companion_for_port(struct usb_port *port_dev) > { > struct acpi_device *adev; > - struct acpi_pld_info *pld; > - acpi_handle *handle; > - acpi_status status; > > adev = usb_acpi_get_companion_for_port(port_dev); > if (!adev) > return NULL; > > - handle = adev->handle; > - status = acpi_get_physical_device_location(handle, &pld); > - if (ACPI_SUCCESS(status) && pld) { > - port_dev->location = USB_ACPI_LOCATION_VALID > - | pld->group_token << 8 | pld->group_position; > - port_dev->connect_type = usb_acpi_get_connect_type(handle, pld); > - ACPI_FREE(pld); > - } > + usb_acpi_get_connect_type(port_dev, adev->handle); > > return adev; > }
On 2024-03-07 17:04, Klara Modin wrote: > Hi, > > On 2024-02-23 15:03, Mathias Nyman wrote: >> Ports with _UPC (USB Port Capability) ACPI objects stating they are >> "not connectable" are not wired to any connector or internal device. >> They only exist inside the host controller. >> >> These ports may not have an ACPI _PLD (Physical Location of Device) >> object. >> >> Rework the code so that _UPC is read even if _PLD does not exist, and >> make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead >> of "USB_PORT_CONNECT_TYPE_UNKNOWN". >> >> No bugs or known issues are reported due to possibly not parsing _UPC, >> and thus leaving the port connect type as "unknown" instead of >> "not used". Nice to have this fixed but no need to add it to stable >> kernels, or urgency to get it upstream. > > With this patch (f3ac348e6e04501479fecf55250b25ff2092540b in > next-20240307), my machine fails to boot. I was able to get some kernel > console output from the pstore when compiling USB support as a module > instead of built in. Reverting it on top of next-20240307 resolves the > issue for me. Correction, it does boot when building USB support as a module. Got more logs from the unhealthy system and lspci. lsusb shows a single USB 2.0 root hub and nothing else. Calling it with `-v` hangs and produces nothing useful, the same happens with lshw. > > Please tell me if there's anything else you need. > > Kind regards, > Klara Modin >
On 7.3.2024 19.28, Klara Modin wrote: > On 2024-03-07 17:04, Klara Modin wrote: >> Hi, >> >> On 2024-02-23 15:03, Mathias Nyman wrote: >>> Ports with _UPC (USB Port Capability) ACPI objects stating they are >>> "not connectable" are not wired to any connector or internal device. >>> They only exist inside the host controller. >>> >>> These ports may not have an ACPI _PLD (Physical Location of Device) >>> object. >>> >>> Rework the code so that _UPC is read even if _PLD does not exist, and >>> make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead >>> of "USB_PORT_CONNECT_TYPE_UNKNOWN". >>> >>> No bugs or known issues are reported due to possibly not parsing _UPC, >>> and thus leaving the port connect type as "unknown" instead of >>> "not used". Nice to have this fixed but no need to add it to stable >>> kernels, or urgency to get it upstream. >> >> With this patch (f3ac348e6e04501479fecf55250b25ff2092540b in next-20240307), my machine fails to boot. I was able to get some kernel console output from the pstore when compiling USB support as a module instead of built in. Reverting it on top of next-20240307 resolves the issue for me. > > Correction, it does boot when building USB support as a module. Got more logs from the unhealthy system and lspci. lsusb shows a single USB 2.0 root hub and nothing else. Calling it with `-v` hangs and produces nothing useful, the same happens with lshw. > >> >> Please tell me if there's anything else you need. Thanks for reporting this. Looks like *pld might never get set in some error cases, and we end up freeing some random pointer. i.e if status = acpi_get_physical_device_location(handle, &pld) fails Could you try if this helps: diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c index f250dfc3b14d..7f8a912d4fe2 100644 --- a/drivers/usb/core/usb-acpi.c +++ b/drivers/usb/core/usb-acpi.c @@ -154,7 +154,7 @@ usb_acpi_get_connect_type(struct usb_port *port_dev, acpi_handle *handle) enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN; struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *upc = NULL; - struct acpi_pld_info *pld; + struct acpi_pld_info *pld = NULL; acpi_status status; /* Thanks Mathias
On 2024-03-08 10:37, Mathias Nyman wrote: > On 7.3.2024 19.28, Klara Modin wrote: >> On 2024-03-07 17:04, Klara Modin wrote: >>> Hi, >>> >>> On 2024-02-23 15:03, Mathias Nyman wrote: >>>> Ports with _UPC (USB Port Capability) ACPI objects stating they are >>>> "not connectable" are not wired to any connector or internal device. >>>> They only exist inside the host controller. >>>> >>>> These ports may not have an ACPI _PLD (Physical Location of Device) >>>> object. >>>> >>>> Rework the code so that _UPC is read even if _PLD does not exist, and >>>> make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead >>>> of "USB_PORT_CONNECT_TYPE_UNKNOWN". >>>> >>>> No bugs or known issues are reported due to possibly not parsing _UPC, >>>> and thus leaving the port connect type as "unknown" instead of >>>> "not used". Nice to have this fixed but no need to add it to stable >>>> kernels, or urgency to get it upstream. >>> >>> With this patch (f3ac348e6e04501479fecf55250b25ff2092540b in >>> next-20240307), my machine fails to boot. I was able to get some >>> kernel console output from the pstore when compiling USB support as a >>> module instead of built in. Reverting it on top of next-20240307 >>> resolves the issue for me. >> >> Correction, it does boot when building USB support as a module. Got >> more logs from the unhealthy system and lspci. lsusb shows a single >> USB 2.0 root hub and nothing else. Calling it with `-v` hangs and >> produces nothing useful, the same happens with lshw. >> >>> >>> Please tell me if there's anything else you need. > > Thanks for reporting this. > > Looks like *pld might never get set in some error cases, and we end up > freeing some random pointer. > i.e if status = acpi_get_physical_device_location(handle, &pld) fails > > Could you try if this helps: > > diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c > index f250dfc3b14d..7f8a912d4fe2 100644 > --- a/drivers/usb/core/usb-acpi.c > +++ b/drivers/usb/core/usb-acpi.c > @@ -154,7 +154,7 @@ usb_acpi_get_connect_type(struct usb_port *port_dev, > acpi_handle *handle) > enum usb_port_connect_type connect_type = > USB_PORT_CONNECT_TYPE_UNKNOWN; > struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > union acpi_object *upc = NULL; > - struct acpi_pld_info *pld; > + struct acpi_pld_info *pld = NULL; > acpi_status status; > > /* > > Thanks > Mathias That indeed looks to be the case, this fixes the issue for me. Thanks, Tested-by: Klara Modin <klarasmodin@gmail.com>
>> Looks like *pld might never get set in some error cases, and we end up >> freeing some random pointer. >> i.e if status = acpi_get_physical_device_location(handle, &pld) fails >> >> Could you try if this helps: >> >> diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c >> index f250dfc3b14d..7f8a912d4fe2 100644 >> --- a/drivers/usb/core/usb-acpi.c >> +++ b/drivers/usb/core/usb-acpi.c >> @@ -154,7 +154,7 @@ usb_acpi_get_connect_type(struct usb_port *port_dev, acpi_handle *handle) >> enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN; >> struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> union acpi_object *upc = NULL; >> - struct acpi_pld_info *pld; >> + struct acpi_pld_info *pld = NULL; >> acpi_status status; >> >> /* >> >> Thanks >> Mathias > > That indeed looks to be the case, this fixes the issue for me. > > Thanks, > Tested-by: Klara Modin <klarasmodin@gmail.com> Thanks, I'll send a proper patch -Mathias
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c index a34b22537d7c..f250dfc3b14d 100644 --- a/drivers/usb/core/usb-acpi.c +++ b/drivers/usb/core/usb-acpi.c @@ -142,12 +142,19 @@ int usb_acpi_set_power_state(struct usb_device *hdev, int index, bool enable) } EXPORT_SYMBOL_GPL(usb_acpi_set_power_state); -static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, - struct acpi_pld_info *pld) +/* + * Private to usb-acpi, all the core needs to know is that + * port_dev->location is non-zero when it has been set by the firmware. + */ +#define USB_ACPI_LOCATION_VALID (1 << 31) + +static void +usb_acpi_get_connect_type(struct usb_port *port_dev, acpi_handle *handle) { enum usb_port_connect_type connect_type = USB_PORT_CONNECT_TYPE_UNKNOWN; struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *upc = NULL; + struct acpi_pld_info *pld; acpi_status status; /* @@ -158,6 +165,12 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, * a usb device is directly hard-wired to the port. If no visible and * no connectable, the port would be not used. */ + + status = acpi_get_physical_device_location(handle, &pld); + if (ACPI_SUCCESS(status) && pld) + port_dev->location = USB_ACPI_LOCATION_VALID | + pld->group_token << 8 | pld->group_position; + status = acpi_evaluate_object(handle, "_UPC", NULL, &buffer); if (ACPI_FAILURE(status)) goto out; @@ -166,25 +179,22 @@ static enum usb_port_connect_type usb_acpi_get_connect_type(acpi_handle handle, if (!upc || (upc->type != ACPI_TYPE_PACKAGE) || upc->package.count != 4) goto out; + /* UPC states port is connectable */ if (upc->package.elements[0].integer.value) - if (pld->user_visible) + if (!pld) + ; /* keep connect_type as unknown */ + else if (pld->user_visible) connect_type = USB_PORT_CONNECT_TYPE_HOT_PLUG; else connect_type = USB_PORT_CONNECT_TYPE_HARD_WIRED; - else if (!pld->user_visible) + else connect_type = USB_PORT_NOT_USED; out: + port_dev->connect_type = connect_type; kfree(upc); - return connect_type; + ACPI_FREE(pld); } - -/* - * Private to usb-acpi, all the core needs to know is that - * port_dev->location is non-zero when it has been set by the firmware. - */ -#define USB_ACPI_LOCATION_VALID (1 << 31) - static struct acpi_device * usb_acpi_get_companion_for_port(struct usb_port *port_dev) { @@ -222,22 +232,12 @@ static struct acpi_device * usb_acpi_find_companion_for_port(struct usb_port *port_dev) { struct acpi_device *adev; - struct acpi_pld_info *pld; - acpi_handle *handle; - acpi_status status; adev = usb_acpi_get_companion_for_port(port_dev); if (!adev) return NULL; - handle = adev->handle; - status = acpi_get_physical_device_location(handle, &pld); - if (ACPI_SUCCESS(status) && pld) { - port_dev->location = USB_ACPI_LOCATION_VALID - | pld->group_token << 8 | pld->group_position; - port_dev->connect_type = usb_acpi_get_connect_type(handle, pld); - ACPI_FREE(pld); - } + usb_acpi_get_connect_type(port_dev, adev->handle); return adev; }
Ports with _UPC (USB Port Capability) ACPI objects stating they are "not connectable" are not wired to any connector or internal device. They only exist inside the host controller. These ports may not have an ACPI _PLD (Physical Location of Device) object. Rework the code so that _UPC is read even if _PLD does not exist, and make sure the port->connect_type is set to "USB_PORT_NOT_USED" instead of "USB_PORT_CONNECT_TYPE_UNKNOWN". No bugs or known issues are reported due to possibly not parsing _UPC, and thus leaving the port connect type as "unknown" instead of "not used". Nice to have this fixed but no need to add it to stable kernels, or urgency to get it upstream. Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com> --- v1 -> v2 - Commit message improvements - send patch separately for easier picking to usb-next drivers/usb/core/usb-acpi.c | 46 ++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 23 deletions(-)