* [PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues @ 2024-12-31 16:15 Xiong Nandi 2024-12-31 16:15 ` [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb Xiong Nandi 2024-12-31 16:15 ` [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u Xiong Nandi 0 siblings, 2 replies; 11+ messages in thread From: Xiong Nandi @ 2024-12-31 16:15 UTC (permalink / raw) Cc: Xiong Nandi, Hongren Zheng, open list, open list:USB OVER IP DRIVER Xiong Nandi (2): usbip: Fix seqnum sign extension issue in vhci_tx_urb usbip: Correct format specifier for seqnum from %d to %u drivers/usb/usbip/stub_rx.c | 2 +- drivers/usb/usbip/stub_tx.c | 2 +- drivers/usb/usbip/vhci_hcd.c | 2 +- drivers/usb/usbip/vhci_rx.c | 6 +++--- drivers/usb/usbip/vudc_tx.c | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2024-12-31 16:15 [PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues Xiong Nandi @ 2024-12-31 16:15 ` Xiong Nandi 2025-01-02 22:06 ` Shuah Khan 2025-01-15 21:14 ` Shuah Khan 2024-12-31 16:15 ` [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u Xiong Nandi 1 sibling, 2 replies; 11+ messages in thread From: Xiong Nandi @ 2024-12-31 16:15 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: Xiong Nandi, open list:USB OVER IP DRIVER, open list The atomic_inc_return function returns an int, while priv->seqnum is an unsigned long. So we must cast the result to u32 to prevent potential sign extension and size mismatch issues. Signed-off-by: Xiong Nandi <xndchn@gmail.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 b03e5021c25b..f3f260e01791 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) spin_lock_irqsave(&vdev->priv_lock, flags); - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); if (priv->seqnum == 0xffff) dev_info(&urb->dev->dev, "seqnum max\n"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2024-12-31 16:15 ` [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb Xiong Nandi @ 2025-01-02 22:06 ` Shuah Khan 2025-01-03 15:18 ` xndcn 2025-01-15 21:14 ` Shuah Khan 1 sibling, 1 reply; 11+ messages in thread From: Shuah Khan @ 2025-01-02 22:06 UTC (permalink / raw) To: Xiong Nandi, Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: open list:USB OVER IP DRIVER, open list, Shuah Khan On 12/31/24 09:15, Xiong Nandi wrote: > The atomic_inc_return function returns an int, while priv->seqnum is an > unsigned long. So we must cast the result to u32 to prevent potential > sign extension and size mismatch issues. > How did you find the problem? > Signed-off-by: Xiong Nandi <xndchn@gmail.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 b03e5021c25b..f3f260e01791 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > spin_lock_irqsave(&vdev->priv_lock, flags); > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); Why does it make sense to cast it to u32? > if (priv->seqnum == 0xffff) > dev_info(&urb->dev->dev, "seqnum max\n"); thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2025-01-02 22:06 ` Shuah Khan @ 2025-01-03 15:18 ` xndcn 2025-01-09 16:02 ` Shuah Khan 0 siblings, 1 reply; 11+ messages in thread From: xndcn @ 2025-01-03 15:18 UTC (permalink / raw) To: Shuah Khan Cc: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman, open list:USB OVER IP DRIVER, open list Thanks. > How did you find the problem? > Why does it make sense to cast it to u32? After running with usbip enough time, I happened to see logs like this: > [ 293.863125] vhci_hcd vhci_hcd.0: Device attached > [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 > [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > [ 294.204850] vhci_hcd: stop threads > [ 294.204851] vhci_hcd: release socket > [ 294.204853] vhci_hcd: disconnect device Then I notice that on 64bit platform, when atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or 0x80000000), priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be extends to 0xffffffff80000000 So we can fix the issue by cast it to u32. On Fri, Jan 3, 2025 at 6:06 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 12/31/24 09:15, Xiong Nandi wrote: > > The atomic_inc_return function returns an int, while priv->seqnum is an > > unsigned long. So we must cast the result to u32 to prevent potential > > sign extension and size mismatch issues. > > > > How did you find the problem? > > Signed-off-by: Xiong Nandi <xndchn@gmail.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 b03e5021c25b..f3f260e01791 100644 > > --- a/drivers/usb/usbip/vhci_hcd.c > > +++ b/drivers/usb/usbip/vhci_hcd.c > > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > > > spin_lock_irqsave(&vdev->priv_lock, flags); > > > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); > > Why does it make sense to cast it to u32? > > > if (priv->seqnum == 0xffff) > > dev_info(&urb->dev->dev, "seqnum max\n"); > > > thanks, > -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2025-01-03 15:18 ` xndcn @ 2025-01-09 16:02 ` Shuah Khan 2025-01-13 13:29 ` xndcn 0 siblings, 1 reply; 11+ messages in thread From: Shuah Khan @ 2025-01-09 16:02 UTC (permalink / raw) To: xndcn Cc: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman, open list:USB OVER IP DRIVER, open list, Shuah Khan On 1/3/25 08:18, xndcn wrote: > Thanks. > >> How did you find the problem? >> Why does it make sense to cast it to u32? > > After running with usbip enough time, I happened to see logs like this: >> [ 293.863125] vhci_hcd vhci_hcd.0: Device attached >> [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd >> [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 >> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 >> [ 294.204850] vhci_hcd: stop threads >> [ 294.204851] vhci_hcd: release socket >> [ 294.204853] vhci_hcd: disconnect device > > Then I notice that on 64bit platform, when > atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or > 0x80000000), > priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be > extends to 0xffffffff80000000 > So we can fix the issue by cast it to u32. > Can you send me the dmesg without and with your patch? thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2025-01-09 16:02 ` Shuah Khan @ 2025-01-13 13:29 ` xndcn 0 siblings, 0 replies; 11+ messages in thread From: xndcn @ 2025-01-13 13:29 UTC (permalink / raw) To: Shuah Khan Cc: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman, open list:USB OVER IP DRIVER, open list without the patch: > [ 384.276605] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 384.278487] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 1 > [ 384.278509] vhci_hcd: created sysfs vhci_hcd.0 > [ 384.278532] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.12 > [ 384.278535] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 384.278536] usb usb1: Product: USB/IP Virtual Host Controller > [ 384.278538] usb usb1: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 384.278539] usb usb1: SerialNumber: vhci_hcd.0 > [ 384.278630] hub 1-0:1.0: USB hub found > [ 384.278637] hub 1-0:1.0: 8 ports detected > [ 384.278740] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 384.278781] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 2 > [ 384.278788] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > [ 384.278801] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, bcdDevice= 6.12 > [ 384.278802] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 384.278803] usb usb2: Product: USB/IP Virtual Host Controller > [ 384.278804] usb usb2: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 384.278805] usb usb2: SerialNumber: vhci_hcd.0 > [ 384.278866] hub 2-0:1.0: USB hub found > [ 384.278869] hub 2-0:1.0: 8 ports detected > [ 384.279071] insmod (400) used greatest stack depth: 11960 bytes left > [ 550.127351] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3) > [ 550.127356] vhci_hcd vhci_hcd.0: devid(0) speed(3) speed_str(high-speed) > [ 550.127359] vhci_hcd vhci_hcd.0: Device attached > [ 550.341007] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 550.452995] usb 1-1: SetAddress Request (2) to port 0 > [ 550.464332] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > [ 550.464842] vhci_hcd: stop threads > [ 550.464843] vhci_hcd: release socket > [ 550.464845] vhci_hcd: disconnect device and with this patch: > [ 746.253823] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 746.254660] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 1 > [ 746.254669] vhci_hcd: created sysfs vhci_hcd.0 > [ 746.254895] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 6.12 > [ 746.254897] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 746.254898] usb usb1: Product: USB/IP Virtual Host Controller > [ 746.254899] usb usb1: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 746.254899] usb usb1: SerialNumber: vhci_hcd.0 > [ 746.254964] hub 1-0:1.0: USB hub found > [ 746.254985] hub 1-0:1.0: 8 ports detected > [ 746.255042] vhci_hcd vhci_hcd.0: USB/IP Virtual Host Controller > [ 746.255066] vhci_hcd vhci_hcd.0: new USB bus registered, assigned bus number 2 > [ 746.255072] usb usb2: We don't know the algorithms for LPM for this host, disabling LPM. > [ 746.255081] usb usb2: New USB device found, idVendor=1d6b, idProduct=0003, bcdDevice= 6.12 > [ 746.255082] usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1 > [ 746.255083] usb usb2: Product: USB/IP Virtual Host Controller > [ 746.255089] usb usb2: Manufacturer: Linux 6.12.0-00971-g158f238aa69d-dirty vhci_hcd > [ 746.255089] usb usb2: SerialNumber: vhci_hcd.0 > [ 746.255118] hub 2-0:1.0: USB hub found > [ 746.255120] hub 2-0:1.0: 8 ports detected > [ 756.967922] vhci_hcd vhci_hcd.0: pdev(0) rhport(0) sockfd(3) > [ 756.967928] vhci_hcd vhci_hcd.0: devid(0) speed(3) speed_str(high-speed) > [ 756.967933] vhci_hcd vhci_hcd.0: Device attached > [ 757.184367] usb 1-1: new high-speed USB device number 2 using vhci_hcd > [ 757.296479] usb 1-1: SetAddress Request (2) to port 0 > [ 757.309845] usb 1-1: New USB device found, idVendor=1234, idProduct=1234, bcdDevice= 2.80 > [ 757.309848] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > [ 757.309849] usb 1-1: Product: foo > [ 757.309850] usb 1-1: Manufacturer: bar > [ 757.309851] usb 1-1: SerialNumber: 0 to make the bug easier to reproduce ,I have changed the initial value of segnum: diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 8dac1edc74d4..92a60e89b459 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -1192,7 +1192,7 @@ static int vhci_start(struct usb_hcd *hcd) vdev->rhport = rhport; } - atomic_set(&vhci_hcd->seqnum, 0); + atomic_set(&vhci_hcd->seqnum, 2147483646); hcd->power_budget = 0; /* no limit */ hcd->uses_new_polling = 1; On Fri, Jan 10, 2025 at 12:02 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 1/3/25 08:18, xndcn wrote: > > Thanks. > > > >> How did you find the problem? > >> Why does it make sense to cast it to u32? > > > > After running with usbip enough time, I happened to see logs like this: > >> [ 293.863125] vhci_hcd vhci_hcd.0: Device attached > >> [ 294.081110] usb 1-1: new high-speed USB device number 2 using vhci_hcd > >> [ 294.193163] usb 1-1: SetAddress Request (2) to port 0 > >> [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 > >> [ 294.204850] vhci_hcd: stop threads > >> [ 294.204851] vhci_hcd: release socket > >> [ 294.204853] vhci_hcd: disconnect device > > > > Then I notice that on 64bit platform, when > > atomic_inc_return(&vhci_hcd->seqnum) returns (2147483647 + 1, or > > 0x80000000), > > priv->seqnum (which is unsigned long, i.e. u64 on 64bit) will be > > extends to 0xffffffff80000000 > > So we can fix the issue by cast it to u32. > > > > Can you send me the dmesg without and with your patch? > > thanks, > -- Shuah > ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb 2024-12-31 16:15 ` [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb Xiong Nandi 2025-01-02 22:06 ` Shuah Khan @ 2025-01-15 21:14 ` Shuah Khan 1 sibling, 0 replies; 11+ messages in thread From: Shuah Khan @ 2025-01-15 21:14 UTC (permalink / raw) To: Xiong Nandi, Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: open list:USB OVER IP DRIVER, open list, Shuah Khan On 12/31/24 09:15, Xiong Nandi wrote: > The atomic_inc_return function returns an int, while priv->seqnum is an > unsigned long. So we must cast the result to u32 to prevent potential > sign extension and size mismatch issues. > > Signed-off-by: Xiong Nandi <xndchn@gmail.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 b03e5021c25b..f3f260e01791 100644 > --- a/drivers/usb/usbip/vhci_hcd.c > +++ b/drivers/usb/usbip/vhci_hcd.c > @@ -675,7 +675,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) > > spin_lock_irqsave(&vdev->priv_lock, flags); > > - priv->seqnum = atomic_inc_return(&vhci_hcd->seqnum); > + priv->seqnum = (u32)atomic_inc_return(&vhci_hcd->seqnum); > if (priv->seqnum == 0xffff) > dev_info(&urb->dev->dev, "seqnum max\n"); > Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u 2024-12-31 16:15 [PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues Xiong Nandi 2024-12-31 16:15 ` [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb Xiong Nandi @ 2024-12-31 16:15 ` Xiong Nandi 2025-01-02 22:09 ` Shuah Khan 2025-01-15 21:14 ` Shuah Khan 1 sibling, 2 replies; 11+ messages in thread From: Xiong Nandi @ 2024-12-31 16:15 UTC (permalink / raw) To: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: Xiong Nandi, open list:USB OVER IP DRIVER, open list The seqnum field in USBIP protocol is an unsigned value. So we fix the format specifier from %d to %u to correctly display the value. Signed-off-by: Xiong Nandi <xndchn@gmail.com> --- drivers/usb/usbip/stub_rx.c | 2 +- drivers/usb/usbip/stub_tx.c | 2 +- drivers/usb/usbip/vhci_rx.c | 6 +++--- drivers/usb/usbip/vudc_tx.c | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c index 6338d818bc8b..9aa30ef76f3b 100644 --- a/drivers/usb/usbip/stub_rx.c +++ b/drivers/usb/usbip/stub_rx.c @@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev, return 0; } - usbip_dbg_stub_rx("seqnum %d is not pending\n", + usbip_dbg_stub_rx("seqnum %u is not pending\n", pdu->u.cmd_unlink.seqnum); /* diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c index b1c2f6781cb3..7eb2e074012a 100644 --- a/drivers/usb/usbip/stub_tx.c +++ b/drivers/usb/usbip/stub_tx.c @@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev) /* 1. setup usbip_header */ setup_ret_submit_pdu(&pdu_header, urb); - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", pdu_header.base.seqnum); if (priv->sgl) { diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c index 7f2d1c241559..a75f4a898a41 100644 --- a/drivers/usb/usbip/vhci_rx.c +++ b/drivers/usb/usbip/vhci_rx.c @@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, spin_unlock_irqrestore(&vdev->priv_lock, flags); if (!urb) { - pr_err("cannot find a urb of seqnum %u max seqnum %d\n", + pr_err("cannot find a urb of seqnum %u max seqnum %u\n", pdu->base.seqnum, atomic_read(&vhci_hcd->seqnum)); usbip_event_add(ud, VDEV_EVENT_ERROR_TCP); @@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev, * already received the result of its submit result and gave * back the URB. */ - pr_info("the urb (seqnum %d) was already given back\n", + pr_info("the urb (seqnum %u) was already given back\n", pdu->base.seqnum); } else { - usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum); + usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum); /* If unlink is successful, status is -ECONNRESET */ urb->status = pdu->u.ret_unlink.status; diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c index 3ccb17c3e840..30c11bf9f4e7 100644 --- a/drivers/usb/usbip/vudc_tx.c +++ b/drivers/usb/usbip/vudc_tx.c @@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p) /* 1. setup usbip_header */ setup_ret_submit_pdu(&pdu_header, urb_p); - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", pdu_header.base.seqnum); usbip_header_correct_endian(&pdu_header, 1); -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u 2024-12-31 16:15 ` [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u Xiong Nandi @ 2025-01-02 22:09 ` Shuah Khan 2025-01-03 15:25 ` xndcn 2025-01-15 21:14 ` Shuah Khan 1 sibling, 1 reply; 11+ messages in thread From: Shuah Khan @ 2025-01-02 22:09 UTC (permalink / raw) To: Xiong Nandi, Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: open list:USB OVER IP DRIVER, open list, Shuah Khan On 12/31/24 09:15, Xiong Nandi wrote: > The seqnum field in USBIP protocol is an unsigned value. > So we fix the format specifier from %d to %u to correctly > display the value. > How did you find the problem? Include log from the tool or compiler output. > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > --- > drivers/usb/usbip/stub_rx.c | 2 +- > drivers/usb/usbip/stub_tx.c | 2 +- > drivers/usb/usbip/vhci_rx.c | 6 +++--- > drivers/usb/usbip/vudc_tx.c | 2 +- > 4 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c > index 6338d818bc8b..9aa30ef76f3b 100644 > --- a/drivers/usb/usbip/stub_rx.c > +++ b/drivers/usb/usbip/stub_rx.c > @@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev, > return 0; > } > > - usbip_dbg_stub_rx("seqnum %d is not pending\n", > + usbip_dbg_stub_rx("seqnum %u is not pending\n", > pdu->u.cmd_unlink.seqnum); seqnum is unsigned long - don't you have to use %ul? > > /* > diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c > index b1c2f6781cb3..7eb2e074012a 100644 > --- a/drivers/usb/usbip/stub_tx.c > +++ b/drivers/usb/usbip/stub_tx.c > @@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev) > > /* 1. setup usbip_header */ > setup_ret_submit_pdu(&pdu_header, urb); > - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", > + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", > pdu_header.base.seqnum); > > if (priv->sgl) { > diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c > index 7f2d1c241559..a75f4a898a41 100644 > --- a/drivers/usb/usbip/vhci_rx.c > +++ b/drivers/usb/usbip/vhci_rx.c > @@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, > spin_unlock_irqrestore(&vdev->priv_lock, flags); > > if (!urb) { > - pr_err("cannot find a urb of seqnum %u max seqnum %d\n", > + pr_err("cannot find a urb of seqnum %u max seqnum %u\n", > pdu->base.seqnum, > atomic_read(&vhci_hcd->seqnum)); > usbip_event_add(ud, VDEV_EVENT_ERROR_TCP); > @@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev, > * already received the result of its submit result and gave > * back the URB. > */ > - pr_info("the urb (seqnum %d) was already given back\n", > + pr_info("the urb (seqnum %u) was already given back\n", > pdu->base.seqnum); > } else { > - usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum); > + usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum); > > /* If unlink is successful, status is -ECONNRESET */ > urb->status = pdu->u.ret_unlink.status; > diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c > index 3ccb17c3e840..30c11bf9f4e7 100644 > --- a/drivers/usb/usbip/vudc_tx.c > +++ b/drivers/usb/usbip/vudc_tx.c > @@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p) > > /* 1. setup usbip_header */ > setup_ret_submit_pdu(&pdu_header, urb_p); > - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", > + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", > pdu_header.base.seqnum); > usbip_header_correct_endian(&pdu_header, 1); > thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u 2025-01-02 22:09 ` Shuah Khan @ 2025-01-03 15:25 ` xndcn 0 siblings, 0 replies; 11+ messages in thread From: xndcn @ 2025-01-03 15:25 UTC (permalink / raw) To: Shuah Khan Cc: Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman, open list:USB OVER IP DRIVER, open list Thanks. > How did you find the problem? Include log from the tool > or compiler output. As said in [Patch 1/2], I saw this log: > [ 294.204334] vhci_hcd: cannot find a urb of seqnum 2147483648 max seqnum -2147483648 then I found there are also logs with wrong format specifier for seqnum > seqnum is unsigned long - don't you have to use %ul? pdu->u.cmd_unlink.seqnum is defined as u32 actually. In usbip driver, all seqnum in protocol struct (struct usbip_header_basic and struct usbip_header_cmd_unlink), is defined as u32. In other driver specific priv struct, seqnum is defined as unsigned long. It seems only the u32 seqnum have wrong format specifier On Fri, Jan 3, 2025 at 6:09 AM Shuah Khan <skhan@linuxfoundation.org> wrote: > > On 12/31/24 09:15, Xiong Nandi wrote: > > The seqnum field in USBIP protocol is an unsigned value. > > So we fix the format specifier from %d to %u to correctly > > display the value. > > > > How did you find the problem? Include log from the tool > or compiler output. > > > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > > --- > > drivers/usb/usbip/stub_rx.c | 2 +- > > drivers/usb/usbip/stub_tx.c | 2 +- > > drivers/usb/usbip/vhci_rx.c | 6 +++--- > > drivers/usb/usbip/vudc_tx.c | 2 +- > > 4 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c > > index 6338d818bc8b..9aa30ef76f3b 100644 > > --- a/drivers/usb/usbip/stub_rx.c > > +++ b/drivers/usb/usbip/stub_rx.c > > @@ -269,7 +269,7 @@ static int stub_recv_cmd_unlink(struct stub_device *sdev, > > return 0; > > } > > > > - usbip_dbg_stub_rx("seqnum %d is not pending\n", > > + usbip_dbg_stub_rx("seqnum %u is not pending\n", > > pdu->u.cmd_unlink.seqnum); > > seqnum is unsigned long - don't you have to use %ul? > > > > /* > > diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c > > index b1c2f6781cb3..7eb2e074012a 100644 > > --- a/drivers/usb/usbip/stub_tx.c > > +++ b/drivers/usb/usbip/stub_tx.c > > @@ -201,7 +201,7 @@ static int stub_send_ret_submit(struct stub_device *sdev) > > > > /* 1. setup usbip_header */ > > setup_ret_submit_pdu(&pdu_header, urb); > > - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", > > + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", > > pdu_header.base.seqnum); > > > > if (priv->sgl) { > > diff --git a/drivers/usb/usbip/vhci_rx.c b/drivers/usb/usbip/vhci_rx.c > > index 7f2d1c241559..a75f4a898a41 100644 > > --- a/drivers/usb/usbip/vhci_rx.c > > +++ b/drivers/usb/usbip/vhci_rx.c > > @@ -66,7 +66,7 @@ static void vhci_recv_ret_submit(struct vhci_device *vdev, > > spin_unlock_irqrestore(&vdev->priv_lock, flags); > > > > if (!urb) { > > - pr_err("cannot find a urb of seqnum %u max seqnum %d\n", > > + pr_err("cannot find a urb of seqnum %u max seqnum %u\n", > > pdu->base.seqnum, > > atomic_read(&vhci_hcd->seqnum)); > > usbip_event_add(ud, VDEV_EVENT_ERROR_TCP); > > @@ -162,10 +162,10 @@ static void vhci_recv_ret_unlink(struct vhci_device *vdev, > > * already received the result of its submit result and gave > > * back the URB. > > */ > > - pr_info("the urb (seqnum %d) was already given back\n", > > + pr_info("the urb (seqnum %u) was already given back\n", > > pdu->base.seqnum); > > } else { > > - usbip_dbg_vhci_rx("now giveback urb %d\n", pdu->base.seqnum); > > + usbip_dbg_vhci_rx("now giveback urb %u\n", pdu->base.seqnum); > > > > /* If unlink is successful, status is -ECONNRESET */ > > urb->status = pdu->u.ret_unlink.status; > > diff --git a/drivers/usb/usbip/vudc_tx.c b/drivers/usb/usbip/vudc_tx.c > > index 3ccb17c3e840..30c11bf9f4e7 100644 > > --- a/drivers/usb/usbip/vudc_tx.c > > +++ b/drivers/usb/usbip/vudc_tx.c > > @@ -107,7 +107,7 @@ static int v_send_ret_submit(struct vudc *udc, struct urbp *urb_p) > > > > /* 1. setup usbip_header */ > > setup_ret_submit_pdu(&pdu_header, urb_p); > > - usbip_dbg_stub_tx("setup txdata seqnum: %d\n", > > + usbip_dbg_stub_tx("setup txdata seqnum: %u\n", > > pdu_header.base.seqnum); > > usbip_header_correct_endian(&pdu_header, 1); > > > thanks, > -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u 2024-12-31 16:15 ` [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u Xiong Nandi 2025-01-02 22:09 ` Shuah Khan @ 2025-01-15 21:14 ` Shuah Khan 1 sibling, 0 replies; 11+ messages in thread From: Shuah Khan @ 2025-01-15 21:14 UTC (permalink / raw) To: Xiong Nandi, Valentina Manea, Shuah Khan, Hongren Zheng, Greg Kroah-Hartman Cc: open list:USB OVER IP DRIVER, open list, Shuah Khan On 12/31/24 09:15, Xiong Nandi wrote: > The seqnum field in USBIP protocol is an unsigned value. > So we fix the format specifier from %d to %u to correctly > display the value. > > Signed-off-by: Xiong Nandi <xndchn@gmail.com> > --- > drivers/usb/usbip/stub_rx.c | 2 +- > drivers/usb/usbip/stub_tx.c | 2 +- > drivers/usb/usbip/vhci_rx.c | 6 +++--- > drivers/usb/usbip/vudc_tx.c | 2 +- > 4 files changed, 6 insertions(+), 6 deletions(-) > Acked-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-01-15 21:14 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-31 16:15 [PATCH 0/2] usbip: Fix seqnum sign extension and format specifier issues Xiong Nandi 2024-12-31 16:15 ` [PATCH 1/2] usbip: Fix seqnum sign extension issue in vhci_tx_urb Xiong Nandi 2025-01-02 22:06 ` Shuah Khan 2025-01-03 15:18 ` xndcn 2025-01-09 16:02 ` Shuah Khan 2025-01-13 13:29 ` xndcn 2025-01-15 21:14 ` Shuah Khan 2024-12-31 16:15 ` [PATCH 2/2] usbip: Correct format specifier for seqnum from %d to %u Xiong Nandi 2025-01-02 22:09 ` Shuah Khan 2025-01-03 15:25 ` xndcn 2025-01-15 21:14 ` Shuah Khan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox