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

On Feb 14, 2025, at 7:29 AM, Denis Kenzior <[email protected]> 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

-- 
Principal
Nuovations

[email protected]
https://www.nuovations.com/