[PATCH 0/9] USB/IP VHCI suspend fix and driver cleanup

[PATCH 0/9] USB/IP VHCI suspend fix and driver cleanup

[Cristian Ciocaltea]

The USB/IP Virtual Host Controller (VHCI) platform driver is expected to
prevent entering system suspend when at least one remote device is
attached to the virtual USB root hub.

However, in some cases, the detection logic for active USB/IP
connections doesn't seem to work reliably, e.g. when all devices
attached to the virtual hub have been already suspended.  This will
normally lead to a broken suspend state, with unrecoverable resume.

The first patch of the series provides a workaround to ensure the
attached devices do not enter suspend.

Additionally, during the investigation I noticed and fixed a bunch of
coding style issues, hence the subsequent patches contain all the
changes needed to make checkpatch happy for the entire driver.

WARNING:

Please note commit aa7a9275ab81 ("PM: sleep: Suspend async parents after
suspending children") from v6.16-rc1 introduced a regression which
breaks the suspend cancellation and hangs the system.

A fix [1] has been already provided, which is expected to land in
v6.16-rc7.  The patch is currently available in next-20250717, which
this series is also based on, as commit ebd6884167ea ("PM: sleep: Update
power.completion for all devices on errors").

[1] https://lore.kernel.org/all/6191258.lOV4Wx5bFT@rjwysocki.net/

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
Cristian Ciocaltea (9):
      usb: vhci-hcd: Prevent suspending virtually attached devices
      usb: vhci-hcd: Fix space, brace, alignment and line length issues
      usb: vhci-hcd: Simplify NULL comparison
      usb: vhci-hcd: Simplify kzalloc usage
      usb: vhci-hcd: Do not split quoted strings
      usb: vhci-hcd: Fix block comments
      usb: vhci-hcd: Use the paranthesized form of sizeof
      usb: vhci-hcd: Consistently use __func__
      usb: vhci-hcd: Remove ftrace-like logging

 drivers/usb/usbip/vhci_hcd.c | 184 +++++++++++++++++++++----------------------
 1 file changed, 89 insertions(+), 95 deletions(-)
---
base-commit: 024e09e444bd2b06aee9d1f3fe7b313c7a2df1bb
change-id: 20250714-vhci-hcd-suspend-fix-7db5c25c509d

[PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Cristian Ciocaltea]

The VHCI platform driver aims to forbid entering system suspend when at
least one of the virtual USB ports are bound to an active USB/IP
connection.

However, in some cases, the detection logic doesn't work reliably, i.e.
when all devices attached to the virtual root hub have been already
suspended, leading to a broken suspend state, with unrecoverable resume.

Ensure the attached devices do not enter suspend by setting the syscore
PM flag.

Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 				 ctrlreq->wValue, vdev->rhport);
 
 			vdev->udev = usb_get_dev(urb->dev);
+			dev_pm_syscore_device(&vdev->udev->dev, true);
 			usb_put_dev(old);
 
 			spin_lock(&vdev->ud.lock);
@@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
 
 			vdev->udev = usb_get_dev(urb->dev);
+			dev_pm_syscore_device(&vdev->udev->dev, true);
 			usb_put_dev(old);
 			goto out;
 

-- 
2.50.0

[PATCH 2/9] usb: vhci-hcd: Fix space, brace, alignment and line length issues

[Cristian Ciocaltea]

Perform a first round of coding style cleanup:

* Add new lines after several statement blocks
* Avoid line wrapping when 100-column width is not exceeded and it helps
  improve code readability
* Ensure lines do not end with '('
* Drop superfluous spaces or empty lines
* Add spaces where necessary, e.g. around operators
* Add braces for single if-statements when at least one branch of the
  conditional requires them

This helps getting rid of the following checkpatch complaints:

  CHECK: Lines should not end with a '('
  CHECK: braces {} should be used on all arms of this statement
  CHECK: Unbalanced braces around else statement
  CHECK: Blank lines aren't necessary before a close brace '}'
  CHECK: Unnecessary parentheses around
  CHECK: Alignment should match open parenthesis
  CHECK: No space is necessary after a cast
  CHECK: spaces preferred around that '-' (ctx:VxV)
  CHECK: spaces preferred around that '+' (ctx:VxV)

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 127 +++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 60 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 762b60e10a9415e58147cde2f615045da5804a0e..f58ba3b5ed50c5cc68d2180a4df78ab4a5f5061d 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -145,6 +145,7 @@ static void dump_port_status_diff(u32 prev_status, u32 new_status, bool usb3)
 			if (bit == 1) /* USB_PORT_STAT_CONNECTION */
 				pr_debug(" %c%s\n", change, "USB_PORT_STAT_SPEED_5GBPS");
 		}
+
 		bit <<= 1;
 		i++;
 	}
@@ -254,7 +255,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf)
 		}
 	}
 
-	if ((hcd->state == HC_STATE_SUSPENDED) && (changed == 1))
+	if (hcd->state == HC_STATE_SUSPENDED && changed == 1)
 		usb_hcd_resume_root_hub(hcd);
 
 done:
@@ -267,7 +268,6 @@ static struct {
 	struct usb_bos_descriptor bos;
 	struct usb_ss_cap_descriptor ss_cap;
 } __packed usb3_bos_desc = {
-
 	.bos = {
 		.bLength		= USB_DT_BOS_SIZE,
 		.bDescriptorType	= USB_DT_BOS,
@@ -289,8 +289,8 @@ ss_hub_descriptor(struct usb_hub_descriptor *desc)
 	memset(desc, 0, sizeof *desc);
 	desc->bDescriptorType = USB_DT_SS_HUB;
 	desc->bDescLength = 12;
-	desc->wHubCharacteristics = cpu_to_le16(
-		HUB_CHAR_INDV_PORT_LPSM | HUB_CHAR_COMMON_OCPM);
+	desc->wHubCharacteristics = cpu_to_le16(HUB_CHAR_INDV_PORT_LPSM |
+						HUB_CHAR_COMMON_OCPM);
 	desc->bNbrPorts = VHCI_HC_PORTS;
 	desc->u.ss.bHubHdrDecLat = 0x04; /* Worst case: 0.4 micro sec*/
 	desc->u.ss.DeviceRemovable = 0xffff;
@@ -302,11 +302,11 @@ static inline void hub_descriptor(struct usb_hub_descriptor *desc)
 
 	memset(desc, 0, sizeof(*desc));
 	desc->bDescriptorType = USB_DT_HUB;
-	desc->wHubCharacteristics = cpu_to_le16(
-		HUB_CHAR_INDV_PORT_LPSM | HUB_CHAR_COMMON_OCPM);
-
+	desc->wHubCharacteristics = cpu_to_le16(HUB_CHAR_INDV_PORT_LPSM |
+						HUB_CHAR_COMMON_OCPM);
 	desc->bNbrPorts = VHCI_HC_PORTS;
 	BUILD_BUG_ON(VHCI_HC_PORTS > USB_MAXCHILDREN);
+
 	width = desc->bNbrPorts / 8 + 1;
 	desc->bDescLength = USB_DT_HUB_NONVAR_SIZE + 2 * width;
 	memset(&desc->u.hs.DeviceRemovable[0], 0, width);
@@ -347,8 +347,9 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		invalid_rhport = true;
 		if (wIndex > VHCI_HC_PORTS)
 			pr_err("invalid port number %d\n", wIndex);
-	} else
+	} else {
 		rhport = wIndex - 1;
+	}
 
 	vhci_hcd = hcd_to_vhci_hcd(hcd);
 	vhci = vhci_hcd->vhci;
@@ -359,7 +360,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	if (usbip_dbg_flag_vhci_rh) {
 		if (!invalid_rhport)
 			memcpy(prev_port_status, vhci_hcd->port_status,
-				sizeof(prev_port_status));
+			       sizeof(prev_port_status));
 	}
 
 	switch (typeReq) {
@@ -371,6 +372,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			pr_err("invalid port number %d\n", wIndex);
 			goto error;
 		}
+
 		switch (wValue) {
 		case USB_PORT_FEAT_SUSPEND:
 			if (hcd->speed >= HCD_USB3) {
@@ -378,8 +380,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				       "supported for USB 3.0 roothub\n");
 				goto error;
 			}
-			usbip_dbg_vhci_rh(
-				" ClearPortFeature: USB_PORT_FEAT_SUSPEND\n");
+
+			usbip_dbg_vhci_rh(" ClearPortFeature: USB_PORT_FEAT_SUSPEND\n");
 			if (vhci_hcd->port_status[rhport] & USB_PORT_STAT_SUSPEND) {
 				/* 20msec signaling */
 				vhci_hcd->resuming = 1;
@@ -387,8 +389,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			}
 			break;
 		case USB_PORT_FEAT_POWER:
