Linux USB
 help / color / mirror / Atom feed
* Re: [RFC PATCH 1/2] xhci: prevent automatic endpoint restart after stall or error
From: Mathias Nyman @ 2026-03-30 12:51 UTC (permalink / raw)
  To: Thinh Nguyen
  Cc: linux-usb@vger.kernel.org, stern@rowland.harvard.edu,
	michal.pecio@gmail.com, oneukum@suse.com,
	niklas.neronin@linux.intel.com
In-Reply-To: <20260326232400.zkplsxflhykhayyb@synopsys.com>

On 3/27/26 01:24, Thinh Nguyen wrote:
> On Thu, Mar 26, 2026, Mathias Nyman wrote:
>> On 3/26/26 03:19, Thinh Nguyen wrote:
>>> On Wed, Mar 25, 2026, Mathias Nyman wrote:
>>>> On 3/25/26 03:52, Thinh Nguyen wrote:
>>>>> On Mon, Mar 23, 2026, Mathias Nyman wrote:
>>>>>> Avoid automatically restarting bulk or interrupt transfers after a
>>>>>> URB is given back due to stall or error.
>>>>>>
>>>>>> Introduce a 'TD_TAINTED' state for pending TDs queued on a endpoint when
>>>>>> it halted. The actual TD the endpoint halted on is marked TD_HALTED,
>>>>>> and its URB is given back with proper EPROTO or EPIPE error code.
>>>>>>
>>>>>> Don't automatically restart an endpoint if the next queued TD after
>>>>>> the TD_HALTED one is marked tainted.
>>>>>
>>>>> Sounds good for -EPROTO, but will a clear-halt ring the corresponding
>>>>> the endpoint's doorbell for STALL endpoint?
>>>>>
>>>>
>>>> With this change xhci would not restart the stalled endpoint after a clear-halt
>>>> request. The first usb_enqueue() call after clear-halt would start it.
>>>>
>>>> Could make sense to restart the endpoint after a clear-halt, and just add a small
>>>> debug message if the next queued URB is marked 'tainted'.
>>>>
>>>
>>> The -EPROTO should be handled differently than -EPIPE. A STALL endpoint
>>> is part of a normal usb flow. Should the class driver submit a new URB
>>> while the endpoint is STALL, we would always expect a STALL error
>>> response after the endpoint is restarted. That's not the case with
>>> -EPROTO where the data may be corrupted and/or the host and device are
>>> out of sync. We should not continue until the class driver do some
>>> recovery. IMHO, we can keep the handling of -EPIPE how it was before.
>>> Let the xHC tell whether the STALL error still persists instead of
>>> managing it by the xhci driver.
>>>
>> I agree that that we should restart the endpoint if class/core enqueues a new
>> URB _after_ xhci gave back an URB with EPIPE after endpoint STALL.
>>
>> But I don't think we should restart the ring to continue processing URBs that
>> were queued before the endpoint stalled. This would prevent the class/core
>> from even attempting to retire the pending URBs, something USB2.0 spec,
>> '5.8.5 Bulk Transfer Data Sequences' requires:
>>
>> "If a halt condition is detected on a bulk pipe due to transmission errors or
>>   a STALL handshake being returned from the endpoint, all pending IRPs are
>>   retired.  Removal of the halt condition is achieved via software intervention
>>   through a separate control pipe."
>>
> 
> Fair point. Then the core will need to track the endpoint's STALL state
> and parse the clear-halt request to know which endpoint and when to
> clear the STALL before it can accept new URB. So the first usb_enqueue()
> call after clear-halt can start the endpoint again. The xhci will also
> need to have access to this state.

Ideally xhci driver would return the URB with EPIPE after STALL, and not continue
processing URBs before a clear halt is sent, or a new URB is enqueued.
USB core would hold off submitting URBs to xhci, buffering URBs enqueued for this
STALLED endpoint until class driver urb->complete() finishes for that EPIPE URB.

Usb core could flag this endpoint as "halt_pending" before adding the EPIPE URB to
the bh workqueue. Then after urb->complete() work is called and core is sure class
driver  is aware of the EPIPE, then core would clear the flag and flush the buffered
URBs to the host controller drivers, restarting the ring

Class driver urb->complete() would most likely retire/cancel the pending URBs, both the
earlier queued 'tainted' URBs, and the most recent 'buffered' URBs in usb core.
Class driver should clear the halt, but is free to do whatever it wants.
It could choose to send a new URB without clearing the halt,
have it processed, trigger STALL again, and get URB returned with EPIPE status.

I don't think most class/usb device drivers really handle stall that well.
Above might be wishful thinking.

> 
> Currently you have the xhci driver to "retire" the halted URBs. However,
> you also noted that class/core may attempt to retire the pending URBs.
> Who's expected to handle the retirement here?

Maybe we should let core retire the pending URBS, and only fix the xhci driver
'automatic endpoint restart after stall' part after that core change is done.

Should cause less regression.

> 
> On a separate note, will you plan to implement the clear-halt for EPROTO
> in xhci?

I don't think this should be part of xhci driver. Decision to send control requests
to the device should be done by core or class drivers.

Thanks
Mathias



^ permalink raw reply

* Re: correctly handling EPROTO
From: Michal Pecio @ 2026-03-30 12:36 UTC (permalink / raw)
  To: Alan Stern; +Cc: Thinh Nguyen, Oliver Neukum, Bjørn Mork, USB list
In-Reply-To: <ba68bdbb-9ffd-47d3-8915-9e2e40fd6adb@rowland.harvard.edu>

On Sun, 29 Mar 2026 21:32:26 -0400, Alan Stern wrote:
> A lot of video devices use the uvcvideo driver, which (as far as I
> know) relies entirely on isochronous transfers for its streaming data.

UVC allows both isoc and bulk transport. I have one bulk device and I
found that if I randomly change urb->status to -EPROTO, the URB is not
resubmitted but (on xHCI) the endpoint keeps going, until it stops after
5 errors (no URBs left). EHCI would presumably never restart at all.

I suppose changing this might be seen as regression by some, because
the effect of each EPROTO is just a few lost frames, at least in my
very brief testing. I suppose uvcvideo could be patched to clear halt
and resubmit, if this trivial "lossy recovery" is desired.

I also got a chance to see SuperSpeed sequence mismatch thanks to other
bug (xhci_endpoint_reset() called by usb_clear_halt() may do nothing).
The effect is EPROTO, consistent with my prediction based on USB3 spec,
except that on one AMD chipset the URB never completes. Weird.

If mismatch is caused by xhci-hcd clearing only the host side (rather
than the device being cleared as in the bug above) then it seems that
resubmitting without clear halt will never fix a SuperSpeed endpoint.

Regards,
Michal

^ permalink raw reply

* Re: UAS peripheral with unrecognized TRIM support
From: Oliver Neukum @ 2026-03-30 12:17 UTC (permalink / raw)
  To: Johan Gill, linux-usb
In-Reply-To: <1717beab-848b-4927-8fa5-26fb5ae05495@bahnhof.se>



On 28.03.26 17:23, Johan Gill wrote:

> It would be nice to have the adapter recognized as more capable out of the box. Should I aim at patching drivers/usb/storage/unusual_uas.h?

No,

because

1) we cannot be sure why the VPD is wrong. Worst case is that it works
on some devices, but not others and the vendor is conservative
2) this is a SCSI issue, not a USB issue. Storage and UAS devices
can be real converters to a SCSI bus or device enclosures for
arbitrary disks, whose capabilities depend on the concrete disk used
in them

Hence this issue cannot be solved at the level of UAS. You need
to get a quirk on the SCSI level and discuss this with the SCSI
list.

	HTH
		Oliver



^ permalink raw reply

* Re: [PATCH v3 1/1] usb: dwc3: Add optional VBUS regulator support to SpacemiT K1
From: Anand Moon @ 2026-03-30 12:15 UTC (permalink / raw)
  To: Chukun Pan
  Cc: Thinh Nguyen, Yixun Lan, Ze Huang, Greg Kroah-Hartman,
	linux-riscv, linux-kernel, linux-usb, spacemit
In-Reply-To: <20260326100010.3588454-2-amadeus@jmu.edu.cn>

hi Chukun,

On Thu, 26 Mar 2026 at 15:32, Chukun Pan <amadeus@jmu.edu.cn> wrote:
>
> Some SpacemiT K1 boards (like OrangePi R2S) provide USB VBUS
> through a controllable regulator. Add support for the optional
> vbus-supply property so the regulator can be properly managed
> in host mode instead of left always-on. Note that this doesn't
> apply to USB Hub downstream ports with different VBUS supplies.
>
> The enabled and disabled actions of the regulator are handled
> automatically by devm_regulator_get_enable_optional().
>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
Reviewed-by: Anand Moon <linux.amoon@gmail.com>

Thanks
-Anand

^ permalink raw reply

* Re: [PATCH 8/9] usb: xhci: improve debug messages during suspend
From: Neronin, Niklas @ 2026-03-30 11:44 UTC (permalink / raw)
  To: Michal Pecio; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260330111421.65c2eb06.michal.pecio@gmail.com>



On 30/03/2026 12.14, Michal Pecio wrote:
>>
>> Currently, the SRE error is only observed and warning is printed.
>> This patch does not introduce deeper error handling, as the correct
>> response is unclear and changes to suspend behavior may risk
>> regressions once the resume path is updated.
> 
> I think patch 10/9 should add setting xhci->broken_suspend if this is
> detected. It's ridiculous to try State Restore after State Save error.
> At best, it should fail. At worst, it might not fail...

I plan to follow‑up with a cleanup/optimization patch after this patch set.
(Forgot to mention this in the cover letter)

The primary goal of this initial patch set is narrowly scoped:
to change resume handling from a full teardown and reinitialization,
to a minimal reset‑and‑restore flow. This has a higher risk of triggering
issues on some hardware.
Because of that, I deliberately avoided refactoring or optimizing
unrelated xhci code in this first step. Keeping changes minimal to helps
isolate resume issues, if they arise :)

Once this new resume path has had broader testing and any regressions
are resolved, I intend to follow up with cleanup and optimization patches,
where the surrounding code can be refactored with less risk of obscuring
resume memory state issues.

Thanks,
Niklas

^ permalink raw reply

* Re: [PATCH v6 2/4] usb: misc: onboard_usb_hub: Add Genesys Logic GL3590 hub support
From: Dmitry Baryshkov @ 2026-03-30 11:43 UTC (permalink / raw)
  To: Swati Agarwal
  Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
	Conor Dooley, Icenowy Zheng, Matthias Kaehlcke, Mike Looijmans,
	Pin-yen Lin, Chaoyi Chen, J . Neuschäfer, Jens Glathe,
	Bjorn Andersson, Konrad Dybcio, linux-usb, devicetree,
	linux-kernel, linux-arm-msm
In-Reply-To: <20260318040644.3591478-3-swati.agarwal@oss.qualcomm.com>

On Wed, Mar 18, 2026 at 09:36:42AM +0530, Swati Agarwal wrote:
> Add support for the GL3590 4 ports USB3.2 hub.
> 
> Signed-off-by: Swati Agarwal <swati.agarwal@oss.qualcomm.com>
> ---
>  drivers/usb/misc/onboard_usb_dev.c | 1 +
>  drivers/usb/misc/onboard_usb_dev.h | 8 ++++++++
>  2 files changed, 9 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

^ permalink raw reply

* Re: [PATCH] hid: usbhid: fix deadlock in hid_post_reset()
From: Oliver Neukum @ 2026-03-30 11:43 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: bentiss, linux-input, linux-usb
In-Reply-To: <8q66o2o4-7844-6p76-9964-7pr205p190pr@xreary.bet>

On 27.03.26 11:34, Jiri Kosina wrote:
  
> Did you find this just by code inspection, or was this reported with a
> real device?

Pure inspection. We are looking at USB error handling
in general right now.

	Regards
		Oliver


^ permalink raw reply

* Re: [PATCH net v3 04/11] list: Move on_list_rcu() to list.h and add on_list() also
From: David Howells @ 2026-03-30 10:49 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: dhowells, Jakub Kicinski, netdev, Marc Dionne, David S. Miller,
	Eric Dumazet, Paolo Abeni, linux-afs, linux-kernel,
	Mathieu Desnoyers, John Johansen, Minas Harutyunyan, Simon Horman,
	apparmor, linux-usb, stable
In-Reply-To: <CAHk-=wiJ6gEELLviexdmSHnyjVoG7MFo8Qwhd1zxs_tCnL-=gQ@mail.gmail.com>

Linus Torvalds <torvalds@linux-foundation.org> wrote:

> ... and I do *not* see a huge advantage to a helper function that just wraps
> "list_empty()" with another name that is actually *less* descriptive.

I don't like list_empty() as the name of the function used to find out whether
an entry is on a list.  Yes, technically, all it's doing is seeing if the
list_head is 'empty', but, linguistically, it looks wrong: the question you're
asking is not if the list is empty (you're not looking at the list head), but
if the entry is on a list.

So if I see in the code:

	if (list_empty(p))

what is the test actually asking?

Note that various other list types in the kernel have separate "is the list
empty" and "is the entry on a list" primitives, though, granted, usually
because they require separate functions programmatically.

Anyway, I'll find a different way to do this, not involving checking the prev
pointer.  What I don't want to do is hard code "prev == LIST_POISON2" into my
stuff.  Anything like that really needs to be in list.h.

David


^ permalink raw reply

* [PATCH 2/2] net: ipeth: refactor endpoint lookup
From: Johan Hovold @ 2026-03-30 10:26 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-usb, netdev, linux-kernel, Johan Hovold
In-Reply-To: <20260330102611.1671546-1-johan@kernel.org>

Use the common USB helper for looking up bulk and interrupt endpoints
instead of open coding.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/usb/ipheth.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index a19789b57190..bb1364f85bd1 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -573,11 +573,10 @@ static int ipheth_probe(struct usb_interface *intf,
 			const struct usb_device_id *id)
 {
 	struct usb_device *udev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
 	struct usb_host_interface *hintf;
-	struct usb_endpoint_descriptor *endp;
 	struct ipheth_device *dev;
 	struct net_device *netdev;
-	int i;
 	int retval;
 
 	netdev = alloc_etherdev(sizeof(struct ipheth_device));
@@ -603,19 +602,16 @@ static int ipheth_probe(struct usb_interface *intf,
 		goto err_endpoints;
 	}
 
-	for (i = 0; i < hintf->desc.bNumEndpoints; i++) {
-		endp = &hintf->endpoint[i].desc;
-		if (usb_endpoint_is_bulk_in(endp))
-			dev->bulk_in = endp->bEndpointAddress;
-		else if (usb_endpoint_is_bulk_out(endp))
-			dev->bulk_out = endp->bEndpointAddress;
-	}
-	if (!(dev->bulk_in && dev->bulk_out)) {
-		retval = -ENODEV;
+	retval = usb_find_common_endpoints_reverse(hintf, &ep_in, &ep_out,
+						   NULL, NULL);
+	if (retval) {
 		dev_err(&intf->dev, "Unable to find endpoints\n");
 		goto err_endpoints;
 	}
 
+	dev->bulk_in = ep_in->bEndpointAddress;
+	dev->bulk_out = ep_out->bEndpointAddress;
+
 	dev->ctrl_buf = kmalloc(IPHETH_CTRL_BUF_SIZE, GFP_KERNEL);
 	if (dev->ctrl_buf == NULL) {
 		retval = -ENOMEM;
-- 
2.52.0


^ permalink raw reply related

* [PATCH 0/2] net: refactor USB endpoint lookups
From: Johan Hovold @ 2026-03-30 10:26 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-usb, netdev, linux-kernel, Johan Hovold

Use the common USB helpers for looking up bulk and interrupt endpoints
instead of open coding.

Johan


Johan Hovold (2):
  net: hso: refactor endpoint lookup
  net: ipeth: refactor endpoint lookup

 drivers/net/usb/hso.c    | 65 +++++++++++-----------------------------
 drivers/net/usb/ipheth.c | 18 +++++------
 2 files changed, 24 insertions(+), 59 deletions(-)

-- 
2.52.0


^ permalink raw reply

* [PATCH 1/2] net: hso: refactor endpoint lookup
From: Johan Hovold @ 2026-03-30 10:26 UTC (permalink / raw)
  To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni
  Cc: linux-usb, netdev, linux-kernel, Johan Hovold
In-Reply-To: <20260330102611.1671546-1-johan@kernel.org>

Use the common USB helpers for looking up bulk and interrupt endpoints
instead of a custom implementation.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/usb/hso.c | 65 +++++++++++--------------------------------
 1 file changed, 17 insertions(+), 48 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 1825f7cf5dc0..c1aec67688ae 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -298,8 +298,6 @@ static int hso_mux_submit_intr_urb(struct hso_shared_int *mux_int,
 				   struct usb_device *usb, gfp_t gfp);
 static void handle_usb_error(int status, const char *function,
 			     struct hso_device *hso_dev);
-static struct usb_endpoint_descriptor *hso_get_ep(struct usb_interface *intf,
-						  int type, int dir);
 static int hso_get_mux_ports(struct usb_interface *intf, unsigned char *ports);
 static void hso_free_interface(struct usb_interface *intf);
 static int hso_start_serial_device(struct hso_device *hso_dev, gfp_t flags);
@@ -2497,16 +2495,11 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	hso_net->net = net;
 	hso_net->parent = hso_dev;
 
-	hso_net->in_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
-				      USB_DIR_IN);
-	if (!hso_net->in_endp) {
-		dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
-		goto err_net;
-	}
-	hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
-				       USB_DIR_OUT);
-	if (!hso_net->out_endp) {
-		dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
+	result = usb_find_common_endpoints(interface->cur_altsetting,
+					   &hso_net->in_endp, &hso_net->out_endp,
+					   NULL, NULL);
+	if (result) {
+		dev_err(&interface->dev, "Can't find BULK endpoints\n");
 		goto err_net;
 	}
 	SET_NETDEV_DEV(net, &interface->dev);
@@ -2608,10 +2601,12 @@ static void hso_free_serial_device(struct hso_device *hso_dev)
 static struct hso_device *hso_create_bulk_serial_device(
 			struct usb_interface *interface, int port)
 {
+	struct usb_host_interface *iface_desc = interface->cur_altsetting;
 	struct hso_device *hso_dev;
 	struct hso_serial *serial;
 	int num_urbs;
 	struct hso_tiocmget *tiocmget;
+	int ret;
 
 	hso_dev = hso_create_device(interface, port);
 	if (!hso_dev)
@@ -2634,10 +2629,8 @@ static struct hso_device *hso_create_bulk_serial_device(
 		if (!serial->tiocmget->serial_state_notification)
 			goto exit;
 		tiocmget = serial->tiocmget;
-		tiocmget->endp = hso_get_ep(interface,
-					    USB_ENDPOINT_XFER_INT,
-					    USB_DIR_IN);
-		if (!tiocmget->endp) {
+		ret = usb_find_int_in_endpoint(iface_desc, &tiocmget->endp);
+		if (ret) {
 			dev_err(&interface->dev, "Failed to find INT IN ep\n");
 			goto exit;
 		}
@@ -2656,17 +2649,10 @@ static struct hso_device *hso_create_bulk_serial_device(
 				     BULK_URB_TX_SIZE))
 		goto exit;
 
-	serial->in_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
-				     USB_DIR_IN);
-	if (!serial->in_endp) {
-		dev_err(&interface->dev, "Failed to find BULK IN ep\n");
-		goto exit2;
-	}
-
-	if (!
-	    (serial->out_endp =
-	     hso_get_ep(interface, USB_ENDPOINT_XFER_BULK, USB_DIR_OUT))) {
-		dev_err(&interface->dev, "Failed to find BULK OUT ep\n");
+	ret = usb_find_common_endpoints(iface_desc, &serial->in_endp,
+					&serial->out_endp, NULL, NULL);
+	if (ret) {
+		dev_err(&interface->dev, "Failed to find BULK eps\n");
 		goto exit2;
 	}
 
@@ -2754,13 +2740,14 @@ static
 struct hso_shared_int *hso_create_shared_int(struct usb_interface *interface)
 {
 	struct hso_shared_int *mux = kzalloc_obj(*mux);
+	int ret;
 
 	if (!mux)
 		return NULL;
 
-	mux->intr_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_INT,
-				    USB_DIR_IN);
-	if (!mux->intr_endp) {
+	ret = usb_find_int_in_endpoint(interface->cur_altsetting,
+				       &mux->intr_endp);
+	if (ret) {
 		dev_err(&interface->dev, "Can't find INT IN endpoint\n");
 		goto exit;
 	}
@@ -3134,24 +3121,6 @@ static void hso_free_interface(struct usb_interface *interface)
 
 /* Helper functions */
 
-/* Get the endpoint ! */
-static struct usb_endpoint_descriptor *hso_get_ep(struct usb_interface *intf,
-						  int type, int dir)
-{
-	int i;
-	struct usb_host_interface *iface = intf->cur_altsetting;
-	struct usb_endpoint_descriptor *endp;
-
-	for (i = 0; i < iface->desc.bNumEndpoints; i++) {
-		endp = &iface->endpoint[i].desc;
-		if (((endp->bEndpointAddress & USB_ENDPOINT_DIR_MASK) == dir) &&
-		    (usb_endpoint_type(endp) == type))
-			return endp;
-	}
-
-	return NULL;
-}
-
 /* Get the byte that describes which ports are enabled */
 static int hso_get_mux_ports(struct usb_interface *intf, unsigned char *ports)
 {
-- 
2.52.0


^ permalink raw reply related

* Re: [PATCH] USB: cdc-acm: Add support for second ACM channel on more Nokia phones
From: Oliver Neukum @ 2026-03-30 10:04 UTC (permalink / raw)
  To: Рыжов Фёдор,
	linux-kernel@vger.kernel.org
  Cc: gregkh@linuxfoundation.org, linux-usb@vger.kernel.org
In-Reply-To: <a8008c91-52c1-4a5c-ac96-dd45f109c68c@outlook.com>

Hi Fyodor,

On 26.03.26 15:37, Рыжов Фёдор wrote:

> This patch extends the whitelist with all S60v3.x and S60v5.x phones,
> supported by official "Nokia PC Suite" driver for Windows. Device IDs
> are taken from its installation configuration file. Older S60v2.x and
> S60v1.x devices were not added because I have no hardware to test.

Thank you for doing this and testing the patch. One nit.
This has two IDs for the same model. This looks like a typo, even
if it is real. Could you add a comment to such entries?

	Regards
		Oliver


^ permalink raw reply

* [PATCH] HID: usbhid: refactor endpoint lookup
From: Johan Hovold @ 2026-03-30  9:50 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires
  Cc: linux-usb, linux-input, linux-kernel, Johan Hovold

Use the common USB helper for looking up interrupt-in endpoints instead
of open coding.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/hid/usbhid/hid-core.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index ddd5d77fb5a5..2447bc7cb7a1 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1363,19 +1363,17 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 {
 	struct usb_host_interface *interface = intf->cur_altsetting;
 	struct usb_device *dev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *ep;
 	struct usbhid_device *usbhid;
 	struct hid_device *hid;
-	unsigned int n, has_in = 0;
 	size_t len;
 	int ret;
 
 	dbg_hid("HID probe called for ifnum %d\n",
 			intf->altsetting->desc.bInterfaceNumber);
 
-	for (n = 0; n < interface->desc.bNumEndpoints; n++)
-		if (usb_endpoint_is_int_in(&interface->endpoint[n].desc))
-			has_in++;
-	if (!has_in) {
+	ret = usb_find_int_in_endpoint(interface, &ep);
+	if (ret) {
 		hid_err(intf, "couldn't find an input interrupt endpoint\n");
 		return -ENODEV;
 	}
-- 
2.52.0


^ permalink raw reply related

* Re: [PATCH 9/9] usb: xhci: optimize resuming from S4 (suspend-to-disk)
From: Michal Pecio @ 2026-03-30  9:45 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-10-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:40 +0100, Niklas Neronin wrote:
> On resume from S4 (power loss after suspend/hibernation), the xHCI
> driver previously freed, reallocated, and fully reinitialized all
> data structures. Most of this is unnecessary because the data is
> restored from a saved image; only the xHCI registers lose their
> values.

... and all runtime information must be wiped from data structures.
That's the tricky part here.

> 
> This patch optimizes S4 resume by performing only a host controller
> reset, which includes:
> * Freeing or clearing runtime-created data.
> * Rewriting xHCI registers.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c |  4 +--
>  drivers/usb/host/xhci.c     | 53 ++++++++++++++++++++++---------------
>  drivers/usb/host/xhci.h     |  2 ++
>  3 files changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index f1b4f06d4b8b..4156822eb000 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -936,7 +936,7 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   * that tt_info, then free the child first. Recursive.
>   * We can't rely on udev at this point to find child-parent relationships.
>   */
> -static void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
>  {
>  	struct xhci_virt_device *vdev;
>  	struct list_head *tt_list_head;
> @@ -1905,7 +1905,7 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
>  EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>  
>  /* Cleanup roothub bandwidth data */
> -static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
> +void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
>  {
>  	struct xhci_root_port_bw_info *rh_bw;
>  	struct xhci_tt_bw_info *tt_info, *tt_next;
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 232e6143ac4b..8fb2b91fc0cc 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -1082,9 +1082,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  {
>  	u32			command, temp = 0;
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
> +	struct xhci_segment	*seg;
>  	int			retval = 0;
>  	bool			pending_portevent = false;
>  	bool			suspended_usb3_devs = false;
> +	bool			reset_registers = false;
>  
>  	if (!hcd->state)
>  		return 0;
> @@ -1103,10 +1105,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  
>  	spin_lock_irq(&xhci->lock);
>  
> -	if (xhci->quirks & XHCI_RESET_ON_RESUME || xhci->broken_suspend)
> -		power_lost = true;
> -
> -	if (!power_lost) {
> +	if (power_lost || xhci->broken_suspend || xhci->quirks & XHCI_RESET_ON_RESUME) {
> +		xhci_dbg(xhci, "HC state lost, performing host controller reset\n");
> +		reset_registers = true;
> +	} else {
> +		xhci_dbg(xhci, "HC state intact, continuing without reset\n");

Maybe not a bad idea to log this function, but it could simply print
the value of reset_registers instead of two separate xhci_dbg().
And maybe also print power_lost and broken_suspend separately.

>  		/*
>  		 * Some controllers might lose power during suspend, so wait
>  		 * for controller not ready bit to clear, just as in xHC init.
> @@ -1144,11 +1147,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		temp = readl(&xhci->op_regs->status);
>  		if ((temp & (STS_SRE | STS_HCE)) && !(xhci->xhc_state & XHCI_STATE_REMOVING)) {
>  			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> -			power_lost = true;
> +			reset_registers = true;
>  		}
>  	}
>  
> -	if (power_lost) {
> +	if (reset_registers) {
>  		if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) &&
>  				!(xhci_all_ports_seen_u0(xhci))) {
>  			timer_delete_sync(&xhci->comp_mode_recovery_timer);
> @@ -1172,27 +1175,33 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		if (retval)
>  			return retval;

It's suspicious that xhci_halt() is called earlier without checking
retval. If it fails then xhci_reset() returns success and this check
passes and it seems that nothing good will happen.

No sure if the HC can be running here?

>  
> -		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
> -		temp = readl(&xhci->op_regs->status);
> -		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
> -		xhci_disable_interrupter(xhci, xhci->interrupters[0]);
> +		cancel_delayed_work_sync(&xhci->cmd_timer);
> +
> +		/* Delete all remaining commands */
> +		xhci_cleanup_command_queue(xhci);

Considering that xhci_suspend() clears the command ring anyway, it
could probably do this too so we don't need to. 

BTW, debugfs/port_bandwidth interface queues commands and I'm not sure
if it's synchronized in any way with power management. IOW, it might be 
possible that command are pending at suspend, but I'm not sure.

> +
> +		/* Clear data which is re-initilized during runtime */
> +		xhci_for_each_ring_seg(xhci->interrupters[0]->event_ring->first_seg, seg)
> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);
> +
> +		for (int i = xhci->max_slots; i > 0; i--)
> +			xhci_free_virt_devices_depth_first(xhci, i);

This loop is a bit ugly, it could be a function in xhci-mem.

> +
> +		xhci_rh_bw_cleanup(xhci);
> +
> +		xhci->cmd_ring_reserved_trbs = 0;
> +		xhci_for_each_ring_seg(xhci->cmd_ring->first_seg, seg)
> +			memset(seg->trbs, 0, sizeof(union xhci_trb) * TRBS_PER_SEGMENT);

This looks like a bug because it nukes link TRBs. I know that
xhci_init() will fix this up (unless somebody changes that without
updating here), but it looks confusing.

I think calling clear_command_ring() was clearer and less bug prone.

>  
> -		xhci_dbg(xhci, "cleaning up memory\n");
> -		xhci_mem_cleanup(xhci);
>  		xhci_debugfs_exit(xhci);

Interesting, it looks like there used to be a window of time when all
memory was freed but debugfs files still existed? Odd...

> -		xhci_dbg(xhci, "xhci_stop completed - status = %x\n",
> -			    readl(&xhci->op_regs->status));
> -
> -		/* USB core calls the PCI reinit and start functions twice:
> -		 * first with the primary HCD, and then with the secondary HCD.
> -		 * If we don't do the same, the host will never be started.
> -		 */
> -		retval = xhci_mem_init(xhci, GFP_KERNEL);
> -		if (retval)
> -			return retval;
>  
>  		xhci_init(hcd);
>  
> +		/*
> +		 * USB core calls the PCI reinit and start functions twice:
> +		 * first with the primary HCD, and then with the secondary HCD.
> +		 * If we don't do the same, the host will never be started.
> +		 */
>  		xhci_dbg(xhci, "Start the primary HCD\n");
>  		retval = xhci_run(hcd);
>  		if (!retval && xhci->shared_hcd) {
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index ade0198bf9ea..a76e183515b3 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1792,6 +1792,7 @@ void xhci_dbg_trace(struct xhci_hcd *xhci, void (*trace)(struct va_format *),
>  void xhci_mem_cleanup(struct xhci_hcd *xhci);
>  int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags);
>  void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev, int slot_id);
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id);
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id, struct usb_device *udev, gfp_t flags);
>  int xhci_setup_addressable_virt_dev(struct xhci_hcd *xhci, struct usb_device *udev);
>  void xhci_copy_ep0_dequeue_into_input_ctx(struct xhci_hcd *xhci,
> @@ -1803,6 +1804,7 @@ void xhci_update_tt_active_eps(struct xhci_hcd *xhci,
>  		struct xhci_virt_device *virt_dev,
>  		int old_active_eps);
>  void xhci_clear_endpoint_bw_info(struct xhci_bw_info *bw_info);
> +void xhci_rh_bw_cleanup(struct xhci_hcd *xhci);
>  void xhci_update_bw_info(struct xhci_hcd *xhci,
>  		struct xhci_container_ctx *in_ctx,
>  		struct xhci_input_control_ctx *ctrl_ctx,
> -- 
> 2.50.1
> 

^ permalink raw reply

* [PATCH] usb: phy: mxs: manually reset phy regs after a warm reset
From: Xu Yang @ 2026-03-30  9:31 UTC (permalink / raw)
  To: gregkh, Frank.Li, s.hauer, kernel, festevam
  Cc: linux-usb, imx, linux-arm-kernel, linux-kernel

The usb phy registers are not fully reset on warm reset under stress
conditions. We need to manually reset those (CTRL, PWD, DEBUG, PLL_SIC)
regs after a warm reset. This will reset DEBUG and PLL_SIC registers.
CTRL and PWD register are handled by "SFT" bit in stmp_reset_block().

ERR051269: USB PHY registers not fully resetting on warm reset under
           stress conditions

The following USB PHY registers must be written by SW to restore the reset
value after a warm reset:

Reg: ctrl Addr: 0x29910030 Data: 0xc000_0000
Reg: pwd Addr: 0x29910000 Data: 0x001e_1c00
Reg: debug0 Addr: 0x29910050 Data: 0x7f18_0000
Reg: pll_sic Addr: 0x299100a0 Data: 0x00d1_2000

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/phy/phy-mxs-usb.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
index 7069dd3f4d0d..dd42db8a0829 100644
--- a/drivers/usb/phy/phy-mxs-usb.c
+++ b/drivers/usb/phy/phy-mxs-usb.c
@@ -209,6 +209,9 @@ static const struct mxs_phy_data imx6ul_phy_data = {
 static const struct mxs_phy_data imx7ulp_phy_data = {
 };
 
+static const struct mxs_phy_data imx8ulp_phy_data = {
+};
+
 static const struct of_device_id mxs_phy_dt_ids[] = {
 	{ .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, },
 	{ .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, },
@@ -217,6 +220,7 @@ static const struct of_device_id mxs_phy_dt_ids[] = {
 	{ .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, },
 	{ .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, },
 	{ .compatible = "fsl,imx7ulp-usbphy", .data = &imx7ulp_phy_data, },
+	{ .compatible = "fsl,imx8ulp-usbphy", .data = &imx8ulp_phy_data, },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
@@ -248,6 +252,11 @@ static inline bool is_imx7ulp_phy(struct mxs_phy *mxs_phy)
 	return mxs_phy->data == &imx7ulp_phy_data;
 }
 
+static inline bool is_imx8ulp_phy(struct mxs_phy *mxs_phy)
+{
+	return mxs_phy->data == &imx8ulp_phy_data;
+}
+
 static inline bool is_imx6ul_phy(struct mxs_phy *mxs_phy)
 {
 	return mxs_phy->data == &imx6ul_phy_data;
@@ -305,12 +314,29 @@ static int mxs_phy_pll_enable(void __iomem *base, bool enable)
 	return ret;
 }
 
+/*
+ * The imx8ulp phy registers are not properly reset after a warm
+ * reset (ERR051269). Using the following steps to reset DEBUG and
+ * PLL_SIC regs. CTRL and PWD regs are reset by "SFT" bit in
+ * stmp_reset_block().
+ */
+static void mxs_phy_regs_reset(void __iomem *base)
+{
+	writel(0x7f180000, base + HW_USBPHY_DEBUG_SET);
+	writel(~0x7f180000, base + HW_USBPHY_DEBUG_CLR);
+	writel(0x00d12000, base + HW_USBPHY_PLL_SIC_SET);
+	writel(~0x00d12000, base + HW_USBPHY_PLL_SIC_CLR);
+}
+
 static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
 {
 	int ret;
 	void __iomem *base = mxs_phy->phy.io_priv;
 
-	if (is_imx7ulp_phy(mxs_phy)) {
+	if (is_imx8ulp_phy(mxs_phy))
+		mxs_phy_regs_reset(base);
+
+	if (is_imx7ulp_phy(mxs_phy) || is_imx8ulp_phy(mxs_phy)) {
 		ret = mxs_phy_pll_enable(base, true);
 		if (ret)
 			return ret;
@@ -368,7 +394,7 @@ static int mxs_phy_hw_init(struct mxs_phy *mxs_phy)
 	return 0;
 
 disable_pll:
-	if (is_imx7ulp_phy(mxs_phy))
+	if (is_imx7ulp_phy(mxs_phy) || is_imx8ulp_phy(mxs_phy))
 		mxs_phy_pll_enable(base, false);
 	return ret;
 }
@@ -487,7 +513,7 @@ static void mxs_phy_shutdown(struct usb_phy *phy)
 	writel(BM_USBPHY_CTRL_CLKGATE,
 	       phy->io_priv + HW_USBPHY_CTRL_SET);
 
-	if (is_imx7ulp_phy(mxs_phy))
+	if (is_imx7ulp_phy(mxs_phy) || is_imx8ulp_phy(mxs_phy))
 		mxs_phy_pll_enable(phy->io_priv, false);
 
 	if (mxs_phy->phy_3p0)
-- 
2.34.1


^ permalink raw reply related

* Re: [PATCH] usb: cdns3: gadget: fix NULL pointer dereference in ep_queue
From: Peter Chen (CIX) @ 2026-03-30  9:21 UTC (permalink / raw)
  To: Yongchao Wu
  Cc: Pawel Laszczak, Roger Quadros, Greg Kroah-Hartman, linux-usb,
	linux-kernel
In-Reply-To: <20260329013404.116481-1-yongchao.wu@autochips.com>

On 26-03-29 09:34:04, Yongchao Wu wrote:
> When the gadget endpoint is disabled or not yet configured, the ep->desc
> pointer can be NULL. This leads to a NULL pointer dereference when
> __cdns3_gadget_ep_queue() is called, causing a kernel crash.
> 
> Add a check to return -ESHUTDOWN if ep->desc is NULL, which is the
> standard return code for unconfigured endpoints.
> 
> This prevents potential crashes when ep_queue is called on endpoints
> that are not ready.
> 
> Signed-off-by: Yongchao Wu  <yongchao.wu@autochips.com>

Add Fixed-by tag and Cc to stable tree please, others:

Acked-by: Peter Chen <peter.chen@kernel.org>

Peter
> ---
>  drivers/usb/cdns3/cdns3-gadget.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/cdns3-gadget.c b/drivers/usb/cdns3/cdns3-gadget.c
> index d59a60a16..96d2a4c38 100644
> --- a/drivers/usb/cdns3/cdns3-gadget.c
> +++ b/drivers/usb/cdns3/cdns3-gadget.c
> @@ -2589,6 +2589,9 @@ static int __cdns3_gadget_ep_queue(struct usb_ep *ep,
>  	struct cdns3_request *priv_req;
>  	int ret = 0;
>  
> +	if (!ep->desc)
> +		return -ESHUTDOWN;
> +
>  	request->actual = 0;
>  	request->status = -EINPROGRESS;
>  	priv_req = to_cdns3_request(request);
> -- 
> 2.43.0
> 
> 

-- 

Best regards,
Peter

^ permalink raw reply

* Re: [PATCH 1/2] usb: chipidea: core: fix device mode not work in non-lpm
From: Xu Yang @ 2026-03-30  9:13 UTC (permalink / raw)
  To: Peter Chen (CIX); +Cc: gregkh, jun.li, linux-usb, linux-kernel, imx
In-Reply-To: <acoiuow+HrDQTBKz@nchen-desktop>

On Mon, Mar 30, 2026 at 03:14:02PM +0800, Peter Chen (CIX) wrote:
> On 26-03-27 18:54:45, Xu Yang wrote:
> > On Wed, Mar 25, 2026 at 03:08:49PM +0800, Peter Chen (CIX) wrote:
> > > On 26-03-19 17:57:15, Xu Yang wrote:
> > > > In current design, we expect 2 ci_irq() to handle ID and VBUS events in
> > > > usb role switch, like what ci_extcon_wakeup_int() does. Now we only call
> > > > ci_irq() once. However, this won't bring any issues in low power mode,
> > > > because ci_irq() just take the device out of low power mode, and then
> > > > ci_extcon_wakeup_int() will call ci_irq() twice. If the device is not in
> > > > suspend state, the device mode will not work properly because VBUS event
> > > > won'tbe handled (ID event has higher priority) at all.
> > > 
> > > %s/won'tbe/won't be
> > 
> > OK.
> > 
> > > 
> > > Is it possible change ci_irq_handler and handle both events?
> > 
> > Yes, we can change ci_irq_handler() and let it set both ci->id_event and
> > ci->b_sess_valid_event flags, then queue a ci_otg_work() to handle them
> > later. 
> > 
> > I think this just unnecessarily call ci_irq_handler() to handle lpm/non-lpm
> > case as the final path is ci_otg_work() and it will handle lpm/non-lpm case
> > by naturally calling pm_runtime_get/put_sync(), otherwise it relies on
> > ci_extcon_wakeup_int() to achieve the same purpose. 
> > 
> > Both methods work for me, may I know which one do you prefer? :)
> 
> My main concerns are below code, and it may confuse the reader.
> 
> ci->id_event = true;
> ci->b_sess_valid_event = true;
> 
> I may prefer the first option, it is easy to understand.

OK. Thanks for your suggestion. I will rework the patch later.

Best Regards,
Xu Yang

^ permalink raw reply

* Re: [PATCH 8/9] usb: xhci: improve debug messages during suspend
From: Michal Pecio @ 2026-03-30  9:14 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-9-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:39 +0100, Niklas Neronin wrote:
> Improve debug output for suspend failures, particularly when the
> controller handshake does not complete. This will become important as
> upcoming patches significantly rework the resume path, making more
> detailed suspend-side messages valuable for debugging.

As an aside, I think that if this series will cause any problems,
it will be some stale data structures surviving after resume, not
anything going wrong here.

> Add an explicit check of the Save/Restore Error (SRE) flag after a
> successful Save State (CSS) operation. The xHCI specification
> (note in section 4.23.2) states:
> 
>  "After a Save or Restore State operation completes, the
>   Save/Restore Error (SRE) flag in USBSTS should be checked to
>   ensure the operation completed successfully."
> 
> Currently, the SRE error is only observed and warning is printed.
> This patch does not introduce deeper error handling, as the correct
> response is unclear and changes to suspend behavior may risk
> regressions once the resume path is updated.

I think patch 10/9 should add setting xhci->broken_suspend if this is
detected. It's ridiculous to try State Restore after State Save error.
At best, it should fail. At worst, it might not fail...

> 
> Additionally, simplify and clean up the suspend USBSTS CSS/SSS
> handling code, improving readability and quirk handling for AMD
> SNPS xHC controllers that occasionally do not clear the SSS bit.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---

>  drivers/usb/host/xhci.c | 65 +++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 658419eb6827..232e6143ac4b 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -957,11 +957,11 @@ static bool xhci_pending_portevent(struct xhci_hcd *xhci)
>   */
>  int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  {
> -	int			rc = 0;
> +	int			err;
>  	unsigned int		delay = XHCI_MAX_HALT_USEC * 2;
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
>  	u32			command;
> -	u32			res;
> +	u32			usbsts;
>  
>  	if (!hcd->state)
>  		return 0;
> @@ -1007,11 +1007,10 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	/* Some chips from Fresco Logic need an extraordinary delay */
>  	delay *= (xhci->quirks & XHCI_SLOW_SUSPEND) ? 10 : 1;
>  
> -	if (xhci_handshake(&xhci->op_regs->status,
> -		      STS_HALT, STS_HALT, delay)) {
> -		xhci_warn(xhci, "WARN: xHC CMD_RUN timeout\n");
> -		spin_unlock_irq(&xhci->lock);
> -		return -ETIMEDOUT;
> +	err = xhci_handshake(&xhci->op_regs->status, STS_HALT, STS_HALT, delay);
> +	if (err) {
> +		xhci_warn(xhci, "Clearing Run/Stop bit failed %d\n", err);
> +		goto handshake_error;
>  	}
>  	xhci_clear_command_ring(xhci);
>  
> @@ -1022,28 +1021,34 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  	command = readl(&xhci->op_regs->command);
>  	command |= CMD_CSS;
>  	writel(command, &xhci->op_regs->command);
> +
> +	err = xhci_handshake(&xhci->op_regs->status, STS_SAVE, 0, 20 * USEC_PER_MSEC);
> +	usbsts = readl(&xhci->op_regs->status);
>  	xhci->broken_suspend = 0;
> -	if (xhci_handshake(&xhci->op_regs->status,
> -				STS_SAVE, 0, 20 * 1000)) {
> -	/*
> -	 * AMD SNPS xHC 3.0 occasionally does not clear the
> -	 * SSS bit of USBSTS and when driver tries to poll
> -	 * to see if the xHC clears BIT(8) which never happens
> -	 * and driver assumes that controller is not responding
> -	 * and times out. To workaround this, its good to check
> -	 * if SRE and HCE bits are not set (as per xhci
> -	 * Section 5.4.2) and bypass the timeout.
> -	 */
> -		res = readl(&xhci->op_regs->status);
> -		if ((xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) &&
> -		    (((res & STS_SRE) == 0) &&
> -				((res & STS_HCE) == 0))) {
> -			xhci->broken_suspend = 1;
> -		} else {
> -			xhci_warn(xhci, "WARN: xHC save state timeout\n");
> -			spin_unlock_irq(&xhci->lock);
> -			return -ETIMEDOUT;
> +	if (err) {
> +		/*
> +		 * AMD SNPS xHC 3.0 occasionally does not clear the
> +		 * SSS bit of USBSTS and when driver tries to poll
> +		 * to see if the xHC clears BIT(8) which never happens
> +		 * and driver assumes that controller is not responding
> +		 * and times out. To workaround this, its good to check
> +		 * if SRE and HCE bits are not set (as per xhci
> +		 * Section 5.4.2) and bypass the timeout.
> +		 */
> +		if (!(xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)) {
> +			xhci_warn(xhci, "Controller Save State failed %d\n", err);
> +			goto handshake_error;
>  		}
> +
> +		if (usbsts & (STS_SRE | STS_HCE)) {
> +			xhci_warn(xhci, "Controller Save State failed, USBSTS 0x%08x\n", usbsts);
> +			goto handshake_error;
> +		}
> +
> +		xhci_dbg(xhci, "SNPS broken suspend, save state unreliable\n");
> +		xhci->broken_suspend = 1;
> +	} else if (usbsts & STS_SRE) {
> +		xhci_warn(xhci, "Suspend Save Error (SRE), USBSTS 0x%08x\n", usbsts);
>  	}

This is a bit complicated and those warns are almost identical.
Would it be a problem to simply:

if (SRE | HCE)
    xhci_warn("Save State error USBSTS %x")
    // patch 10/9: set broken_suspend here
    // patch 11/9: add HSE to this list

if (err)
    if (quirk)
        xhci->broken_suspend = 1;
    else
        xhci_warn("Save State timeout")
        goto handshake_error

This may sometimes print both warnings, but not a big deal.

>  	spin_unlock_irq(&xhci->lock);
>  
> @@ -1059,7 +1064,11 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
>  				__func__);
>  	}
>  
> -	return rc;
> +	return 0;
> +
> +handshake_error:
> +	spin_unlock_irq(&xhci->lock);
> +	return -ETIMEDOUT;
>  }
>  EXPORT_SYMBOL_GPL(xhci_suspend);
>  
> -- 
> 2.50.1
> 

^ permalink raw reply

* Re: [PATCH 7/9] usb: xhci: split core allocation and initialization
From: Michal Pecio @ 2026-03-30  8:57 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-8-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:38 +0100, Niklas Neronin wrote:
> Separate allocation and initialization in the xHCI core:
> * xhci_mem_init() now only handles memory allocation.
> * xhci_init() now only handles initialization.
> 
> This split allows xhci_init() to be reused when resuming from S4
> suspend-to-disk.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c |  3 +++
>  drivers/usb/host/xhci.c     | 30 ++++++++++--------------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 2cd6111c9707..f1b4f06d4b8b 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2421,6 +2421,8 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>  	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
>  	dma_addr_t	dma;
>  
> +	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
> +
>  	/*
>  	 * xHCI section 5.4.6 - Device Context array must be
>  	 * "physically contiguous and 64-byte (cache line) aligned".
> @@ -2510,6 +2512,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>  	if (xhci_setup_port_arrays(xhci, flags))
>  		goto fail;
>  
> +	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);

Patch looks OK, but I don't think there is a need to log this. The
function is completely boring, just some memory allocations while the
controller is halted and nothing happens.

Maybe it doesn't even make sense to log the beginning either. We know
which functions call this and when, we also know that they will return
ENOMEM if this fails.

>  	return 0;
>  
>  fail:
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 4e811a2668e6..658419eb6827 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -536,24 +536,13 @@ static void xhci_set_dev_notifications(struct xhci_hcd *xhci)
>  	writel(dev_notf, &xhci->op_regs->dev_notification);
>  }
>  
> -/*
> - * Initialize memory for HCD and xHC (one-time init).
> - *
> - * Program the PAGESIZE register, initialize the device context array, create
> - * device contexts (?), set up a command ring segment (or two?), create event
> - * ring (one for now).
> - */
> -static int xhci_init(struct usb_hcd *hcd)
> +/* Setup basic xHCI registers */
> +static void xhci_init(struct usb_hcd *hcd)
>  {
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	int retval;
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
>  
> -	retval = xhci_mem_init(xhci, GFP_KERNEL);
> -	if (retval)
> -		return retval;
> -
>  	/* Set the Number of Device Slots Enabled to the maximum supported value */
>  	xhci_enable_max_dev_slots(xhci);
>  
> @@ -589,7 +578,6 @@ static int xhci_init(struct usb_hcd *hcd)
>  	}
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Finished %s", __func__);
> -	return 0;
>  }
>  
>  /*-------------------------------------------------------------------------*/
> @@ -1190,11 +1178,12 @@ int xhci_resume(struct xhci_hcd *xhci, bool power_lost, bool is_auto_resume)
>  		 * first with the primary HCD, and then with the secondary HCD.
>  		 * If we don't do the same, the host will never be started.
>  		 */
> -		xhci_dbg(xhci, "Initialize the xhci_hcd\n");
> -		retval = xhci_init(hcd);
> +		retval = xhci_mem_init(xhci, GFP_KERNEL);
>  		if (retval)
>  			return retval;
>  
> +		xhci_init(hcd);
> +
>  		xhci_dbg(xhci, "Start the primary HCD\n");
>  		retval = xhci_run(hcd);
>  		if (!retval && xhci->shared_hcd) {
> @@ -5526,12 +5515,13 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  
>  	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>  
> -	xhci_dbg(xhci, "Calling HCD init\n");
> -	/* Initialize HCD and host controller data structures. */
> -	retval = xhci_init(hcd);
> +	/* Allocate xHCI data structures */
> +	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	if (retval)
>  		return retval;
> -	xhci_dbg(xhci, "Called HCD init\n");
> +
> +	/* Initialize HCD and host controller data structures */
> +	xhci_init(hcd);
>  
>  	if (xhci_hcd_is_usb3(hcd))
>  		xhci_hcd_init_usb3_data(xhci, hcd);
> -- 
> 2.50.1
> 

^ permalink raw reply

* Re: [PATCH 5/9] usb: xhci: move ring initialization
From: Neronin, Niklas @ 2026-03-30  8:53 UTC (permalink / raw)
  To: Michal Pecio; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260330104207.40c7500e.michal.pecio@gmail.com>



On 30/03/2026 11.42, Michal Pecio wrote:
> On Fri, 27 Mar 2026 13:34:36 +0100, Niklas Neronin wrote:
>> Move ring initialization from xhci_ring_alloc() to xhci_ring_init().
>> Call xhci_ring_init() after xhci_ring_alloc();
> 
> This adds more code and more opportunities for bugs.
> Can't ring_alloc() just call ring_init() itself?

Sure, but the naming would not be accurate.
ring_create() or ring_alloc_and_init() would be accurate.

> 
>> in the future it can also be used to re-initialize the ring during
>> resume.
> 
> Yes, but it seems we don't have the opposite problem: there is no
> need to allocate rings but never initialize them.

Technically there is, during ring linking. The "new" ring does not
need to be initialized, as it will be initialized according to the
"dst" rings values.

This is not implemented in this patch series, but I planned to do it
later.

> 
>>
>> Additionally, remove xhci_dbg_trace() from xhci_mem_init(). The
>> command ring's first segment DMA address is now printed during the
>> trace call in xhci_ring_init().
>>
>> This refactoring lays also the groundwork for eventually replacing:
>> * xhci_dbc_ring_init()
>> * xhci_clear_command_ring()
> 
> Or xhci_clear_command_ring() could just call memset() and ring_init(),
> instead of duplicating this sequence in every place which needs it.

xhci_clear_command_ring() clears the command ring specifically.

The idea is to have (naming not set in stone):
ring_alloc() <- allocate necessary ring memory
ring_free()  <- free all ring memory
ring_init()  <- initialize necessary ring memory
ring_reset() <- resets the ring

The reader can then easily understand what each function does.

>  
>>
>> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>


^ permalink raw reply

* Re: [PATCH 6/9] usb: xhci: move initialization for lifetime objects
From: Michal Pecio @ 2026-03-30  8:49 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-7-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:37 +0100, Niklas Neronin wrote:
> Initialize objects that exist for the lifetime of the driver only once,
> rather than repeatedly. These objects do not require re-initialization
> after events such as S4 (suspend-to-disk).
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c |  1 -
>  drivers/usb/host/xhci.c     | 15 ++++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index ca4463eebc49..2cd6111c9707 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2009,7 +2009,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  	xhci->port_caps = NULL;
>  	xhci->interrupters = NULL;
>  
> -	xhci->page_size = 0;
>  	xhci->usb2_rhub.bus_state.bus_suspended = 0;
>  	xhci->usb3_rhub.bus_state.bus_suspended = 0;
>  }
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 170615dd1e93..4e811a2668e6 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -549,13 +549,6 @@ static int xhci_init(struct usb_hcd *hcd)
>  	int retval;
>  
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Starting %s", __func__);
> -	spin_lock_init(&xhci->lock);
> -
> -	INIT_LIST_HEAD(&xhci->cmd_list);
> -	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
> -	init_completion(&xhci->cmd_ring_stop_completion);
> -	xhci_hcd_page_size(xhci);
> -	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));
>  
>  	retval = xhci_mem_init(xhci, GFP_KERNEL);
>  	if (retval)
> @@ -5525,6 +5518,14 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
>  		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
>  	}
>  
> +	spin_lock_init(&xhci->lock);
> +	INIT_LIST_HEAD(&xhci->cmd_list);
> +	INIT_DELAYED_WORK(&xhci->cmd_timer, xhci_handle_command_timeout);
> +	init_completion(&xhci->cmd_ring_stop_completion);
> +	xhci_hcd_page_size(xhci);
> +
> +	memset(xhci->devs, 0, MAX_HC_SLOTS * sizeof(*xhci->devs));

This seems to do nothing because we just got a freshly kzalloc'd
xhci_hcd from __usb_create_hcd(), as far as I understand.

On the other hand, doing it in xhci_init() used to cover for the
possibility of xhci_mem_cleanup() failing to fully clear xhci->dev.

So same questions:
Do we trust xhci_free_virt_devices_depth_first() to do its job?
Does it need to become more robust?

>  	xhci_dbg(xhci, "Calling HCD init\n");
>  	/* Initialize HCD and host controller data structures. */
>  	retval = xhci_init(hcd);
> -- 
> 2.50.1
> 

^ permalink raw reply

* Re: [PATCH 5/9] usb: xhci: move ring initialization
From: Michal Pecio @ 2026-03-30  8:42 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-6-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:36 +0100, Niklas Neronin wrote:
> Move ring initialization from xhci_ring_alloc() to xhci_ring_init().
> Call xhci_ring_init() after xhci_ring_alloc();

This adds more code and more opportunities for bugs.
Can't ring_alloc() just call ring_init() itself?

> in the future it can also be used to re-initialize the ring during
> resume.

Yes, but it seems we don't have the opposite problem: there is no
need to allocate rings but never initialize them.

> 
> Additionally, remove xhci_dbg_trace() from xhci_mem_init(). The
> command ring's first segment DMA address is now printed during the
> trace call in xhci_ring_init().
> 
> This refactoring lays also the groundwork for eventually replacing:
> * xhci_dbc_ring_init()
> * xhci_clear_command_ring()

Or xhci_clear_command_ring() could just call memset() and ring_init(),
instead of duplicating this sequence in every place which needs it.
 
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>

^ permalink raw reply

* Re: [PATCH 3/9] usb: xhci: factor out roothub bandwidth cleanup
From: Michal Pecio @ 2026-03-30  8:29 UTC (permalink / raw)
  To: Niklas Neronin; +Cc: mathias.nyman, linux-usb, raoxu
In-Reply-To: <20260327123441.806564-4-niklas.neronin@linux.intel.com>

On Fri, 27 Mar 2026 13:34:34 +0100, Niklas Neronin wrote:
> Introduce xhci_rh_bw_cleanup() to release all bandwidth tracking
> structures associated with xHCI roothub ports.
> 
> The new helper clears:
>  * TT bandwidth entries
>  * Per-interval endpoint lists
> 
> This refactors and consolidates the existing per-port cleanup logic
> previously embedded in xhci_mem_cleanup(), reducing duplication and
> making the teardown sequence easier to follow.
> 
> The helper will also be reused for upcoming S4 resume handling.
> 
> Signed-off-by: Niklas Neronin <niklas.neronin@linux.intel.com>
> ---
>  drivers/usb/host/xhci-mem.c | 50 +++++++++++++++++++++----------------
>  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 75bc1eb98b76..d4a9dbed8f16 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1895,10 +1895,36 @@ void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrup
>  }
>  EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>  
> +/* Cleanup roothub bandwidth data */
> +static void xhci_rh_bw_cleanup(struct xhci_hcd *xhci)
> +{
> +	struct xhci_root_port_bw_info *rh_bw;
> +	struct xhci_tt_bw_info *tt_info, *tt_next;
> +	struct list_head *eps, *ep, *ep_next;
> +
> +	for (int i = 0; i < xhci->max_ports; i++) {
> +		rh_bw = &xhci->rh_bw[i];
> +
> +		/* Clear and free all TT bandwidth entries */
> +		list_for_each_entry_safe(tt_info, tt_next, &rh_bw->tts, tt_list) {
> +			list_del(&tt_info->tt_list);
> +			kfree(tt_info);
> +		}

This loop is theoretically pointless, because each tt_info corresponds
to a hub virtual device, and all this stuff should have been destroyed
by xhci_free_virt_devices_depth_first() earlier.

If anything, it seems to paper over potential memory leaks in there.

> +
> +		/* Clear per-interval endpoint lists */
> +		for (int j = 0; j < XHCI_MAX_INTERVAL; j++) {
> +			eps = &rh_bw->bw_table.interval_bw[j].endpoints;
> +
> +			list_for_each_safe(ep, ep_next, eps)
> +				list_del_init(ep);
> +		}

This loop used to run before xhci_free_virt_devices_depth_first(), but
now it will run after. It seems that the endpoints here are virt_ep
which also should be gone already, so this loop likely does nothing
(empty list) or writes to virtual devices after free (somebody forgot
to unlink some endpoints from the list).

Do we trust xhci_free_virt_devices_depth_first() to work correctly?
If yes then it seems this whole function is unnecessary.

If not, perhaps delete this monstrosity and write a simpler cleanup:

1. for each slot_id
  - disable debugfs
  - free virt_device and child allocations but don't worry about tt_info
2. for each root hub port
  - free all tt_info allocations but don't worry about eps or vdevs

And since this will be used by reset on resume:

3. zero out DCBAA and xhci->devs
4. reinitialize xhci->rb_hw

> +	}
> +}
> +
>  void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  {
>  	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
> -	int i, j;
> +	int i;
>  
>  	cancel_delayed_work_sync(&xhci->cmd_timer);
>  
> @@ -1917,15 +1943,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed command ring");
>  	xhci_cleanup_command_queue(xhci);
>  
> -	for (i = 0; i < xhci->max_ports && xhci->rh_bw; i++) {
> -		struct xhci_interval_bw_table *bwt = &xhci->rh_bw[i].bw_table;
> -		for (j = 0; j < XHCI_MAX_INTERVAL; j++) {
> -			struct list_head *ep = &bwt->interval_bw[j].endpoints;
> -			while (!list_empty(ep))
> -				list_del_init(ep->next);
> -		}
> -	}
> -
>  	for (i = xhci->max_slots; i > 0; i--)
>  		xhci_free_virt_devices_depth_first(xhci, i);
>  
> @@ -1959,18 +1976,9 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>  
>  	scratchpad_free(xhci);
>  
> -	if (!xhci->rh_bw)
> -		goto no_bw;
> +	if (xhci->rh_bw)
> +		xhci_rh_bw_cleanup(xhci);
>  
> -	for (i = 0; i < xhci->max_ports; i++) {
> -		struct xhci_tt_bw_info *tt, *n;
> -		list_for_each_entry_safe(tt, n, &xhci->rh_bw[i].tts, tt_list) {
> -			list_del(&tt->tt_list);
> -			kfree(tt);
> -		}
> -	}
> -
> -no_bw:
>  	xhci->cmd_ring_reserved_trbs = 0;
>  	xhci->usb2_rhub.num_ports = 0;
>  	xhci->usb3_rhub.num_ports = 0;
> -- 
> 2.50.1
> 

^ permalink raw reply

* Re: [PATCH v1 0/4] usb: serial: mxuport: extend MXU50U support and runtime controls
From: Johan Hovold @ 2026-03-30  7:54 UTC (permalink / raw)
  To: Crescent Hsieh; +Cc: Greg Kroah-Hartman, linux-usb, linux-kernel
In-Reply-To: <acolFoqcYDuxwmPX@moxa-ThinkCentre-M90t>

On Mon, Mar 30, 2026 at 03:24:06PM +0800, Crescent Hsieh wrote:

> Gentle ping on this series.
> Please let me know if I should revise or split anything.

This one was posted less than a week ago and is still in my queue. I'll
get to reviewing it in time, but you should generally wait at least a
couple of weeks before sending reminders.

> Related firmware patch:
> https://lore.kernel.org/all/20260324024635.349522-1-crescentcy.hsieh@moxa.com/

That's great that you've already pushed the firmware.

Johan

^ permalink raw reply

* Re: [PATCH v2] dma-debug: suppress cacheline overlap warning when arch has no DMA alignment requirement
From: Marek Szyprowski @ 2026-03-30  7:44 UTC (permalink / raw)
  To: Mikhail Gavrilov, robin.murphy
  Cc: iommu, linux-kernel, linux-usb, linux-mm, harry, vbabka, akpm,
	stern, linux, andy.shevchenko, hch, Jeff.kirsher, catalin.marinas
In-Reply-To: <20260327124156.24820-1-mikhail.v.gavrilov@gmail.com>

On 27.03.2026 13:41, Mikhail Gavrilov wrote:
> When CONFIG_DMA_API_DEBUG is enabled, the DMA debug infrastructure
> tracks active mappings per cacheline and warns if two different DMA
> mappings share the same cacheline ("cacheline tracking EEXIST,
> overlapping mappings aren't supported").
>
> On x86_64, ARCH_KMALLOC_MINALIGN defaults to 8, so small kmalloc
> allocations (e.g. the 8-byte hub->buffer and hub->status in the USB
> hub driver) frequently land in the same 64-byte cacheline.  When both
> are DMA-mapped, this triggers a false positive warning.
>
> This has been reported repeatedly since v5.14 (when the EEXIST check
> was added) across various USB host controllers and devices including
> xhci_hcd with USB hubs, USB audio devices, and USB ethernet adapters.
>
> The cacheline overlap is only a real concern on architectures that
> require DMA buffer alignment to cacheline boundaries (i.e. where
> ARCH_DMA_MINALIGN >= L1_CACHE_BYTES).  On architectures like x86_64
> where dma_get_cache_alignment() returns 1, the hardware is
> cache-coherent and overlapping cacheline mappings are harmless.
>
> Suppress the EEXIST warning when dma_get_cache_alignment() is less
> than L1_CACHE_BYTES, indicating the architecture does not require
> cacheline-aligned DMA buffers.
>
> Verified with a kernel module reproducer that performs two kmalloc(8)
> allocations back-to-back and DMA-maps both:
>
>   Before: allocations share a cacheline, EEXIST fires within ~50 pairs
>   After:  same cacheline pair found, but no warning emitted
>
> Fixes: 2b4bbc6231d7 ("dma-debug: report -EEXIST errors in add_dma_entry")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215740
> Suggested-by: Harry Yoo <harry@kernel.org>
> Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
> Signed-off-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>

Applied to dma-mapping-fixes. Thanks!

> ---
>
> v1 -> v2:
>   - Moved fix from include/linux/slab.h (ARCH_KMALLOC_MINALIGN)
>     to kernel/dma/debug.c per Harry Yoo's suggestion.
>   - Instead of forcing cacheline-aligned allocations, suppress
>     the warning when the architecture has no DMA alignment
>     requirement (dma_get_cache_alignment() < L1_CACHE_BYTES).
>
> v1: https://lore.kernel.org/all/20260327055846.248829-1-mikhail.v.gavrilov@gmail.com/
>
> Reproducer module that triggers the bug reliably:
>   https://bugzilla.kernel.org/attachment.cgi?id=309769
>
>  kernel/dma/debug.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/kernel/dma/debug.c b/kernel/dma/debug.c
> index 0677918f06a8..1a725edbbbf6 100644
> --- a/kernel/dma/debug.c
> +++ b/kernel/dma/debug.c
> @@ -615,6 +615,7 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)
>  	} else if (rc == -EEXIST &&
>  		   !(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
>  		   !(entry->is_cache_clean && overlap_cache_clean) &&
> +		   dma_get_cache_alignment() >= L1_CACHE_BYTES &&
>  		   !(IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) &&
>  		     is_swiotlb_active(entry->dev))) {
>  		err_printk(entry->dev, entry,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox