| Message ID | d2b56b2248111502169c4639329ad8f2fa5715dd.1739406657.git.gerickson@nuovations.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
| Series | Add QMI Device Service Request Rate-limit Option | expand |
Hi Grant, On 2/12/25 6:33 PM, Grant Erickson wrote: > Determine if we need to rate-limit QMI services requests to a > transport-specific minimum request period. If so, return true so that > the queue can be retried again later. > > Adds the following data members to 'qmi_transport': > > * Minimum request period > - The minimum period, in microseconds, for back-to-back QMI > service requests. > > * Last request time > - Time, in microseconds, when the last QMI service request was > sent. > > to support the implementation. > --- > drivers/qmimodem/qmi.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c > index bbe6440abe0f..ace83044138a 100644 > --- a/drivers/qmimodem/qmi.c > +++ b/drivers/qmimodem/qmi.c > @@ -92,6 +92,18 @@ struct qmi_transport { > const struct qmi_transport_ops *ops; > struct debug_data debug; > bool writer_active : 1; > + > + /** > + * If specified via qmi_qmux_device_options, the minimum period > + * for back-to-back QMI service requests. > + */ > + unsigned int min_req_period_us; > + > + /** > + * The time, in microseconds, when the last QMI service request > + * was sent. > + */ > + uint64_t last_req_sent_time_us; > }; > > struct qmi_qmux_device { > @@ -653,8 +665,25 @@ static bool can_write_data(struct l_io *io, void *user_data) > { > struct qmi_transport *transport = user_data; > struct qmi_request *req; > + const bool throttle_enabled = transport->min_req_period_us > 0; > + uint64_t now; > + uint64_t delta; > int r; > > + /* > + * Determine if we need to rate-limit commands to a > + * transport-specific minimum request period. If so, > + * return true so that the queue can be retried again > + * later. > + */ > + if (throttle_enabled && transport->last_req_sent_time_us != 0) { > + now = l_time_now(); > + delta = l_time_diff(now, transport->last_req_sent_time_us); > + > + if (delta < transport->min_req_period_us) > + return true; > + } > + > req = l_queue_pop_head(transport->req_queue); > if (!req) > return false; > @@ -665,6 +694,9 @@ static bool can_write_data(struct l_io *io, void *user_data) > return false; > } > > + if (throttle_enabled) > + transport->last_req_sent_time_us = now; Pretty sure this won't work and gcc will complain. Compilers are still not great at tying together multiple conditionals... You might still be better off keeping this inside the if() you added above... Also, don't you need last_req_sent_time_us to be initialized to be non-zero somehow? Otherwise it is dumb luck that last_req_sent_time_us is set to anything the first time (probably garbage data) Confirmed. CI complains: Alpine (musl) gcc optimized =========================== Configure: PASS Build: FAIL drivers/qmimodem/qmi.c: In function 'can_write_data': drivers/qmimodem/qmi.c:698:50: error: 'now' may be used uninitialized [-Werror=maybe-uninitialized] 698 | transport->last_req_sent_time_us = now; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~ drivers/qmimodem/qmi.c:669:18: note: 'now' was declared here 669 | uint64_t now; | ^~~ cc1: all warnings being treated as errors make[1]: *** [Makefile:4090: drivers/qmimodem/qmi.o] Error 1 make[1]: Target 'all-am' not remade because of errors. make: *** [Makefile:2405: all] Error 2 > + > if (l_queue_length(transport->req_queue) > 0) > return true; > Regards, -Denis
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c index bbe6440abe0f..ace83044138a 100644 --- a/drivers/qmimodem/qmi.c +++ b/drivers/qmimodem/qmi.c @@ -92,6 +92,18 @@ struct qmi_transport { const struct qmi_transport_ops *ops; struct debug_data debug; bool writer_active : 1; + + /** + * If specified via qmi_qmux_device_options, the minimum period + * for back-to-back QMI service requests. + */ + unsigned int min_req_period_us; + + /** + * The time, in microseconds, when the last QMI service request + * was sent. + */ + uint64_t last_req_sent_time_us; }; struct qmi_qmux_device { @@ -653,8 +665,25 @@ static bool can_write_data(struct l_io *io, void *user_data) { struct qmi_transport *transport = user_data; struct qmi_request *req; + const bool throttle_enabled = transport->min_req_period_us > 0; + uint64_t now; + uint64_t delta; int r; + /* + * Determine if we need to rate-limit commands to a + * transport-specific minimum request period. If so, + * return true so that the queue can be retried again + * later. + */ + if (throttle_enabled && transport->last_req_sent_time_us != 0) { + now = l_time_now(); + delta = l_time_diff(now, transport->last_req_sent_time_us); + + if (delta < transport->min_req_period_us) + return true; + } + req = l_queue_pop_head(transport->req_queue); if (!req) return false; @@ -665,6 +694,9 @@ static bool can_write_data(struct l_io *io, void *user_data) return false; } + if (throttle_enabled) + transport->last_req_sent_time_us = now; + if (l_queue_length(transport->req_queue) > 0) return true;
Determine if we need to rate-limit QMI services requests to a transport-specific minimum request period. If so, return true so that the queue can be retried again later. Adds the following data members to 'qmi_transport': * Minimum request period - The minimum period, in microseconds, for back-to-back QMI service requests. * Last request time - Time, in microseconds, when the last QMI service request was sent. to support the implementation. --- drivers/qmimodem/qmi.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)