-			usbip_dbg_vhci_rh(
-				" ClearPortFeature: USB_PORT_FEAT_POWER\n");
+			usbip_dbg_vhci_rh(" ClearPortFeature: USB_PORT_FEAT_POWER\n");
 			if (hcd->speed >= HCD_USB3)
 				vhci_hcd->port_status[rhport] &= ~USB_SS_PORT_STAT_POWER;
 			else
@@ -399,6 +400,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 					  wValue);
 			if (wValue >= 32)
 				goto error;
+
 			vhci_hcd->port_status[rhport] &= ~(1 << wValue);
 			break;
 		}
@@ -406,15 +408,15 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	case GetHubDescriptor:
 		usbip_dbg_vhci_rh(" GetHubDescriptor\n");
 		if (hcd->speed >= HCD_USB3 &&
-				(wLength < USB_DT_SS_HUB_SIZE ||
-				 wValue != (USB_DT_SS_HUB << 8))) {
+		    (wLength < USB_DT_SS_HUB_SIZE || wValue != (USB_DT_SS_HUB << 8))) {
 			pr_err("Wrong hub descriptor type for USB 3.0 roothub.\n");
 			goto error;
 		}
+
 		if (hcd->speed >= HCD_USB3)
-			ss_hub_descriptor((struct usb_hub_descriptor *) buf);
+			ss_hub_descriptor((struct usb_hub_descriptor *)buf);
 		else
-			hub_descriptor((struct usb_hub_descriptor *) buf);
+			hub_descriptor((struct usb_hub_descriptor *)buf);
 		break;
 	case DeviceRequest | USB_REQ_GET_DESCRIPTOR:
 		if (hcd->speed < HCD_USB3)
@@ -428,7 +430,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		break;
 	case GetHubStatus:
 		usbip_dbg_vhci_rh(" GetHubStatus\n");
-		*(__le32 *) buf = cpu_to_le32(0);
+		*(__le32 *)buf = cpu_to_le32(0);
 		break;
 	case GetPortStatus:
 		usbip_dbg_vhci_rh(" GetPortStatus port %x\n", wIndex);
@@ -464,10 +466,9 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				VDEV_ST_NOTASSIGNED ||
 			    vhci_hcd->vdev[rhport].ud.status ==
 				VDEV_ST_USED) {
-				usbip_dbg_vhci_rh(
-					" enable rhport %d (status %u)\n",
-					rhport,
-					vhci_hcd->vdev[rhport].ud.status);
+				usbip_dbg_vhci_rh(" enable rhport %d (status %u)\n",
+						  rhport,
+						  vhci_hcd->vdev[rhport].ud.status);
 				vhci_hcd->port_status[rhport] |=
 					USB_PORT_STAT_ENABLE;
 			}
@@ -488,8 +489,9 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				}
 			}
 		}
-		((__le16 *) buf)[0] = cpu_to_le16(vhci_hcd->port_status[rhport]);
-		((__le16 *) buf)[1] =
+
+		((__le16 *)buf)[0] = cpu_to_le16(vhci_hcd->port_status[rhport]);
+		((__le16 *)buf)[1] =
 			cpu_to_le16(vhci_hcd->port_status[rhport] >> 16);
 
 		usbip_dbg_vhci_rh(" GetPortStatus bye %x %x\n", ((u16 *)buf)[0],
@@ -502,25 +504,23 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	case SetPortFeature:
 		switch (wValue) {
 		case USB_PORT_FEAT_LINK_STATE:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_LINK_STATE\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_LINK_STATE\n");
 			if (hcd->speed < HCD_USB3) {
 				pr_err("USB_PORT_FEAT_LINK_STATE req not "
 				       "supported for USB 2.0 roothub\n");
 				goto error;
 			}
+
 			/*
 			 * Since this is dummy we don't have an actual link so
 			 * there is nothing to do for the SET_LINK_STATE cmd
 			 */
 			break;
 		case USB_PORT_FEAT_U1_TIMEOUT:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_U1_TIMEOUT\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_U1_TIMEOUT\n");
 			fallthrough;
 		case USB_PORT_FEAT_U2_TIMEOUT:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
 			/* TODO: add suspend/resume support! */
 			if (hcd->speed < HCD_USB3) {
 				pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
@@ -529,8 +529,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			}
 			break;
 		case USB_PORT_FEAT_SUSPEND:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_SUSPEND\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_SUSPEND\n");
 			/* Applicable only for USB2.0 hub */
 			if (hcd->speed >= HCD_USB3) {
 				pr_err("USB_PORT_FEAT_SUSPEND req not "
@@ -546,24 +545,24 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			vhci_hcd->port_status[rhport] |= USB_PORT_STAT_SUSPEND;
 			break;
 		case USB_PORT_FEAT_POWER:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_POWER\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_POWER\n");
 			if (invalid_rhport) {
 				pr_err("invalid port number %d\n", wIndex);
 				goto error;
 			}
+
 			if (hcd->speed >= HCD_USB3)
 				vhci_hcd->port_status[rhport] |= USB_SS_PORT_STAT_POWER;
 			else
 				vhci_hcd->port_status[rhport] |= USB_PORT_STAT_POWER;
 			break;
 		case USB_PORT_FEAT_BH_PORT_RESET:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_BH_PORT_RESET\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_BH_PORT_RESET\n");
 			if (invalid_rhport) {
 				pr_err("invalid port number %d\n", wIndex);
 				goto error;
 			}
+
 			/* Applicable only for USB3.0 hub */
 			if (hcd->speed < HCD_USB3) {
 				pr_err("USB_PORT_FEAT_BH_PORT_RESET req not "
@@ -572,12 +571,12 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			}
 			fallthrough;
 		case USB_PORT_FEAT_RESET:
-			usbip_dbg_vhci_rh(
-				" SetPortFeature: USB_PORT_FEAT_RESET\n");
+			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_RESET\n");
 			if (invalid_rhport) {
 				pr_err("invalid port number %d\n", wIndex);
 				goto error;
 			}
+
 			/* if it's already enabled, disable */
 			if (hcd->speed >= HCD_USB3) {
 				vhci_hcd->port_status[rhport] = 0;
@@ -601,18 +600,21 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				pr_err("invalid port number %d\n", wIndex);
 				goto error;
 			}
+
 			if (wValue >= 32)
 				goto error;
+
 			if (hcd->speed >= HCD_USB3) {
 				if ((vhci_hcd->port_status[rhport] &
 				     USB_SS_PORT_STAT_POWER) != 0) {
 					vhci_hcd->port_status[rhport] |= (1 << wValue);
 				}
-			} else
+			} else {
 				if ((vhci_hcd->port_status[rhport] &
 				     USB_PORT_STAT_POWER) != 0) {
 					vhci_hcd->port_status[rhport] |= (1 << wValue);
 				}
+			}
 		}
 		break;
 	case GetPortErrorCount:
@@ -623,7 +625,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			goto error;
 		}
 		/* We'll always return 0 since this is a dummy hub */
-		*(__le32 *) buf = cpu_to_le32(0);
+		*(__le32 *)buf = cpu_to_le32(0);
 		break;
 	case SetHubDepth:
 		usbip_dbg_vhci_rh(" SetHubDepth\n");
@@ -635,7 +637,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		break;
 	default:
 		pr_err("default hub control req: %04x v%04x i%04x l%d\n",
-			typeReq, wValue, wIndex, wLength);
+		       typeReq, wValue, wIndex, wLength);
 error:
 		/* "protocol stall" on error */
 		retval = -EPIPE;
@@ -683,7 +685,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
 	priv->vdev = vdev;
 	priv->urb = urb;
 
-	urb->hcpriv = (void *) priv;
+	urb->hcpriv = (void *)priv;
 
 	list_add_tail(&priv->list, &vdev->priv_tx);
 
@@ -705,10 +707,10 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		pr_err("invalid port number %d\n", portnum);
 		return -ENODEV;
 	}
-	vdev = &vhci_hcd->vdev[portnum-1];
+	vdev = &vhci_hcd->vdev[portnum - 1];
 
 	if (!urb->transfer_buffer && !urb->num_sgs &&
-	     urb->transfer_buffer_length) {
+	    urb->transfer_buffer_length) {
 		dev_dbg(dev, "Null URB transfer buffer\n");
 		return -EINVAL;
 	}
@@ -730,6 +732,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		spin_unlock_irqrestore(&vhci->lock, flags);
 		return -ENODEV;
 	}
+
 	spin_unlock(&vdev->ud.lock);
 
 	ret = usb_hcd_link_urb_to_ep(hcd, urb);
@@ -749,7 +752,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		struct usb_device *old;
 		__u8 type = usb_pipetype(urb->pipe);
 		struct usb_ctrlrequest *ctrlreq =
-			(struct usb_ctrlrequest *) urb->setup_packet;
+			(struct usb_ctrlrequest *)urb->setup_packet;
 
 		if (type != PIPE_CONTROL || !ctrlreq) {
 			dev_err(dev, "invalid request to devnum 0\n");
@@ -782,7 +785,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 
 		case USB_REQ_GET_DESCRIPTOR:
 			if (ctrlreq->wValue == cpu_to_le16(USB_DT_DEVICE << 8))
-				usbip_dbg_vhci_hc(
+				usbip_dbg_vhci_hc(/**/
 					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
 
 			vdev->udev = usb_get_dev(urb->dev);
@@ -799,7 +802,6 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 			ret =  -EINVAL;
 			goto no_need_xmit;
 		}
-
 	}
 
 out:
@@ -820,6 +822,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 		usb_hcd_giveback_urb(hcd, urb, urb->status);
 		local_irq_enable();
 	}
+
 	return ret;
 }
 
@@ -876,6 +879,7 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	struct vhci_priv *priv;
 	struct vhci_device *vdev;
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&vhci->lock, flags);
 
@@ -887,14 +891,10 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		return -EIDRM;
 	}
 
-	{
-		int ret = 0;
-
-		ret = usb_hcd_check_unlink_urb(hcd, urb, status);
-		if (ret) {
-			spin_unlock_irqrestore(&vhci->lock, flags);
-			return ret;
-		}
+	ret = usb_hcd_check_unlink_urb(hcd, urb, status);
+	if (ret) {
+		spin_unlock_irqrestore(&vhci->lock, flags);
+		return ret;
 	}
 
 	 /* send unlink request here? */
@@ -957,7 +957,7 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 }
 
 static void vhci_cleanup_unlink_list(struct vhci_device *vdev,
-		struct list_head *unlink_list)
+				     struct list_head *unlink_list)
 {
 	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
 	struct usb_hcd *hcd = vhci_hcd_to_hcd(vhci_hcd);
@@ -1136,7 +1136,7 @@ static int hcd_name_to_id(const char *name)
 	if (c == NULL)
 		return 0;
 
-	ret = kstrtol(c+1, 10, &val);
+	ret = kstrtol(c + 1, 10, &val);
 	if (ret < 0)
 		return ret;
 
@@ -1213,12 +1213,14 @@ static int vhci_start(struct usb_hcd *hcd)
 			dev_err(hcd_dev(hcd), "init attr group failed, err = %d\n", err);
 			return err;
 		}
+
 		err = sysfs_create_group(&hcd_dev(hcd)->kobj, &vhci_attr_group);
 		if (err) {
 			dev_err(hcd_dev(hcd), "create sysfs files failed, err = %d\n", err);
 			vhci_finish_attr_group();
 			return err;
 		}
+
 		pr_info("created sysfs %s\n", hcd_name(hcd));
 	}
 
@@ -1280,10 +1282,12 @@ static int vhci_bus_resume(struct usb_hcd *hcd)
 	dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__);
 
 	spin_lock_irqsave(&vhci->lock, flags);
+
 	if (!HCD_HW_ACCESSIBLE(hcd))
 		rc = -ESHUTDOWN;
 	else
 		hcd->state = HC_STATE_RUNNING;
+
 	spin_unlock_irqrestore(&vhci->lock, flags);
 
 	return rc;
@@ -1297,8 +1301,8 @@ static int vhci_bus_resume(struct usb_hcd *hcd)
 
 /* Change a group of bulk endpoints to support multiple stream IDs */
 static int vhci_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
-	struct usb_host_endpoint **eps, unsigned int num_eps,
-	unsigned int num_streams, gfp_t mem_flags)
+			      struct usb_host_endpoint **eps, unsigned int num_eps,
+			      unsigned int num_streams, gfp_t mem_flags)
 {
 	dev_dbg(&hcd->self.root_hub->dev, "vhci_alloc_streams not implemented\n");
 	return 0;
@@ -1306,7 +1310,7 @@ static int vhci_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
 
 /* Reverts a group of bulk endpoints back to not using stream IDs. */
 static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
-	struct usb_host_endpoint **eps, unsigned int num_eps,
+			     struct usb_host_endpoint **eps, unsigned int num_eps,
 	gfp_t mem_flags)
 {
 	dev_dbg(&hcd->self.root_hub->dev, "vhci_free_streams not implemented\n");
@@ -1356,6 +1360,7 @@ static int vhci_hcd_probe(struct platform_device *pdev)
 		pr_err("create primary hcd failed\n");
 		return -ENOMEM;
 	}
+
 	hcd_hs->has_tt = 1;
 
 	/*
@@ -1471,6 +1476,7 @@ static int vhci_hcd_resume(struct platform_device *pdev)
 	hcd = platform_get_drvdata(pdev);
 	if (!hcd)
 		return 0;
+
 	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	usb_hcd_poll_rh_status(hcd);
 
@@ -1502,6 +1508,7 @@ static void del_platform_devices(void)
 		platform_device_unregister(vhcis[i].pdev);
 		vhcis[i].pdev = NULL;
 	}
+
 	sysfs_remove_link(&platform_bus.kobj, driver_name);
 }
 

-- 
2.50.0

[PATCH 3/9] usb: vhci-hcd: Simplify NULL comparison

[Cristian Ciocaltea]

Use the shorter variants as suggested by checkpatch.pl:

  CHECK: Comparison to NULL could be written "!c"
  CHECK: Comparison to NULL could be written "!vhcis"

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index f58ba3b5ed50c5cc68d2180a4df78ab4a5f5061d..7e0f11fdbf4fe919265db31768c3a19d27dbdd18 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1133,7 +1133,7 @@ static int hcd_name_to_id(const char *name)
 	int ret;
 
 	c = strchr(name, '.');
-	if (c == NULL)
+	if (!c)
 		return 0;
 
 	ret = kstrtol(c + 1, 10, &val);
@@ -1523,7 +1523,7 @@ static int __init vhci_hcd_init(void)
 		vhci_num_controllers = 1;
 
 	vhcis = kcalloc(vhci_num_controllers, sizeof(struct vhci), GFP_KERNEL);
-	if (vhcis == NULL)
+	if (!vhcis)
 		return -ENOMEM;
 
 	ret = platform_driver_register(&vhci_driver);

-- 
2.50.0

[PATCH 4/9] usb: vhci-hcd: Simplify kzalloc usage

[Cristian Ciocaltea]

Use the shorter variants as suggested by checkpatch.pl:

  CHECK: Prefer kzalloc(sizeof(*priv)...) over kzalloc(sizeof(struct vhci_priv)...)
  CHECK: Prefer kzalloc(sizeof(*unlink)...) over kzalloc(sizeof(struct vhci_unlink)...)

This also improves further maintainability.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 7e0f11fdbf4fe919265db31768c3a19d27dbdd18..3f888455d238b983a6aafa52418fb89a914c32b5 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -670,7 +670,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev)
 	struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev);
 	unsigned long flags;
 
-	priv = kzalloc(sizeof(struct vhci_priv), GFP_ATOMIC);
+	priv = kzalloc(sizeof(*priv), GFP_ATOMIC);
 	if (!priv) {
 		usbip_event_add(&vdev->ud, VDEV_EVENT_ERROR_MALLOC);
 		return;
@@ -928,7 +928,7 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		spin_lock(&vdev->priv_lock);
 
 		/* setup CMD_UNLINK pdu */
