* [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected @ 2019-11-29 14:08 Erkka Talvitie 2019-12-02 19:43 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-11-29 14:08 UTC (permalink / raw) To: stern, gregkh; +Cc: linux-usb, claus.baumgartner, Erkka Talvitie When disconnecting a USB hub that has some child device(s) connected to it (such as a USB mouse), then the stack tries to clear halt and reset device(s) which are _already_ physically disconnected. The issue has been reproduced with: CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. SW: U-Boot 2019.07 and kernel 4.19.40. In this situation there will be error bit for MMF active yet the CERR equals EHCI_TUNE_CERR + halt. Existing implementation interprets this as a stall [1] (chapter 8.4.5). Fix for the issue is at first to check for a stall that comes after an error (the CERR has been decreased). Then after that, check for other errors. And at last check for stall without other errors (the CERR equals EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). What happens after the fix is that when disconnecting a hub with attached device(s) the situation is not interpret as a stall. The specification [2] is not clear about error priorities, but since there is no explicit error bit for the stall, it is assumed to be lower priority than other errors. [1] https://www.usb.org/document-library/usb-20-specification, usb_20.pdf [2] https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/ehci-specification-for-usb.pdf Signed-off-by: Erkka Talvitie <erkka.talvitie@vincit.fi> --- Following test steps were executed to first demonstrate the issue and then to verify the fix: --> Attach a mouse into host port and then disconnect it. --> Attach a keyboard into host port and then disconnect it. --> Attach a KVM switch into host port and then disconnect it. --> Attach a KVM switch into host port, attach a mouse into the KVM and then disconnect it. --> Re-attach the mouse into the KVM and then disconnect the KVM switch. --> Attach a KVM switch into host port, attach a keyboard into the KVM and then disconnect it. --> Re-attach the keyboard into the KVM and then disconnect the KVM switch. --> Attach a KVM switch into host port, attach a mouse and a keyboard into the KVM and then disconnect the KVM switch. --> End test. Output without the fix: --> Attach a mouse into host port and then disconnect it. kernel: [ 374.409837] usb 2-1.2: new low-speed USB device number 16 using ci_hdrc kernel: [ 374.673010] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2:1.0/0003:046D:C05F.0006/input/input7 kernel: [ 374.674044] hid-generic 0003:046D:C05F.0006: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2/input0 weston[464]: [11:14:07.717] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[464]: [11:14:07.717] event6 - Logitech USB Optical Mouse: device is a pointer weston[464]: [11:14:07.718] libinput: configuring device "Logitech USB Optical Mouse". weston[464]: [11:14:07.718] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 378.931298] usb 2-1.2: USB disconnect, device number 16 weston[464]: [11:14:11.668] event6 - Logitech USB Optical Mouse: device removed --> Attach a keyboard into host port and then disconnect it. kernel: [ 400.759884] usb 2-1.2: new low-speed USB device number 17 using ci_hdrc kernel: [ 401.025695] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2:1.0/0003:413C:2106.0007/input/input8 kernel: [ 401.090413] hid-generic 0003:413C:2106.0007: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2/input0 systemd-logind[392]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[464]: [11:14:33.992] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[464]: [11:14:33.994] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[464]: [11:14:33.995] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[464]: [11:14:33.996] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 404.230882] usb 2-1.2: USB disconnect, device number 17 weston[464]: [11:14:37.047] event6 - Dell Dell QuietKey Keyboard: device removed --> Attach a KVM switch into host port and then disconnect it. kernel: [ 424.059844] usb 2-1.2: new high-speed USB device number 18 using ci_hdrc kernel: [ 424.315581] hub 2-1.2:1.0: USB hub found kernel: [ 424.316179] hub 2-1.2:1.0: 4 ports detected kernel: [ 428.268438] usb 2-1.2: USB disconnect, device number 18 --> Attach a KVM switch into host port, attach a mouse into the KVM and then disconnect it. kernel: [ 443.509853] usb 2-1.2: new high-speed USB device number 19 using ci_hdrc kernel: [ 443.763137] hub 2-1.2:1.0: USB hub found kernel: [ 443.763860] hub 2-1.2:1.0: 4 ports detected kernel: [ 448.299911] usb 2-1.2.2: new low-speed USB device number 20 using ci_hdrc kernel: [ 448.463160] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.0008/input/input9 kernel: [ 448.463380] hid-generic 0003:046D:C05F.0008: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[464]: [11:15:21.508] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[464]: [11:15:21.508] event6 - Logitech USB Optical Mouse: device is a pointer weston[464]: [11:15:21.509] libinput: configuring device "Logitech USB Optical Mouse". weston[464]: [11:15:21.509] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 453.915419] usb 2-1.2.2: USB disconnect, device number 20 weston[464]: [11:15:26.655] event6 - Logitech USB Optical Mouse: device removed --> Re-attach the mouse into the KVM and then disconnect the KVM switch. kernel: [ 468.519841] usb 2-1.2.2: new low-speed USB device number 21 using ci_hdrc kernel: [ 468.683002] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.0009/input/input10 kernel: [ 468.683202] hid-generic 0003:046D:C05F.0009: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[464]: [11:15:41.727] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[464]: [11:15:41.728] event6 - Logitech USB Optical Mouse: device is a pointer weston[464]: [11:15:41.728] libinput: configuring device "Logitech USB Optical Mouse". weston[464]: [11:15:41.728] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 471.118091] usb 2-1.2: clear tt 1 (0150) error -71 kernel: [ 471.134041] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.138266] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.142515] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.146773] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.151013] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.151020] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 471.155259] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 471.159542] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.163765] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.168007] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.172267] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.176507] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.176512] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 471.180763] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 471.185022] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.189261] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.194198] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.198273] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.202514] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.202521] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 471.206757] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 471.211024] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.215257] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.219524] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.223767] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.228007] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 471.228013] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 471.232261] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 471.236514] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 471.240801] hub 2-1.2:1.0: hub_ext_port_status failed (err = -71) weston[464]: [11:15:43.974] event6 - Logitech USB Optical Mouse: device removed kernel: [ 471.276392] usb 2-1.2: USB disconnect, device number 19 kernel: [ 471.276404] usb 2-1.2.2: USB disconnect, device number 21 --> Attach a KVM switch into host port, attach a keyboard into the KVM and then disconnect it. kernel: [ 489.339839] usb 2-1.2: new high-speed USB device number 22 using ci_hdrc kernel: [ 489.602886] hub 2-1.2:1.0: USB hub found kernel: [ 489.603397] hub 2-1.2:1.0: 4 ports detected kernel: [ 493.869845] usb 2-1.2.2: new low-speed USB device number 23 using ci_hdrc kernel: [ 494.046950] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:413C:2106.000A/input/input11 kernel: [ 494.112909] hid-generic 0003:413C:2106.000A: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.2/input0 systemd-logind[392]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[464]: [11:16:07.033] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[464]: [11:16:07.038] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[464]: [11:16:07.040] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[464]: [11:16:07.040] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 498.303010] usb 2-1.2.2: USB disconnect, device number 23 weston[464]: [11:16:11.135] event6 - Dell Dell QuietKey Keyboard: device removed --> Re-attach the keyboard into the KVM and then disconnect the KVM switch. kernel: [ 510.509858] usb 2-1.2.2: new low-speed USB device number 24 using ci_hdrc kernel: [ 510.673159] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:413C:2106.000B/input/input12 kernel: [ 510.740488] hid-generic 0003:413C:2106.000B: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.2/input0 systemd-logind[392]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[464]: [11:16:23.669] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[464]: [11:16:23.670] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[464]: [11:16:23.672] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[464]: [11:16:23.673] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 513.808762] usb 2-1.2: clear tt 1 (0180) error -71 kernel: [ 513.824014] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.828254] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.832510] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.837034] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.841135] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.841141] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 513.845391] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 513.849575] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.853760] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.858009] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.862264] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.866514] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.866521] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 513.870812] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 513.875012] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.879253] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.883511] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.887763] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.892012] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.892019] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 513.896267] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 513.900572] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.904776] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.909004] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.913290] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.917512] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 513.917517] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 513.921769] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 513.926009] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 513.930372] hub 2-1.2:1.0: hub_ext_port_status failed (err = -71) weston[464]: [11:16:26.752] event6 - Dell Dell QuietKey Keyboard: device removed kernel: [ 514.028708] usb 2-1.2: USB disconnect, device number 22 kernel: [ 514.028721] usb 2-1.2.2: USB disconnect, device number 24 --> Attach a KVM switch into host port, attach a mouse and a keyboard into the KVM and then disconnect the KVM switch. kernel: [ 530.299896] usb 2-1.2: new high-speed USB device number 25 using ci_hdrc kernel: [ 530.553126] hub 2-1.2:1.0: USB hub found kernel: [ 530.553656] hub 2-1.2:1.0: 4 ports detected kernel: [ 534.580137] usb 2-1.2.2: new low-speed USB device number 26 using ci_hdrc kernel: [ 534.743092] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.000C/input/input13 kernel: [ 534.744022] hid-generic 0003:046D:C05F.000C: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[464]: [11:16:47.778] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[464]: [11:16:47.778] event6 - Logitech USB Optical Mouse: device is a pointer weston[464]: [11:16:47.779] libinput: configuring device "Logitech USB Optical Mouse". weston[464]: [11:16:47.779] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 536.889928] usb 2-1.2.4: new low-speed USB device number 27 using ci_hdrc kernel: [ 537.054252] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.4/2-1.2.4:1.0/0003:413C:2106.000D/input/input14 kernel: [ 537.120711] hid-generic 0003:413C:2106.000D: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.4/input0 systemd-logind[392]: Watching system buttons on /dev/input/event7 (Dell Dell QuietKey Keyboard) weston[464]: [11:16:50.022] event7 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[464]: [11:16:50.024] event7 - Dell Dell QuietKey Keyboard: device is a keyboard weston[464]: [11:16:50.025] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[464]: [11:16:50.026] associating input device event7 with output HDMI-A-1 (none by udev) kernel: [ 540.962649] usb 2-1.2: clear tt 1 (01a0) error -71 kernel: [ 540.966889] usb 2-1.2: clear tt 1 (01b0) error -71 kernel: [ 540.974144] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 540.978389] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 540.982519] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 540.986762] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 540.991019] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 540.991025] usb 2-1.2-port4: Cannot enable. Maybe the USB cable is bad? kernel: [ 540.995254] usb 2-1.2-port4: cannot disable (err = -71) kernel: [ 540.999529] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.003785] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.008015] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.012276] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.016511] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.016517] usb 2-1.2-port4: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.020910] usb 2-1.2-port4: cannot disable (err = -71) kernel: [ 541.025107] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.029286] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.033522] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.037793] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.042021] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.042028] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.046269] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 541.050543] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.054753] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.059005] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.063266] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.067513] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.067518] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.071769] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 541.076005] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.080324] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.084543] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.088762] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.093018] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.093024] usb 2-1.2-port4: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.097261] usb 2-1.2-port4: cannot disable (err = -71) kernel: [ 541.101515] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.105751] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.110169] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.114265] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.118507] usb 2-1.2-port4: cannot reset (err = -71) kernel: [ 541.118512] usb 2-1.2-port4: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.122763] usb 2-1.2-port4: cannot disable (err = -71) kernel: [ 541.127006] usb 2-1.2-port4: cannot disable (err = -71) kernel: [ 541.131266] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.135547] hub 2-1.2:1.0: hub_ext_port_status failed (err = -71) kernel: [ 541.139763] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.144017] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.148262] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.152513] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.152519] usb 2-1.2-port2: Cannot enable. Maybe the USB cable is bad? kernel: [ 541.156752] usb 2-1.2-port2: cannot disable (err = -71) kernel: [ 541.161031] usb 2-1.2-port2: cannot reset (err = -71) kernel: [ 541.164659] usb 2-1.2: USB disconnect, device number 25 kernel: [ 541.164670] usb 2-1.2.2: USB disconnect, device number 26 kernel: [ 541.165358] usb 2-1.2-port2: cannot reset (err = -71) weston[464]: [11:16:53.936] event7 - Dell Dell QuietKey Keyboard: device removed weston[464]: [11:16:53.976] event6 - Logitech USB Optical Mouse: device removed kernel: [ 541.271277] usb 2-1.2.4: USB disconnect, device number 27 --> End test. Output with the fix: --> Attach a mouse into host port and then disconnect it. kernel: [ 101.229751] usb 2-1.2: new low-speed USB device number 16 using ci_hdrc kernel: [ 101.496424] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2:1.0/0003:046D:C05F.0006/input/input7 kernel: [ 101.497252] hid-generic 0003:046D:C05F.0006: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2/input0 weston[486]: [11:34:50.974] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[486]: [11:34:50.975] event6 - Logitech USB Optical Mouse: device is a pointer weston[486]: [11:34:50.975] libinput: configuring device "Logitech USB Optical Mouse". weston[486]: [11:34:50.975] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 104.418306] usb 2-1.2: USB disconnect, device number 16 weston[486]: [11:34:53.604] event6 - Logitech USB Optical Mouse: device removed --> Attach a keyboard into host port and then disconnect it. kernel: [ 126.829750] usb 2-1.2: new low-speed USB device number 17 using ci_hdrc kernel: [ 127.096213] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2:1.0/0003:413C:2106.0007/input/input8 kernel: [ 127.160286] hid-generic 0003:413C:2106.0007: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2/input0 systemd-logind[418]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[486]: [11:35:16.511] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[486]: [11:35:16.511] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[486]: [11:35:16.513] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[486]: [11:35:16.513] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 129.506479] usb 2-1.2: USB disconnect, device number 17 weston[486]: [11:35:18.775] event6 - Dell Dell QuietKey Keyboard: device removed --> Attach a KVM switch into host port and then disconnect it. kernel: [ 160.369756] usb 2-1.2: new high-speed USB device number 18 using ci_hdrc kernel: [ 160.622949] hub 2-1.2:1.0: USB hub found kernel: [ 160.623495] hub 2-1.2:1.0: 4 ports detected kernel: [ 163.298449] usb 2-1.2: USB disconnect, device number 18 --> Attach a KVM switch into host port, attach a mouse into the KVM and then disconnect it. kernel: [ 184.939760] usb 2-1.2: new high-speed USB device number 19 using ci_hdrc kernel: [ 185.195174] hub 2-1.2:1.0: USB hub found kernel: [ 185.195706] hub 2-1.2:1.0: 4 ports detected kernel: [ 190.249755] usb 2-1.2.2: new low-speed USB device number 20 using ci_hdrc kernel: [ 190.416307] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.0008/input/input9 kernel: [ 190.417250] hid-generic 0003:046D:C05F.0008: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[486]: [11:36:19.907] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[486]: [11:36:19.907] event6 - Logitech USB Optical Mouse: device is a pointer weston[486]: [11:36:19.908] libinput: configuring device "Logitech USB Optical Mouse". weston[486]: [11:36:19.908] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 193.531312] usb 2-1.2.2: USB disconnect, device number 20 weston[486]: [11:36:22.721] event6 - Logitech USB Optical Mouse: device removed --> Re-attach the mouse into the KVM and then disconnect the KVM switch. kernel: [ 208.169755] usb 2-1.2.2: new low-speed USB device number 21 using ci_hdrc kernel: [ 208.333289] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.0009/input/input10 kernel: [ 208.335566] hid-generic 0003:046D:C05F.0009: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[486]: [11:36:37.805] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[486]: [11:36:37.806] event6 - Logitech USB Optical Mouse: device is a pointer weston[486]: [11:36:37.806] libinput: configuring device "Logitech USB Optical Mouse". weston[486]: [11:36:37.806] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 211.426306] usb 2-1.2: USB disconnect, device number 19 kernel: [ 211.426317] usb 2-1.2.2: USB disconnect, device number 21 weston[486]: [11:36:40.616] event6 - Logitech USB Optical Mouse: device removed --> Attach a KVM switch into host port, attach a keyboard into the KVM and then disconnect it. kernel: [ 230.249761] usb 2-1.2: new high-speed USB device number 22 using ci_hdrc kernel: [ 230.502823] hub 2-1.2:1.0: USB hub found kernel: [ 230.503332] hub 2-1.2:1.0: 4 ports detected kernel: [ 237.349770] usb 2-1.2.2: new low-speed USB device number 23 using ci_hdrc kernel: [ 237.516824] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:413C:2106.000A/input/input11 kernel: [ 237.580297] hid-generic 0003:413C:2106.000A: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.2/input0 systemd-logind[418]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[486]: [11:37:06.967] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[486]: [11:37:06.969] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[486]: [11:37:06.971] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[486]: [11:37:06.972] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 240.637437] usb 2-1.2.2: USB disconnect, device number 23 weston[486]: [11:37:09.897] event6 - Dell Dell QuietKey Keyboard: device removed --> Re-attach the keyboard into the KVM and then disconnect the KVM switch. kernel: [ 257.319980] usb 2-1.2.2: new low-speed USB device number 24 using ci_hdrc kernel: [ 257.483579] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:413C:2106.000B/input/input12 kernel: [ 257.560285] hid-generic 0003:413C:2106.000B: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.2/input0 systemd-logind[418]: Watching system buttons on /dev/input/event6 (Dell Dell QuietKey Keyboard) weston[486]: [11:37:26.926] event6 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[486]: [11:37:26.928] event6 - Dell Dell QuietKey Keyboard: device is a keyboard weston[486]: [11:37:26.930] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[486]: [11:37:26.931] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 260.323187] usb 2-1.2: USB disconnect, device number 22 kernel: [ 260.323199] usb 2-1.2.2: USB disconnect, device number 24 weston[486]: [11:37:29.585] event6 - Dell Dell QuietKey Keyboard: device removed --> Attach a KVM switch into host port, attach a mouse and a keyboard into the KVM and then disconnect the KVM switch. kernel: [ 282.479747] usb 2-1.2: new high-speed USB device number 25 using ci_hdrc kernel: [ 282.732817] hub 2-1.2:1.0: USB hub found kernel: [ 282.733327] hub 2-1.2:1.0: 4 ports detected kernel: [ 288.299751] usb 2-1.2.2: new low-speed USB device number 26 using ci_hdrc kernel: [ 288.476005] input: Logitech USB Optical Mouse as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.2/2-1.2.2:1.0/0003:046D:C05F.000C/input/input13 kernel: [ 288.476222] hid-generic 0003:046D:C05F.000C: input: USB HID v1.11 Mouse [Logitech USB Optical Mouse] on usb-ci_hdrc.1-1.2.2/input0 weston[486]: [11:37:57.956] event6 - Logitech USB Optical Mouse: is tagged by udev as: Mouse weston[486]: [11:37:57.956] event6 - Logitech USB Optical Mouse: device is a pointer weston[486]: [11:37:57.957] libinput: configuring device "Logitech USB Optical Mouse". weston[486]: [11:37:57.957] associating input device event6 with output HDMI-A-1 (none by udev) kernel: [ 291.629751] usb 2-1.2.4: new low-speed USB device number 27 using ci_hdrc kernel: [ 291.793640] input: Dell Dell QuietKey Keyboard as /devices/soc0/soc/2100000.aips-bus/2184200.usb/ci_hdrc.1/usb2/2-1/2-1.2/2-1.2.4/2-1.2.4:1.0/0003:413C:2106.000D/input/input14 kernel: [ 291.860657] hid-generic 0003:413C:2106.000D: input: USB HID v1.10 Keyboard [Dell Dell QuietKey Keyboard] on usb-ci_hdrc.1-1.2.4/input0 systemd-logind[418]: Watching system buttons on /dev/input/event7 (Dell Dell QuietKey Keyboard) weston[486]: [11:38:01.238] event7 - Dell Dell QuietKey Keyboard: is tagged by udev as: Keyboard weston[486]: [11:38:01.242] event7 - Dell Dell QuietKey Keyboard: device is a keyboard weston[486]: [11:38:01.243] libinput: configuring device "Dell Dell QuietKey Keyboard". weston[486]: [11:38:01.243] associating input device event7 with output HDMI-A-1 (none by udev) kernel: [ 295.394440] usb 2-1.2: USB disconnect, device number 25 kernel: [ 295.394450] usb 2-1.2.2: USB disconnect, device number 26 weston[486]: [11:38:04.580] event6 - Logitech USB Optical Mouse: device removed kernel: [ 295.431931] usb 2-1.2.4: USB disconnect, device number 27 weston[486]: [11:38:04.725] event7 - Dell Dell QuietKey Keyboard: device removed --> End test. --- drivers/usb/host/ehci-q.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index 3276304..da7fd12 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -206,8 +206,9 @@ static int qtd_copy_status ( if (token & QTD_STS_BABBLE) { /* FIXME "must" disable babbling device's port too */ status = -EOVERFLOW; - /* CERR nonzero + halt --> stall */ - } else if (QTD_CERR(token)) { + /* CERR nonzero and less than EHCI_TUNE_CERR + halt --> stall. + This handles situation where stall comes after an error. */ + } else if (QTD_CERR(token) && QTD_CERR(token) < EHCI_TUNE_CERR) { status = -EPIPE; /* In theory, more than one of the following bits can be set @@ -228,6 +229,10 @@ static int qtd_copy_status ( usb_pipeendpoint(urb->pipe), usb_pipein(urb->pipe) ? "in" : "out"); status = -EPROTO; + /* CERR equals EHCI_TUNE_CERR, no other errors + halt --> stall. + This handles situation where stall comes without error bits set. */ + } else if (QTD_CERR(token)) { + status = -EPIPE; } else { /* unknown */ status = -EPROTO; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-11-29 14:08 [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected Erkka Talvitie @ 2019-12-02 19:43 ` Alan Stern 2019-12-03 9:38 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2019-12-02 19:43 UTC (permalink / raw) To: Erkka Talvitie; +Cc: gregkh, linux-usb, claus.baumgartner On Fri, 29 Nov 2019, Erkka Talvitie wrote: > When disconnecting a USB hub that has some child device(s) connected to it > (such as a USB mouse), then the stack tries to clear halt and > reset device(s) which are _already_ physically disconnected. That behavior is understandable. The kernel doesn't know that the device has been disconnected until it can process the notification from an upstream hub, and it can't process that notification while it's trying to reset the device. > The issue has been reproduced with: > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > SW: U-Boot 2019.07 and kernel 4.19.40. > > In this situation there will be error bit for MMF active yet the > CERR equals EHCI_TUNE_CERR + halt. Why? In general, setting the MMF bit does not cause the halt bit to be set (set Table 4-13 in the EHCI spec). In fact, MMF refers to errors that occur on the host, not bus errors caused by a disconnected device. > Existing implementation > interprets this as a stall [1] (chapter 8.4.5). That is the correct thing to do. When a transaction error occurs during a Complete-Split transaction, the host controller is supposed to decrement the CERR value, set the XACT bit, and retry the transaction unless the CERR value is 0 or there isn't enough time left in the microframe. The fact that you saw CERR equal to EHCI_TUNE_CERR and XACT clear probably means that your EHCI hardware is not behaving properly. > Fix for the issue is at first to check for a stall that comes after > an error (the CERR has been decreased). > > Then after that, check for other errors. > > And at last check for stall without other errors (the CERR equals > EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). > > What happens after the fix is that when disconnecting a hub with > attached device(s) the situation is not interpret as a stall. > > The specification [2] is not clear about error priorities, but > since there is no explicit error bit for the stall, it is > assumed to be lower priority than other errors. On the contrary, the specification is very clear. Since transaction errors cause CERR to be decremented until it reaches 0, a nonzero value for CERR means the endpoint was halted for some other reason. And the only other reason is a stall. (Or end of the microframe, but there's no way to tell if that happened.) > [1] https://www.usb.org/document-library/usb-20-specification, usb_20.pdf > [2] https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/ehci-specification-for-usb.pdf > > Signed-off-by: Erkka Talvitie <erkka.talvitie@vincit.fi> Can you duplicate this behavior on a standard PC, say with an Intel EHCI controller? > drivers/usb/host/ehci-q.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > index 3276304..da7fd12 100644 > --- a/drivers/usb/host/ehci-q.c > +++ b/drivers/usb/host/ehci-q.c > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > if (token & QTD_STS_BABBLE) { > /* FIXME "must" disable babbling device's port too */ > status = -EOVERFLOW; > - /* CERR nonzero + halt --> stall */ > - } else if (QTD_CERR(token)) { > + /* CERR nonzero and less than EHCI_TUNE_CERR + halt --> stall. > + This handles situation where stall comes after an error. */ This comment doesn't make sense. Who cares whether a stall comes after an error or not? It's still a stall and should be reported. > + } else if (QTD_CERR(token) && QTD_CERR(token) < EHCI_TUNE_CERR) { > status = -EPIPE; If an error occurs and the transaction is retried and the retry gets a stall, then the final status should be -EPIPE, not something else. > /* In theory, more than one of the following bits can be set > @@ -228,6 +229,10 @@ static int qtd_copy_status ( > usb_pipeendpoint(urb->pipe), > usb_pipein(urb->pipe) ? "in" : "out"); > status = -EPROTO; > + /* CERR equals EHCI_TUNE_CERR, no other errors + halt --> stall. > + This handles situation where stall comes without error bits set. */ If CERR is equal to EHCI_TUNE_CERR then no other errors could have occurred (since any error will decrement CERR). So why shouldn't this case be included with the earlier case? > + } else if (QTD_CERR(token)) { > + status = -EPIPE; > } else { /* unknown */ > status = -EPROTO; > } Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-02 19:43 ` Alan Stern @ 2019-12-03 9:38 ` Erkka Talvitie 2019-12-03 10:54 ` Erkka Talvitie 2019-12-03 19:01 ` Alan Stern 0 siblings, 2 replies; 16+ messages in thread From: Erkka Talvitie @ 2019-12-03 9:38 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner Thank you for the comments. > -----Original Message----- > From: Alan Stern <stern@rowland.harvard.edu> > Sent: maanantai 2. joulukuuta 2019 21.44 > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > When disconnecting a USB hub that has some child device(s) connected > > to it (such as a USB mouse), then the stack tries to clear halt and > > reset device(s) which are _already_ physically disconnected. > > That behavior is understandable. The kernel doesn't know that the device > has been disconnected until it can process the notification from an upstream > hub, and it can't process that notification while it's trying to reset the device. > Ok. I was thinking that in this use case , it should not be trying to clear the halt (and reset the device when the clear halt fails). And this behavior was altered by this RFC. > > The issue has been reproduced with: > > > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > > SW: U-Boot 2019.07 and kernel 4.19.40. > > > > In this situation there will be error bit for MMF active yet the CERR > > equals EHCI_TUNE_CERR + halt. > > Why? In general, setting the MMF bit does not cause the halt bit to be set > (set Table 4-13 in the EHCI spec). In fact, MMF refers to errors that occur on > the host, not bus errors caused by a disconnected device. I do not know for sure why that happens. I was suspecting that there has been MMF error and a stall at the same time. And in this RFC it was assumed that MMF is with greater priority than stall. The disconnecting of a hub with attached devices cause the MMF error bit set even though it is a host side error. > > > Existing implementation > > interprets this as a stall [1] (chapter 8.4.5). > > That is the correct thing to do. When a transaction error occurs during a > Complete-Split transaction, the host controller is supposed to decrement the > CERR value, set the XACT bit, and retry the transaction unless the CERR value > is 0 or there isn't enough time left in the microframe. > > The fact that you saw CERR equal to EHCI_TUNE_CERR and XACT clear > probably means that your EHCI hardware is not behaving properly. If you refer to the XactErr bit (Table 4-13 [2] )with the "XACT clear" then unfortunately I did not check it's state ,so I am not sure if it was clear. In this patch, like also in the existing implementation, the MMF bit is checked first and since it is active in this situation the XactErr is not checked. I will check this. But as in this use case the CERR has not been decreased yet there is error bit active (MMF) do you see it is still correct to interpret it as a stall (even when the halt bit is set)? I have tried to find out more details about our EHCI controller version, but I have only found out those CPU versions. It might help in a search whether this could be a HW issue. > > > Fix for the issue is at first to check for a stall that comes after an > > error (the CERR has been decreased). > > > > Then after that, check for other errors. > > > > And at last check for stall without other errors (the CERR equals > > EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). > > > > What happens after the fix is that when disconnecting a hub with > > attached device(s) the situation is not interpret as a stall. > > > > The specification [2] is not clear about error priorities, but since > > there is no explicit error bit for the stall, it is assumed to be > > lower priority than other errors. > > On the contrary, the specification is very clear. Since transaction errors cause > CERR to be decremented until it reaches 0, a nonzero value for CERR means > the endpoint was halted for some other reason. And the only other reason > is a stall. (Or end of the microframe, but there's no way to tell if that > happened.) I see your point. EHCI specification states that babble is a serious error and it will also cause the halt. The babble error bit is checked first. But the specification does not say about order of the other errors or about what to do if there is an error, no retries executed, yet a halt (stall). For example should the XactErr be checked before the MMF. >(Or end of the microframe, but there's no way to tell if that happened.) I was not able to locate this from the specification. Could you please point out where this statement is from? Could the way to tell if "end of microframe" happened, be what is done here - check for MMF error bit and if CERR has not been decreased? > > > [1] https://www.usb.org/document-library/usb-20-specification, > > usb_20.pdf [2] > > > https://www.intel.com/content/dam/www/public/us/en/documents/techn > ical > > -specifications/ehci-specification-for-usb.pdf > > > > Signed-off-by: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Can you duplicate this behavior on a standard PC, say with an Intel EHCI > controller? We tested with native Linux PC and the error did not reproduce. However I am not sure about the used host controller in that PC. I will check that or try to get a setup with Intel EHCI. > > > drivers/usb/host/ehci-q.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > index 3276304..da7fd12 100644 > > --- a/drivers/usb/host/ehci-q.c > > +++ b/drivers/usb/host/ehci-q.c > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > if (token & QTD_STS_BABBLE) { > > /* FIXME "must" disable babbling > device's port too */ > > status = -EOVERFLOW; > > - /* CERR nonzero + halt --> stall */ > > - } else if (QTD_CERR(token)) { > > + /* CERR nonzero and less than > EHCI_TUNE_CERR + halt --> stall. > > + This handles situation where stall comes after > an error. */ > > This comment doesn't make sense. Who cares whether a stall comes after > an error or not? It's still a stall and should be reported. This was basically a comment trying to answer to this commit: ba516de332c0 USB: EHCI: check for STALL before other errors "The existing code doesn't do this properly, because it tests for MMF (Missed MicroFrame) and DBE (Data Buffer Error) before testing the retry counter. Thus, if a transaction gets either MMF or DBE the corresponding flag is set and the transaction is retried. If the second attempt receives a STALL then -EPIPE is the correct return value. But the existing code would see the MMF or DBE flag instead and return -EPROTO, -ENOSR, or -ECOMM." The comment tries to explain that it will not revert the fix made in the commit ba516de332c0. > > > + } else if (QTD_CERR(token) && > QTD_CERR(token) < EHCI_TUNE_CERR) { > > status = -EPIPE; > > If an error occurs and the transaction is retried and the retry gets a stall, then > the final status should be -EPIPE, not something else. This is how the RFC also works. If the transaction has been retried and gets stall then -EPIPE is returned. Or if there are no errors but there is a stall then -EPIPE is returned. The only difference in this patch in comparison to the existing implementation is that if there is an error but the transaction has not been retried it is not interpret as a stall even if there is a halt. > > > /* In theory, more than one of the following bits > can be set @@ > > -228,6 +229,10 @@ static int qtd_copy_status ( > > > usb_pipeendpoint(urb->pipe), > > usb_pipein(urb- > >pipe) ? "in" : "out"); > > status = -EPROTO; > > + /* CERR equals EHCI_TUNE_CERR, no other > errors + halt --> stall. > > + This handles situation where stall comes > without error bits set. > > +*/ > > If CERR is equal to EHCI_TUNE_CERR then no other errors could have > occurred (since any error will decrement CERR). So why shouldn't this case > be included with the earlier case? That is what I also understood from the EHCI specification. If there is an error the CERR should decrease. Only babble, data buffer error and stall (or no error) will not decrement the CERR. However in this use case there is an error (MMF) but the CERR still equals to the EHCI_TUNE_CERR. So that's why the RFC separates these. This is the logic in the RFC: 1. The first if handles the situation where the stall comes after there has been an error AND a retry. CERR has been decreased. This is so that ba516de332c0 is not reverted. 2. The second if handles the situation where the halt has been caused by the stall AND there are no other errors. 3. If there are errors + halt, but no retries executed (CERR equals EHCI_TUNE_CERR) the response here is to return error value according to the error bit, not returning EPIPE according to the stall. > > > + } else if (QTD_CERR(token)) { > > + status = -EPIPE; > > } else { /* unknown */ > > status = -EPROTO; > > } > > Alan Stern Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-03 9:38 ` Erkka Talvitie @ 2019-12-03 10:54 ` Erkka Talvitie 2019-12-03 13:35 ` Erkka Talvitie 2019-12-03 19:01 ` Alan Stern 1 sibling, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-03 10:54 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > Sent: tiistai 3. joulukuuta 2019 11.39 > To: 'Alan Stern' <stern@rowland.harvard.edu> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > Thank you for the comments. > > > -----Original Message----- > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: maanantai 2. joulukuuta 2019 21.44 > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > When disconnecting a USB hub that has some child device(s) connected > > > to it (such as a USB mouse), then the stack tries to clear halt and > > > reset device(s) which are _already_ physically disconnected. > > > > That behavior is understandable. The kernel doesn't know that the > > device has been disconnected until it can process the notification > > from an > upstream > > hub, and it can't process that notification while it's trying to reset > > the > device. > > > > Ok. I was thinking that in this use case , it should not be trying to clear the halt > (and reset the device when the clear halt fails). And this behavior was altered > by this RFC. > > > > The issue has been reproduced with: > > > > > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > > > SW: U-Boot 2019.07 and kernel 4.19.40. > > > > > > In this situation there will be error bit for MMF active yet the > > > CERR equals EHCI_TUNE_CERR + halt. > > > > Why? In general, setting the MMF bit does not cause the halt bit to > > be > set > > (set Table 4-13 in the EHCI spec). In fact, MMF refers to errors that > occur on > > the host, not bus errors caused by a disconnected device. > > I do not know for sure why that happens. I was suspecting that there has > been MMF error and a stall at the same time. And in this RFC it was assumed > that MMF is with greater priority than stall. > The disconnecting of a hub with attached devices cause the MMF error bit > set even though it is a host side error. > > > > > > Existing implementation > > > interprets this as a stall [1] (chapter 8.4.5). > > > > That is the correct thing to do. When a transaction error occurs > > during a Complete-Split transaction, the host controller is supposed > > to decrement > the > > CERR value, set the XACT bit, and retry the transaction unless the > > CERR > value > > is 0 or there isn't enough time left in the microframe. > > > > The fact that you saw CERR equal to EHCI_TUNE_CERR and XACT clear > > probably means that your EHCI hardware is not behaving properly. > > If you refer to the XactErr bit (Table 4-13 [2] )with the "XACT clear" then > unfortunately I did not check it's state ,so I am not sure if it was clear. > In this patch, like also in the existing implementation, the MMF bit is checked > first and since it is active in this situation the XactErr is not checked. > > I will check this. > > But as in this use case the CERR has not been decreased yet there is error bit > active (MMF) do you see it is still correct to interpret it as a stall (even when > the halt bit is set)? > > I have tried to find out more details about our EHCI controller version, but I > have only found out those CPU versions. It might help in a search whether > this could be a HW issue. > > > > > > Fix for the issue is at first to check for a stall that comes after > > > an error (the CERR has been decreased). > > > > > > Then after that, check for other errors. > > > > > > And at last check for stall without other errors (the CERR equals > > > EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). > > > > > > What happens after the fix is that when disconnecting a hub with > > > attached device(s) the situation is not interpret as a stall. > > > > > > The specification [2] is not clear about error priorities, but since > > > there is no explicit error bit for the stall, it is assumed to be > > > lower priority than other errors. > > > > On the contrary, the specification is very clear. Since transaction > errors cause > > CERR to be decremented until it reaches 0, a nonzero value for CERR > > means the endpoint was halted for some other reason. And the only > > other reason is a stall. (Or end of the microframe, but there's no > > way to tell if that > > happened.) > > I see your point. EHCI specification states that babble is a serious error and it > will also cause the halt. The babble error bit is checked first. But the > specification does not say about order of the other errors or about what to > do if there is an error, no retries executed, yet a halt (stall). For example > should the XactErr be checked before the MMF. > > >(Or end of the microframe, but there's no way to tell if that > >happened.) > > I was not able to locate this from the specification. Could you please point > out where this statement is from? > Could the way to tell if "end of microframe" happened, be what is done here > - check for MMF error bit and if CERR has not been decreased? > > > > > > [1] https://www.usb.org/document-library/usb-20-specification, > > > usb_20.pdf [2] > > > > > > https://www.intel.com/content/dam/www/public/us/en/documents/techn > > ical > > > -specifications/ehci-specification-for-usb.pdf > > > > > > Signed-off-by: Erkka Talvitie <erkka.talvitie@vincit.fi> > > > > Can you duplicate this behavior on a standard PC, say with an Intel > > EHCI controller? > > We tested with native Linux PC and the error did not reproduce. However I > am not sure about the used host controller in that PC. > I will check that or try to get a setup with Intel EHCI. The PC where the issue did not reproduce was ThinkPad T480 with: 3c:00.0 USB controller: Intel Corporation Device 15c1 (rev 01) [ 4.229310] xhci_hcd 0000:3c:00.0: xHCI Host Controller [ 4.238578] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver [ 4.239754] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver [ 4.240857] ehci-pci: EHCI PCI platform driver [ 4.241437] ohci-pci: OHCI PCI platform driver [ 4.243080] uhci_hcd: USB Universal Host Controller Interface driver > > > > > > drivers/usb/host/ehci-q.c | 9 +++++++-- > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > > index 3276304..da7fd12 100644 > > > --- a/drivers/usb/host/ehci-q.c > > > +++ b/drivers/usb/host/ehci-q.c > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > if (token & QTD_STS_BABBLE) { > > > /* FIXME "must" disable babbling > > device's port too */ > > > status = -EOVERFLOW; > > > - /* CERR nonzero + halt --> stall */ > > > - } else if (QTD_CERR(token)) { > > > + /* CERR nonzero and less than > > EHCI_TUNE_CERR + halt --> stall. > > > + This handles situation where stall comes after > > an error. */ > > > > This comment doesn't make sense. Who cares whether a stall comes > > after an error or not? It's still a stall and should be reported. > > This was basically a comment trying to answer to this commit: > ba516de332c0 USB: EHCI: check for STALL before other errors > > "The existing code doesn't do this properly, because it tests for MMF > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > retry counter. Thus, if a transaction gets either MMF or DBE the > corresponding flag is set and the transaction is retried. If the > second attempt receives a STALL then -EPIPE is the correct return > value. But the existing code would see the MMF or DBE flag instead > and return -EPROTO, -ENOSR, or -ECOMM." > > The comment tries to explain that it will not revert the fix made in the > commit ba516de332c0. > > > > > > > + } else if (QTD_CERR(token) && > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > status = -EPIPE; > > > > If an error occurs and the transaction is retried and the retry gets a > stall, then > > the final status should be -EPIPE, not something else. > > This is how the RFC also works. If the transaction has been retried and gets > stall then -EPIPE is returned. > Or if there are no errors but there is a stall then -EPIPE is returned. > > The only difference in this patch in comparison to the existing > implementation is that if there is an error but the transaction has not been > retried it is not interpret as a stall even if there is a halt. > > > > > > /* In theory, more than one of the following bits > > can be set @@ > > > -228,6 +229,10 @@ static int qtd_copy_status ( > > > > > usb_pipeendpoint(urb->pipe), > > > usb_pipein(urb- > > >pipe) ? "in" : "out"); > > > status = -EPROTO; > > > + /* CERR equals EHCI_TUNE_CERR, no other > > errors + halt --> stall. > > > + This handles situation where stall comes > > without error bits set. > > > +*/ > > > > If CERR is equal to EHCI_TUNE_CERR then no other errors could have > > occurred (since any error will decrement CERR). So why shouldn't this > case > > be included with the earlier case? > > That is what I also understood from the EHCI specification. If there is an error > the CERR should decrease. Only babble, data buffer error and stall (or no > error) will not decrement the CERR. > However in this use case there is an error (MMF) but the CERR still equals to > the EHCI_TUNE_CERR. > > So that's why the RFC separates these. This is the logic in the RFC: > > 1. The first if handles the situation where the stall comes after there has > been an error AND a retry. CERR has been decreased. This is so that > ba516de332c0 is not reverted. > 2. The second if handles the situation where the halt has been caused by the > stall AND there are no other errors. > 3. If there are errors + halt, but no retries executed (CERR equals > EHCI_TUNE_CERR) the response here is to return error value according to > the error bit, not returning EPIPE according to the stall. I am using CERR here confusingly. It is not a retry counter, instead it is an error counter. > > > > > > + } else if (QTD_CERR(token)) { > > > + status = -EPIPE; > > > } else { /* unknown */ > > > status = -EPROTO; > > > } > > > > Alan Stern > > Erkka Talvitie > Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-03 10:54 ` Erkka Talvitie @ 2019-12-03 13:35 ` Erkka Talvitie 0 siblings, 0 replies; 16+ messages in thread From: Erkka Talvitie @ 2019-12-03 13:35 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > Sent: tiistai 3. joulukuuta 2019 12.54 > To: 'Alan Stern' <stern@rowland.harvard.edu> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > > -----Original Message----- > > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Sent: tiistai 3. joulukuuta 2019 11.39 > > To: 'Alan Stern' <stern@rowland.harvard.edu> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > Thank you for the comments. > > > > > -----Original Message----- > > > From: Alan Stern <stern@rowland.harvard.edu> > > > Sent: maanantai 2. joulukuuta 2019 21.44 > > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > > claus.baumgartner@med.ge.com > > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > > disconnected > > > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > > > When disconnecting a USB hub that has some child device(s) > > > > connected to it (such as a USB mouse), then the stack tries to > > > > clear halt and reset device(s) which are _already_ physically > disconnected. > > > > > > That behavior is understandable. The kernel doesn't know that the > > > device has been disconnected until it can process the notification > > > from an > > upstream > > > hub, and it can't process that notification while it's trying to > > > reset the > > device. > > > > > > > Ok. I was thinking that in this use case , it should not be trying to > clear the halt > > (and reset the device when the clear halt fails). And this behavior > > was > altered > > by this RFC. > > > > > > The issue has been reproduced with: > > > > > > > > CPU: IMX6D5EYM10AD or MCIMX6D5EYM10AE. > > > > SW: U-Boot 2019.07 and kernel 4.19.40. > > > > > > > > In this situation there will be error bit for MMF active yet the > > > > CERR equals EHCI_TUNE_CERR + halt. > > > > > > Why? In general, setting the MMF bit does not cause the halt bit to > > > be > > set > > > (set Table 4-13 in the EHCI spec). In fact, MMF refers to errors > > > that > > occur on > > > the host, not bus errors caused by a disconnected device. > > > > I do not know for sure why that happens. I was suspecting that there > > has been MMF error and a stall at the same time. And in this RFC it > > was > assumed > > that MMF is with greater priority than stall. > > The disconnecting of a hub with attached devices cause the MMF error > > bit set even though it is a host side error. > > > > > > > > > Existing implementation > > > > interprets this as a stall [1] (chapter 8.4.5). > > > > > > That is the correct thing to do. When a transaction error occurs > > > during a Complete-Split transaction, the host controller is supposed > > > to decrement > > the > > > CERR value, set the XACT bit, and retry the transaction unless the > > > CERR > > value > > > is 0 or there isn't enough time left in the microframe. > > > > > > The fact that you saw CERR equal to EHCI_TUNE_CERR and XACT clear > > > probably means that your EHCI hardware is not behaving properly. > > > > If you refer to the XactErr bit (Table 4-13 [2] )with the "XACT clear" > then > > unfortunately I did not check it's state ,so I am not sure if it was > clear. > > In this patch, like also in the existing implementation, the MMF bit > > is > checked > > first and since it is active in this situation the XactErr is not checked. > > > > I will check this. > > > > But as in this use case the CERR has not been decreased yet there is > > error > bit > > active (MMF) do you see it is still correct to interpret it as a stall > (even when > > the halt bit is set)? > > > > I have tried to find out more details about our EHCI controller > > version, > but I > > have only found out those CPU versions. It might help in a search > > whether this could be a HW issue. > > > > > > > > > Fix for the issue is at first to check for a stall that comes > > > > after an error (the CERR has been decreased). > > > > > > > > Then after that, check for other errors. > > > > > > > > And at last check for stall without other errors (the CERR equals > > > > EHCI_TUNE_CERR as stall does not decrease the CERR [2] (table 3-16)). > > > > > > > > What happens after the fix is that when disconnecting a hub with > > > > attached device(s) the situation is not interpret as a stall. > > > > > > > > The specification [2] is not clear about error priorities, but > > > > since there is no explicit error bit for the stall, it is assumed > > > > to be lower priority than other errors. > > > > > > On the contrary, the specification is very clear. Since transaction > > errors cause > > > CERR to be decremented until it reaches 0, a nonzero value for CERR > > > means the endpoint was halted for some other reason. And the only > > > other reason is a stall. (Or end of the microframe, but there's no > > > way to tell if that > > > happened.) > > > > I see your point. EHCI specification states that babble is a serious > > error > and it > > will also cause the halt. The babble error bit is checked first. But > > the specification does not say about order of the other errors or > > about what > to > > do if there is an error, no retries executed, yet a halt (stall). For > example > > should the XactErr be checked before the MMF. > > > > >(Or end of the microframe, but there's no way to tell if that > > >happened.) > > > > I was not able to locate this from the specification. Could you please > point > > out where this statement is from? > > Could the way to tell if "end of microframe" happened, be what is done > here > > - check for MMF error bit and if CERR has not been decreased? > > > > > > > > > [1] https://www.usb.org/document-library/usb-20-specification, > > > > usb_20.pdf [2] > > > > > > > > > > https://www.intel.com/content/dam/www/public/us/en/documents/techn > > > ical > > > > -specifications/ehci-specification-for-usb.pdf > > > > > > > > Signed-off-by: Erkka Talvitie <erkka.talvitie@vincit.fi> > > > > > > Can you duplicate this behavior on a standard PC, say with an Intel > > > EHCI controller? > > > > We tested with native Linux PC and the error did not reproduce. > > However I am not sure about the used host controller in that PC. > > I will check that or try to get a setup with Intel EHCI. > > The PC where the issue did not reproduce was ThinkPad T480 with: > > 3c:00.0 USB controller: Intel Corporation Device 15c1 (rev 01) > > [ 4.229310] xhci_hcd 0000:3c:00.0: xHCI Host Controller > [ 4.238578] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver > [ 4.239754] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver > [ 4.240857] ehci-pci: EHCI PCI platform driver > [ 4.241437] ohci-pci: OHCI PCI platform driver > [ 4.243080] uhci_hcd: USB Universal Host Controller Interface driver > Another test, HP Proliant Microserver Gen8 Linux version 4.2.3-300.fc23.x86_64 ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver ehci-pci: EHCI PCI platform driver With this setup the behavior reproduces. kernel: usb 3-1.2: clear tt 1 (0080) error -71 kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: cannot reset (err = -71) kernel: usb 3-1.2-port1: Cannot enable. Maybe the USB cable is bad? kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: usb 3-1.2-port1: cannot disable (err = -71) kernel: hub 3-1.2:1.0: hub_port_status failed (err = -71) > > > > > > > > > drivers/usb/host/ehci-q.c | 9 +++++++-- > > > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > > > index 3276304..da7fd12 100644 > > > > --- a/drivers/usb/host/ehci-q.c > > > > +++ b/drivers/usb/host/ehci-q.c > > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > > if (token & QTD_STS_BABBLE) { > > > > /* FIXME "must" disable babbling > > > device's port too */ > > > > status = -EOVERFLOW; > > > > - /* CERR nonzero + halt --> stall */ > > > > - } else if (QTD_CERR(token)) { > > > > + /* CERR nonzero and less than > > > EHCI_TUNE_CERR + halt --> stall. > > > > + This handles situation where stall comes after > > > an error. */ > > > > > > This comment doesn't make sense. Who cares whether a stall comes > > > after an error or not? It's still a stall and should be reported. > > > > This was basically a comment trying to answer to this commit: > > ba516de332c0 USB: EHCI: check for STALL before other errors > > > > "The existing code doesn't do this properly, because it tests for MMF > > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > > retry counter. Thus, if a transaction gets either MMF or DBE the > > corresponding flag is set and the transaction is retried. If the > > second attempt receives a STALL then -EPIPE is the correct return > > value. But the existing code would see the MMF or DBE flag instead > > and return -EPROTO, -ENOSR, or -ECOMM." > > > > The comment tries to explain that it will not revert the fix made in > > the commit ba516de332c0. > > > > > > > > > > > + } else if (QTD_CERR(token) && > > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > > status = -EPIPE; > > > > > > If an error occurs and the transaction is retried and the retry gets > > > a > > stall, then > > > the final status should be -EPIPE, not something else. > > > > This is how the RFC also works. If the transaction has been retried > > and > gets > > stall then -EPIPE is returned. > > Or if there are no errors but there is a stall then -EPIPE is returned. > > > > The only difference in this patch in comparison to the existing > > implementation is that if there is an error but the transaction has > > not > been > > retried it is not interpret as a stall even if there is a halt. > > > > > > > > > /* In theory, more than one of the following bits > > > can be set @@ > > > > -228,6 +229,10 @@ static int qtd_copy_status ( > > > > > > > usb_pipeendpoint(urb->pipe), > > > > usb_pipein(urb- > > > >pipe) ? "in" : "out"); > > > > status = -EPROTO; > > > > + /* CERR equals EHCI_TUNE_CERR, no other > > > errors + halt --> stall. > > > > + This handles situation where stall comes > > > without error bits set. > > > > +*/ > > > > > > If CERR is equal to EHCI_TUNE_CERR then no other errors could have > > > occurred (since any error will decrement CERR). So why shouldn't > > > this > > case > > > be included with the earlier case? > > > > That is what I also understood from the EHCI specification. If there > > is an > error > > the CERR should decrease. Only babble, data buffer error and stall (or > > no > > error) will not decrement the CERR. > > However in this use case there is an error (MMF) but the CERR still > > equals > to > > the EHCI_TUNE_CERR. > > > > So that's why the RFC separates these. This is the logic in the RFC: > > > > 1. The first if handles the situation where the stall comes after > > there > has > > been an error AND a retry. CERR has been decreased. This is so that > > ba516de332c0 is not reverted. > > 2. The second if handles the situation where the halt has been caused > > by > the > > stall AND there are no other errors. > > 3. If there are errors + halt, but no retries executed (CERR equals > > EHCI_TUNE_CERR) the response here is to return error value according > > to the error bit, not returning EPIPE according to the stall. > > I am using CERR here confusingly. It is not a retry counter, instead it is an > error counter. > > > > > > > > > > + } else if (QTD_CERR(token)) { > > > > + status = -EPIPE; > > > > } else { /* unknown */ > > > > status = -EPROTO; > > > > } > > > > > > Alan Stern > > > > Erkka Talvitie > > > > Erkka Talvitie > Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-03 9:38 ` Erkka Talvitie 2019-12-03 10:54 ` Erkka Talvitie @ 2019-12-03 19:01 ` Alan Stern 2019-12-04 8:55 ` Erkka Talvitie 1 sibling, 1 reply; 16+ messages in thread From: Alan Stern @ 2019-12-03 19:01 UTC (permalink / raw) To: Erkka Talvitie; +Cc: gregkh, linux-usb, claus.baumgartner On Tue, 3 Dec 2019, Erkka Talvitie wrote: > Thank you for the comments. > > > -----Original Message----- > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: maanantai 2. joulukuuta 2019 21.44 > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > When disconnecting a USB hub that has some child device(s) connected > > > to it (such as a USB mouse), then the stack tries to clear halt and > > > reset device(s) which are _already_ physically disconnected. > > > > That behavior is understandable. The kernel doesn't know that the device > > has been disconnected until it can process the notification from an > upstream > > hub, and it can't process that notification while it's trying to reset the > device. > > > > Ok. I was thinking that in this use case , it should not be trying to clear > the halt (and reset the device when the clear halt fails). And this behavior > was altered by this RFC. Actually, the situation is a little different from what I described above. When you unplug the high-speed hub, the kernel doesn't know the hub has been disconnected until it receives a notification from the upstream hub. The kernel checks for those notifications at roughly 250-ms intervals, so it can take up to that long before the kernel realizes the high-speed hub is gone. Until that time, the kernel will keep trying to reset and communicate with the hub and the devices that were attached to it. You can see this in the logs that you posted in your original report. In each case, the "cannot reset" and -71 errors lasted for less than 250 ms. I just tried doing the same experiment on my PC (which does use all Intel hardware and an EHCI controller). Here's the output from when I unplugged the high-speed hub: [ 6321.245528] usb 1-1.4: clear tt 4 (00a0) error -71 [ 6321.250903] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.255155] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.259403] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.263657] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.267905] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.267910] usb 1-1.4-port4: Cannot enable. Maybe the USB cable is bad? [ 6321.272155] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.276405] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.280653] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.284905] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.289155] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.293403] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.293407] usb 1-1.4-port4: Cannot enable. Maybe the USB cable is bad? [ 6321.297656] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.301904] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.306152] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.310402] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.314653] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.318904] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.318908] usb 1-1.4-port4: Cannot enable. Maybe the USB cable is bad? [ 6321.323154] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.327404] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.331651] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.335902] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.340155] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.344402] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.344406] usb 1-1.4-port4: Cannot enable. Maybe the USB cable is bad? [ 6321.348652] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.352904] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.357154] hub 1-1.4:1.0: hub_ext_port_status failed (err = -71) [ 6321.437000] usb 1-1.4: USB disconnect, device number 9 [ 6321.437010] usb 1-1.4.4: USB disconnect, device number 10 As you can see, the time interval runs from 6321.245 to 6321.437, roughly 192 ms < 250 ms. This is the expected behavior. I did not try to check whether the MMF bit got set or what value CERR had. > But as in this use case the CERR has not been decreased yet there is error > bit active (MMF) do you see it is still correct to interpret it as a stall > (even when the halt bit is set)? See below. > I have tried to find out more details about our EHCI controller version, but > I have only found out those CPU versions. It might help in a search whether > this could be a HW issue. > > > The specification [2] is not clear about error priorities, but since > > > there is no explicit error bit for the stall, it is assumed to be > > > lower priority than other errors. > > > > On the contrary, the specification is very clear. Since transaction > errors cause > > CERR to be decremented until it reaches 0, a nonzero value for CERR means > > the endpoint was halted for some other reason. And the only other reason > > is a stall. (Or end of the microframe, but there's no way to tell if that > > happened.) > > I see your point. EHCI specification states that babble is a serious error > and it will also cause the halt. The babble error bit is checked first. But > the specification does not say about order of the other errors or about what > to do if there is an error, no retries executed, yet a halt (stall). For > example should the XactErr be checked before the MMF. I think the order doesn't matter. In fact, it's possible that both errors occurred, since the transaction gets retried multiple times. > >(Or end of the microframe, but there's no way to tell if that happened.) > > I was not able to locate this from the specification. Could you please point > out where this statement is from? "Enhanced Host Controller Interface Specification for Universal Serial Bus", rev 1.0 (2002), p. 110: Transaction Error (XactErr). Timeout, data CRC failure, etc. The CErr field is decremented and the XactErr bit in the Status field is set to a one. The complete split transaction is immediately retried (if Cerr is non-zero). If there is not enough time in the micro-frame to complete the retry and the endpoint is an IN, or CErr is decremented to a zero from a one, the queue is halted. If there is not enough time in the micro-frame to complete the retry and the endpoint is an OUT and CErr is not zero, then this state is exited (i.e. return to Do Start Split). This results in a retry of the entire OUT split transaction, at the next poll period. Refer to Chapter 11 Hubs (specifically the section full- and low-speed Interrupts) in the USB Specification Revision 2.0 for detailed requirements on why these errors must be immediately retried. • > Could the way to tell if "end of microframe" happened, be what is done here > - check for MMF error bit and if CERR has not been decreased? No, because the "end of microframe" situation happens when the host controller is handling a transaction error, whereas MMF gets set when the host controller detects an error on the host. > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > > index 3276304..da7fd12 100644 > > > --- a/drivers/usb/host/ehci-q.c > > > +++ b/drivers/usb/host/ehci-q.c > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > if (token & QTD_STS_BABBLE) { > > > /* FIXME "must" disable babbling > > device's port too */ > > > status = -EOVERFLOW; > > > - /* CERR nonzero + halt --> stall */ > > > - } else if (QTD_CERR(token)) { > > > + /* CERR nonzero and less than > > EHCI_TUNE_CERR + halt --> stall. > > > + This handles situation where stall comes after > > an error. */ > > > > This comment doesn't make sense. Who cares whether a stall comes after > > an error or not? It's still a stall and should be reported. > > This was basically a comment trying to answer to this commit: > ba516de332c0 USB: EHCI: check for STALL before other errors > > "The existing code doesn't do this properly, because it tests for MMF > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > retry counter. Thus, if a transaction gets either MMF or DBE the > corresponding flag is set and the transaction is retried. If the > second attempt receives a STALL then -EPIPE is the correct return > value. But the existing code would see the MMF or DBE flag instead > and return -EPROTO, -ENOSR, or -ECOMM." > > The comment tries to explain that it will not revert the fix made in the > commit ba516de332c0. Okay, I get it. You're trying to rely on the strange behavior of the MMF bit. I'm not sure this is a good idea. Suppose MMF gets set for some other reason (a genuine error on the host) and then the transaction gets a STALL on the next retry. Since host errors don't decrement CERR, your patch would cause the driver to return -EPROTO instead of -EPIPE. > > > + } else if (QTD_CERR(token) && > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > status = -EPIPE; > > > > If an error occurs and the transaction is retried and the retry gets a > stall, then > > the final status should be -EPIPE, not something else. > > This is how the RFC also works. If the transaction has been retried and gets > stall then -EPIPE is returned. > Or if there are no errors but there is a stall then -EPIPE is returned. > > The only difference in this patch in comparison to the existing > implementation is that if there is an error but the > transaction has not been retried it is not interpret as a stall even if > there is a halt. Sometimes that will be the right behavior and other times it won't. However, it looks like there may be a way to tell which situation we are in. Setting the MMF bit will cause the queue to halt immediately if the transaction is IN, but not if it is OUT (see Table 4-13 in the EHCI spec). So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then we should return -EPIPE, as the existing code does. But if QTD_PID == 1 then the code should continue, as your patch does -- with one difference: Put the additional check for EHCI_TUNE_CERR between the tests for DBE and XACT instead of after XACT (because XACT would decrement CERR whereas DBE wouldn't). Can you make that change and test it? Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-03 19:01 ` Alan Stern @ 2019-12-04 8:55 ` Erkka Talvitie 2019-12-04 13:18 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-04 8:55 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner Thank you for the comments. > -----Original Message----- > From: Alan Stern <stern@rowland.harvard.edu> > Sent: tiistai 3. joulukuuta 2019 21.01 > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > On Tue, 3 Dec 2019, Erkka Talvitie wrote: > > > Thank you for the comments. > > > > > -----Original Message----- > > > From: Alan Stern <stern@rowland.harvard.edu> > > > Sent: maanantai 2. joulukuuta 2019 21.44 > > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > > claus.baumgartner@med.ge.com > > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > > disconnected > > > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > > > When disconnecting a USB hub that has some child device(s) > > > > connected to it (such as a USB mouse), then the stack tries to > > > > clear halt and reset device(s) which are _already_ physically > disconnected. > > > > > > That behavior is understandable. The kernel doesn't know that the > > > device has been disconnected until it can process the notification > > > from an > > upstream > > > hub, and it can't process that notification while it's trying to > > > reset the > > device. > > > > > > > Ok. I was thinking that in this use case , it should not be trying to > > clear the halt (and reset the device when the clear halt fails). And > > this behavior was altered by this RFC. > > Actually, the situation is a little different from what I described above. When > you unplug the high-speed hub, the kernel doesn't know the hub has been > disconnected until it receives a notification from the upstream hub. The > kernel checks for those notifications at roughly 250-ms intervals, so it can > take up to that long before the kernel realizes the high-speed hub is gone. > Until that time, the kernel will keep trying to reset and communicate with the > hub and the devices that were attached to it. > > You can see this in the logs that you posted in your original report. > In each case, the "cannot reset" and -71 errors lasted for less than > 250 ms. > > I just tried doing the same experiment on my PC (which does use all Intel > hardware and an EHCI controller). Here's the output from when I unplugged > the high-speed hub: > > [ 6321.245528] usb 1-1.4: clear tt 4 (00a0) error -71 [ 6321.250903] usb 1-1.4- > port4: cannot reset (err = -71) [ 6321.255155] usb 1-1.4-port4: cannot reset > (err = -71) [ 6321.259403] usb 1-1.4-port4: cannot reset (err = -71) [ > 6321.263657] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.267905] usb 1- > 1.4-port4: cannot reset (err = -71) [ 6321.267910] usb 1-1.4-port4: Cannot > enable. Maybe the USB cable is bad? > [ 6321.272155] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.276405] usb > 1-1.4-port4: cannot reset (err = -71) [ 6321.280653] usb 1-1.4-port4: cannot > reset (err = -71) [ 6321.284905] usb 1-1.4-port4: cannot reset (err = -71) [ > 6321.289155] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.293403] usb 1- > 1.4-port4: cannot reset (err = -71) [ 6321.293407] usb 1-1.4-port4: Cannot > enable. Maybe the USB cable is bad? > [ 6321.297656] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.301904] usb > 1-1.4-port4: cannot reset (err = -71) [ 6321.306152] usb 1-1.4-port4: cannot > reset (err = -71) [ 6321.310402] usb 1-1.4-port4: cannot reset (err = -71) [ > 6321.314653] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.318904] usb 1- > 1.4-port4: cannot reset (err = -71) [ 6321.318908] usb 1-1.4-port4: Cannot > enable. Maybe the USB cable is bad? > [ 6321.323154] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.327404] usb > 1-1.4-port4: cannot reset (err = -71) [ 6321.331651] usb 1-1.4-port4: cannot > reset (err = -71) [ 6321.335902] usb 1-1.4-port4: cannot reset (err = -71) [ > 6321.340155] usb 1-1.4-port4: cannot reset (err = -71) [ 6321.344402] usb 1- > 1.4-port4: cannot reset (err = -71) [ 6321.344406] usb 1-1.4-port4: Cannot > enable. Maybe the USB cable is bad? > [ 6321.348652] usb 1-1.4-port4: cannot disable (err = -71) [ 6321.352904] usb > 1-1.4-port4: cannot disable (err = -71) [ 6321.357154] hub 1-1.4:1.0: > hub_ext_port_status failed (err = -71) [ 6321.437000] usb 1-1.4: USB > disconnect, device number 9 [ 6321.437010] usb 1-1.4.4: USB disconnect, > device number 10 > > As you can see, the time interval runs from 6321.245 to 6321.437, roughly 192 > ms < 250 ms. This is the expected behavior. > > I did not try to check whether the MMF bit got set or what value CERR had. > Ok, makes sense. Thank you for testing this with your setup. > > > But as in this use case the CERR has not been decreased yet there is error > > bit active (MMF) do you see it is still correct to interpret it as a stall > > (even when the halt bit is set)? > > See below. > > > I have tried to find out more details about our EHCI controller version, but > > I have only found out those CPU versions. It might help in a search whether > > this could be a HW issue. > > > > > > The specification [2] is not clear about error priorities, but since > > > > there is no explicit error bit for the stall, it is assumed to be > > > > lower priority than other errors. > > > > > > On the contrary, the specification is very clear. Since transaction > > errors cause > > > CERR to be decremented until it reaches 0, a nonzero value for CERR > means > > > the endpoint was halted for some other reason. And the only other > reason > > > is a stall. (Or end of the microframe, but there's no way to tell if that > > > happened.) > > > > I see your point. EHCI specification states that babble is a serious error > > and it will also cause the halt. The babble error bit is checked first. But > > the specification does not say about order of the other errors or about > what > > to do if there is an error, no retries executed, yet a halt (stall). For > > example should the XactErr be checked before the MMF. > > I think the order doesn't matter. In fact, it's possible that both > errors occurred, since the transaction gets retried multiple times. True as the error bits are "sticky". For example XactErr in EHCI specification [2] table 3-16: " If the host controller sets this bit to a one, then it remains a one for the duration of the transfer." > > > >(Or end of the microframe, but there's no way to tell if that happened.) > > > > I was not able to locate this from the specification. Could you please point > > out where this statement is from? > > "Enhanced Host Controller Interface Specification for Universal Serial > Bus", rev 1.0 (2002), p. 110: > > Transaction Error (XactErr). Timeout, data CRC failure, etc. The CErr > field is decremented and the XactErr bit in the Status field is set to > a one. The complete split transaction is immediately retried (if Cerr > is non-zero). If there is not enough time in the micro-frame to > complete the retry and the endpoint is an IN, or CErr is decremented to > a zero from a one, the queue is halted. If there is not enough time in > the micro-frame to complete the retry and the endpoint is an OUT and > CErr is not zero, then this state is exited (i.e. return to Do Start > Split). This results in a retry of the entire OUT split transaction, at > the next poll period. Refer to Chapter 11 Hubs (specifically the > section full- and low-speed Interrupts) in the USB Specification > Revision 2.0 for detailed requirements on why these errors must be > immediately retried. • > There it is, thank you. > > Could the way to tell if "end of microframe" happened, be what is done > here > > - check for MMF error bit and if CERR has not been decreased? > > No, because the "end of microframe" situation happens when the host > controller is handling a transaction error, whereas MMF gets set when > the host controller detects an error on the host. Ok, I see. > > > > > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c > > > > index 3276304..da7fd12 100644 > > > > --- a/drivers/usb/host/ehci-q.c > > > > +++ b/drivers/usb/host/ehci-q.c > > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > > if (token & QTD_STS_BABBLE) { > > > > /* FIXME "must" disable babbling > > > device's port too */ > > > > status = -EOVERFLOW; > > > > - /* CERR nonzero + halt --> stall */ > > > > - } else if (QTD_CERR(token)) { > > > > + /* CERR nonzero and less than > > > EHCI_TUNE_CERR + halt --> stall. > > > > + This handles situation where stall comes after > > > an error. */ > > > > > > This comment doesn't make sense. Who cares whether a stall comes > after > > > an error or not? It's still a stall and should be reported. > > > > This was basically a comment trying to answer to this commit: > > ba516de332c0 USB: EHCI: check for STALL before other errors > > > > "The existing code doesn't do this properly, because it tests for MMF > > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > > retry counter. Thus, if a transaction gets either MMF or DBE the > > corresponding flag is set and the transaction is retried. If the > > second attempt receives a STALL then -EPIPE is the correct return > > value. But the existing code would see the MMF or DBE flag instead > > and return -EPROTO, -ENOSR, or -ECOMM." > > > > The comment tries to explain that it will not revert the fix made in the > > commit ba516de332c0. > > Okay, I get it. You're trying to rely on the strange behavior of the > MMF bit. > > I'm not sure this is a good idea. Suppose MMF gets set for some other > reason (a genuine error on the host) and then the transaction gets a > STALL on the next retry. Since host errors don't decrement CERR, your > patch would cause the driver to return -EPROTO instead of -EPIPE. Hmm. I originally understood from the documentation that the MMF does decrement the CERR. I followed the EHCI specification table 3-16, the row about the CERR. There are listed the errors that do decrement the CERR and which do not decrement. Data buffer error, stall, babble and no error (of course) do not decrement. Transaction errors do decrement the CERR. As there was no mention about MMF I have been in the impression that it is included to the transaction errors of the table. Now I was able to find from the documentation you pointed out (in your next comment below) that your statement is correct. EHCI specification, table 4-13 contains Interrupt IN / OUT conditions which prove your point. > > > > > + } else if (QTD_CERR(token) && > > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > > status = -EPIPE; > > > > > > If an error occurs and the transaction is retried and the retry gets a > > stall, then > > > the final status should be -EPIPE, not something else. > > > > This is how the RFC also works. If the transaction has been retried and gets > > stall then -EPIPE is returned. > > Or if there are no errors but there is a stall then -EPIPE is returned. > > > > The only difference in this patch in comparison to the existing > > implementation is that if there is an error but the > > transaction has not been retried it is not interpret as a stall even if > > there is a halt. > > Sometimes that will be the right behavior and other times it won't. > However, it looks like there may be a way to tell which situation we > are in. Setting the MMF bit will cause the queue to halt immediately > if the transaction is IN, but not if it is OUT (see Table 4-13 in the > EHCI spec). Thank you, this Table 4-13 was a vital piece of documentation that I either missed or failed to read carefully enough. Now this could explain why the queue is halted with MMF set, even if it would not be a stall. Well done! > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then we > should return -EPIPE, as the existing code does. But if QTD_PID == 1 > then the code should continue, as your patch does -- with one > difference: Put the additional check for EHCI_TUNE_CERR between the > tests for DBE and XACT instead of after XACT (because XACT would > decrement CERR whereas DBE wouldn't). Good point, I agree. > > Can you make that change and test it? Sure, I have made the change and test it as soon as possible. > > Alan Stern Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-04 8:55 ` Erkka Talvitie @ 2019-12-04 13:18 ` Erkka Talvitie 2019-12-04 14:24 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-04 13:18 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > Sent: keskiviikko 4. joulukuuta 2019 10.55 > To: 'Alan Stern' <stern@rowland.harvard.edu> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > Thank you for the comments. > > > -----Original Message----- > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: tiistai 3. joulukuuta 2019 21.01 > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > On Tue, 3 Dec 2019, Erkka Talvitie wrote: > > > > > Thank you for the comments. > > > > > > > -----Original Message----- > > > > From: Alan Stern <stern@rowland.harvard.edu> > > > > Sent: maanantai 2. joulukuuta 2019 21.44 > > > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > > > claus.baumgartner@med.ge.com > > > > Subject: Re: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub > > > > is disconnected > > > > > > > > On Fri, 29 Nov 2019, Erkka Talvitie wrote: > > > > > > > > > When disconnecting a USB hub that has some child device(s) > > > > > connected to it (such as a USB mouse), then the stack tries to > > > > > clear halt and reset device(s) which are _already_ physically > > disconnected. > > > > > > > > That behavior is understandable. The kernel doesn't know that the > > > > device has been disconnected until it can process the notification > > > > from an > > > upstream > > > > hub, and it can't process that notification while it's trying to > > > > reset the > > > device. > > > > > > > > > > Ok. I was thinking that in this use case , it should not be trying > > > to clear the halt (and reset the device when the clear halt fails). > > > And this behavior was altered by this RFC. > > > > Actually, the situation is a little different from what I described > > above. When you unplug the high-speed hub, the kernel doesn't know > > the hub has been disconnected until it receives a notification from > > the upstream hub. The kernel checks for those notifications at > > roughly 250-ms intervals, so it can take up to that long before the kernel > realizes the high-speed hub is gone. > > Until that time, the kernel will keep trying to reset and communicate > > with the hub and the devices that were attached to it. > > > > You can see this in the logs that you posted in your original report. > > In each case, the "cannot reset" and -71 errors lasted for less than > > 250 ms. > > > > I just tried doing the same experiment on my PC (which does use all > > Intel hardware and an EHCI controller). Here's the output from when I > > unplugged the high-speed hub: > > > > [ 6321.245528] usb 1-1.4: clear tt 4 (00a0) error -71 [ 6321.250903] > > usb 1-1.4- > > port4: cannot reset (err = -71) [ 6321.255155] usb 1-1.4-port4: cannot > > reset (err = -71) [ 6321.259403] usb 1-1.4-port4: cannot reset (err = > > -71) [ 6321.263657] usb 1-1.4-port4: cannot reset (err = -71) [ > > 6321.267905] usb 1- > > 1.4-port4: cannot reset (err = -71) [ 6321.267910] usb 1-1.4-port4: > > Cannot enable. Maybe the USB cable is bad? > > [ 6321.272155] usb 1-1.4-port4: cannot disable (err = -71) [ > > 6321.276405] usb > > 1-1.4-port4: cannot reset (err = -71) [ 6321.280653] usb 1-1.4-port4: > > cannot reset (err = -71) [ 6321.284905] usb 1-1.4-port4: cannot reset > > (err = -71) [ 6321.289155] usb 1-1.4-port4: cannot reset (err = -71) [ > > 6321.293403] usb 1- > > 1.4-port4: cannot reset (err = -71) [ 6321.293407] usb 1-1.4-port4: > > Cannot enable. Maybe the USB cable is bad? > > [ 6321.297656] usb 1-1.4-port4: cannot disable (err = -71) [ > > 6321.301904] usb > > 1-1.4-port4: cannot reset (err = -71) [ 6321.306152] usb 1-1.4-port4: > > cannot reset (err = -71) [ 6321.310402] usb 1-1.4-port4: cannot reset > > (err = -71) [ 6321.314653] usb 1-1.4-port4: cannot reset (err = -71) [ > > 6321.318904] usb 1- > > 1.4-port4: cannot reset (err = -71) [ 6321.318908] usb 1-1.4-port4: > > Cannot enable. Maybe the USB cable is bad? > > [ 6321.323154] usb 1-1.4-port4: cannot disable (err = -71) [ > > 6321.327404] usb > > 1-1.4-port4: cannot reset (err = -71) [ 6321.331651] usb 1-1.4-port4: > > cannot reset (err = -71) [ 6321.335902] usb 1-1.4-port4: cannot reset > > (err = -71) [ 6321.340155] usb 1-1.4-port4: cannot reset (err = -71) [ > > 6321.344402] usb 1- > > 1.4-port4: cannot reset (err = -71) [ 6321.344406] usb 1-1.4-port4: > > Cannot enable. Maybe the USB cable is bad? > > [ 6321.348652] usb 1-1.4-port4: cannot disable (err = -71) [ > > 6321.352904] usb > > 1-1.4-port4: cannot disable (err = -71) [ 6321.357154] hub 1-1.4:1.0: > > hub_ext_port_status failed (err = -71) [ 6321.437000] usb 1-1.4: USB > > disconnect, device number 9 [ 6321.437010] usb 1-1.4.4: USB > > disconnect, device number 10 > > > > As you can see, the time interval runs from 6321.245 to 6321.437, > > roughly 192 ms < 250 ms. This is the expected behavior. > > > > I did not try to check whether the MMF bit got set or what value CERR had. > > > > Ok, makes sense. Thank you for testing this with your setup. > > > > > > But as in this use case the CERR has not been decreased yet there is > > > error bit active (MMF) do you see it is still correct to interpret > > > it as a stall (even when the halt bit is set)? > > > > See below. > > > > > I have tried to find out more details about our EHCI controller > > > version, but I have only found out those CPU versions. It might help > > > in a search whether this could be a HW issue. > > > > > > > > > The specification [2] is not clear about error priorities, but > > > > > since there is no explicit error bit for the stall, it is > > > > > assumed to be lower priority than other errors. > > > > > > > > On the contrary, the specification is very clear. Since > > > > transaction > > > errors cause > > > > CERR to be decremented until it reaches 0, a nonzero value for > > > > CERR > > means > > > > the endpoint was halted for some other reason. And the only other > > reason > > > > is a stall. (Or end of the microframe, but there's no way to tell > > > > if that > > > > happened.) > > > > > > I see your point. EHCI specification states that babble is a serious > > > error and it will also cause the halt. The babble error bit is > > > checked first. But the specification does not say about order of the > > > other errors or about > > what > > > to do if there is an error, no retries executed, yet a halt (stall). > > > For example should the XactErr be checked before the MMF. > > > > I think the order doesn't matter. In fact, it's possible that both > > errors occurred, since the transaction gets retried multiple times. > > True as the error bits are "sticky". > > For example XactErr in EHCI specification [2] table 3-16: > " If the host controller sets this bit to a one, then it remains a one for the > duration of the transfer." > > > > > > >(Or end of the microframe, but there's no way to tell if that > > > >happened.) > > > > > > I was not able to locate this from the specification. Could you > > > please point out where this statement is from? > > > > "Enhanced Host Controller Interface Specification for Universal Serial > > Bus", rev 1.0 (2002), p. 110: > > > > Transaction Error (XactErr). Timeout, data CRC failure, etc. The CErr > > field is decremented and the XactErr bit in the Status field is set to > > a one. The complete split transaction is immediately retried (if Cerr > > is non-zero). If there is not enough time in the micro-frame to > > complete the retry and the endpoint is an IN, or CErr is decremented > > to a zero from a one, the queue is halted. If there is not enough time > > in the micro-frame to complete the retry and the endpoint is an OUT > > and CErr is not zero, then this state is exited (i.e. return to Do > > Start Split). This results in a retry of the entire OUT split > > transaction, at the next poll period. Refer to Chapter 11 Hubs > > (specifically the section full- and low-speed Interrupts) in the USB > > Specification Revision 2.0 for detailed requirements on why these > > errors must be immediately retried. • > > > > There it is, thank you. > > > > Could the way to tell if "end of microframe" happened, be what is > > > done > > here > > > - check for MMF error bit and if CERR has not been decreased? > > > > No, because the "end of microframe" situation happens when the host > > controller is handling a transaction error, whereas MMF gets set when > > the host controller detects an error on the host. > > Ok, I see. > > > > > > > > diff --git a/drivers/usb/host/ehci-q.c > > > > > b/drivers/usb/host/ehci-q.c index 3276304..da7fd12 100644 > > > > > --- a/drivers/usb/host/ehci-q.c > > > > > +++ b/drivers/usb/host/ehci-q.c > > > > > @@ -206,8 +206,9 @@ static int qtd_copy_status ( > > > > > if (token & QTD_STS_BABBLE) { > > > > > /* FIXME "must" disable babbling > > > > device's port too */ > > > > > status = -EOVERFLOW; > > > > > - /* CERR nonzero + halt --> stall */ > > > > > - } else if (QTD_CERR(token)) { > > > > > + /* CERR nonzero and less than > > > > EHCI_TUNE_CERR + halt --> stall. > > > > > + This handles situation where stall comes after > > > > an error. */ > > > > > > > > This comment doesn't make sense. Who cares whether a stall comes > > after > > > > an error or not? It's still a stall and should be reported. > > > > > > This was basically a comment trying to answer to this commit: > > > ba516de332c0 USB: EHCI: check for STALL before other errors > > > > > > "The existing code doesn't do this properly, because it tests for MMF > > > (Missed MicroFrame) and DBE (Data Buffer Error) before testing the > > > retry counter. Thus, if a transaction gets either MMF or DBE the > > > corresponding flag is set and the transaction is retried. If the > > > second attempt receives a STALL then -EPIPE is the correct return > > > value. But the existing code would see the MMF or DBE flag instead > > > and return -EPROTO, -ENOSR, or -ECOMM." > > > > > > The comment tries to explain that it will not revert the fix made in > > > the commit ba516de332c0. > > > > Okay, I get it. You're trying to rely on the strange behavior of the > > MMF bit. > > > > I'm not sure this is a good idea. Suppose MMF gets set for some other > > reason (a genuine error on the host) and then the transaction gets a > > STALL on the next retry. Since host errors don't decrement CERR, your > > patch would cause the driver to return -EPROTO instead of -EPIPE. > > Hmm. I originally understood from the documentation that the MMF does > decrement the CERR. I followed the EHCI specification table 3-16, the row > about the CERR. > There are listed the errors that do decrement the CERR and which do not > decrement. Data buffer error, stall, babble and no error (of course) do not > decrement. > Transaction errors do decrement the CERR. As there was no mention about > MMF I have been in the impression that it is included to the transaction > errors of the table. > > Now I was able to find from the documentation you pointed out (in your > next comment below) that your statement is correct. EHCI specification, > table 4-13 contains Interrupt IN / OUT conditions which prove your point. > > > > > > > > + } else if (QTD_CERR(token) && > > > > QTD_CERR(token) < EHCI_TUNE_CERR) { > > > > > status = -EPIPE; > > > > > > > > If an error occurs and the transaction is retried and the retry > > > > gets a > > > stall, then > > > > the final status should be -EPIPE, not something else. > > > > > > This is how the RFC also works. If the transaction has been retried > > > and gets stall then -EPIPE is returned. > > > Or if there are no errors but there is a stall then -EPIPE is returned. > > > > > > The only difference in this patch in comparison to the existing > > > implementation is that if there is an error but the transaction has > > > not been retried it is not interpret as a stall even if there is a > > > halt. > > > > Sometimes that will be the right behavior and other times it won't. > > However, it looks like there may be a way to tell which situation we > > are in. Setting the MMF bit will cause the queue to halt immediately > > if the transaction is IN, but not if it is OUT (see Table 4-13 in the > > EHCI spec). > > Thank you, this Table 4-13 was a vital piece of documentation that I either > missed or failed to read carefully enough. > Now this could explain why the queue is halted with MMF set, even if it > would not be a stall. Well done! > > > > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then we > > should return -EPIPE, as the existing code does. But if QTD_PID == 1 > > then the code should continue, as your patch does -- with one > > difference: Put the additional check for EHCI_TUNE_CERR between the > > tests for DBE and XACT instead of after XACT (because XACT would > > decrement CERR whereas DBE wouldn't). > > Good point, I agree. > > > > > Can you make that change and test it? > > Sure, I have made the change and test it as soon as possible. I am not actually totally sure if I understood you correctly, but I tested a change where the first stall check is like this (PID_CODE_IN is defined as 1): - } else if (QTD_CERR(token)) { + } else if (QTD_CERR(token) && (QTD_PID (token) != PID_CODE_IN)) { status = -EPIPE; And the second stall check (now between DBE and XACT): + } else if (QTD_CERR(token)) { + status = -EPIPE; Is this what you meant? Please correct me if I am wrong. Anyways with these changes the issue does not reproduce. > > > > > Alan Stern > > Erkka Talvitie > Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-04 13:18 ` Erkka Talvitie @ 2019-12-04 14:24 ` Alan Stern 2019-12-04 14:37 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2019-12-04 14:24 UTC (permalink / raw) To: Erkka Talvitie; +Cc: gregkh, linux-usb, claus.baumgartner On Wed, 4 Dec 2019, Erkka Talvitie wrote: > > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then we > > > should return -EPIPE, as the existing code does. But if QTD_PID == 1 > > > then the code should continue, as your patch does -- with one > > > difference: Put the additional check for EHCI_TUNE_CERR between the > > > tests for DBE and XACT instead of after XACT (because XACT would > > > decrement CERR whereas DBE wouldn't). > > > > Good point, I agree. > > > > > > > > Can you make that change and test it? > > > > Sure, I have made the change and test it as soon as possible. > > I am not actually totally sure if I understood you correctly, but I tested a change where the first stall check is like this (PID_CODE_IN is defined as 1): > > - } else if (QTD_CERR(token)) { > + } else if (QTD_CERR(token) && (QTD_PID (token) != PID_CODE_IN)) { > status = -EPIPE; > > And the second stall check (now between DBE and XACT): > + } else if (QTD_CERR(token)) { > + status = -EPIPE; > > Is this what you meant? Please correct me if I am wrong. Actually, what I meant for the first part was: } else if (QTD_CERR(token) && (QTD_CERR(token) < EHCI_TUNE_CERR || QTD_PID(token) != PID_CODE_IN)) { And of course there should be a comment before this line, explaining what it does. By the way, the accepted format for multi-line comments is: /* * Blah blah blah * Blah blah blah */ The second part of the patch looks okay (but again, with a comment). > Anyways with these changes the issue does not reproduce. Very good. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-04 14:24 ` Alan Stern @ 2019-12-04 14:37 ` Erkka Talvitie 2019-12-05 10:35 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-04 14:37 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Alan Stern <stern@rowland.harvard.edu> > Sent: keskiviikko 4. joulukuuta 2019 16.24 > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > On Wed, 4 Dec 2019, Erkka Talvitie wrote: > > > > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then we > > > > should return -EPIPE, as the existing code does. But if QTD_PID > > > > == 1 then the code should continue, as your patch does -- with one > > > > difference: Put the additional check for EHCI_TUNE_CERR between > > > > the tests for DBE and XACT instead of after XACT (because XACT > > > > would decrement CERR whereas DBE wouldn't). > > > > > > Good point, I agree. > > > > > > > > > > > Can you make that change and test it? > > > > > > Sure, I have made the change and test it as soon as possible. > > > > I am not actually totally sure if I understood you correctly, but I tested a > change where the first stall check is like this (PID_CODE_IN is defined as 1): > > > > - } else if (QTD_CERR(token)) { > > + } else if (QTD_CERR(token) && (QTD_PID (token) != > > + PID_CODE_IN)) { > > status = -EPIPE; > > > > And the second stall check (now between DBE and XACT): > > + } else if (QTD_CERR(token)) { > > + status = -EPIPE; > > > > Is this what you meant? Please correct me if I am wrong. > > Actually, what I meant for the first part was: > > } else if (QTD_CERR(token) && > (QTD_CERR(token) > < EHCI_TUNE_CERR || > QTD_PID(token) != > PID_CODE_IN)) { Ok, now I understand the change. Good. > > And of course there should be a comment before this line, explaining what it > does. By the way, the accepted format for multi-line comments > is: > > /* > * Blah blah blah > * Blah blah blah > */ > Thanks for the information. I noticed that my comments were different than the existing ones in the file and I was already about to change my comments to match the existing style. > The second part of the patch looks okay (but again, with a comment). Yes, I will add the comment here also. > > > Anyways with these changes the issue does not reproduce. > > Very good. I will do the changes and re-test. > > Alan Stern Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-04 14:37 ` Erkka Talvitie @ 2019-12-05 10:35 ` Erkka Talvitie 2019-12-05 14:37 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-05 10:35 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > Sent: keskiviikko 4. joulukuuta 2019 16.38 > To: 'Alan Stern' <stern@rowland.harvard.edu> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > > > > -----Original Message----- > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: keskiviikko 4. joulukuuta 2019 16.24 > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > On Wed, 4 Dec 2019, Erkka Talvitie wrote: > > > > > > > So if CERR == EHCI_TUNE_CERR and the QTD_PID != 1 (not IN) then > > > > > we should return -EPIPE, as the existing code does. But if > > > > > QTD_PID == 1 then the code should continue, as your patch does > > > > > -- with one > > > > > difference: Put the additional check for EHCI_TUNE_CERR between > > > > > the tests for DBE and XACT instead of after XACT (because XACT > > > > > would decrement CERR whereas DBE wouldn't). > > > > > > > > Good point, I agree. > > > > > > > > > > > > > > Can you make that change and test it? > > > > > > > > Sure, I have made the change and test it as soon as possible. > > > > > > I am not actually totally sure if I understood you correctly, but I > tested a > > change where the first stall check is like this (PID_CODE_IN is > > defined as > 1): > > > > > > - } else if (QTD_CERR(token)) { > > > + } else if (QTD_CERR(token) && (QTD_PID (token) != > > > + PID_CODE_IN)) { > > > status = -EPIPE; > > > > > > And the second stall check (now between DBE and XACT): > > > + } else if (QTD_CERR(token)) { > > > + status = -EPIPE; > > > > > > Is this what you meant? Please correct me if I am wrong. > > > > Actually, what I meant for the first part was: > > > > } else if (QTD_CERR(token) && > > (QTD_CERR(token) > > < EHCI_TUNE_CERR || > > QTD_PID(token) != > > PID_CODE_IN)) { > > Ok, now I understand the change. Good. I tested this change and the issue did not reproduce. However when I was writing the comments for the patch I started to think what happens in this following scenario: The PID Code is IN. 1. First there will be XACT, the CERR is decremented, let's say from 3 to 2 and the host controller executes a retry. 2. On this next try there will happen the condition mentioned in the Table 4-13 of the EHCI specification so that the MMF is set and the queue is halted (because it is IN). 3. To my understanding now the execution enters to this first stall check if, as CERR is > 0 and CERR < EHCI_TUNE_CERR. 4. The -EPIPE (stall) is returned when actually the queue was halted due to MMF - not stall - and the -EPROTO should be returned. Is my logic correct or am I missing something? If you agree with me then I would like to present you a bit more bold (in a sense of amount of refactoring) RFC. In high level this another RFC separates 1. error check and 2. stall check. For me this approach is a bit easier to understand from the code. Or then please propose another solution. If you do not agree with this scenario then never mind the above suggestion about RFC doing more refactoring. > > > > > And of course there should be a comment before this line, explaining > > what > it > > does. By the way, the accepted format for multi-line comments > > is: > > > > /* > > * Blah blah blah > > * Blah blah blah > > */ > > > > Thanks for the information. I noticed that my comments were different than > the existing ones in the file and I was already about to change my comments > to match the existing style. > > > The second part of the patch looks okay (but again, with a comment). > > Yes, I will add the comment here also. > > > > > > Anyways with these changes the issue does not reproduce. > > > > Very good. > > I will do the changes and re-test. > > > > > Alan Stern > > Erkka Talvitie Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-05 10:35 ` Erkka Talvitie @ 2019-12-05 14:37 ` Alan Stern 2019-12-05 15:00 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2019-12-05 14:37 UTC (permalink / raw) To: Erkka Talvitie; +Cc: gregkh, linux-usb, claus.baumgartner On Thu, 5 Dec 2019, Erkka Talvitie wrote: > I tested this change and the issue did not reproduce. > > However when I was writing the comments for the patch I started to think > what happens in this following scenario: > > The PID Code is IN. > > 1. First there will be XACT, the CERR is decremented, let's say from 3 to 2 > and the host controller executes a retry. > 2. On this next try there will happen the condition mentioned in the Table > 4-13 of the EHCI specification so that the MMF is set and the queue is > halted (because it is IN). > 3. To my understanding now the execution enters to this first stall check > if, as CERR is > 0 and CERR < EHCI_TUNE_CERR. > 4. The -EPIPE (stall) is returned when actually the queue was halted due to > MMF - not stall - and the -EPROTO should be returned. > > Is my logic correct or am I missing something? The same thought had occurred to me. > If you agree with me then I would like to present you a bit more bold (in a > sense of amount of refactoring) RFC. In high level this another RFC > separates 1. error check and 2. stall check. For me this approach is a bit > easier to understand from the code. Or then please propose another > solution. I was going to suggest: Just check for MMF and PID == IN before checking for STALL. Everything else can remain the way it is. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-05 14:37 ` Alan Stern @ 2019-12-05 15:00 ` Erkka Talvitie 2019-12-09 9:57 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-05 15:00 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Alan Stern <stern@rowland.harvard.edu> > Sent: torstai 5. joulukuuta 2019 16.38 > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > On Thu, 5 Dec 2019, Erkka Talvitie wrote: > > > I tested this change and the issue did not reproduce. > > > > However when I was writing the comments for the patch I started to > > think what happens in this following scenario: > > > > The PID Code is IN. > > > > 1. First there will be XACT, the CERR is decremented, let's say from 3 > > to 2 and the host controller executes a retry. > > 2. On this next try there will happen the condition mentioned in the > > Table > > 4-13 of the EHCI specification so that the MMF is set and the queue is > > halted (because it is IN). > > 3. To my understanding now the execution enters to this first stall > > check if, as CERR is > 0 and CERR < EHCI_TUNE_CERR. > > 4. The -EPIPE (stall) is returned when actually the queue was halted > > due to MMF - not stall - and the -EPROTO should be returned. > > > > Is my logic correct or am I missing something? > > The same thought had occurred to me. > > > If you agree with me then I would like to present you a bit more bold > > (in a sense of amount of refactoring) RFC. In high level this another > > RFC separates 1. error check and 2. stall check. For me this approach > > is a bit easier to understand from the code. Or then please propose > > another solution. > > I was going to suggest: Just check for MMF and PID == IN before checking for > STALL. Everything else can remain the way it is. Ok, just to double check: I take the existing driver code (I will not apply the earlier RFC on top of that) and apply one more check before the original stall check (that is): } else if (QTD_CERR(token)) { The check that I will add is checking MMF bit && PID == IN, and this check comes right after the babble check, right? Good, seems like a simple change. Yet I still prefer to test the change. Unfortunately that goes to the next week as we have a national holiday tomorrow. I will get back to you most likely on Monday. > > Alan Stern > Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-05 15:00 ` Erkka Talvitie @ 2019-12-09 9:57 ` Erkka Talvitie 2019-12-09 15:24 ` Alan Stern 0 siblings, 1 reply; 16+ messages in thread From: Erkka Talvitie @ 2019-12-09 9:57 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Erkka Talvitie <erkka.talvitie@vincit.fi> > Sent: torstai 5. joulukuuta 2019 17.00 > To: 'Alan Stern' <stern@rowland.harvard.edu> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > > > > -----Original Message----- > > From: Alan Stern <stern@rowland.harvard.edu> > > Sent: torstai 5. joulukuuta 2019 16.38 > > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > > claus.baumgartner@med.ge.com > > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > > disconnected > > > > On Thu, 5 Dec 2019, Erkka Talvitie wrote: > > > > > I tested this change and the issue did not reproduce. > > > > > > However when I was writing the comments for the patch I started to > > > think what happens in this following scenario: > > > > > > The PID Code is IN. > > > > > > 1. First there will be XACT, the CERR is decremented, let's say from > > > 3 to 2 and the host controller executes a retry. > > > 2. On this next try there will happen the condition mentioned in the > > > Table > > > 4-13 of the EHCI specification so that the MMF is set and the queue > > > is halted (because it is IN). > > > 3. To my understanding now the execution enters to this first stall > > > check if, as CERR is > 0 and CERR < EHCI_TUNE_CERR. > > > 4. The -EPIPE (stall) is returned when actually the queue was halted > > > due to MMF - not stall - and the -EPROTO should be returned. > > > > > > Is my logic correct or am I missing something? > > > > The same thought had occurred to me. > > > > > If you agree with me then I would like to present you a bit more > > > bold (in a sense of amount of refactoring) RFC. In high level this > > > another RFC separates 1. error check and 2. stall check. For me this > > > approach is a bit easier to understand from the code. Or then please > > > propose another solution. > > > > I was going to suggest: Just check for MMF and PID == IN before > > checking > for > > STALL. Everything else can remain the way it is. > > Ok, just to double check: > > I take the existing driver code (I will not apply the earlier RFC on top of > that) and apply one more check before the original stall check (that is): > } else if (QTD_CERR(token)) { > > The check that I will add is checking MMF bit && PID == IN, and this check > comes right after the babble check, right? > > Good, seems like a simple change. Yet I still prefer to test the change. > Unfortunately that goes to the next week as we have a national holiday > tomorrow. > I will get back to you most likely on Monday. I tested this change and it removes the error messages from the output. > > > > > Alan Stern > > > Erkka Talvitie Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-09 9:57 ` Erkka Talvitie @ 2019-12-09 15:24 ` Alan Stern 2019-12-10 6:31 ` Erkka Talvitie 0 siblings, 1 reply; 16+ messages in thread From: Alan Stern @ 2019-12-09 15:24 UTC (permalink / raw) To: Erkka Talvitie; +Cc: gregkh, linux-usb, claus.baumgartner On Mon, 9 Dec 2019, Erkka Talvitie wrote: > > Ok, just to double check: > > > > I take the existing driver code (I will not apply the earlier RFC on top > of > > that) and apply one more check before the original stall check (that is): > > } else if (QTD_CERR(token)) { > > > > The check that I will add is checking MMF bit && PID == IN, and this check > > comes right after the babble check, right? > > > > Good, seems like a simple change. Yet I still prefer to test the change. > > Unfortunately that goes to the next week as we have a national holiday > > tomorrow. > > I will get back to you most likely on Monday. > > I tested this change and it removes the error messages from the output. Great! Okay, feel free to submit a new patch. Alan Stern ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected 2019-12-09 15:24 ` Alan Stern @ 2019-12-10 6:31 ` Erkka Talvitie 0 siblings, 0 replies; 16+ messages in thread From: Erkka Talvitie @ 2019-12-10 6:31 UTC (permalink / raw) To: 'Alan Stern'; +Cc: gregkh, linux-usb, claus.baumgartner > -----Original Message----- > From: Alan Stern <stern@rowland.harvard.edu> > Sent: maanantai 9. joulukuuta 2019 17.25 > To: Erkka Talvitie <erkka.talvitie@vincit.fi> > Cc: gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; > claus.baumgartner@med.ge.com > Subject: RE: [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is > disconnected > > On Mon, 9 Dec 2019, Erkka Talvitie wrote: > > > > Ok, just to double check: > > > > > > I take the existing driver code (I will not apply the earlier RFC on > > > top > > of > > > that) and apply one more check before the original stall check (that is): > > > } else if (QTD_CERR(token)) { > > > > > > The check that I will add is checking MMF bit && PID == IN, and this > > > check comes right after the babble check, right? > > > > > > Good, seems like a simple change. Yet I still prefer to test the change. > > > Unfortunately that goes to the next week as we have a national > > > holiday tomorrow. > > > I will get back to you most likely on Monday. > > > > I tested this change and it removes the error messages from the output. > > Great! Okay, feel free to submit a new patch. Thank you, a new patch was submitted. > > Alan Stern Erkka Talvitie ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-12-10 6:31 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-29 14:08 [RFCv1 1/1] USB: EHCI: Do not return -EPIPE when hub is disconnected Erkka Talvitie 2019-12-02 19:43 ` Alan Stern 2019-12-03 9:38 ` Erkka Talvitie 2019-12-03 10:54 ` Erkka Talvitie 2019-12-03 13:35 ` Erkka Talvitie 2019-12-03 19:01 ` Alan Stern 2019-12-04 8:55 ` Erkka Talvitie 2019-12-04 13:18 ` Erkka Talvitie 2019-12-04 14:24 ` Alan Stern 2019-12-04 14:37 ` Erkka Talvitie 2019-12-05 10:35 ` Erkka Talvitie 2019-12-05 14:37 ` Alan Stern 2019-12-05 15:00 ` Erkka Talvitie 2019-12-09 9:57 ` Erkka Talvitie 2019-12-09 15:24 ` Alan Stern 2019-12-10 6:31 ` Erkka Talvitie
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).