Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.

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