-		unlink = kzalloc(sizeof(struct vhci_unlink), GFP_ATOMIC);
+		unlink = kzalloc(sizeof(*unlink), GFP_ATOMIC);
 		if (!unlink) {
 			spin_unlock(&vdev->priv_lock);
 			spin_unlock_irqrestore(&vhci->lock, flags);

-- 
2.50.0

[PATCH 5/9] usb: vhci-hcd: Do not split quoted strings

[Cristian Ciocaltea]

Join the split strings to make checkpatch happy and regain ability to
grep for those log messages in the source code:

  WARNING: quoted string split across lines

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3f888455d238b983a6aafa52418fb89a914c32b5..53691d5e77d386b0b0e16fe9d08824baa04c0b1e 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -376,8 +376,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		switch (wValue) {
 		case USB_PORT_FEAT_SUSPEND:
 			if (hcd->speed >= HCD_USB3) {
-				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not "
-				       "supported for USB 3.0 roothub\n");
+				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not supported for USB 3.0 roothub\n");
 				goto error;
 			}
 
@@ -506,8 +505,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		case USB_PORT_FEAT_LINK_STATE:
 			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_LINK_STATE\n");
 			if (hcd->speed < HCD_USB3) {
-				pr_err("USB_PORT_FEAT_LINK_STATE req not "
-				       "supported for USB 2.0 roothub\n");
+				pr_err("USB_PORT_FEAT_LINK_STATE req not supported for USB 2.0 roothub\n");
 				goto error;
 			}
 
@@ -523,8 +521,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_U2_TIMEOUT\n");
 			/* TODO: add suspend/resume support! */
 			if (hcd->speed < HCD_USB3) {
-				pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not "
-				       "supported for USB 2.0 roothub\n");
+				pr_err("USB_PORT_FEAT_U1/2_TIMEOUT req not supported for USB 2.0 roothub\n");
 				goto error;
 			}
 			break;
@@ -532,8 +529,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_SUSPEND\n");
 			/* Applicable only for USB2.0 hub */
 			if (hcd->speed >= HCD_USB3) {
-				pr_err("USB_PORT_FEAT_SUSPEND req not "
-				       "supported for USB 3.0 roothub\n");
+				pr_err("USB_PORT_FEAT_SUSPEND req not supported for USB 3.0 roothub\n");
 				goto error;
 			}
 
@@ -565,8 +561,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 			/* Applicable only for USB3.0 hub */
 			if (hcd->speed < HCD_USB3) {
-				pr_err("USB_PORT_FEAT_BH_PORT_RESET req not "
-				       "supported for USB 2.0 roothub\n");
+				pr_err("USB_PORT_FEAT_BH_PORT_RESET req not supported for USB 2.0 roothub\n");
 				goto error;
 			}
 			fallthrough;
@@ -620,8 +615,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	case GetPortErrorCount:
 		usbip_dbg_vhci_rh(" GetPortErrorCount\n");
 		if (hcd->speed < HCD_USB3) {
-			pr_err("GetPortErrorCount req not "
-			       "supported for USB 2.0 roothub\n");
+			pr_err("GetPortErrorCount req not supported for USB 2.0 roothub\n");
 			goto error;
 		}
 		/* We'll always return 0 since this is a dummy hub */
@@ -630,8 +624,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	case SetHubDepth:
 		usbip_dbg_vhci_rh(" SetHubDepth\n");
 		if (hcd->speed < HCD_USB3) {
-			pr_err("SetHubDepth req not supported for "
-			       "USB 2.0 roothub\n");
+			pr_err("SetHubDepth req not supported for USB 2.0 roothub\n");
 			goto error;
 		}
 		break;

-- 
2.50.0

[PATCH 6/9] usb: vhci-hcd: Fix block comments

[Cristian Ciocaltea]

Address a couple of comment formatting issues indicated by
checkpatch.pl:

  WARNING: Block comments use a trailing */ on a separate line

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 53691d5e77d386b0b0e16fe9d08824baa04c0b1e..a2a8418c77e58ae9e06d673d0012b972c92ee33b 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -879,7 +879,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 	priv = urb->hcpriv;
 	if (!priv) {
 		/* URB was never linked! or will be soon given back by
-		 * vhci_rx. */
+		 * vhci_rx.
+		 */
 		spin_unlock_irqrestore(&vhci->lock, flags);
 		return -EIDRM;
 	}
@@ -936,7 +937,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
 		unlink->unlink_seqnum = priv->seqnum;
 
 		/* send cmd_unlink and try to cancel the pending URB in the
-		 * peer */
+		 * peer
+		 */
 		list_add_tail(&unlink->list, &vdev->unlink_tx);
 		wake_up(&vdev->waitq_tx);
 

-- 
2.50.0

[PATCH 7/9] usb: vhci-hcd: Use the paranthesized form of sizeof

[Cristian Ciocaltea]

Fix the sizeof usage issue reported by checkpatch.pl:

  WARNING: sizeof *desc should be sizeof(*desc)

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index a2a8418c77e58ae9e06d673d0012b972c92ee33b..841902482fb15d1d86525f23492e4966f35630a0 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -286,7 +286,7 @@ static struct {
 static inline void
 ss_hub_descriptor(struct usb_hub_descriptor *desc)
 {
-	memset(desc, 0, sizeof *desc);
+	memset(desc, 0, sizeof(*desc));
 	desc->bDescriptorType = USB_DT_SS_HUB;
 	desc->bDescLength = 12;
 	desc->wHubCharacteristics = cpu_to_le16(HUB_CHAR_INDV_PORT_LPSM |

-- 
2.50.0

[PATCH 8/9] usb: vhci-hcd: Consistently use __func__

[Cristian Ciocaltea]

Replace all explicit function names in string literals with __func__ and
silent several checkpatch complaints similar to the following one:

  WARNING: Prefer using '"%s...", __func__' to using 'vhci_start', this function's name, in a string

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 841902482fb15d1d86525f23492e4966f35630a0..95034440c84f931bdf47552b499e0fdc6f726e59 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -160,7 +160,7 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed)
 	u32		status;
 	unsigned long	flags;
 
-	usbip_dbg_vhci_rh("rh_port_connect %d\n", rhport);
+	usbip_dbg_vhci_rh("%s %d\n", __func__, rhport);
 
 	spin_lock_irqsave(&vhci->lock, flags);
 
@@ -194,7 +194,7 @@ static void rh_port_disconnect(struct vhci_device *vdev)
 	u32		status;
 	unsigned long	flags;
 
-	usbip_dbg_vhci_rh("rh_port_disconnect %d\n", rhport);
+	usbip_dbg_vhci_rh("%s %d\n", __func__, rhport);
 
 	spin_lock_irqsave(&vhci->lock, flags);
 
@@ -1172,7 +1172,7 @@ static int vhci_start(struct usb_hcd *hcd)
 	int id, rhport;
 	int err;
 
-	usbip_dbg_vhci_hc("enter vhci_start\n");
+	usbip_dbg_vhci_hc("enter %s\n", __func__);
 
 	if (usb_hcd_is_primary_hcd(hcd))
 		spin_lock_init(&vhci_hcd->vhci->lock);
@@ -1299,7 +1299,7 @@ static int vhci_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
 			      struct usb_host_endpoint **eps, unsigned int num_eps,
 			      unsigned int num_streams, gfp_t mem_flags)
 {
-	dev_dbg(&hcd->self.root_hub->dev, "vhci_alloc_streams not implemented\n");
+	dev_dbg(&hcd->self.root_hub->dev, "%s not implemented\n", __func__);
 	return 0;
 }
 
@@ -1308,7 +1308,7 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
 			     struct usb_host_endpoint **eps, unsigned int num_eps,
 	gfp_t mem_flags)
 {
-	dev_dbg(&hcd->self.root_hub->dev, "vhci_free_streams not implemented\n");
+	dev_dbg(&hcd->self.root_hub->dev, "%s not implemented\n", __func__);
 	return 0;
 }
 

-- 
2.50.0

[PATCH 9/9] usb: vhci-hcd: Remove ftrace-like logging

[Cristian Ciocaltea]

Drop all superfluous log entries as reported by checkpatch:

  WARNING: Unnecessary ftrace-like logging - prefer using ftrace

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/usb/usbip/vhci_hcd.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 95034440c84f931bdf47552b499e0fdc6f726e59..cb28be94de02627d01fcdab97a3571bca9cc0858 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1172,8 +1172,6 @@ static int vhci_start(struct usb_hcd *hcd)
 	int id, rhport;
 	int err;
 
-	usbip_dbg_vhci_hc("enter %s\n", __func__);
-
 	if (usb_hcd_is_primary_hcd(hcd))
 		spin_lock_init(&vhci_hcd->vhci->lock);
 
@@ -1259,8 +1257,6 @@ static int vhci_bus_suspend(struct usb_hcd *hcd)
 	struct vhci *vhci = *((void **)dev_get_platdata(hcd->self.controller));
 	unsigned long flags;
 
-	dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__);
-
 	spin_lock_irqsave(&vhci->lock, flags);
 	hcd->state = HC_STATE_SUSPENDED;
 	spin_unlock_irqrestore(&vhci->lock, flags);
@@ -1274,8 +1270,6 @@ static int vhci_bus_resume(struct usb_hcd *hcd)
 	int rc = 0;
 	unsigned long flags;
 
