| Message ID | 31c04f926af6ddfe0e244c2d6b0999ee9442659a.1739509438.git.gerickson@nuovations.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add QMI Device Service Request Rate-limit Option | expand |
Hi Grant, > 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 = 0; I think this is still wrong? There's a reason why we have doc/coding-style.txt item M7. > + 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) { Consider the starting condition: - last_req_sent_time_us -> zero - throttle_enabled -> true We don't enter into this if(), 'now' is still zero. > + 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; > + throttle_enabled is true, we set last_req_sent_time_us to 0. In effect your throttling doesn't work...? > if (l_queue_length(transport->req_queue) > 0) > return true; > Regards, -Denis
On Feb 14, 2025, at 7:29 AM, Denis Kenzior <denkenz@gmail.com> wrote: > > > Hi Grant, > > > 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 = 0; > > I think this is still wrong? There's a reason why we have doc/coding-style.txt item M7. > >> + 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) { > > Consider the starting condition: > > - last_req_sent_time_us -> zero > - throttle_enabled -> true > > We don't enter into this if(), 'now' is still zero. > >> + 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; >> + > > throttle_enabled is true, we set last_req_sent_time_us to 0. In effect your throttling doesn't work...? Confirmed, thanks. v4 submitted. I’ve noted that the default ofono warning flags do not highlight the potential conditional issue with ’now’ on arm, aarch64, or x86_64 with gcc or clang. If I dial up warnings with -Wall -Wextra -Weverything, then I get the warning. With that in mind, do you want ’now’ to follow M7 with the default warning flag set or to not follow M7 but ensuring the warning is addressed using default initialization with a non-default set of warning flags? Best, Grant
Hi Grant, > > I’ve noted that the default ofono warning flags do not highlight the potential conditional issue with ’now’ on arm, aarch64, or x86_64 with gcc or clang. If I dial up warnings with -Wall -Wextra -Weverything, then I get the warning. This sometimes depends on the version of gcc as well. > > With that in mind, do you want ’now’ to follow M7 with the default warning flag set or to not follow M7 but ensuring the warning is addressed using default initialization with a non-default set of warning flags? M7 should be followed, and if the warning is known to be spurious, then a pragma may be added. See ell for examples of this (grep for _Pragma). However, _Pragma is not always followed. Easiest may be to give the compiler a chance of misinterpreting the intent :) Regards, -Denis
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c index bbe6440abe0f..f26a9816c71c 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 = 0; + 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(+)