* [PATCH v2 1/7] qmi: Added enumeration for device quirk flags.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 0:33 ` [PATCH v2 2/7] qmi: Added structure for device options Grant Erickson
` (5 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
Defines device-specific "quirks" that may alter the default behavior
of the QMI driver for a specific, instantiated device.
---
drivers/qmimodem/qmi.h | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 69698ee049c6..e74ff3feaeec 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -56,6 +56,24 @@ enum qmi_error {
QMI_ERROR_INVALID_QMI_COMMAND = 71,
};
+/**
+ * This defines device-specific "quirks" that may alter the default
+ * behavior of the QMI driver for a specific, instantiated device.
+ */
+enum qmi_qmux_device_quirk {
+ /**
+ * The device has no "quirks".
+ */
+ QMI_QMUX_DEVICE_QUIRK_NONE = 0x00,
+
+ /**
+ * The device has a "quirk" where it can lock up and hang (not
+ * responding to subsequent commands) due to high QMI service
+ * request arrival rates.
+ */
+ QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT = 0x01
+};
+
typedef void (*qmi_destroy_func_t)(void *user_data);
struct qmi_service;
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/7] qmi: Added structure for device options.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
2025-02-13 0:33 ` [PATCH v2 1/7] qmi: Added enumeration for device quirk flags Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 0:33 ` [PATCH v2 3/7] qmi: Added device options parameter to 'qmi_qmux_device_new' Grant Erickson
` (4 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
Defines options that may alter the default behavior of the QMI driver
for a specific, instantiated device.
---
drivers/qmimodem/qmi.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index e74ff3feaeec..4ba27847176f 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -74,6 +74,25 @@ enum qmi_qmux_device_quirk {
QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT = 0x01
};
+/**
+ * Defines options that may alter the default behavior of the QMI
+ * driver for a specific, instantiated device.
+ */
+struct qmi_qmux_device_options {
+ /**
+ * Device-specific "quirk".
+ */
+ enum qmi_qmux_device_quirk quirks;
+
+ /**
+ * If quirks has #QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT set, this
+ * is the minimum period, in microseconds, in which back-to-back
+ * QMI service requests may be sent to avoid triggering a
+ * firmware lock up and hang.
+ */
+ unsigned int min_req_period_us;
+};
+
typedef void (*qmi_destroy_func_t)(void *user_data);
struct qmi_service;
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 3/7] qmi: Added device options parameter to 'qmi_qmux_device_new'.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
2025-02-13 0:33 ` [PATCH v2 1/7] qmi: Added enumeration for device quirk flags Grant Erickson
2025-02-13 0:33 ` [PATCH v2 2/7] qmi: Added structure for device options Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 0:33 ` [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data' Grant Erickson
` (3 subsequent siblings)
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
An optional pointer to immutable device options that may alter the
default behavior of the QMI driver for the instantiated device.
---
drivers/qmimodem/qmi.c | 3 ++-
drivers/qmimodem/qmi.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index 10dbdaac8bf6..bbe6440abe0f 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1567,7 +1567,8 @@ static const struct qmi_transport_ops qmux_ops = {
.write = qmi_qmux_device_write,
};
-struct qmi_qmux_device *qmi_qmux_device_new(const char *device)
+struct qmi_qmux_device *qmi_qmux_device_new(const char *device,
+ const struct qmi_qmux_device_options *options)
{
struct qmi_qmux_device *qmux;
int fd;
diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 4ba27847176f..09b551d74823 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -107,7 +107,8 @@ typedef void (*qmi_qrtr_node_lookup_done_func_t)(void *);
typedef void (*qmi_service_result_func_t)(struct qmi_result *, void *);
-struct qmi_qmux_device *qmi_qmux_device_new(const char *device);
+struct qmi_qmux_device *qmi_qmux_device_new(const char *device,
+ const struct qmi_qmux_device_options *options);
void qmi_qmux_device_free(struct qmi_qmux_device *qmux);
void qmi_qmux_device_set_debug(struct qmi_qmux_device *qmux,
qmi_debug_func_t func, void *user_data);
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
` (2 preceding siblings ...)
2025-02-13 0:33 ` [PATCH v2 3/7] qmi: Added device options parameter to 'qmi_qmux_device_new' Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 2:07 ` Denis Kenzior
2025-02-13 0:33 ` [PATCH v2 5/7] qmi: Handle request rate limit option in 'qmi_qmux_device_new' Grant Erickson
` (2 subsequent siblings)
6 siblings, 1 reply; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
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;
+
if (l_queue_length(transport->req_queue) > 0)
return true;
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.
2025-02-13 0:33 ` [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data' Grant Erickson
@ 2025-02-13 2:07 ` Denis Kenzior
2025-02-14 4:22 ` Grant Erickson
0 siblings, 1 reply; 12+ messages in thread
From: Denis Kenzior @ 2025-02-13 2:07 UTC (permalink / raw)
To: Grant Erickson, ofono
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.
2025-02-13 2:07 ` Denis Kenzior
@ 2025-02-14 4:22 ` Grant Erickson
2025-02-14 4:54 ` Grant Erickson
2025-02-14 15:23 ` Denis Kenzior
0 siblings, 2 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-14 4:22 UTC (permalink / raw)
To: Denis Kenzior; +Cc: ofono
On Feb 12, 2025, at 6:07 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> 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.
>> ---
>> @@ -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.
Surprisingly, GCC 8.2.0 didn’t squawk about it:
% ${SYSROOT}usr/bin/arm-dey-linux-gnueabi/arm-dey-linux-gnueabi-gcc --version
arm-dey-linux-gnueabi-gcc (GCC) 8.2.0
Copyright (C) 2018 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
% ${SYSROOT}usr/bin/arm-dey-linux-gnueabi/arm-dey-linux-gnueabi-gcc
-DHAVE_CONFIG_H -I. -I${PROJECTROOT}third_party/ofono/repo -I./include -I./src
-I${PROJECTROOT}third_party/ofono/repo/src
-I${PROJECTROOT}third_party/ofono/repo/gdbus
-I${PROJECTROOT}third_party/ofono/repo/gisi
-I${PROJECTROOT}third_party/ofono/repo/gatchat
-I${PROJECTROOT}third_party/ofono/repo/gril --sysroot=${PROJECTROOT}rootfs
-isystem ${SYSROOT}usr/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/linux/linux-dey/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/dbus/usr/include/dbus-1.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/dbus/usr/lib/dbus-1.0/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/include/gio-unix-2.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/include/glib-2.0
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/glib/usr/lib/glib-2.0/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/ell/usr/include
-I${PROJECTROOT}results/${BUILDPRODUCT}/digi/dey/8.2.0/${BUILDCONFIG}/third_party/eudev/usr/include
-DOFONO_PLUGIN_BUILTIN -DPLUGINDIR=\""/usr/lib/ofono/plugins"\"
-DUNITDIR=\""./unit/"\" --sysroot=${PROJECTROOT}rootfs -mcpu=cortex-a8
-mfloat-abi=hard -mfpu=neon -fno-omit-frame-pointer -fno-strict-aliasing
-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -MT drivers/qmimodem/qmi.o -MD -MP
-MF $depbase.Tpo -c -o drivers/qmimodem/qmi.o
${PROJECTROOT}third_party/ofono/repo/drivers/qmimodem/qmi.c && mv -f
$depbase.Tpo $depbase.Po
I’ve another evaluation branch with GCC 12.3.1. I’ll cross-check there.
> Compilers are still not great at tying together multiple conditionals... You might still be better off keeping this inside the if() you added above...
There are two separate actions to be taken:
1. Performing the rate limit check before the request is dequeued and sent.
2. Setting the last sent time after the request is dequeued and successfully sent.
So, there need to be two, separate conditional blocks for each.
> 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)
I thought about zero-initializing it in ‘qmi_qmux_device_new’; however, diving into the implementation of ‘l_new’, it appears as though all memory is zeroed out, so the zero-initialization seemed redundant.
I’ll have a v3 shortly based on the feedback from GCC 12.3.1.
Best,
Grant
--
Principal
Nuovations
gerickson@nuovations.com
https://www.nuovations.com/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.
2025-02-14 4:22 ` Grant Erickson
@ 2025-02-14 4:54 ` Grant Erickson
2025-02-14 15:23 ` Denis Kenzior
1 sibling, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-14 4:54 UTC (permalink / raw)
To: Denis Kenzior; +Cc: ofono
On Feb 13, 2025, at 8:22 PM, Grant Erickson <gerickson@nuovations.com> wrote:
>
>
> On Feb 12, 2025, at 6:07 PM, Denis Kenzior <denkenz@gmail.com> wrote:
>> Pretty sure this won't work and gcc will complain.
>
> Surprisingly, GCC 8.2.0 didn’t squawk about it:
Surprisingly, GCC 12.3.1 didn’t squawk about it either:
% ${SYSROOT}/bin/arm-none-linux-gnueabihf-gcc --version
arm-none-linux-gnueabihf-gcc (Arm GNU Toolchain 12.3.Rel1 (Build arm-12.35))
12.3.1 20230626 Copyright (C) 2022 Free Software Foundation, Inc. This is free
software; see the source for copying conditions. There is NO warranty; not even
for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
${SYSROOT}/bin/arm-none-linux-gnueabihf-gcc -DHAVE_CONFIG_H -I.
-I${PROJECTROOT}/third_party/ofono/repo -I./include -I./src
-I${PROJECTROOT}/third_party/ofono/repo/src
-I${PROJECTROOT}/third_party/ofono/repo/gdbus
-I${PROJECTROOT}/third_party/ofono/repo/gisi
-I${PROJECTROOT}/third_party/ofono/repo/gatchat
-I${PROJECTROOT}/third_party/ofono/repo/gril
--sysroot=${SYSROOT}/arm-none-linux-gnueabihf/libc
-D_POSIX_C_SOURCE=201112L -D_DEFAULT_SOURCE
-D_XOPEN_SOURCE -isystem ${SYSROOT}/arm-none-linux-gnueabihf/libc/usr/include
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/linux/linux-dey/include
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/dbus/usr/include/dbus-1.0
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/dbus/usr/lib/dbus-1.0/include
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/glib/usr/include/gio-unix-2.0
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/glib/usr/include/glib-2.0
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/glib/usr/lib/glib-2.0/include
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/ell/usr/include
-I${PROJECTROOT}/results/${BUILDPRODUCT}/arm/gnu-toolchain/12.3.1/${BUILDCONFIG}/third_party/eudev/usr/include -DOFONO_PLUGIN_BUILTIN
-DPLUGINDIR=\""/usr/lib/ofono/plugins"\" -DUNITDIR=\""./unit/"\"
--sysroot=${SYSROOT}/arm-none-linux-gnueabihf/libc -mcpu=cortex-a8
-mfloat-abi=hard -mfpu=neon -fno-omit-frame-pointer -fno-strict-aliasing
-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -MT drivers/qmimodem/qmi.o -MD -MP -MF
$depbase.Tpo -c -o drivers/qmimodem/qmi.o
${PROJECTROOT}/third_party/ofono/repo/drivers/qmimodem/qmi.c &&\ mv -f
$depbase.Tpo $depbase.Po
Regardless, I’ll address it and have a v3 shortly.
--
Principal
Nuovations
gerickson@nuovations.com
https://www.nuovations.com/
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data'.
2025-02-14 4:22 ` Grant Erickson
2025-02-14 4:54 ` Grant Erickson
@ 2025-02-14 15:23 ` Denis Kenzior
1 sibling, 0 replies; 12+ messages in thread
From: Denis Kenzior @ 2025-02-14 15:23 UTC (permalink / raw)
To: Grant Erickson; +Cc: ofono
Hi Grant,
>
> There are two separate actions to be taken:
>
> 1. Performing the rate limit check before the request is dequeued and sent.
> 2. Setting the last sent time after the request is dequeued and successfully sent.
I'd rearrange the whole thing such that 2 isn't needed. If you update the last
sent time in 1, the worst that can happen is that the write() will fail. Which
will probably trigger a socket close shortly afterward anyway.
>
> So, there need to be two, separate conditional blocks for each.
>
Only if you want to deal with compiler warnings ;)
>> 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)
>
> I thought about zero-initializing it in ‘qmi_qmux_device_new’; however, diving into the
That is not what I'm talking about. l_new does indeed zero-initialize
everything. Explicit init to 0 is not needed (or wanted). See my reply to v3.
implementation of ‘l_new’, it appears as though all memory is zeroed out, so the
zero-initialization seemed redundant.
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 5/7] qmi: Handle request rate limit option in 'qmi_qmux_device_new'.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
` (3 preceding siblings ...)
2025-02-13 0:33 ` [PATCH v2 4/7] qmi: Implement QMI service request rate limiting in 'can_write_data' Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 0:33 ` [PATCH v2 6/7] udevng: Set the QMI minimum service request period for Quectel BG96 modems Grant Erickson
2025-02-13 0:33 ` [PATCH v2 7/7] gobi: Pass QMI qmux device options to 'qmi_qmux_device_new' Grant Erickson
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
If options are specified and QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT is
asserted, set the qmux transport minimum service request period from
the specified options.
---
drivers/qmimodem/qmi.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index ace83044138a..f544894ad4d5 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1611,6 +1611,16 @@ struct qmi_qmux_device *qmi_qmux_device_new(const char *device,
qmux = l_new(struct qmi_qmux_device, 1);
+ if (options != NULL) {
+ if ((options->quirks & QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT)
+ == QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT) {
+ qmux->transport.min_req_period_us = options->min_req_period_us;
+
+ ofono_info("QMI minimum service request period %u us",
+ qmux->transport.min_req_period_us);
+ }
+ }
+
if (qmi_transport_open(&qmux->transport, fd, &qmux_ops) < 0) {
close(fd);
l_free(qmux);
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 6/7] udevng: Set the QMI minimum service request period for Quectel BG96 modems.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
` (4 preceding siblings ...)
2025-02-13 0:33 ` [PATCH v2 5/7] qmi: Handle request rate limit option in 'qmi_qmux_device_new' Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
2025-02-13 0:33 ` [PATCH v2 7/7] gobi: Pass QMI qmux device options to 'qmi_qmux_device_new' Grant Erickson
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
The Quectel Wireless Solutions Co., Ltd. (2c7c) BG96 CAT-M1/NB-IoT
modem (0296) has a firmware issue where it can lock up and hang (not
responding to subsequent commands) due to high QMI service request
arrival rates. If the vendor and model match those, then rate limit
QMI service requests to no more than one every 2,000 us.
---
plugins/udevng.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index 64875a47752b..b8df66de5c71 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1118,6 +1118,20 @@ static gboolean setup_quectelqmi(struct modem_info *modem)
DBG("%s", modem->syspath);
+ /*
+ * The Quectel Wireless Solutions Co., Ltd. (2c7c) BG96
+ * CAT-M1/NB-IoT modem (0296) has a firmware issue where it can
+ * lock up and hang (not responding to subsequent commands) due to
+ * high QMI service request arrival rates. If the vendor and model
+ * match those, then rate limit QMI service requests to no more
+ * than one every 2,000 us.
+ */
+ if (l_streq0(modem->vendor, "2c7c")) {
+ if (l_streq0(modem->model, "0296"))
+ ofono_modem_set_integer(modem->modem,
+ "RequestThrottleTimeUs", 2000);
+ }
+
for (list = modem->devices; list; list = g_slist_next(list)) {
const struct device_info *info = list->data;
const char *subsystem =
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 7/7] gobi: Pass QMI qmux device options to 'qmi_qmux_device_new'.
2025-02-13 0:33 [PATCH v2 0/7] Add QMI Device Service Request Rate-limit Option Grant Erickson
` (5 preceding siblings ...)
2025-02-13 0:33 ` [PATCH v2 6/7] udevng: Set the QMI minimum service request period for Quectel BG96 modems Grant Erickson
@ 2025-02-13 0:33 ` Grant Erickson
6 siblings, 0 replies; 12+ messages in thread
From: Grant Erickson @ 2025-02-13 0:33 UTC (permalink / raw)
To: ofono
If the QMI minimum service request period property is set on the modem
object, then set the appropriate QMI QMUX device option quirk and
value.
---
plugins/gobi.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/plugins/gobi.c b/plugins/gobi.c
index 027aa91d6ade..dfc7402e36aa 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -842,9 +842,19 @@ error:
__shutdown_device(modem);
}
+/*
+ * Enable the modem. The following modem properties are optionally set:
+ *
+ * RequestThrottleTimeUs
+ * The minimum period, in microseconds, in which back-to-back *
+ * modem service requests may be sent.
+ *
+ */
static int gobi_enable(struct ofono_modem *modem)
{
struct gobi_data *data = ofono_modem_get_data(modem);
+ struct qmi_qmux_device_options options = { QMI_QMUX_DEVICE_QUIRK_NONE, 0 };
+ int min_req_period_us;
const char *device;
DBG("%p", modem);
@@ -853,7 +863,17 @@ static int gobi_enable(struct ofono_modem *modem)
if (!device)
return -EINVAL;
- data->device = qmi_qmux_device_new(device);
+ /*
+ * If the QMI minimum service request period property is set, then
+ * set the appropriate QMI QMUX device quirk and value.
+ */
+ min_req_period_us = ofono_modem_get_integer(modem, "RequestThrottleTimeUs");
+ if (min_req_period_us > 0) {
+ options.quirks |= QMI_QMUX_DEVICE_QUIRK_REQ_RATE_LIMIT;
+ options.min_req_period_us = min_req_period_us;
+ }
+
+ data->device = qmi_qmux_device_new(device, &options);
if (!data->device)
return -EIO;
--
2.45.0
^ permalink raw reply related [flat|nested] 12+ messages in thread