-	dev_dbg(&hcd->self.root_hub->dev, "%s\n", __func__);
-
 	spin_lock_irqsave(&vhci->lock, flags);
 
 	if (!HCD_HW_ACCESSIBLE(hcd))
@@ -1427,8 +1421,6 @@ static int vhci_hcd_suspend(struct platform_device *pdev, pm_message_t state)
 	int ret = 0;
 	unsigned long flags;
 
-	dev_dbg(&pdev->dev, "%s\n", __func__);
-
 	hcd = platform_get_drvdata(pdev);
 	if (!hcd)
 		return 0;
@@ -1466,8 +1458,6 @@ static int vhci_hcd_resume(struct platform_device *pdev)
 {
 	struct usb_hcd *hcd;
 
-	dev_dbg(&pdev->dev, "%s\n", __func__);
-
 	hcd = platform_get_drvdata(pdev);
 	if (!hcd)
 		return 0;

-- 
2.50.0

Re: [PATCH 8/9] usb: vhci-hcd: Consistently use __func__

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 06:54:57PM +0300, Cristian Ciocaltea wrote:
> Replace all explicit function names in string literals with __func__ and
> silent several checkpatch complaints similar to the following one:
> 
>   WARNING: Prefer using '"%s...", __func__' to using 'vhci_start', this function's name, in a string
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 841902482fb15d1d86525f23492e4966f35630a0..95034440c84f931bdf47552b499e0fdc6f726e59 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -160,7 +160,7 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed)
>  	u32		status;
>  	unsigned long	flags;
>  
> -	usbip_dbg_vhci_rh("rh_port_connect %d\n", rhport);
> +	usbip_dbg_vhci_rh("%s %d\n", __func__, rhport);

So now you have __func__ twice in the log (hint, pr_debug() provides you
the function name.)

For "trace" debugging lines like this, just remove them, ftrace is
better to use instead.


>  
>  	spin_lock_irqsave(&vhci->lock, flags);
>  
> @@ -194,7 +194,7 @@ static void rh_port_disconnect(struct vhci_device *vdev)
>  	u32		status;
>  	unsigned long	flags;
>  
> -	usbip_dbg_vhci_rh("rh_port_disconnect %d\n", rhport);
> +	usbip_dbg_vhci_rh("%s %d\n", __func__, rhport);

Can be removed.

>  
>  	spin_lock_irqsave(&vhci->lock, flags);
>  
> @@ -1172,7 +1172,7 @@ static int vhci_start(struct usb_hcd *hcd)
>  	int id, rhport;
>  	int err;
>  
> -	usbip_dbg_vhci_hc("enter vhci_start\n");
> +	usbip_dbg_vhci_hc("enter %s\n", __func__);

Also removed.

>  
>  	if (usb_hcd_is_primary_hcd(hcd))
>  		spin_lock_init(&vhci_hcd->vhci->lock);
> @@ -1299,7 +1299,7 @@ static int vhci_alloc_streams(struct usb_hcd *hcd, struct usb_device *udev,
>  			      struct usb_host_endpoint **eps, unsigned int num_eps,
>  			      unsigned int num_streams, gfp_t mem_flags)
>  {
> -	dev_dbg(&hcd->self.root_hub->dev, "vhci_alloc_streams not implemented\n");
> +	dev_dbg(&hcd->self.root_hub->dev, "%s not implemented\n", __func__);

Just drop __func__ as it's now duplicated.

>  	return 0;
>  }
>  
> @@ -1308,7 +1308,7 @@ static int vhci_free_streams(struct usb_hcd *hcd, struct usb_device *udev,
>  			     struct usb_host_endpoint **eps, unsigned int num_eps,
>  	gfp_t mem_flags)
>  {
> -	dev_dbg(&hcd->self.root_hub->dev, "vhci_free_streams not implemented\n");
> +	dev_dbg(&hcd->self.root_hub->dev, "%s not implemented\n", __func__);

Same here.

thanks,

greg k-h

Re: [PATCH 2/9] usb: vhci-hcd: Fix space, brace, alignment and line length issues

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 06:54:51PM +0300, Cristian Ciocaltea wrote:
> Perform a first round of coding style cleanup:
> 
> * Add new lines after several statement blocks
> * Avoid line wrapping when 100-column width is not exceeded and it helps
>   improve code readability
> * Ensure lines do not end with '('
> * Drop superfluous spaces or empty lines
> * Add spaces where necessary, e.g. around operators
> * Add braces for single if-statements when at least one branch of the
>   conditional requires them
> 
> This helps getting rid of the following checkpatch complaints:
> 
>   CHECK: Lines should not end with a '('
>   CHECK: braces {} should be used on all arms of this statement
>   CHECK: Unbalanced braces around else statement
>   CHECK: Blank lines aren't necessary before a close brace '}'
>   CHECK: Unnecessary parentheses around
>   CHECK: Alignment should match open parenthesis
>   CHECK: No space is necessary after a cast
>   CHECK: spaces preferred around that '-' (ctx:VxV)
>   CHECK: spaces preferred around that '+' (ctx:VxV)
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Coding style cleanups need to be "one patch per logical change", not
"fix them all in one patch!" type of thing.

Sorry, but can you break this out better?

thanks,

greg k-h

Re: [PATCH 6/9] usb: vhci-hcd: Fix block comments

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 06:54:55PM +0300, Cristian Ciocaltea wrote:
> Address a couple of comment formatting issues indicated by
> checkpatch.pl:
> 
>   WARNING: Block comments use a trailing */ on a separate line
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 53691d5e77d386b0b0e16fe9d08824baa04c0b1e..a2a8418c77e58ae9e06d673d0012b972c92ee33b 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -879,7 +879,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  	priv = urb->hcpriv;
>  	if (!priv) {
>  		/* URB was never linked! or will be soon given back by
> -		 * vhci_rx. */
> +		 * vhci_rx.
> +		 */

The comment should just be one line long.

>  		spin_unlock_irqrestore(&vhci->lock, flags);
>  		return -EIDRM;
>  	}
> @@ -936,7 +937,8 @@ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
>  		unlink->unlink_seqnum = priv->seqnum;
>  
>  		/* send cmd_unlink and try to cancel the pending URB in the
> -		 * peer */
> +		 * peer
> +		 */

Same here.

thanks,

greg k-h

Re: [PATCH 5/9] usb: vhci-hcd: Do not split quoted strings

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 06:54:54PM +0300, Cristian Ciocaltea wrote:
> Join the split strings to make checkpatch happy and regain ability to
> grep for those log messages in the source code:
> 
>   WARNING: quoted string split across lines
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 3f888455d238b983a6aafa52418fb89a914c32b5..53691d5e77d386b0b0e16fe9d08824baa04c0b1e 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -376,8 +376,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  		switch (wValue) {
>  		case USB_PORT_FEAT_SUSPEND:
>  			if (hcd->speed >= HCD_USB3) {
> -				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not "
> -				       "supported for USB 3.0 roothub\n");
> +				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not supported for USB 3.0 roothub\n");

Why the leading " "?

And shouldn't this be dev_err()?

>  				goto error;
>  			}
>  
> @@ -506,8 +505,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>  		case USB_PORT_FEAT_LINK_STATE:
>  			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_LINK_STATE\n");
>  			if (hcd->speed < HCD_USB3) {
> -				pr_err("USB_PORT_FEAT_LINK_STATE req not "
> -				       "supported for USB 2.0 roothub\n");
> +				pr_err("USB_PORT_FEAT_LINK_STATE req not supported for USB 2.0 roothub\n");

dev_err()?

Same for the others.

thanks,

greg k-h

Re: [PATCH 2/9] usb: vhci-hcd: Fix space, brace, alignment and line length issues

[Cristian Ciocaltea]

On 7/17/25 7:18 PM, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 06:54:51PM +0300, Cristian Ciocaltea wrote:
>> Perform a first round of coding style cleanup:
>>
>> * Add new lines after several statement blocks
>> * Avoid line wrapping when 100-column width is not exceeded and it helps
>>   improve code readability
>> * Ensure lines do not end with '('
>> * Drop superfluous spaces or empty lines
>> * Add spaces where necessary, e.g. around operators
>> * Add braces for single if-statements when at least one branch of the
>>   conditional requires them
>>
>> This helps getting rid of the following checkpatch complaints:
>>
>>   CHECK: Lines should not end with a '('
>>   CHECK: braces {} should be used on all arms of this statement
>>   CHECK: Unbalanced braces around else statement
>>   CHECK: Blank lines aren't necessary before a close brace '}'
>>   CHECK: Unnecessary parentheses around
>>   CHECK: Alignment should match open parenthesis
>>   CHECK: No space is necessary after a cast
>>   CHECK: spaces preferred around that '-' (ctx:VxV)
>>   CHECK: spaces preferred around that '+' (ctx:VxV)
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> Coding style cleanups need to be "one patch per logical change", not
> "fix them all in one patch!" type of thing.
> 
> Sorry, but can you break this out better?

I could split this into something like:

- Fix spaces & blank lines
  CHECK: Blank lines aren't necessary before a close brace '}'
  CHECK: No space is necessary after a cast
  CHECK: spaces preferred around that '-' (ctx:VxV)
  CHECK: spaces preferred around that '+' (ctx:VxV)

- Fix braces
  CHECK: braces {} should be used on all arms of this statement
  CHECK: Unbalanced braces around else statement

- Fix alignment & line length
  CHECK: Lines should not end with a '('
  CHECK: Alignment should match open parenthesis
  
- Misc?!
  CHECK: Unnecessary parentheses around

Thanks,
Cristian

Re: [PATCH 5/9] usb: vhci-hcd: Do not split quoted strings

[Cristian Ciocaltea]

On 7/17/25 7:19 PM, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 06:54:54PM +0300, Cristian Ciocaltea wrote:
>> Join the split strings to make checkpatch happy and regain ability to
>> grep for those log messages in the source code:
>>
>>   WARNING: quoted string split across lines
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/usb/usbip/vhci_hcd.c | 21 +++++++--------------
>>  1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index 3f888455d238b983a6aafa52418fb89a914c32b5..53691d5e77d386b0b0e16fe9d08824baa04c0b1e 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -376,8 +376,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>  		switch (wValue) {
>>  		case USB_PORT_FEAT_SUSPEND:
>>  			if (hcd->speed >= HCD_USB3) {
>> -				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not "
>> -				       "supported for USB 3.0 roothub\n");
>> +				pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not supported for USB 3.0 roothub\n");
> 
> Why the leading " "?

Yeah, not sure if that was on purpose, but I agree it doesn't make much
sense.  Will fix this up. 

> And shouldn't this be dev_err()?

I'll add a separate patch to convert all pr_*() instances, as there are
plenty of them.

>>  				goto error;
>>  			}
>>  
>> @@ -506,8 +505,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
>>  		case USB_PORT_FEAT_LINK_STATE:
>>  			usbip_dbg_vhci_rh(" SetPortFeature: USB_PORT_FEAT_LINK_STATE\n");
>>  			if (hcd->speed < HCD_USB3) {
>> -				pr_err("USB_PORT_FEAT_LINK_STATE req not "
>> -				       "supported for USB 2.0 roothub\n");
>> +				pr_err("USB_PORT_FEAT_LINK_STATE req not supported for USB 2.0 roothub\n");
> 
> dev_err()?
> 
> Same for the others.
> 
> thanks,
> 
> greg k-h

Re: [PATCH 8/9] usb: vhci-hcd: Consistently use __func__

[Cristian Ciocaltea]

On 7/17/25 7:18 PM, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 06:54:57PM +0300, Cristian Ciocaltea wrote:
>> Replace all explicit function names in string literals with __func__ and
>> silent several checkpatch complaints similar to the following one:
>>
>>   WARNING: Prefer using '"%s...", __func__' to using 'vhci_start', this function's name, in a string
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/usb/usbip/vhci_hcd.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index 841902482fb15d1d86525f23492e4966f35630a0..95034440c84f931bdf47552b499e0fdc6f726e59 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -160,7 +160,7 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed)
>>  	u32		status;
>>  	unsigned long	flags;
>>  
>> -	usbip_dbg_vhci_rh("rh_port_connect %d\n", rhport);
>> +	usbip_dbg_vhci_rh("%s %d\n", __func__, rhport);
> 
> So now you have __func__ twice in the log (hint, pr_debug() provides you
> the function name.)
> 
> For "trace" debugging lines like this, just remove them, ftrace is
> better to use instead.

I should probably fix the whole logging stuff as part of the pr_*() -> dev_*() 
conversion.

Thanks,
Cristian

Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Alan Stern]

On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
> The VHCI platform driver aims to forbid entering system suspend when at
> least one of the virtual USB ports are bound to an active USB/IP
> connection.
> 
> However, in some cases, the detection logic doesn't work reliably, i.e.
> when all devices attached to the virtual root hub have been already
> suspended, leading to a broken suspend state, with unrecoverable resume.
> 
> Ensure the attached devices do not enter suspend by setting the syscore
> PM flag.
> 
> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  				 ctrlreq->wValue, vdev->rhport);
>  
>  			vdev->udev = usb_get_dev(urb->dev);
> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>  			usb_put_dev(old);
>  
>  			spin_lock(&vdev->ud.lock);
> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>  
>  			vdev->udev = usb_get_dev(urb->dev);
> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>  			usb_put_dev(old);
>  			goto out;

This looks very strange indeed.

First, why is vhci_urb_enqueue() the right place to do this?  I should 
think you would want to do this just once per device, at the time it is 
attached.  Not every time a new URB is enqueued.

Second, how do these devices ever go back to being regular non-syscore 
things?

Third, if this change isn't merely a temporary placeholder, it certainly 
needs to have a comment in the code to explain what it does and why.

Fourth, does calling dev_pm_syscore_device() really prevent the device 
from going into suspend?  What about runtime suspend?  And what good 
does it to do prevent the device from being suspended if the entire 
server gets suspended?

Fifth, the patch description says the purpose is to prevent the server 
from going into system suspend.  How does marking some devices with 
dev_pm_syscore_device() accomplish this?

Alan Stern

Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Shuah Khan]

On 7/17/25 12:26, Alan Stern wrote:
> On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
>> The VHCI platform driver aims to forbid entering system suspend when at
>> least one of the virtual USB ports are bound to an active USB/IP
>> connection.
>>
>> However, in some cases, the detection logic doesn't work reliably, i.e.
>> when all devices attached to the virtual root hub have been already
>> suspended, leading to a broken suspend state, with unrecoverable resume.
>>
>> Ensure the attached devices do not enter suspend by setting the syscore
>> PM flag.
>>
>> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>   drivers/usb/usbip/vhci_hcd.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
>> --- a/drivers/usb/usbip/vhci_hcd.c
>> +++ b/drivers/usb/usbip/vhci_hcd.c
>> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   				 ctrlreq->wValue, vdev->rhport);
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   
>>   			spin_lock(&vdev->ud.lock);
>> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>   					"Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>>   
>>   			vdev->udev = usb_get_dev(urb->dev);
>> +			dev_pm_syscore_device(&vdev->udev->dev, true);
>>   			usb_put_dev(old);
>>   			goto out;
> 
> This looks very strange indeed.
> 
> First, why is vhci_urb_enqueue() the right place to do this?  I should
> think you would want to do this just once per device, at the time it is
> attached.  Not every time a new URB is enqueued.

Correct. This isn't the right place to do this even if we want to go with
the option to prevent suspend. The possible place to do this would be
from rh_port_connect() in which case you will have access to usb_hcd device.

This has to be undone from rh_port_disconnect(). Also how does this impact
the usbip_host - we still need to handle usbip_host suspend.

> 
> Second, how do these devices ever go back to being regular non-syscore
> things?
> 
> Third, if this change isn't merely a temporary placeholder, it certainly
> needs to have a comment in the code to explain what it does and why.
> 
> Fourth, does calling dev_pm_syscore_device() really prevent the device
> from going into suspend?  What about runtime suspend?  And what good
> does it to do prevent the device from being suspended if the entire
> server gets suspended?
> 
> Fifth, the patch description says the purpose is to prevent the server
> from going into system suspend.  How does marking some devices with
> dev_pm_syscore_device() accomplish this?
> 

We have been discussing suspend/resume and reboot behavior in another thread
that proposed converting vhci_hcd to use faux bus.

In addition to what Alan is asking, To handle suspend/resume cleanly, the
following has to happen at a higher level:

- Let the usbip hots host know client is suspending the connection.
   The physical device isn't suspended on the host.
- suspend the virtual devices and vhci_hcd

Do the reverse to resume.

I would say:

- We don't want vhci_hcd and usbip_host preventing suspend
- It might be cleaner and safer to detach the devices during
   suspend on both ends. This is similar to what happens now when
   usbip host and vhci_hcd are removed.
- Note that usbip_host and vhci_hcd don't fully support suspend and
   resume at the moment.

thanks,
-- Shuah

Re: [PATCH 2/9] usb: vhci-hcd: Fix space, brace, alignment and line length issues

