Page MenuHomeFreeBSD

Enable umodem clear stall on open
ClosedPublic

Authored by daveb_spectralogic.com on Aug 29 2022, 8:56 PM.
Tags
None
Referenced Files
F86593798: D36391.id109982.diff
Sat, Jun 22, 6:14 PM
Unknown Object (File)
Wed, May 29, 9:55 PM
Unknown Object (File)
May 8 2024, 10:49 AM
Unknown Object (File)
May 8 2024, 10:49 AM
Unknown Object (File)
May 8 2024, 10:48 AM
Unknown Object (File)
May 8 2024, 10:48 AM
Unknown Object (File)
May 8 2024, 9:06 AM
Unknown Object (File)
Mar 9 2024, 7:42 AM
Subscribers

Details

Summary

For the following USB serial device:
umodem0: <STMicroelectronics STM32 Virtual ComPort, class 2/2, rev 2.00/2.00, addr 1> on usbus0

Attached via the following USB 3.0 controller:
xhci0: <AMD Starship USB 3.0 controller> mem 0xa0000000-0xa00fffff irq 103 at device 0.3 numa-domain 2 on pci18

A usage model that behaves as follows always fails to receive the response on the second iteration:

  • open("/dev/cuaU0", ...);
  • write(fd, cmd, sizeof(cmd));
  • read(fd, response, sizeof(response);
  • close(fd);

The first attempt after a boot will succeed; the second will fail to receive the response,
The third will succeed, the fourth fail, ....

Preceeding each open(2) with the following command is a workaround:
usbconfig -d <bus>.<address> reset

A better solution is to enable usb/serial/umodem.c to clear stall on ucom_cfg_open().
In order for this to work 100%, usb/serial/usb_serial.c@ucom_queue_command() must
wait for ucom_cfg_open() to execute as it does for ucom_cfg_close().

Test Plan

For the open/write/read/close usage model described above, tested
over 15k iterations on 2 separate platforms with the hardware specified above.

Similarily for the following older USB controler
ehci0: <Intel Patsburg USB 2.0 controller> mem 0xde423000-0xde4233ff irq 16 at device 26.0 numa-domain 0 on pci0

Note: prior to the proposed change, the Patsburg controller did not experience
failures under the open/write/read/close model; it continues to operate normally
with the change.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/dev/usb/serial/usb_serial.c
737 ↗(On Diff #109972)

Can you explain why moving this flag is needed?

sys/dev/usb/serial/usb_serial.c
737 ↗(On Diff #109972)

Admittedly a somewhat gratuitous change. Logically, don't set the flag before the operation is known to be complete. I can revert.

sys/dev/usb/serial/usb_serial.c
737 ↗(On Diff #109972)

Yes, please revert this chunk. Else looks good. I will check the history of umodem.c a bit first, if we had some clear stall logic before or not .... Stay tuned.

Move setting of UCOM_FLAG_LL_READY back to original location.

hselasky added inline comments.
sys/dev/usb/serial/usb_serial.c
633 ↗(On Diff #109982)

I think this chunk is also not needed. We'll wait for the open during the next fn check.

This revision is now accepted and ready to land.Aug 30 2022, 1:53 PM
This revision was automatically updated to reflect the committed changes.
sys/dev/usb/serial/usb_serial.c
633 ↗(On Diff #109982)

Without waiting, some open/write/read/close cycles fail; with waiting all cycles succeeded.

Please test the final patch. Let me know if it work too.

Thank you!

sys/dev/usb/serial/usb_serial.c
633 ↗(On Diff #109982)

Can you make that a separate patch, because this is a different issue than the clear-stall one!

Never mind. I'll just re-use this issue.

This differential revision now contain two patches if you look at the top, but only the first is shown.

I believe setting stall at attach is superfluous when you set it at open!

Let me know how the testing goes!

Thank you!

--HPS