risc-v/mpfs: usb: fix fierce cpu polling if remote closes #186
Conversation
| @@ -3460,11 +3487,28 @@ static int mpfs_usb_interrupt(int irq, void *context, void *arg) | |||
| { | |||
| for (i = 0; i < MPFS_USB_NENDPOINTS; i++) | |||
| { | |||
| /* Check if dead connections are back in business */ | |||
|
|
|||
| if (g_linkdead) | |||
There was a problem hiding this comment.
Like the comment says, maybe it is worthwhile moving this global linkdead info to the ep (or use privep->linkdead here).
There was a problem hiding this comment.
The thing is that a TX ep has died, and we're checking the RX ep, which is completely different EP. Not sure there's any relation to TX/RX eps; so to say if TX EP1 has died, activity on RX EP1 or EP2 might wake it up. Need to check though.
There was a problem hiding this comment.
Yes you are right, was not careful enough. I don't think anything in the USB standard mandates that the end point numbers for IN/OUT should be the same for the same connection. This comment can be ignored.
There was a problem hiding this comment.
Maybe it's possible to find out the pair, IN/OUT but not sure if it's visible in this driver level. I'll have a look, it'd be the best option, especially if there's multiple endpoints (IN, IN ,OUT OUT)..
There was a problem hiding this comment.
By tracing mpfs_allocep() I can see the eps being requested, but it's up to the higher level to pick the ones, so the driver doesn't see any IN/OUT pairs to rely on.
There was a problem hiding this comment.
For example, CDCACM has the following (configurable) epnos:
devinfo.epno[CDCACM_EP_INTIN_IDX] = CONFIG_CDCACM_EPINTIN;
devinfo.epno[CDCACM_EP_BULKIN_IDX] = CONFIG_CDCACM_EPBULKIN;
devinfo.epno[CDCACM_EP_BULKOUT_IDX] = CONFIG_CDCACM_EPBULKOUT;
|
Verified that this does fix the problem, CPU load of mavlink_if2 drops from 30% to 3.5% (which is the normal level). |
If the remote end just closes an endpoint and no longer handles it, the system is prone to intensive cpu polling via mpfs_write_tx_fifo() especially if the device side doesn't know a thing about what the remote did. Fix this by marking the EP as dead, which will skip all writes causing unnecessary polling. The EP is back in business if the remote end sends some data (rx) or the connection is re-established. Signed-off-by: Eero Nurkkala <[email protected]>
eenurkka
force-pushed
the
usb-polling
branch
from
34dd2e9 to
58aec7f
Compare
CC: modlib/modlib_depend.c "modlib/modlib_bind.c", line 741: warning #186-D: pointless comparison of unsigned integer with zero if (rel->r_offset < 0) ^ Signed-off-by: guoshichao <[email protected]>
"ioexpander/gpio.c", line 529: warning #186-D: pointless comparison of unsigned integer with zero if (pintype < GPIO_INPUT_PIN || pintype >= GPIO_NPINTYPES) ^ Signed-off-by: guoshichao <[email protected]>
If the remote end just closes an endpoint and no longer handles it, the system is prone to intensive cpu polling via mpfs_write_tx_fifo() especially if the device side doesn't know a thing about what the remote did.
Fix this by marking the EP as dead, which will skip all writes causing unnecessary polling. The EP is back in business if the remote end sends some data (rx) or the connection is re-established.
Summary
PX4 <-> QGroundControl ACM only connection no longer spins the CPU unnecessarily
Impact
No longer high CPU loads
Testing
Saluki-v2