Re: [PATCH 08/10] qmi: Implement QMI service request rate limiting in 'can_write_data'.

Hi Grant,

On 2/11/25 11:52 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.
> ---
>   drivers/qmimodem/qmi.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 

Patches 5, 6 and 8 should belong in the same commit.  Prefer to group new member 
definitions with their first use.

> diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
> index 00086578da10..591129c252e3 100644
> --- a/drivers/qmimodem/qmi.c
> +++ b/drivers/qmimodem/qmi.c
> @@ -665,8 +665,23 @@ static bool can_write_data(struct l_io *io, void *user_data)
>   {
>   	struct qmi_transport *transport = user_data;
>   	struct qmi_request *req;
> +	const uint64_t now = l_time_now();
> +	uint64_t delta = 0;
>   	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 (transport->last_req_sent_time_us != 0) {

Shouldn't this check min_req_period_us first, before incurring l_time_now overhead?

> +		delta = l_time_diff(now, transport->last_req_sent_time_us);
> +
> +		if (delta < transport->min_req_period_us)
> +			return true;
> +	}
> +

Something like:
if (throttling_enabled) {
	now = l_time_now();
	delta = l_time_diff(now, transport->last_req_sent_time_us);

	if (delta < min_req_period_us)
		return true;
^^^^ Note, that this is a poor approach since it just busy-loops the event-loop 
until the throttle time gate expires.  Much better would be to use some sort of 
timeout.

	transport->last_req_sent_time_us = now;
}

>   	req = l_queue_pop_head(transport->req_queue);
>   	if (!req)
>   		return false;

Regards,
-Denis