[Greg Kroah-Hartman]

On Thu, Jul 17, 2025 at 08:26:54PM +0300, Cristian Ciocaltea wrote:
> On 7/17/25 7:18 PM, Greg Kroah-Hartman wrote:
> > On Thu, Jul 17, 2025 at 06:54:51PM +0300, Cristian Ciocaltea wrote:
> >> Perform a first round of coding style cleanup:
> >>
> >> * Add new lines after several statement blocks
> >> * Avoid line wrapping when 100-column width is not exceeded and it helps
> >>   improve code readability
> >> * Ensure lines do not end with '('
> >> * Drop superfluous spaces or empty lines
> >> * Add spaces where necessary, e.g. around operators
> >> * Add braces for single if-statements when at least one branch of the
> >>   conditional requires them
> >>
> >> This helps getting rid of the following checkpatch complaints:
> >>
> >>   CHECK: Lines should not end with a '('
> >>   CHECK: braces {} should be used on all arms of this statement
> >>   CHECK: Unbalanced braces around else statement
> >>   CHECK: Blank lines aren't necessary before a close brace '}'
> >>   CHECK: Unnecessary parentheses around
> >>   CHECK: Alignment should match open parenthesis
> >>   CHECK: No space is necessary after a cast
> >>   CHECK: spaces preferred around that '-' (ctx:VxV)
> >>   CHECK: spaces preferred around that '+' (ctx:VxV)
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > 
> > Coding style cleanups need to be "one patch per logical change", not
> > "fix them all in one patch!" type of thing.
> > 
> > Sorry, but can you break this out better?
> 
> I could split this into something like:
> 
> - Fix spaces & blank lines
>   CHECK: Blank lines aren't necessary before a close brace '}'
>   CHECK: No space is necessary after a cast
>   CHECK: spaces preferred around that '-' (ctx:VxV)
>   CHECK: spaces preferred around that '+' (ctx:VxV)
> 
> - Fix braces
>   CHECK: braces {} should be used on all arms of this statement
>   CHECK: Unbalanced braces around else statement
> 
> - Fix alignment & line length
>   CHECK: Lines should not end with a '('
>   CHECK: Alignment should match open parenthesis
>   
> - Misc?!
>   CHECK: Unnecessary parentheses around

Why not one per CHECK: type?

Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Cristian Ciocaltea]

Hi Alan, Shuah,

On 7/17/25 10:55 PM, Shuah Khan wrote:
> On 7/17/25 12:26, Alan Stern wrote:
>> On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
>>> The VHCI platform driver aims to forbid entering system suspend when at
>>> least one of the virtual USB ports are bound to an active USB/IP
>>> connection.
>>>
>>> However, in some cases, the detection logic doesn't work reliably, i.e.
>>> when all devices attached to the virtual root hub have been already
>>> suspended, leading to a broken suspend state, with unrecoverable resume.
>>>
>>> Ensure the attached devices do not enter suspend by setting the syscore
>>> PM flag.
>>>
>>> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>> ---
>>>   drivers/usb/usbip/vhci_hcd.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>>> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
>>> --- a/drivers/usb/usbip/vhci_hcd.c
>>> +++ b/drivers/usb/usbip/vhci_hcd.c
>>> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>>                    ctrlreq->wValue, vdev->rhport);
>>>                 vdev->udev = usb_get_dev(urb->dev);
>>> +            dev_pm_syscore_device(&vdev->udev->dev, true);
>>>               usb_put_dev(old);
>>>                 spin_lock(&vdev->ud.lock);
>>> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>>                       "Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>>>                 vdev->udev = usb_get_dev(urb->dev);
>>> +            dev_pm_syscore_device(&vdev->udev->dev, true);
>>>               usb_put_dev(old);
>>>               goto out;
>>
>> This looks very strange indeed.
>>
>> First, why is vhci_urb_enqueue() the right place to do this?  I should
>> think you would want to do this just once per device, at the time it is
>> attached.  Not every time a new URB is enqueued.
> 
> Correct. This isn't the right place to do this even if we want to go with
> the option to prevent suspend. The possible place to do this would be
> from rh_port_connect() in which case you will have access to usb_hcd device.

Oh, I chose to handle this in vhci_urb_enqueue() as it seemed to be the only
place where vdev->udev gets assigned.  Now I wonder if that assignment
should really be here, but probably I'm missing something obvious as I'm
still in the process of getting familiar with the code base.

> This has to be undone from rh_port_disconnect(). Also how does this impact
> the usbip_host - we still need to handle usbip_host suspend.

I've only addressed usbip_vhci suspend prevention at the moment, as that was
supposed to work.

>>
>> Second, how do these devices ever go back to being regular non-syscore
>> things?

This only handles the client side, i.e. the virtually attached devices,
hence I didn't pay much attention to undo the syscore thing (wasn't
straightforward to accomplish via the URB handling path anyway).  I'll
definitely fix this up by moving to rh_port_[dis]connect(), as Shuah
suggested.

>> Third, if this change isn't merely a temporary placeholder, it certainly
>> needs to have a comment in the code to explain what it does and why.

Indeed, will document this.

>> Fourth, does calling dev_pm_syscore_device() really prevent the device
>> from going into suspend?  

Yes, this is managed by core PM infra which basically skips processing of
any PM callbacks when the flag is set - e.g. see how dev->power.syscore is
handled in device_suspend():

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c?h=v6.16-rc6#n1800

>> What about runtime suspend?  

I think that's a slightly different topic - so far I've been only focused on
system suspend.

> And what good
>> does it to do prevent the device from being suspended if the entire
>> server gets suspended?

As mentioned above, the target for now is to unbreak the system suspend
prevention on the client side.  The server side doesn't seem to support it.

>>
>> Fifth, the patch description says the purpose is to prevent the server
>> from going into system suspend.  

Hmm, I might need to improve the description in this case, as I only
mentioned VHCI and the virtual hub/ports, hence I was referring to the
client side only.

>> How does marking some devices with
>> dev_pm_syscore_device() accomplish this?

Please check the link above.

> 
> We have been discussing suspend/resume and reboot behavior in another thread
> that proposed converting vhci_hcd to use faux bus.
> 
> In addition to what Alan is asking, To handle suspend/resume cleanly, the
> following has to happen at a higher level:
> 
> - Let the usbip hots host know client is suspending the connection.
>   The physical device isn't suspended on the host.
> - suspend the virtual devices and vhci_hcd
> 
> Do the reverse to resume.

Right, I was actually looking into having a proper suspend/resume support
rather than just preventing it on the client side (for now), but that's
clearly not an easy task to accomplish, as it requires extending the USP/IP
protocol and most probably also the user space tools.

> 
> I would say:
> 
> - We don't want vhci_hcd and usbip_host preventing suspend

That's understandable, but currently vhci_hcd is supposed to prevent
suspend, which mostly works but it's unreliable, hence my initial goal was
to provide a simple fix for it before attempting to experiment with more
invasive changes.

> - It might be cleaner and safer to detach the devices during
>   suspend on both ends. This is similar to what happens now when
>   usbip host and vhci_hcd are removed.
> - Note that usbip_host and vhci_hcd don't fully support suspend and
>   resume at the moment.

Thank you both for the initial feedback!

Regards,
Cristian

Re: [PATCH 2/9] usb: vhci-hcd: Fix space, brace, alignment and line length issues

[Cristian Ciocaltea]

