[PATCH v2 0/4] Enable to set DbC strings through sysfs

[PATCH v2 0/4] Enable to set DbC strings through sysfs

[Łukasz Bartosik]

From: Łukasz Bartosik <ukaszb@chromium.org>

This patchset enables setting DbC serial number,
product name and manufacturer name through sysfs.

Testing performed with this patchset:

1.DbC is enabled and enumerates on host side with the following
default values of product, manufactuer and serial values:
"
[496803.112431] usb 2-4: new SuperSpeed USB device number 106 using xhci_hcd
[496803.128540] usb 2-4: LPM exit latency is zeroed, disabling LPM.
[496803.129387] usb 2-4: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[496803.130173] usb 2-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[496803.130858] usb 2-4: Product: Linux USB Debug Target
[496803.131343] usb 2-4: Manufacturer: Linux Foundation
[496803.131821] usb 2-4: SerialNumber: 0001
```

View default DbC values in sysfs:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
Linux USB Debug Target
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
Linux Foundation
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
0001
"

2. Try to set product, manufacturer and serial to empty:

echo disable > /sys/bus/pci/devices/0000:00:14.0/dbc
echo "" > /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
echo "" > /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
echo "" > /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial  
echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

Verify through sysfs empty values were not set:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
Linux USB Debug Target
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
Linux Foundation
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
0001
"

Verify DbC enumerates with default values:
"
[206723.036606] usb 2-3: new SuperSpeed USB device number 52 using xhci_hcd
[206723.056594] usb 2-3: LPM exit latency is zeroed, disabling LPM.
[206723.057795] usb 2-3: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[206723.058592] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[206723.059289] usb 2-3: Product: Linux USB Debug Target
[206723.059796] usb 2-3: Manufacturer: Linux Foundation
[206723.060284] usb 2-3: SerialNumber: 0001
"


3. Update product, manufacturer and serial values:

echo disable > /sys/bus/pci/devices/0000:00:14.0/dbc
echo "A" > /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
echo "B" > /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
echo "C" > /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

Verify through sysfs new values were set:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
A
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
B
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
C
"

Verify DbC enumerates with new values:
"
[222501.763069] usb 2-3: new SuperSpeed USB device number 66 using xhci_hcd
[222501.786948] usb 2-3: LPM exit latency is zeroed, disabling LPM.
[222501.801052] usb 2-3: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[222501.801857] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[222501.802556] usb 2-3: Product: A
[222501.802958] usb 2-3: Manufacturer: B
[222501.803329] usb 2-3: SerialNumber: C
"



4. Update product, manufacturer and serial values:

echo disable > /sys/bus/pci/devices/0000:00:14.0/dbc
echo "New_product_name" > /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
echo "New_manufacturer_name" > /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
echo "ABCDEF123456" > /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

Verify through sysfs new values were set:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
New_product_name
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
New_manufacturer_name
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
ABCDEF123456
"

Verify DbC enumerates with new values:
"
[222740.958461] usb 2-3: new SuperSpeed USB device number 67 using xhci_hcd
[222740.974545] usb 2-3: LPM exit latency is zeroed, disabling LPM.
[222740.975442] usb 2-3: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[222740.976236] usb 2-3: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[222740.976929] usb 2-3: Product: New_product_name
[222740.977372] usb 2-3: Manufacturer: New_manufacturer_name
[222740.977896] usb 2-3: SerialNumber: ABCDEF123456
"


5. Try to update product, manufacturer and serial values with new values longer
than maximum 63 characters (half of USB_MAX_STRING_LEN):

echo disable > /sys/bus/pci/devices/0000:00:14.0/dbc
echo "AAAAAAAAA_BBBBBBBBB_CCCCCCCCC_DDDDDDDDD_EEEEEEEEE_FFFFFFFFF_GGGG" > /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct                   
echo "HHHHHHHHH_IIIIIIIII_JJJJJJJJJ_KKKKKKKKK_LLLLLLLLL_MMMMMMMMM_NNNN" > /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer              
echo "OOOOOOOOO_PPPPPPPPP_RRRRRRRRR_SSSSSSSSS_TTTTTTTTT_WWWWWWWWW_YYYY" > /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial  
echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

Verify through sysfs new values were not set:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
New_product_name
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
New_manufacturer_name
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
ABCDEF123456
"

Verify DbC enumerates with previous values:
"
[497908.814834] usb 2-4: new SuperSpeed USB device number 108 using xhci_hcd
[497908.831057] usb 2-4: LPM exit latency is zeroed, disabling LPM.
[497908.844994] usb 2-4: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[497908.845797] usb 2-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[497908.846482] usb 2-4: Product: New_product_name
[497908.846965] usb 2-4: Manufacturer: New_manufacturer_name
[497908.847505] usb 2-4: SerialNumber: ABCDEF123456
"


6. Update product, manufacturer and serial values with new values
whose length is maximum 63 characters (half of USB_MAX_STRING_LEN):

echo disable > /sys/bus/pci/devices/0000:00:14.0/dbc
echo "AAAAAAAAA_BBBBBBBBB_CCCCCCCCC_DDDDDDDDD_EEEEEEEEE_FFFFFFFFF_GGG" > /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct                    
echo "HHHHHHHHH_IIIIIIIII_JJJJJJJJJ_KKKKKKKKK_LLLLLLLLL_MMMMMMMMM_NNN" > /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer               
echo "OOOOOOOOO_PPPPPPPPP_RRRRRRRRR_SSSSSSSSS_TTTTTTTTT_WWWWWWWWW_YYY" > /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial  
echo enable > /sys/bus/pci/devices/0000:00:14.0/dbc

Verify through sysfs new values were set:
"
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iProduct
AAAAAAAAA_BBBBBBBBB_CCCCCCCCC_DDDDDDDDD_EEEEEEEEE_FFFFFFFFF_GGG
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iManufacturer
HHHHHHHHH_IIIIIIIII_JJJJJJJJJ_KKKKKKKKK_LLLLLLLLL_MMMMMMMMM_NNN
cat /sys/bus/pci/devices/0000:00:14.0/dbc_iSerial
OOOOOOOOO_PPPPPPPPP_RRRRRRRRR_SSSSSSSSS_TTTTTTTTT_WWWWWWWWW_YYY
"

Verify DbC enumerates with new values:
"
[499856.473572] usb 2-4: new SuperSpeed USB device number 119 using xhci_hcd
[499856.489786] usb 2-4: LPM exit latency is zeroed, disabling LPM.
[499856.492080] usb 2-4: New USB device found, idVendor=18d1, idProduct=0010, bcdDevice= 0.10
[499856.492871] usb 2-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3
[499856.493624] usb 2-4: Product: AAAAAAAAA_BBBBBBBBB_CCCCCCCCC_DDDDDDDDD_EEEEEEEEE_FFFFFFFFF_GGG
[499856.494445] usb 2-4: Manufacturer: HHHHHHHHH_IIIIIIIII_JJJJJJJJJ_KKKKKKKKK_LLLLLLLLL_MMMMMMMMM_NNN
[499856.495307] usb 2-4: SerialNumber: OOOOOOOOO_PPPPPPPPP_RRRRRRRRR_SSSSSSSSS_TTTTTTTTT_WWWWWWWWW_YYY
"



7. Repeat steps 2-6 with passing -n option to each echo command to prevent outputting the trailing newline



Changes in V2:
- Documented new sysfs entries
- Fixed *_store functions to handle correctly case when there
is no trailing newline

Łukasz Bartosik (4):
  xhci: dbc: prepare to expose strings through sysfs
  xhci: dbc: allow to set serial number through sysfs
  xhci: dbc: allow to set product name through sysfs
  xhci: dbc: allow to set manufacturer name through sysfs

 .../testing/sysfs-bus-pci-drivers-xhci_hcd    |  36 +++
 drivers/usb/host/xhci-dbgcap.c                | 252 +++++++++++++-----
 drivers/usb/host/xhci-dbgcap.h                |  24 +-
 3 files changed, 237 insertions(+), 75 deletions(-)

-- 
2.51.0.710.ga91ca5db03-goog

[PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Łukasz Bartosik]

From: Łukasz Bartosik <ukaszb@chromium.org>

Reorganize the code to prepare ground for setting serial number,
product and manufacturer names through sysfs. This commit:
1. Introduces new buffers for storing serial number, product and
   manufacturer name in utf8. The buffers will be used by sysfs
   *_show and *_store functions.
2. Increases USB string descriptor data maximum length to the
   value from USB specification (126 bytes of data).
3. Adds new helper functions get_str_desc_len, prepare_len
   and xhci_dbc_populate_str_desc.

Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
---
 drivers/usb/host/xhci-dbgcap.c | 145 ++++++++++++++++++---------------
 drivers/usb/host/xhci-dbgcap.h |  24 ++++--
 2 files changed, 94 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 63edf2d8f245..c2fecaffd6f3 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -52,55 +52,6 @@ static void dbc_ring_free(struct device *dev, struct xhci_ring *ring)
 	kfree(ring);
 }
 
-static u32 xhci_dbc_populate_strings(struct dbc_str_descs *strings)
-{
-	struct usb_string_descriptor	*s_desc;
-	u32				string_length;
-
-	/* Serial string: */
-	s_desc = (struct usb_string_descriptor *)strings->serial;
-	utf8s_to_utf16s(DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL),
-			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
-			DBC_MAX_STRING_LENGTH);
-
-	s_desc->bLength		= (strlen(DBC_STRING_SERIAL) + 1) * 2;
-	s_desc->bDescriptorType	= USB_DT_STRING;
-	string_length		= s_desc->bLength;
-	string_length		<<= 8;
-
-	/* Product string: */
-	s_desc = (struct usb_string_descriptor *)strings->product;
-	utf8s_to_utf16s(DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT),
-			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
-			DBC_MAX_STRING_LENGTH);
-
-	s_desc->bLength		= (strlen(DBC_STRING_PRODUCT) + 1) * 2;
-	s_desc->bDescriptorType	= USB_DT_STRING;
-	string_length		+= s_desc->bLength;
-	string_length		<<= 8;
-
-	/* Manufacture string: */
-	s_desc = (struct usb_string_descriptor *)strings->manufacturer;
-	utf8s_to_utf16s(DBC_STRING_MANUFACTURER,
-			strlen(DBC_STRING_MANUFACTURER),
-			UTF16_LITTLE_ENDIAN, (wchar_t *)s_desc->wData,
-			DBC_MAX_STRING_LENGTH);
-
-	s_desc->bLength		= (strlen(DBC_STRING_MANUFACTURER) + 1) * 2;
-	s_desc->bDescriptorType	= USB_DT_STRING;
-	string_length		+= s_desc->bLength;
-	string_length		<<= 8;
-
-	/* String0: */
-	strings->string0[0]	= 4;
-	strings->string0[1]	= USB_DT_STRING;
-	strings->string0[2]	= 0x09;
-	strings->string0[3]	= 0x04;
-	string_length		+= 4;
-
-	return string_length;
-}
-
 static void xhci_dbc_init_ep_contexts(struct xhci_dbc *dbc)
 {
 	struct xhci_ep_ctx      *ep_ctx;
@@ -124,7 +75,63 @@ static void xhci_dbc_init_ep_contexts(struct xhci_dbc *dbc)
 	ep_ctx->deq             = cpu_to_le64(deq | dbc->ring_in->cycle_state);
 }
 
-static void xhci_dbc_init_contexts(struct xhci_dbc *dbc, u32 string_length)
+static u8 get_str_desc_len(const char *desc)
+{
+	return ((struct usb_string_descriptor *)desc)->bLength;
+}
+
+static u32 prepare_len(struct dbc_str_descs *descs)
+{
+	u32 len;
+
+	len = get_str_desc_len(descs->serial);
+	len <<= 8;
+	len += get_str_desc_len(descs->product);
+	len <<= 8;
+	len += get_str_desc_len(descs->manufacturer);
+	len <<= 8;
+	len += get_str_desc_len(descs->string0);
+
+	return len;
+}
+
+static int xhci_dbc_populate_str_desc(char *desc, const char *src)
+{
+	struct usb_string_descriptor	*s_desc;
+	int				utf16_len;
+
+	s_desc = (struct usb_string_descriptor *)desc;
+	utf16_len = utf8s_to_utf16s(src, strlen(src), UTF16_LITTLE_ENDIAN,
+				    (wchar_t *)s_desc->wData, USB_MAX_STRING_LEN);
+	if (utf16_len < 0)
+		return utf16_len;
+
+	s_desc->bLength		= utf16_len * 2 + 2;
+	s_desc->bDescriptorType	= USB_DT_STRING;
+
+	return s_desc->bLength;
+}
+
+static void xhci_dbc_populate_str_descs(struct dbc_str_descs *str_descs,
+					struct dbc_str *str)
+{
+	/* Serial string: */
+	xhci_dbc_populate_str_desc(str_descs->serial, str->serial);
+
+	/* Product string: */
+	xhci_dbc_populate_str_desc(str_descs->product, str->product);
+
+	/* Manufacturer string: */
+	xhci_dbc_populate_str_desc(str_descs->manufacturer, str->manufacturer);
+
+	/* String0: */
+	str_descs->string0[0]	= 4;
+	str_descs->string0[1]	= USB_DT_STRING;
+	str_descs->string0[2]	= 0x09;
+	str_descs->string0[3]	= 0x04;
+}
+
+static void xhci_dbc_init_contexts(struct xhci_dbc *dbc)
 {
 	struct dbc_info_context	*info;
 	u32			dev_info;
@@ -135,12 +142,12 @@ static void xhci_dbc_init_contexts(struct xhci_dbc *dbc, u32 string_length)
 
 	/* Populate info Context: */
 	info			= (struct dbc_info_context *)dbc->ctx->bytes;
-	dma			= dbc->string_dma;
+	dma			= dbc->str_descs_dma;
 	info->string0		= cpu_to_le64(dma);
-	info->manufacturer	= cpu_to_le64(dma + DBC_MAX_STRING_LENGTH);
-	info->product		= cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 2);
-	info->serial		= cpu_to_le64(dma + DBC_MAX_STRING_LENGTH * 3);
-	info->length		= cpu_to_le32(string_length);
+	info->manufacturer	= cpu_to_le64(dma + USB_MAX_STRING_DESC_LEN);
+	info->product		= cpu_to_le64(dma + USB_MAX_STRING_DESC_LEN*2);
+	info->serial		= cpu_to_le64(dma + USB_MAX_STRING_DESC_LEN*3);
+	info->length		= cpu_to_le32(prepare_len(dbc->str_descs));
 
 	/* Populate bulk in and out endpoint contexts: */
 	xhci_dbc_init_ep_contexts(dbc);
@@ -525,7 +532,6 @@ static int xhci_dbc_mem_init(struct xhci_dbc *dbc, gfp_t flags)
 {
 	int			ret;
 	dma_addr_t		deq;
-	u32			string_length;
 	struct device		*dev = dbc->dev;
 
 	/* Allocate various rings for events and transfers: */
@@ -552,11 +558,11 @@ static int xhci_dbc_mem_init(struct xhci_dbc *dbc, gfp_t flags)
 		goto ctx_fail;
 
 	/* Allocate the string table: */
-	dbc->string_size = sizeof(*dbc->string);
-	dbc->string = dma_alloc_coherent(dev, dbc->string_size,
-					 &dbc->string_dma, flags);
-	if (!dbc->string)
-		goto string_fail;
+	dbc->str_descs_size = sizeof(*dbc->str_descs);
+	dbc->str_descs = dma_alloc_coherent(dev, dbc->str_descs_size,
+					    &dbc->str_descs_dma, flags);
+	if (!dbc->str_descs)
+		goto str_descs_fail;
 
 	/* Setup ERST register: */
 	writel(dbc->erst.num_entries, &dbc->regs->ersts);
@@ -566,16 +572,16 @@ static int xhci_dbc_mem_init(struct xhci_dbc *dbc, gfp_t flags)
 				   dbc->ring_evt->dequeue);
 	lo_hi_writeq(deq, &dbc->regs->erdp);
 
-	/* Setup strings and contexts: */
-	string_length = xhci_dbc_populate_strings(dbc->string);
-	xhci_dbc_init_contexts(dbc, string_length);
+	/* Setup string descriptors and contexts: */
+	xhci_dbc_populate_str_descs(dbc->str_descs, &dbc->str);
+	xhci_dbc_init_contexts(dbc);
 
 	xhci_dbc_eps_init(dbc);
 	dbc->state = DS_INITIALIZED;
 
 	return 0;
 
-string_fail:
+str_descs_fail:
 	dbc_free_ctx(dev, dbc->ctx);
 	dbc->ctx = NULL;
 ctx_fail:
@@ -600,8 +606,8 @@ static void xhci_dbc_mem_cleanup(struct xhci_dbc *dbc)
 
 	xhci_dbc_eps_exit(dbc);
 
-	dma_free_coherent(dbc->dev, dbc->string_size, dbc->string, dbc->string_dma);
-	dbc->string = NULL;
+	dma_free_coherent(dbc->dev, dbc->str_descs_size, dbc->str_descs, dbc->str_descs_dma);
+	dbc->str_descs = NULL;
 
 	dbc_free_ctx(dbc->dev, dbc->ctx);
 	dbc->ctx = NULL;
@@ -1314,6 +1320,11 @@ xhci_alloc_dbc(struct device *dev, void __iomem *base, const struct dbc_driver *
 	dbc->bInterfaceProtocol = DBC_PROTOCOL;
 	dbc->poll_interval = DBC_POLL_INTERVAL_DEFAULT;
 
+	/* initialize serial, product and manufacturer with default values */
+	memcpy(dbc->str.serial, DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL)+1);
+	memcpy(dbc->str.product, DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT)+1);
+	memcpy(dbc->str.manufacturer, DBC_STRING_MANUFACTURER, strlen(DBC_STRING_MANUFACTURER)+1);
+
 	if (readl(&dbc->regs->control) & DBC_CTRL_DBC_ENABLE)
 		goto err;
 
diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
index 47ac72c2286d..0e6addafea6c 100644
--- a/drivers/usb/host/xhci-dbgcap.h
+++ b/drivers/usb/host/xhci-dbgcap.h
@@ -47,7 +47,6 @@ struct dbc_info_context {
 #define DBC_DOOR_BELL_TARGET(p)		(((p) & 0xff) << 8)
 
 #define DBC_MAX_PACKET			1024
-#define DBC_MAX_STRING_LENGTH		64
 #define DBC_STRING_MANUFACTURER		"Linux Foundation"
 #define DBC_STRING_PRODUCT		"Linux USB Debug Target"
 #define DBC_STRING_SERIAL		"0001"
@@ -63,11 +62,19 @@ struct dbc_info_context {
 #define DBC_PORTSC_LINK_CHANGE		BIT(22)
 #define DBC_PORTSC_CONFIG_CHANGE	BIT(23)
 
+#define USB_MAX_STRING_DESC_LEN		(USB_MAX_STRING_LEN + 2)
+
 struct dbc_str_descs {
-	char	string0[DBC_MAX_STRING_LENGTH];
-	char	manufacturer[DBC_MAX_STRING_LENGTH];
-	char	product[DBC_MAX_STRING_LENGTH];
-	char	serial[DBC_MAX_STRING_LENGTH];
+	char	string0[USB_MAX_STRING_DESC_LEN];
+	char	manufacturer[USB_MAX_STRING_DESC_LEN];
+	char	product[USB_MAX_STRING_DESC_LEN];
+	char	serial[USB_MAX_STRING_DESC_LEN];
+};
+
+struct dbc_str {
+	char	manufacturer[USB_MAX_STRING_LEN/2+1];
+	char	product[USB_MAX_STRING_LEN/2+1];
+	char	serial[USB_MAX_STRING_LEN/2+1];
 };
 
 #define DBC_PROTOCOL			1	/* GNU Remote Debug Command */
@@ -132,9 +139,10 @@ struct xhci_dbc {
 	struct xhci_erst		erst;
 	struct xhci_container_ctx	*ctx;
 
-	struct dbc_str_descs		*string;
-	dma_addr_t			string_dma;
-	size_t				string_size;
+	struct dbc_str_descs		*str_descs;
+	dma_addr_t			str_descs_dma;
+	size_t				str_descs_size;
+	struct dbc_str			str;
 	u16				idVendor;
 	u16				idProduct;
 	u16				bcdDevice;
-- 
2.51.0.710.ga91ca5db03-goog

[PATCH v2 2/4] xhci: dbc: allow to set serial number through sysfs

[Łukasz Bartosik]

From: Łukasz Bartosik <ukaszb@chromium.org>

Add code which allows to set serial number of a DbC
device through sysfs.

Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
---
 .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 13 +++++++
 drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
index fc82aa4e54b0..071688dbd969 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
+++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
@@ -85,3 +85,16 @@ Description:
 		up to 5000. The default value is 64 ms.
 		This polling interval is used while DbC is enabled but has no
 		active data transfers.
+
+What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iSerial
+Date:          October 2025
+Contact:       Łukasz Bartosik <ukaszb@chromium.org>
+Description:
+               The dbc_iSerial attribute allows to change the iSerial field
+               presented in the USB device descriptor by xhci debug device.
+               Value can only be changed while debug capability (DbC) is in
+               disabled state to prevent USB device descriptor change while
+               connected to a USB host.
+               The default value is "0001".
+               The field length can be from 1 to 63 characters.
+
diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index c2fecaffd6f3..7ad83548ba4d 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
 	return size;
 }
 
+static ssize_t dbc_iSerial_show(struct device *dev,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+
+	return sysfs_emit(buf, "%s\n", dbc->str.serial);
+}
+
+static ssize_t dbc_iSerial_store(struct device *dev,
+			     struct device_attribute *attr,
+			     const char *buf, size_t size)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+	size_t len;
+
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+
+	len = strcspn(buf, "\n");
+	if (!len)
+		return -EINVAL;
+
+	if (len > USB_MAX_STRING_LEN/2)
+		return -E2BIG;
+
+	memcpy(dbc->str.serial, buf, len);
+	dbc->str.serial[len] = '\0';
+
+	return size;
+}
+
 static ssize_t dbc_bInterfaceProtocol_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
@@ -1287,6 +1321,7 @@ static DEVICE_ATTR_RW(dbc);
 static DEVICE_ATTR_RW(dbc_idVendor);
 static DEVICE_ATTR_RW(dbc_idProduct);
 static DEVICE_ATTR_RW(dbc_bcdDevice);
+static DEVICE_ATTR_RW(dbc_iSerial);
 static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
 static DEVICE_ATTR_RW(dbc_poll_interval_ms);
 
@@ -1295,6 +1330,7 @@ static struct attribute *dbc_dev_attrs[] = {
 	&dev_attr_dbc_idVendor.attr,
 	&dev_attr_dbc_idProduct.attr,
 	&dev_attr_dbc_bcdDevice.attr,
+	&dev_attr_dbc_iSerial.attr,
 	&dev_attr_dbc_bInterfaceProtocol.attr,
 	&dev_attr_dbc_poll_interval_ms.attr,
 	NULL
-- 
2.51.0.710.ga91ca5db03-goog

[PATCH v2 3/4] xhci: dbc: allow to set product name through sysfs

[Łukasz Bartosik]

From: Łukasz Bartosik <ukaszb@chromium.org>

Add code which allows to set product name of a DbC
device through sysfs.

Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
---
 .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 11 ++++++
 drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
index 071688dbd969..57ba37606f79 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
+++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
@@ -98,3 +98,14 @@ Description:
                The default value is "0001".
                The field length can be from 1 to 63 characters.
 
+What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iProduct
+Date:          October 2025
+Contact:       Łukasz Bartosik <ukaszb@chromium.org>
+Description:
+               The dbc_iProduct attribute allows to change the iProduct field
+               presented in the USB device descriptor by xhci debug device.
+               Value can only be changed while debug capability (DbC) is in
+               disabled state to prevent USB device descriptor change while
+               connected to a USB host.
+               The default value is "Linux USB Debug Target".
+               The field length can be from 1 to 63 characters.
diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index 7ad83548ba4d..bc782f6b533e 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
 	return size;
 }
 
+static ssize_t dbc_iProduct_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+
+	return sysfs_emit(buf, "%s\n", dbc->str.product);
+}
+
+static ssize_t dbc_iProduct_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t size)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+	size_t len;
+
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+
+	len = strcspn(buf, "\n");
+	if (!len)
+		return -EINVAL;
+
+	if (len > USB_MAX_STRING_LEN/2)
+		return -E2BIG;
+
+	memcpy(dbc->str.product, buf, len);
+	dbc->str.product[len] = '\0';
+
+	return size;
+}
+
 static ssize_t dbc_iSerial_show(struct device *dev,
 			    struct device_attribute *attr,
 			    char *buf)
@@ -1321,6 +1355,7 @@ static DEVICE_ATTR_RW(dbc);
 static DEVICE_ATTR_RW(dbc_idVendor);
 static DEVICE_ATTR_RW(dbc_idProduct);
 static DEVICE_ATTR_RW(dbc_bcdDevice);
+static DEVICE_ATTR_RW(dbc_iProduct);
 static DEVICE_ATTR_RW(dbc_iSerial);
 static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
 static DEVICE_ATTR_RW(dbc_poll_interval_ms);
@@ -1330,6 +1365,7 @@ static struct attribute *dbc_dev_attrs[] = {
 	&dev_attr_dbc_idVendor.attr,
 	&dev_attr_dbc_idProduct.attr,
 	&dev_attr_dbc_bcdDevice.attr,
+	&dev_attr_dbc_iProduct.attr,
 	&dev_attr_dbc_iSerial.attr,
 	&dev_attr_dbc_bInterfaceProtocol.attr,
 	&dev_attr_dbc_poll_interval_ms.attr,
-- 
2.51.0.710.ga91ca5db03-goog

[PATCH v2 4/4] xhci: dbc: allow to set manufacturer name through sysfs

[Łukasz Bartosik]

From: Łukasz Bartosik <ukaszb@chromium.org>

Add code which allows to set manufacturer name of a DbC
device through sysfs.

Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
---
 .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 12 +++++++
 drivers/usb/host/xhci-dbgcap.c                | 35 +++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
index 57ba37606f79..26ca6a870e44 100644
--- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
+++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
@@ -109,3 +109,15 @@ Description:
                connected to a USB host.
                The default value is "Linux USB Debug Target".
                The field length can be from 1 to 63 characters.
+
+What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_Manufacturer
+Date:          October 2025
+Contact:       Łukasz Bartosik <ukaszb@chromium.org>
+Description:
+               The dbc_iManufacturer attribute allows to change the iManufacturer
+               field presented in the USB device descriptor by xhci debug device.
+               Value can only be changed while debug capability (DbC) is in
+               disabled state to prevent USB device descriptor change while
+               connected to a USB host.
+               The default value is "Linux Foundation".
+               The field length can be from 1 to 63 characters.
diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
index bc782f6b533e..badd20f2dcd9 100644
--- a/drivers/usb/host/xhci-dbgcap.c
+++ b/drivers/usb/host/xhci-dbgcap.c
@@ -1200,6 +1200,39 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
 	return size;
 }
 
+static ssize_t dbc_iManufacturer_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+
+	return sysfs_emit(buf, "%s\n", dbc->str.manufacturer);
+}
+
+static ssize_t dbc_iManufacturer_store(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t size)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
+	struct xhci_dbc	*dbc = xhci->dbc;
+	size_t len;
+
+	if (dbc->state != DS_DISABLED)
+		return -EBUSY;
+
+	len = strcspn(buf, "\n");
+	if (!len)
+		return -EINVAL;
+
+	if (len > USB_MAX_STRING_LEN/2)
+		return -E2BIG;
+
+	memcpy(dbc->str.manufacturer, buf, len);
+	dbc->str.manufacturer[len] = '\0';
+
+	return size;
+}
 static ssize_t dbc_iProduct_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
@@ -1355,6 +1388,7 @@ static DEVICE_ATTR_RW(dbc);
 static DEVICE_ATTR_RW(dbc_idVendor);
 static DEVICE_ATTR_RW(dbc_idProduct);
 static DEVICE_ATTR_RW(dbc_bcdDevice);
+static DEVICE_ATTR_RW(dbc_iManufacturer);
 static DEVICE_ATTR_RW(dbc_iProduct);
 static DEVICE_ATTR_RW(dbc_iSerial);
 static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
@@ -1365,6 +1399,7 @@ static struct attribute *dbc_dev_attrs[] = {
 	&dev_attr_dbc_idVendor.attr,
 	&dev_attr_dbc_idProduct.attr,
 	&dev_attr_dbc_bcdDevice.attr,
+	&dev_attr_dbc_iManufacturer.attr,
 	&dev_attr_dbc_iProduct.attr,
 	&dev_attr_dbc_iSerial.attr,
 	&dev_attr_dbc_bInterfaceProtocol.attr,
-- 
2.51.0.710.ga91ca5db03-goog

Re: [PATCH v2 2/4] xhci: dbc: allow to set serial number through sysfs

[Greg Kroah-Hartman]

On Tue, Oct 07, 2025 at 09:39:00PM +0000, Łukasz Bartosik wrote:
> From: Łukasz Bartosik <ukaszb@chromium.org>
> 
> Add code which allows to set serial number of a DbC
> device through sysfs.
> 
> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> ---
>  .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 13 +++++++
>  drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> index fc82aa4e54b0..071688dbd969 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> @@ -85,3 +85,16 @@ Description:
>  		up to 5000. The default value is 64 ms.
>  		This polling interval is used while DbC is enabled but has no
>  		active data transfers.
> +
> +What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iSerial
> +Date:          October 2025
> +Contact:       Łukasz Bartosik <ukaszb@chromium.org>
> +Description:
> +               The dbc_iSerial attribute allows to change the iSerial field
> +               presented in the USB device descriptor by xhci debug device.
> +               Value can only be changed while debug capability (DbC) is in
> +               disabled state to prevent USB device descriptor change while
> +               connected to a USB host.
> +               The default value is "0001".
> +               The field length can be from 1 to 63 characters.
> +
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index c2fecaffd6f3..7ad83548ba4d 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
>  	return size;
>  }
>  
> +static ssize_t dbc_iSerial_show(struct device *dev,
> +			    struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
> +	struct xhci_dbc	*dbc = xhci->dbc;
> +
> +	return sysfs_emit(buf, "%s\n", dbc->str.serial);
> +}
> +
> +static ssize_t dbc_iSerial_store(struct device *dev,
> +			     struct device_attribute *attr,
> +			     const char *buf, size_t size)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
> +	struct xhci_dbc	*dbc = xhci->dbc;
> +	size_t len;
> +
> +	if (dbc->state != DS_DISABLED)
> +		return -EBUSY;
> +
> +	len = strcspn(buf, "\n");

So you are requiring the \n to be there?  Why?  What tool will write it?
You don't document this in up in the documentation :)

> +	if (!len)
> +		return -EINVAL;
> +
> +	if (len > USB_MAX_STRING_LEN/2)
> +		return -E2BIG;
> +
> +	memcpy(dbc->str.serial, buf, len);
> +	dbc->str.serial[len] = '\0';

As you know that buf is zero terminated, you can do a strcpy instead of
this two-step process.  Ah, but that \n character, why not just zap it
out if it is present?

thanks,

greg k-h

Re: [PATCH v2 2/4] xhci: dbc: allow to set serial number through sysfs

[Łukasz Bartosik]

On Wed, Oct 8, 2025 at 1:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 07, 2025 at 09:39:00PM +0000, Łukasz Bartosik wrote:
> > From: Łukasz Bartosik <ukaszb@chromium.org>
> >
> > Add code which allows to set serial number of a DbC
> > device through sysfs.
> >
> > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> > ---
> >  .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 13 +++++++
> >  drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > index fc82aa4e54b0..071688dbd969 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > @@ -85,3 +85,16 @@ Description:
> >               up to 5000. The default value is 64 ms.
> >               This polling interval is used while DbC is enabled but has no
> >               active data transfers.
> > +
> > +What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iSerial
> > +Date:          October 2025
> > +Contact:       Łukasz Bartosik <ukaszb@chromium.org>
> > +Description:
> > +               The dbc_iSerial attribute allows to change the iSerial field
> > +               presented in the USB device descriptor by xhci debug device.
> > +               Value can only be changed while debug capability (DbC) is in
> > +               disabled state to prevent USB device descriptor change while
> > +               connected to a USB host.
> > +               The default value is "0001".
> > +               The field length can be from 1 to 63 characters.
> > +
> > diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> > index c2fecaffd6f3..7ad83548ba4d 100644
> > --- a/drivers/usb/host/xhci-dbgcap.c
> > +++ b/drivers/usb/host/xhci-dbgcap.c
> > @@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
> >       return size;
> >  }
> >
> > +static ssize_t dbc_iSerial_show(struct device *dev,
> > +                         struct device_attribute *attr,
> > +                         char *buf)
> > +{
> > +     struct xhci_hcd *xhci = hcd_to_xhci(dev_get_drvdata(dev));
> > +     struct xhci_dbc *dbc = xhci->dbc;
> > +
> > +     return sysfs_emit(buf, "%s\n", dbc->str.serial);
> > +}
> > +
> > +static ssize_t dbc_iSerial_store(struct device *dev,
> > +                          struct device_attribute *attr,
> > +                          const char *buf, size_t size)
> > +{
> > +     struct xhci_hcd *xhci = hcd_to_xhci(dev_get_drvdata(dev));
> > +     struct xhci_dbc *dbc = xhci->dbc;
> > +     size_t len;
> > +
> > +     if (dbc->state != DS_DISABLED)
> > +             return -EBUSY;
> > +
> > +     len = strcspn(buf, "\n");
>
> So you are requiring the \n to be there?  Why?  What tool will write it?

I'm not requiring \n to be present. The purpose of this line is to
exclude \n from
being copied.  These new fields accessible through sysfs (serial,
manufacturer, product) will be written by ChromeOS software stack.

> You don't document this in up in the documentation :)
>

I added the description of dbc_iSerial, dbc_iProduct and dbc_iManufacturer
to the sysfs-bus-pci-drivers-xhci_hcd.
What else would you like to see added there ?

> > +     if (!len)
> > +             return -EINVAL;
> > +
> > +     if (len > USB_MAX_STRING_LEN/2)
> > +             return -E2BIG;
> > +
> > +     memcpy(dbc->str.serial, buf, len);
> > +     dbc->str.serial[len] = '\0';
>
> As you know that buf is zero terminated, you can do a strcpy instead of
> this two-step process.  Ah, but that \n character, why not just zap it
> out if it is present?
>

I could use strcpy however I want to get rid of \n if it is present.

Thanks,
Łukasz

> thanks,
>
> greg k-h

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Mathias Nyman]

On 10/8/25 00:38, Łukasz Bartosik wrote:
> From: Łukasz Bartosik <ukaszb@chromium.org>
> 
> Reorganize the code to prepare ground for setting serial number,
> product and manufacturer names through sysfs. This commit:
> 1. Introduces new buffers for storing serial number, product and
>     manufacturer name in utf8. The buffers will be used by sysfs
>     *_show and *_store functions.
> 2. Increases USB string descriptor data maximum length to the
>     value from USB specification (126 bytes of data).
> 3. Adds new helper functions get_str_desc_len, prepare_len
>     and xhci_dbc_populate_str_desc.
> 
> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>

This change does in general look good to me.

It gets rid of the code duplication in current xhci_dbc_populate_strings(),
and retains new strings over suspend/resume.

I mostly have minor nitpicks regarding naming and other subjective matters

> ---
>   drivers/usb/host/xhci-dbgcap.c | 145 ++++++++++++++++++---------------
>   drivers/usb/host/xhci-dbgcap.h |  24 ++++--
>   2 files changed, 94 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 63edf2d8f245..c2fecaffd6f3 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -124,7 +75,63 @@ static void xhci_dbc_init_ep_contexts(struct xhci_dbc *dbc)
>   	ep_ctx->deq             = cpu_to_le64(deq | dbc->ring_in->cycle_state);
>   }
>   
> -static void xhci_dbc_init_contexts(struct xhci_dbc *dbc, u32 string_length)
> +static u8 get_str_desc_len(const char *desc)
> +{
> +	return ((struct usb_string_descriptor *)desc)->bLength;
> +}
> +
> +static u32 prepare_len(struct dbc_str_descs *descs)

prepare_len() is very generic, how about something like

dbc_prepare_info_context_str_len()

> +{
> +	u32 len;
> +
> +	len = get_str_desc_len(descs->serial);
> +	len <<= 8;
> +	len += get_str_desc_len(descs->product);
> +	len <<= 8;
> +	len += get_str_desc_len(descs->manufacturer);
> +	len <<= 8;
> +	len += get_str_desc_len(descs->string0);
> +
> +	return len;
> +}
> +
> +static int xhci_dbc_populate_str_desc(char *desc, const char *src)
> +{
> +	struct usb_string_descriptor	*s_desc;
> +	int				utf16_len;
> +
> +	s_desc = (struct usb_string_descriptor *)desc;
> +	utf16_len = utf8s_to_utf16s(src, strlen(src), UTF16_LITTLE_ENDIAN,
> +				    (wchar_t *)s_desc->wData, USB_MAX_STRING_LEN);

The "utf16_len" got me confused.
It's not wrong, but I first assumed it is bytes this utf16 formatted text
takes, when it turns out to be number of u16 entries in the text.

> +	if (utf16_len < 0)
> +		return utf16_len;
> +
> +	s_desc->bLength		= utf16_len * 2 + 2;
> +	s_desc->bDescriptorType	= USB_DT_STRING;
> +
> +	return s_desc->bLength;
> +}
> +
> +static void xhci_dbc_populate_str_descs(struct dbc_str_descs *str_descs,
> +					struct dbc_str *str)
> +{
> +	/* Serial string: */
> +	xhci_dbc_populate_str_desc(str_descs->serial, str->serial);
> +
> +	/* Product string: */
> +	xhci_dbc_populate_str_desc(str_descs->product, str->product);
> +
> +	/* Manufacturer string: */
> +	xhci_dbc_populate_str_desc(str_descs->manufacturer, str->manufacturer);
> +
> +	/* String0: */
> +	str_descs->string0[0]	= 4;
> +	str_descs->string0[1]	= USB_DT_STRING;
> +	str_descs->string0[2]	= 0x09;
> +	str_descs->string0[3]	= 0x04;
> +}

> @@ -1314,6 +1320,11 @@ xhci_alloc_dbc(struct device *dev, void __iomem *base, const struct dbc_driver *
>   	dbc->bInterfaceProtocol = DBC_PROTOCOL;
>   	dbc->poll_interval = DBC_POLL_INTERVAL_DEFAULT;
>   
> +	/* initialize serial, product and manufacturer with default values */
> +	memcpy(dbc->str.serial, DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL)+1);
> +	memcpy(dbc->str.product, DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT)+1);
> +	memcpy(dbc->str.manufacturer, DBC_STRING_MANUFACTURER, strlen(DBC_STRING_MANUFACTURER)+1);
> +

Maybe it would be cleaner to just define a default struct for the strings and assign it here.
We could get rid of the #define  DBC_STRING_* from the header as well.

i.e.:

static const struct dbc_str dbc_str_default = {
	.manufacturer = "Linux Foundation",
	.product = "Linux USB Debug Target",
	.serial = "0001",
};

xhci_alloc_dbc(..)
{
	...
	/* initialize serial, product and manufacturer with default values */
	dbc->str = dbc_str_default;
}>   	if (readl(&dbc->regs->control) & DBC_CTRL_DBC_ENABLE)
>   		goto err;
>   
> diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> index 47ac72c2286d..0e6addafea6c 100644
> --- a/drivers/usb/host/xhci-dbgcap.h
> +++ b/drivers/usb/host/xhci-dbgcap.h
> @@ -47,7 +47,6 @@ struct dbc_info_context {
>   #define DBC_DOOR_BELL_TARGET(p)		(((p) & 0xff) << 8)
>   
>   #define DBC_MAX_PACKET			1024
> -#define DBC_MAX_STRING_LENGTH		64
>   #define DBC_STRING_MANUFACTURER		"Linux Foundation"
>   #define DBC_STRING_PRODUCT		"Linux USB Debug Target"
>   #define DBC_STRING_SERIAL		"0001"
> @@ -63,11 +62,19 @@ struct dbc_info_context {
>   #define DBC_PORTSC_LINK_CHANGE		BIT(22)
>   #define DBC_PORTSC_CONFIG_CHANGE	BIT(23)
>   
> +#define USB_MAX_STRING_DESC_LEN		(USB_MAX_STRING_LEN + 2)

This looks like somthing that would be defined in ch9.h or usb.h.

Maybe a more local name like DBC_MAX_STRING_DESC_LEN


> +
>   struct dbc_str_descs {
> -	char	string0[DBC_MAX_STRING_LENGTH];
> -	char	manufacturer[DBC_MAX_STRING_LENGTH];
> -	char	product[DBC_MAX_STRING_LENGTH];
> -	char	serial[DBC_MAX_STRING_LENGTH];
> +	char	string0[USB_MAX_STRING_DESC_LEN];
> +	char	manufacturer[USB_MAX_STRING_DESC_LEN];
> +	char	product[USB_MAX_STRING_DESC_LEN];
> +	char	serial[USB_MAX_STRING_DESC_LEN];
> +};
> +
> +struct dbc_str {> +	char	manufacturer[USB_MAX_STRING_LEN/2+1];
> +	char	product[USB_MAX_STRING_LEN/2+1];
> +	char	serial[USB_MAX_STRING_LEN/2+1];
>   };

Maybe some comment above to clarify the odd size

/* utf8 strings used to create the USB_MAX_STRING_LEN utf16 string descriptors */

Thanks
Mathias

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Łukasz Bartosik]

On Fri, Oct 10, 2025 at 1:09 PM Mathias Nyman <mathias.nyman@intel.com> wrote:
>
> On 10/8/25 00:38, Łukasz Bartosik wrote:
> > From: Łukasz Bartosik <ukaszb@chromium.org>
> >
> > Reorganize the code to prepare ground for setting serial number,
> > product and manufacturer names through sysfs. This commit:
> > 1. Introduces new buffers for storing serial number, product and
> >     manufacturer name in utf8. The buffers will be used by sysfs
> >     *_show and *_store functions.
> > 2. Increases USB string descriptor data maximum length to the
> >     value from USB specification (126 bytes of data).
> > 3. Adds new helper functions get_str_desc_len, prepare_len
> >     and xhci_dbc_populate_str_desc.
> >
> > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
>
> This change does in general look good to me.
>
> It gets rid of the code duplication in current xhci_dbc_populate_strings(),
> and retains new strings over suspend/resume.
>
> I mostly have minor nitpicks regarding naming and other subjective matters
>

Thank you Mathias for the review.

> > ---
> >   drivers/usb/host/xhci-dbgcap.c | 145 ++++++++++++++++++---------------
> >   drivers/usb/host/xhci-dbgcap.h |  24 ++++--
> >   2 files changed, 94 insertions(+), 75 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> > index 63edf2d8f245..c2fecaffd6f3 100644
> > --- a/drivers/usb/host/xhci-dbgcap.c
> > +++ b/drivers/usb/host/xhci-dbgcap.c
> > @@ -124,7 +75,63 @@ static void xhci_dbc_init_ep_contexts(struct xhci_dbc *dbc)
> >       ep_ctx->deq             = cpu_to_le64(deq | dbc->ring_in->cycle_state);
> >   }
> >
> > -static void xhci_dbc_init_contexts(struct xhci_dbc *dbc, u32 string_length)
> > +static u8 get_str_desc_len(const char *desc)
> > +{
> > +     return ((struct usb_string_descriptor *)desc)->bLength;
> > +}
> > +
> > +static u32 prepare_len(struct dbc_str_descs *descs)
>
> prepare_len() is very generic, how about something like
>
> dbc_prepare_info_context_str_len()
>

I will change it in patchset v3.

> > +{
> > +     u32 len;
> > +
> > +     len = get_str_desc_len(descs->serial);
> > +     len <<= 8;
> > +     len += get_str_desc_len(descs->product);
> > +     len <<= 8;
> > +     len += get_str_desc_len(descs->manufacturer);
> > +     len <<= 8;
> > +     len += get_str_desc_len(descs->string0);
> > +
> > +     return len;
> > +}
> > +
> > +static int xhci_dbc_populate_str_desc(char *desc, const char *src)
> > +{
> > +     struct usb_string_descriptor    *s_desc;
> > +     int                             utf16_len;
> > +
> > +     s_desc = (struct usb_string_descriptor *)desc;
> > +     utf16_len = utf8s_to_utf16s(src, strlen(src), UTF16_LITTLE_ENDIAN,
> > +                                 (wchar_t *)s_desc->wData, USB_MAX_STRING_LEN);
>
> The "utf16_len" got me confused.
> It's not wrong, but I first assumed it is bytes this utf16 formatted text
> takes, when it turns out to be number of u16 entries in the text.
>

Do you have a suggestion how to rename it to be more self commenting ?
Or maybe I will rename it to len and add a comment that it holds a number
of u16 entries ? WDYT ?

> > +     if (utf16_len < 0)
> > +             return utf16_len;
> > +
> > +     s_desc->bLength         = utf16_len * 2 + 2;
> > +     s_desc->bDescriptorType = USB_DT_STRING;
> > +
> > +     return s_desc->bLength;
> > +}
> > +
> > +static void xhci_dbc_populate_str_descs(struct dbc_str_descs *str_descs,
> > +                                     struct dbc_str *str)
> > +{
> > +     /* Serial string: */
> > +     xhci_dbc_populate_str_desc(str_descs->serial, str->serial);
> > +
> > +     /* Product string: */
> > +     xhci_dbc_populate_str_desc(str_descs->product, str->product);
> > +
> > +     /* Manufacturer string: */
> > +     xhci_dbc_populate_str_desc(str_descs->manufacturer, str->manufacturer);
> > +
> > +     /* String0: */
> > +     str_descs->string0[0]   = 4;
> > +     str_descs->string0[1]   = USB_DT_STRING;
> > +     str_descs->string0[2]   = 0x09;
> > +     str_descs->string0[3]   = 0x04;
> > +}
>
> > @@ -1314,6 +1320,11 @@ xhci_alloc_dbc(struct device *dev, void __iomem *base, const struct dbc_driver *
> >       dbc->bInterfaceProtocol = DBC_PROTOCOL;
> >       dbc->poll_interval = DBC_POLL_INTERVAL_DEFAULT;
> >
> > +     /* initialize serial, product and manufacturer with default values */
> > +     memcpy(dbc->str.serial, DBC_STRING_SERIAL, strlen(DBC_STRING_SERIAL)+1);
> > +     memcpy(dbc->str.product, DBC_STRING_PRODUCT, strlen(DBC_STRING_PRODUCT)+1);
> > +     memcpy(dbc->str.manufacturer, DBC_STRING_MANUFACTURER, strlen(DBC_STRING_MANUFACTURER)+1);
> > +
>
> Maybe it would be cleaner to just define a default struct for the strings and assign it here.
> We could get rid of the #define  DBC_STRING_* from the header as well.
>
> i.e.:
>
> static const struct dbc_str dbc_str_default = {
>         .manufacturer = "Linux Foundation",
>         .product = "Linux USB Debug Target",
>         .serial = "0001",
> };
>
> xhci_alloc_dbc(..)
> {
>         ...
>         /* initialize serial, product and manufacturer with default values */
>         dbc->str = dbc_str_default;

Neat ;). I will change it in patchset v3.

> }>      if (readl(&dbc->regs->control) & DBC_CTRL_DBC_ENABLE)
> >               goto err;
> >
> > diff --git a/drivers/usb/host/xhci-dbgcap.h b/drivers/usb/host/xhci-dbgcap.h
> > index 47ac72c2286d..0e6addafea6c 100644
> > --- a/drivers/usb/host/xhci-dbgcap.h
> > +++ b/drivers/usb/host/xhci-dbgcap.h
> > @@ -47,7 +47,6 @@ struct dbc_info_context {
> >   #define DBC_DOOR_BELL_TARGET(p)             (((p) & 0xff) << 8)
> >
> >   #define DBC_MAX_PACKET                      1024
> > -#define DBC_MAX_STRING_LENGTH                64
> >   #define DBC_STRING_MANUFACTURER             "Linux Foundation"
> >   #define DBC_STRING_PRODUCT          "Linux USB Debug Target"
> >   #define DBC_STRING_SERIAL           "0001"
> > @@ -63,11 +62,19 @@ struct dbc_info_context {
> >   #define DBC_PORTSC_LINK_CHANGE              BIT(22)
> >   #define DBC_PORTSC_CONFIG_CHANGE    BIT(23)
> >
> > +#define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN + 2)
>
> This looks like somthing that would be defined in ch9.h or usb.h.
>

Unfortunately I can see only USB_MAX_STRING_LEN but no definition
for a maximum USB string descriptor size.

> Maybe a more local name like DBC_MAX_STRING_DESC_LEN
>

I will rename it and also remove magic number:
#define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN +
sizeof(struct usb_string_descriptor))

>
> > +
> >   struct dbc_str_descs {
> > -     char    string0[DBC_MAX_STRING_LENGTH];
> > -     char    manufacturer[DBC_MAX_STRING_LENGTH];
> > -     char    product[DBC_MAX_STRING_LENGTH];
> > -     char    serial[DBC_MAX_STRING_LENGTH];
> > +     char    string0[USB_MAX_STRING_DESC_LEN];
> > +     char    manufacturer[USB_MAX_STRING_DESC_LEN];
> > +     char    product[USB_MAX_STRING_DESC_LEN];
> > +     char    serial[USB_MAX_STRING_DESC_LEN];
> > +};
> > +
> > +struct dbc_str {> +  char    manufacturer[USB_MAX_STRING_LEN/2+1];
> > +     char    product[USB_MAX_STRING_LEN/2+1];
> > +     char    serial[USB_MAX_STRING_LEN/2+1];
> >   };
>
> Maybe some comment above to clarify the odd size
>
> /* utf8 strings used to create the USB_MAX_STRING_LEN utf16 string descriptors */
>

I will add the comment in the patchset v3.

Thanks,
Łukasz

> Thanks
> Mathias
>

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Alan Stern]

On Sat, Oct 11, 2025 at 01:22:45PM +0200, Łukasz Bartosik wrote:
> > > @@ -63,11 +62,19 @@ struct dbc_info_context {
> > >   #define DBC_PORTSC_LINK_CHANGE              BIT(22)
> > >   #define DBC_PORTSC_CONFIG_CHANGE    BIT(23)
> > >
> > > +#define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN + 2)
> >
> > This looks like somthing that would be defined in ch9.h or usb.h.
> >
> 
> Unfortunately I can see only USB_MAX_STRING_LEN but no definition
> for a maximum USB string descriptor size.

The maximum length of a string descriptor is 255, because the bLength 
field in the descriptor is __u8.  In practice the maximum length is 254, 
because a string descriptor consists of a 2-byte header followed by a 
bunch of UTF-16LE characters (so 2 bytes each).  This allows for only 
126 characters (or rather, code points) max in the string, which is 
where USB_MAX_STRING_LEN comes from.

> > Maybe a more local name like DBC_MAX_STRING_DESC_LEN
> >
> 
> I will rename it and also remove magic number:
> #define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN +
> sizeof(struct usb_string_descriptor))

You might as well just set this to 254.

Alan Stern

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Łukasz Bartosik]

On Sat, Oct 11, 2025 at 7:13 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, Oct 11, 2025 at 01:22:45PM +0200, Łukasz Bartosik wrote:
> > > > @@ -63,11 +62,19 @@ struct dbc_info_context {
> > > >   #define DBC_PORTSC_LINK_CHANGE              BIT(22)
> > > >   #define DBC_PORTSC_CONFIG_CHANGE    BIT(23)
> > > >
> > > > +#define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN + 2)
> > >
> > > This looks like somthing that would be defined in ch9.h or usb.h.
> > >
> >
> > Unfortunately I can see only USB_MAX_STRING_LEN but no definition
> > for a maximum USB string descriptor size.
>
> The maximum length of a string descriptor is 255, because the bLength
> field in the descriptor is __u8.  In practice the maximum length is 254,
> because a string descriptor consists of a 2-byte header followed by a
> bunch of UTF-16LE characters (so 2 bytes each).  This allows for only
> 126 characters (or rather, code points) max in the string, which is
> where USB_MAX_STRING_LEN comes from.
>

Thank you Alan for pointing out USB_MAX_STRING_LEN is maximum length
in utf16 code points ;).

> > > Maybe a more local name like DBC_MAX_STRING_DESC_LEN
> > >
> >
> > I will rename it and also remove magic number:
> > #define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN +
> > sizeof(struct usb_string_descriptor))
>
> You might as well just set this to 254.
>

Mathias, do you have a preference here?
I mean whether to use
#define USB_MAX_STRING_DESC_LEN             254
or
#define USB_MAX_STRING_DESC_LEN              (2*USB_MAX_STRING_LEN +
sizeof(struct usb_string_descriptor))

No matter which one is chosen I will add a comment with Alan's explanation.

Thanks,
Łukasz

> Alan Stern

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Mathias Nyman]

On 10/12/25 18:57, Łukasz Bartosik wrote:
> On Sat, Oct 11, 2025 at 7:13 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> On Sat, Oct 11, 2025 at 01:22:45PM +0200, Łukasz Bartosik wrote:
>>>>> @@ -63,11 +62,19 @@ struct dbc_info_context {
>>>>>    #define DBC_PORTSC_LINK_CHANGE              BIT(22)
>>>>>    #define DBC_PORTSC_CONFIG_CHANGE    BIT(23)
>>>>>
>>>>> +#define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN + 2)
>>>>
>>>> This looks like somthing that would be defined in ch9.h or usb.h.
>>>>
>>>
>>> Unfortunately I can see only USB_MAX_STRING_LEN but no definition
>>> for a maximum USB string descriptor size.
>>
>> The maximum length of a string descriptor is 255, because the bLength
>> field in the descriptor is __u8.  In practice the maximum length is 254,
>> because a string descriptor consists of a 2-byte header followed by a
>> bunch of UTF-16LE characters (so 2 bytes each).  This allows for only
>> 126 characters (or rather, code points) max in the string, which is
>> where USB_MAX_STRING_LEN comes from.
>>
> 
> Thank you Alan for pointing out USB_MAX_STRING_LEN is maximum length
> in utf16 code points ;).
> 
>>>> Maybe a more local name like DBC_MAX_STRING_DESC_LEN
>>>>
>>>
>>> I will rename it and also remove magic number:
>>> #define USB_MAX_STRING_DESC_LEN              (USB_MAX_STRING_LEN +
>>> sizeof(struct usb_string_descriptor))
>>
>> You might as well just set this to 254.
>>
> 
> Mathias, do you have a preference here?
> I mean whether to use
> #define USB_MAX_STRING_DESC_LEN             254
> or
> #define USB_MAX_STRING_DESC_LEN              (2*USB_MAX_STRING_LEN +
> sizeof(struct usb_string_descriptor))
> 
> No matter which one is chosen I will add a comment with Alan's explanation.

Setting it to 254 with a comment based on Alan's explanations sounds good to me.

Thanks
Mathias

Re: [PATCH v2 1/4] xhci: dbc: prepare to expose strings through sysfs

[Mathias Nyman]

On 10/11/25 14:22, Łukasz Bartosik wrote:
> On Fri, Oct 10, 2025 at 1:09 PM Mathias Nyman <mathias.nyman@intel.com> wrote:
>>
>> On 10/8/25 00:38, Łukasz Bartosik wrote:
>>> From: Łukasz Bartosik <ukaszb@chromium.org>
>>>

>>> +static int xhci_dbc_populate_str_desc(char *desc, const char *src)
>>> +{
>>> +     struct usb_string_descriptor    *s_desc;
>>> +     int                             utf16_len;
>>> +
>>> +     s_desc = (struct usb_string_descriptor *)desc;
>>> +     utf16_len = utf8s_to_utf16s(src, strlen(src), UTF16_LITTLE_ENDIAN,
>>> +                                 (wchar_t *)s_desc->wData, USB_MAX_STRING_LEN);
>>
>> The "utf16_len" got me confused.
>> It's not wrong, but I first assumed it is bytes this utf16 formatted text
>> takes, when it turns out to be number of u16 entries in the text.
>>
> 
> Do you have a suggestion how to rename it to be more self commenting ?
> Or maybe I will rename it to len and add a comment that it holds a number
> of u16 entries ? WDYT ?

I don't have a good suggestion. Naming is hard.
'len' with a comment sounds good.

Thanks
Mathias

Re: [PATCH v2 3/4] xhci: dbc: allow to set product name through sysfs

[Mathias Nyman]

On 10/8/25 00:39, Łukasz Bartosik wrote:
> From: Łukasz Bartosik <ukaszb@chromium.org>
> 
> Add code which allows to set product name of a DbC
> device through sysfs.
> 
> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> ---
>   .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 11 ++++++
>   drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
>   2 files changed, 47 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> index 071688dbd969..57ba37606f79 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> @@ -98,3 +98,14 @@ Description:
>                  The default value is "0001".
>                  The field length can be from 1 to 63 characters.
>   
> +What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iProduct
> +Date:          October 2025
> +Contact:       Łukasz Bartosik <ukaszb@chromium.org>
> +Description:
> +               The dbc_iProduct attribute allows to change the iProduct field
> +               presented in the USB device descriptor by xhci debug device.
> +               Value can only be changed while debug capability (DbC) is in
> +               disabled state to prevent USB device descriptor change while
> +               connected to a USB host.
> +               The default value is "Linux USB Debug Target".
> +               The field length can be from 1 to 63 characters.
> diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> index 7ad83548ba4d..bc782f6b533e 100644
> --- a/drivers/usb/host/xhci-dbgcap.c
> +++ b/drivers/usb/host/xhci-dbgcap.c
> @@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
>   	return size;
>   }
>   
> +static ssize_t dbc_iProduct_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
> +	struct xhci_dbc	*dbc = xhci->dbc;
> +
> +	return sysfs_emit(buf, "%s\n", dbc->str.product);
> +}
> +
> +static ssize_t dbc_iProduct_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t size)
> +{
> +	struct xhci_hcd	*xhci = hcd_to_xhci(dev_get_drvdata(dev));
> +	struct xhci_dbc	*dbc = xhci->dbc;
> +	size_t len;
> +
> +	if (dbc->state != DS_DISABLED)
> +		return -EBUSY;
> +
> +	len = strcspn(buf, "\n");
> +	if (!len)
> +		return -EINVAL;
> +
> +	if (len > USB_MAX_STRING_LEN/2)
> +		return -E2BIG;
> +
> +	memcpy(dbc->str.product, buf, len);
> +	dbc->str.product[len] = '\0';
> +
> +	return size;
> +}
> +
>   static ssize_t dbc_iSerial_show(struct device *dev,
>   			    struct device_attribute *attr,
>   			    char *buf)
> @@ -1321,6 +1355,7 @@ static DEVICE_ATTR_RW(dbc);
>   static DEVICE_ATTR_RW(dbc_idVendor);
>   static DEVICE_ATTR_RW(dbc_idProduct);
>   static DEVICE_ATTR_RW(dbc_bcdDevice);
> +static DEVICE_ATTR_RW(dbc_iProduct);
>   static DEVICE_ATTR_RW(dbc_iSerial);
>   static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
>   static DEVICE_ATTR_RW(dbc_poll_interval_ms);
> @@ -1330,6 +1365,7 @@ static struct attribute *dbc_dev_attrs[] = {
>   	&dev_attr_dbc_idVendor.attr,
>   	&dev_attr_dbc_idProduct.attr,
>   	&dev_attr_dbc_bcdDevice.attr,
> +	&dev_attr_dbc_iProduct.attr,

Small naming thing again.

"dbc_idProduct" and dbc_iProduct" are a bit too similar.

Usb core uses "product" and "manufacturer" sysfs entries to show these strings.
I think dbc should use similar "dbc_product" and "dbc_manufacturer" naming.

the "iProduct" is a byte in device descriptor telling the index of the product string
descriptor.

Thanks
Mathias

Re: [PATCH v2 3/4] xhci: dbc: allow to set product name through sysfs

[Łukasz Bartosik]

On Mon, Oct 13, 2025 at 11:01 AM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 10/8/25 00:39, Łukasz Bartosik wrote:
> > From: Łukasz Bartosik <ukaszb@chromium.org>
> >
> > Add code which allows to set product name of a DbC
> > device through sysfs.
> >
> > Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
> > ---
> >   .../testing/sysfs-bus-pci-drivers-xhci_hcd    | 11 ++++++
> >   drivers/usb/host/xhci-dbgcap.c                | 36 +++++++++++++++++++
> >   2 files changed, 47 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > index 071688dbd969..57ba37606f79 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
> > @@ -98,3 +98,14 @@ Description:
> >                  The default value is "0001".
> >                  The field length can be from 1 to 63 characters.
> >
> > +What:          /sys/bus/pci/drivers/xhci_hcd/.../dbc_iProduct
> > +Date:          October 2025
> > +Contact:       Łukasz Bartosik <ukaszb@chromium.org>
> > +Description:
> > +               The dbc_iProduct attribute allows to change the iProduct field
> > +               presented in the USB device descriptor by xhci debug device.
> > +               Value can only be changed while debug capability (DbC) is in
> > +               disabled state to prevent USB device descriptor change while
> > +               connected to a USB host.
> > +               The default value is "Linux USB Debug Target".
> > +               The field length can be from 1 to 63 characters.
> > diff --git a/drivers/usb/host/xhci-dbgcap.c b/drivers/usb/host/xhci-dbgcap.c
> > index 7ad83548ba4d..bc782f6b533e 100644
> > --- a/drivers/usb/host/xhci-dbgcap.c
> > +++ b/drivers/usb/host/xhci-dbgcap.c
> > @@ -1200,6 +1200,40 @@ static ssize_t dbc_bcdDevice_store(struct device *dev,
> >       return size;
> >   }
> >
> > +static ssize_t dbc_iProduct_show(struct device *dev,
> > +                              struct device_attribute *attr,
> > +                              char *buf)
> > +{
> > +     struct xhci_hcd *xhci = hcd_to_xhci(dev_get_drvdata(dev));
> > +     struct xhci_dbc *dbc = xhci->dbc;
> > +
> > +     return sysfs_emit(buf, "%s\n", dbc->str.product);
> > +}
> > +
> > +static ssize_t dbc_iProduct_store(struct device *dev,
> > +                               struct device_attribute *attr,
> > +                               const char *buf, size_t size)
> > +{
> > +     struct xhci_hcd *xhci = hcd_to_xhci(dev_get_drvdata(dev));
> > +     struct xhci_dbc *dbc = xhci->dbc;
> > +     size_t len;
> > +
> > +     if (dbc->state != DS_DISABLED)
> > +             return -EBUSY;
> > +
> > +     len = strcspn(buf, "\n");
> > +     if (!len)
> > +             return -EINVAL;
> > +
> > +     if (len > USB_MAX_STRING_LEN/2)
> > +             return -E2BIG;
> > +
> > +     memcpy(dbc->str.product, buf, len);
> > +     dbc->str.product[len] = '\0';
> > +
> > +     return size;
> > +}
> > +
> >   static ssize_t dbc_iSerial_show(struct device *dev,
> >                           struct device_attribute *attr,
> >                           char *buf)
> > @@ -1321,6 +1355,7 @@ static DEVICE_ATTR_RW(dbc);
> >   static DEVICE_ATTR_RW(dbc_idVendor);
> >   static DEVICE_ATTR_RW(dbc_idProduct);
> >   static DEVICE_ATTR_RW(dbc_bcdDevice);
> > +static DEVICE_ATTR_RW(dbc_iProduct);
> >   static DEVICE_ATTR_RW(dbc_iSerial);
> >   static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
> >   static DEVICE_ATTR_RW(dbc_poll_interval_ms);
> > @@ -1330,6 +1365,7 @@ static struct attribute *dbc_dev_attrs[] = {
> >       &dev_attr_dbc_idVendor.attr,
> >       &dev_attr_dbc_idProduct.attr,
> >       &dev_attr_dbc_bcdDevice.attr,
> > +     &dev_attr_dbc_iProduct.attr,
>
> Small naming thing again.
>
> "dbc_idProduct" and dbc_iProduct" are a bit too similar.
>
> Usb core uses "product" and "manufacturer" sysfs entries to show these strings.
> I think dbc should use similar "dbc_product" and "dbc_manufacturer" naming.
>

I will rename it in patchset v3.

> the "iProduct" is a byte in device descriptor telling the index of the product string
> descriptor.
>

The same applies to iSerial then. Should I rename it to "dbc_serial" as well ?

Thanks,
Łukasz

> Thanks
> Mathias
>

Re: [PATCH v2 3/4] xhci: dbc: allow to set product name through sysfs

[Mathias Nyman]

On 10/14/25 01:19, Łukasz Bartosik wrote:
> On Mon, Oct 13, 2025 at 11:01 AM Mathias Nyman
> <mathias.nyman@linux.intel.com> wrote:
>>
>> On 10/8/25 00:39, Łukasz Bartosik wrote:
>>> From: Łukasz Bartosik <ukaszb@chromium.org>
>>>
>>> Add code which allows to set product name of a DbC
>>> device through sysfs.
>>>
>>> Signed-off-by: Łukasz Bartosik <ukaszb@chromium.org>
>>> ---
>>> @@ -1321,6 +1355,7 @@ static DEVICE_ATTR_RW(dbc);
>>>    static DEVICE_ATTR_RW(dbc_idVendor);
>>>    static DEVICE_ATTR_RW(dbc_idProduct);
>>>    static DEVICE_ATTR_RW(dbc_bcdDevice);
>>> +static DEVICE_ATTR_RW(dbc_iProduct);
>>>    static DEVICE_ATTR_RW(dbc_iSerial);
>>>    static DEVICE_ATTR_RW(dbc_bInterfaceProtocol);
>>>    static DEVICE_ATTR_RW(dbc_poll_interval_ms);
>>> @@ -1330,6 +1365,7 @@ static struct attribute *dbc_dev_attrs[] = {
>>>        &dev_attr_dbc_idVendor.attr,
>>>        &dev_attr_dbc_idProduct.attr,
>>>        &dev_attr_dbc_bcdDevice.attr,
>>> +     &dev_attr_dbc_iProduct.attr,
>>
>> Small naming thing again.
>>
>> "dbc_idProduct" and dbc_iProduct" are a bit too similar.
>>
>> Usb core uses "product" and "manufacturer" sysfs entries to show these strings.
>> I think dbc should use similar "dbc_product" and "dbc_manufacturer" naming.
>>
> 
> I will rename it in patchset v3.
> 
>> the "iProduct" is a byte in device descriptor telling the index of the product string
>> descriptor.
>>
> 
> The same applies to iSerial then. Should I rename it to "dbc_serial" as well ?

I think that would work better yes

Thanks
Mathias