On 7/18/25 9:26 AM, Greg Kroah-Hartman wrote:
> On Thu, Jul 17, 2025 at 08:26:54PM +0300, Cristian Ciocaltea wrote:
>> On 7/17/25 7:18 PM, Greg Kroah-Hartman wrote:
>>> On Thu, Jul 17, 2025 at 06:54:51PM +0300, Cristian Ciocaltea wrote:
>>>> Perform a first round of coding style cleanup:
>>>>
>>>> * Add new lines after several statement blocks
>>>> * Avoid line wrapping when 100-column width is not exceeded and it helps
>>>>   improve code readability
>>>> * Ensure lines do not end with '('
>>>> * Drop superfluous spaces or empty lines
>>>> * Add spaces where necessary, e.g. around operators
>>>> * Add braces for single if-statements when at least one branch of the
>>>>   conditional requires them
>>>>
>>>> This helps getting rid of the following checkpatch complaints:
>>>>
>>>>   CHECK: Lines should not end with a '('
>>>>   CHECK: braces {} should be used on all arms of this statement
>>>>   CHECK: Unbalanced braces around else statement
>>>>   CHECK: Blank lines aren't necessary before a close brace '}'
>>>>   CHECK: Unnecessary parentheses around
>>>>   CHECK: Alignment should match open parenthesis
>>>>   CHECK: No space is necessary after a cast
>>>>   CHECK: spaces preferred around that '-' (ctx:VxV)
>>>>   CHECK: spaces preferred around that '+' (ctx:VxV)
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>
>>> Coding style cleanups need to be "one patch per logical change", not
>>> "fix them all in one patch!" type of thing.
>>>
>>> Sorry, but can you break this out better?
>>
>> I could split this into something like:
>>
>> - Fix spaces & blank lines
>>   CHECK: Blank lines aren't necessary before a close brace '}'
>>   CHECK: No space is necessary after a cast
>>   CHECK: spaces preferred around that '-' (ctx:VxV)
>>   CHECK: spaces preferred around that '+' (ctx:VxV)
>>
>> - Fix braces
>>   CHECK: braces {} should be used on all arms of this statement
>>   CHECK: Unbalanced braces around else statement
>>
>> - Fix alignment & line length
>>   CHECK: Lines should not end with a '('
>>   CHECK: Alignment should match open parenthesis
>>   
>> - Misc?!
>>   CHECK: Unnecessary parentheses around
> 
> Why not one per CHECK: type?

I've been trying to squash all formatting changes which are kind of similar,
but sure I can handle them separately.

Thanks,
Cristian

Re: [PATCH 1/9] usb: vhci-hcd: Prevent suspending virtually attached devices

[Cristian Ciocaltea]

On 7/18/25 9:41 AM, Cristian Ciocaltea wrote:
> Hi Alan, Shuah,
> 
> On 7/17/25 10:55 PM, Shuah Khan wrote:
>> On 7/17/25 12:26, Alan Stern wrote:
>>> On Thu, Jul 17, 2025 at 06:54:50PM +0300, Cristian Ciocaltea wrote:
>>>> The VHCI platform driver aims to forbid entering system suspend when at
>>>> least one of the virtual USB ports are bound to an active USB/IP
>>>> connection.
>>>>
>>>> However, in some cases, the detection logic doesn't work reliably, i.e.
>>>> when all devices attached to the virtual root hub have been already
>>>> suspended, leading to a broken suspend state, with unrecoverable resume.
>>>>
>>>> Ensure the attached devices do not enter suspend by setting the syscore
>>>> PM flag.
>>>>
>>>> Fixes: 04679b3489e0 ("Staging: USB/IP: add client driver")
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>>> ---
>>>>   drivers/usb/usbip/vhci_hcd.c | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>>>> index e70fba9f55d6a0edf3c5fde56a614dd3799406a1..762b60e10a9415e58147cde2f615045da5804a0e 100644
>>>> --- a/drivers/usb/usbip/vhci_hcd.c
>>>> +++ b/drivers/usb/usbip/vhci_hcd.c
>>>> @@ -765,6 +765,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>>>                    ctrlreq->wValue, vdev->rhport);
>>>>                 vdev->udev = usb_get_dev(urb->dev);
>>>> +            dev_pm_syscore_device(&vdev->udev->dev, true);
>>>>               usb_put_dev(old);
>>>>                 spin_lock(&vdev->ud.lock);
>>>> @@ -785,6 +786,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>>>                       "Not yet?:Get_Descriptor to device 0 (get max pipe size)\n");
>>>>                 vdev->udev = usb_get_dev(urb->dev);
>>>> +            dev_pm_syscore_device(&vdev->udev->dev, true);
>>>>               usb_put_dev(old);
>>>>               goto out;
>>>
>>> This looks very strange indeed.
>>>
>>> First, why is vhci_urb_enqueue() the right place to do this?  I should
>>> think you would want to do this just once per device, at the time it is
>>> attached.  Not every time a new URB is enqueued.
>>
>> Correct. This isn't the right place to do this even if we want to go with
>> the option to prevent suspend. The possible place to do this would be
>> from rh_port_connect() in which case you will have access to usb_hcd device.
> 
> Oh, I chose to handle this in vhci_urb_enqueue() as it seemed to be the only
> place where vdev->udev gets assigned.  Now I wonder if that assignment
> should really be here, but probably I'm missing something obvious as I'm
> still in the process of getting familiar with the code base.

After digging a bit further into the process of establishing a virtual
connection, I concluded rh_port_connect() cannot be used for the syscore
flag setup since the usb_device instance part of struct vhci_device
(vdev->udev here) is not yet initialized at that time.  That is because this
function is called right after the userspace tool responsible for initiating
a new USB/IP attachment writes the remote device information into sysfs -
see attach_store() in drivers/usb/usbip/vhci_sysfs.c.

The earliest execution context providing access to usb_device seems to be
during the enumeration process, which would explain why the vdev->udev
assignment has been done in vhci_urb_enqueue().  This should be normally
needed only when handling USB_REQ_GET_DESCRIPTOR, but for some reason
there's a reassignment in the USB_REQ_SET_ADDRESS handler as well.  I'd
assume it's possible that usb_device instance may change after
USB_REQ_GET_DESCRIPTOR, which is supposed to always precede
USB_REQ_SET_ADDRESS.

However, in all my tests done so far the operations where performed on the
same instance, hence I'm not sure if this is a real possibility or just a
leftover from downstream/experimental code base.

To remain on the safe side, I'll keep both calls to dev_pm_syscore_device(),
while adding in each case a comment on top with the relevant information. 

>> This has to be undone from rh_port_disconnect(). Also how does this impact
>> the usbip_host - we still need to handle usbip_host suspend.
> 
> I've only addressed usbip_vhci suspend prevention at the moment, as that was
> supposed to work.
> 
>>>
>>> Second, how do these devices ever go back to being regular non-syscore
>>> things?
> 
> This only handles the client side, i.e. the virtually attached devices,
> hence I didn't pay much attention to undo the syscore thing (wasn't
> straightforward to accomplish via the URB handling path anyway).  I'll
> definitely fix this up by moving to rh_port_[dis]connect(), as Shuah
> suggested.
> 
>>> Third, if this change isn't merely a temporary placeholder, it certainly
>>> needs to have a comment in the code to explain what it does and why.
> 
> Indeed, will document this.
> 
>>> Fourth, does calling dev_pm_syscore_device() really prevent the device
>>> from going into suspend?  
> 
> Yes, this is managed by core PM infra which basically skips processing of
> any PM callbacks when the flag is set - e.g. see how dev->power.syscore is
> handled in device_suspend():
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/base/power/main.c?h=v6.16-rc6#n1800
> 
>>> What about runtime suspend?  
> 
> I think that's a slightly different topic - so far I've been only focused on
> system suspend.
> 
>> And what good
>>> does it to do prevent the device from being suspended if the entire
>>> server gets suspended?
> 
> As mentioned above, the target for now is to unbreak the system suspend
> prevention on the client side.  The server side doesn't seem to support it.
> 
>>>
>>> Fifth, the patch description says the purpose is to prevent the server
>>> from going into system suspend.  
> 
> Hmm, I might need to improve the description in this case, as I only
> mentioned VHCI and the virtual hub/ports, hence I was referring to the
> client side only.
> 
>>> How does marking some devices with
>>> dev_pm_syscore_device() accomplish this?
> 
> Please check the link above.
> 
>>
>> We have been discussing suspend/resume and reboot behavior in another thread
>> that proposed converting vhci_hcd to use faux bus.
>>
>> In addition to what Alan is asking, To handle suspend/resume cleanly, the
>> following has to happen at a higher level:
>>
>> - Let the usbip hots host know client is suspending the connection.
>>   The physical device isn't suspended on the host.
>> - suspend the virtual devices and vhci_hcd
>>
>> Do the reverse to resume.
> 
> Right, I was actually looking into having a proper suspend/resume support
> rather than just preventing it on the client side (for now), but that's
> clearly not an easy task to accomplish, as it requires extending the USP/IP
> protocol and most probably also the user space tools.
> 
>>
>> I would say:
>>
>> - We don't want vhci_hcd and usbip_host preventing suspend
> 
> That's understandable, but currently vhci_hcd is supposed to prevent
> suspend, which mostly works but it's unreliable, hence my initial goal was
> to provide a simple fix for it before attempting to experiment with more
> invasive changes.
> 
>> - It might be cleaner and safer to detach the devices during
>>   suspend on both ends. This is similar to what happens now when
>>   usbip host and vhci_hcd are removed.
>> - Note that usbip_host and vhci_hcd don't fully support suspend and
>>   resume at the moment.
> 
> Thank you both for the initial feedback!
> 
> Regards,
> Cristian
>