[PATCH 00/10] Add QMI Device Service Request Rate-limit Option

[PATCH 00/10] Add QMI Device Service Request Rate-limit Option

[Grant Erickson]

Certain modems, among them the Quectel Wireless Solutions Co.,
Ltd. (2c7c) BG96 CAT-M1/NB-IoT modem (0296), have a firmware issue
where they can lock up and hang (not responding to subsequent
commands) due to high QMI service request arrival rates.

QMI service request rate limiting is achieved by:

1. Adding a new, optional options structure to qmi_qmux_device_new
   that has its rate-limit "quirk" and minimum request period fields
   set when the modem "QMIMinReqPeriodUs" integer property is set.

2. Adding logic to the QMI driver that, when a minimum request period
   is set, limits the QMI service request rate accordingly.

(1) and (2) are used by the udevng and gobi plugins such that when the
modem vendor and model match 2c7c/0296, then the option to rate limit
QMI service requests is set to no more than one every 2,000 us.

Grant Erickson (10):
  qmi: Added minimum service request period property string.
  qmi: Added enumeration for device quirk flags.
  qmi: Added structure for device options.
  qmi: Added device options parameter to 'qmi_qmux_device_new'.
  qmi: Added minimum request period data member to 'qmi_transport'.
  qmi: Added last request time data member to 'qmi_transport'.
  qmi: Handle request rate limit option in 'qmi_qmux_device_new'.
  qmi: Implement QMI service request rate limiting in 'can_write_data'.
  udevng: Set the QMI minimum service request period for Quectel BG96
    modems.
  gobi: Pass QMI qmux device options to 'qmi_qmux_device_new'.

 drivers/qmimodem/qmi.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 drivers/qmimodem/qmi.h | 42 +++++++++++++++++++++++++++++++++++++++++-
 plugins/gobi.c         | 14 +++++++++++++-
 plugins/udevng.c       | 14 ++++++++++++++
 4 files changed, 109 insertions(+), 3 deletions(-)

-- 
2.45.0

[PATCH 01/10] qmi: Added minimum service request period property string.

[Grant Erickson]

This may be used by ofono_modem_{g,s}et_integer to get or set the QMI
minimum service request period, in microseconds, modem property.
---
 drivers/qmimodem/qmi.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
index 69698ee049c6..0c786ed065a3 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -43,6 +43,8 @@
 #define QMI_SERVICE_RMS		225	/* Remote management service */
 #define QMI_SERVICE_OMA		226	/* OMA device management service */
 
+#define QMI_PROP_MIN_REQ_PERIOD_US "QMIMinReqPeriodUs"
+
 enum qmi_data_endpoint_type {
 	QMI_DATA_ENDPOINT_TYPE_UNKNOWN   = 0x00,
 	QMI_DATA_ENDPOINT_TYPE_HSIC      = 0x01,
-- 
2.45.0

[PATCH 02/10] qmi: Added enumeration for device quirk flags.

[Grant Erickson]

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 0c786ed065a3..913035897ea4 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -58,6 +58,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

[PATCH 03/10] qmi: Added structure for device options.

[Grant Erickson]

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 913035897ea4..3f60e90c015a 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -76,6 +76,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

[PATCH 04/10] qmi: Added device options parameter to 'qmi_qmux_device_new'.

[Grant Erickson]

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 3f60e90c015a..d83dd30fb4a9 100644
--- a/drivers/qmimodem/qmi.h
+++ b/drivers/qmimodem/qmi.h
@@ -109,7 +109,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

[PATCH 05/10] qmi: Added minimum request period data member to 'qmi_transport'.

[Grant Erickson]

If specified via qmi_qmux_device_options, the minimum period, in
microseconds, for back-to-back QMI service requests.
---
 drivers/qmimodem/qmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index bbe6440abe0f..b16fee252db6 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -92,6 +92,12 @@ 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;
 };
 
 struct qmi_qmux_device {
-- 
2.45.0

[PATCH 06/10] qmi: Added last request time data member to 'qmi_transport'.

[Grant Erickson]

The time, in microseconds, when the last QMI service request was sent.
---
 drivers/qmimodem/qmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/qmimodem/qmi.c b/drivers/qmimodem/qmi.c
index b16fee252db6..2623e0312db4 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -98,6 +98,12 @@ struct qmi_transport {
 	 *  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 {
-- 
2.45.0

[PATCH 07/10] qmi: Handle request rate limit option in 'qmi_qmux_device_new'.

[Grant Erickson]

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 2623e0312db4..00086578da10 100644
--- a/drivers/qmimodem/qmi.c
+++ b/drivers/qmimodem/qmi.c
@@ -1591,6 +1591,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

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

[Grant Erickson]

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(+)

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) {
+		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;
@@ -677,6 +692,8 @@ static bool can_write_data(struct l_io *io, void *user_data)
 		return false;
 	}
 
+	transport->last_req_sent_time_us = now;
+
 	if (l_queue_length(transport->req_queue) > 0)
 		return true;
 
-- 
2.45.0

[PATCH 09/10] udevng: Set the QMI minimum service request period for Quectel BG96 modems.

[Grant Erickson]

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..e7cf494a70ff 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 (g_strcmp0(modem->vendor, "2c7c") == 0) {
+		if (g_strcmp0(modem->model, "0296") == 0) {
+			ofono_modem_set_integer(modem->modem, "QMIMinReqPeriodUs", 2000);
+		}
+	}
+
 	for (list = modem->devices; list; list = g_slist_next(list)) {
 		const struct device_info *info = list->data;
 		const char *subsystem =
-- 
2.45.0

[PATCH 10/10] gobi: Pass QMI qmux device options to 'qmi_qmux_device_new'.

[Grant Erickson]

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 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/plugins/gobi.c b/plugins/gobi.c
index 027aa91d6ade..9ec15deb6fa9 100644
--- a/plugins/gobi.c
+++ b/plugins/gobi.c
@@ -845,6 +845,8 @@ error:
 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 +855,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, QMI_PROP_MIN_REQ_PERIOD_US);
+	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

Re: [PATCH 01/10] qmi: Added minimum service request period property string.

[Denis Kenzior]

Hi Grant,

On 2/11/25 11:52 PM, Grant Erickson wrote:
> This may be used by ofono_modem_{g,s}et_integer to get or set the QMI
> minimum service request period, in microseconds, modem property.
> ---
>   drivers/qmimodem/qmi.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
> index 69698ee049c6..0c786ed065a3 100644
> --- a/drivers/qmimodem/qmi.h
> +++ b/drivers/qmimodem/qmi.h
> @@ -43,6 +43,8 @@
>   #define QMI_SERVICE_RMS		225	/* Remote management service */
>   #define QMI_SERVICE_OMA		226	/* OMA device management service */
>   
> +#define QMI_PROP_MIN_REQ_PERIOD_US "QMIMinReqPeriodUs"
> +

This doesn't really belong here.  Properties that are communicated between 
udevng and gobi/qrtrqmi are documented above the probe() method implementation. 
Also, maybe 'RequestThrottleTimeUs'?

>   enum qmi_data_endpoint_type {
>   	QMI_DATA_ENDPOINT_TYPE_UNKNOWN   = 0x00,
>   	QMI_DATA_ENDPOINT_TYPE_HSIC      = 0x01,

Regards,
-Denis

Re: [PATCH 03/10] qmi: Added structure for device options.

[Denis Kenzior]

Hi Grant,

On 2/11/25 11:52 PM, Grant Erickson wrote:
> 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 913035897ea4..3f60e90c015a 100644
> --- a/drivers/qmimodem/qmi.h
> +++ b/drivers/qmimodem/qmi.h
> @@ -76,6 +76,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;

Why do you need both?  Isn't setting min_req_period_us non-zero enough?  Also, 
strictly speaking this may also apply to qrtr devices.  Maybe just add
bool qmi_qmux_device_set_request_throttle(device, us_time) ...?

> +};
> +
>   typedef void (*qmi_destroy_func_t)(void *user_data);
>   
>   struct qmi_service;

Regards,
-Denis

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

[Denis Kenzior]

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

Re: [PATCH 09/10] udevng: Set the QMI minimum service request period for Quectel BG96 modems.

[Denis Kenzior]

Hi Grant,

On 2/11/25 11:52 PM, Grant Erickson wrote:
> 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..e7cf494a70ff 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 (g_strcmp0(modem->vendor, "2c7c") == 0) {

Please use ell for all new code.  l_streq0 or so.

> +		if (g_strcmp0(modem->model, "0296") == 0) {
> +			ofono_modem_set_integer(modem->modem, "QMIMinReqPeriodUs", 2000);
> +		}
> +	}

The {} are unnecessary.

> +
>   	for (list = modem->devices; list; list = g_slist_next(list)) {
>   		const struct device_info *info = list->data;
>   		const char *subsystem =

Regards,
-Denis

Re: [PATCH 01/10] qmi: Added minimum service request period property string.

[Grant Erickson]

On Feb 12, 2025, at 11:15 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/11/25 11:52 PM, Grant Erickson wrote:
>> This may be used by ofono_modem_{g,s}et_integer to get or set the QMI
>> minimum service request period, in microseconds, modem property.
>> ---
>>  drivers/qmimodem/qmi.h | 2 ++
>>  1 file changed, 2 insertions(+)
>> diff --git a/drivers/qmimodem/qmi.h b/drivers/qmimodem/qmi.h
>> index 69698ee049c6..0c786ed065a3 100644
>> --- a/drivers/qmimodem/qmi.h
>> +++ b/drivers/qmimodem/qmi.h
>> @@ -43,6 +43,8 @@
>>  #define QMI_SERVICE_RMS 225 /* Remote management service */
>>  #define QMI_SERVICE_OMA 226 /* OMA device management service */
>>  +#define QMI_PROP_MIN_REQ_PERIOD_US "QMIMinReqPeriodUs"
>> +
> 
> This doesn't really belong here.  Properties that are communicated between udevng and gobi/qrtrqmi are documented above the probe() method implementation. Also, maybe 'RequestThrottleTimeUs'?

Noted, will change this in a v2 patch set.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

Re: [PATCH 03/10] qmi: Added structure for device options.

[Grant Erickson]

On Feb 12, 2025, at 11:21 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/11/25 11:52 PM, Grant Erickson wrote:
>> 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 913035897ea4..3f60e90c015a 100644
>> --- a/drivers/qmimodem/qmi.h
>> +++ b/drivers/qmimodem/qmi.h
>> @@ -76,6 +76,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;
> 
> Why do you need both?  Isn't setting min_req_period_us non-zero enough?  Also, strictly speaking this may also apply to qrtr devices.  Maybe just add
> bool qmi_qmux_device_set_request_throttle(device, us_time) ...?

Denis,

I had originally started with a hard-coded 1,000 us value and the quirk flag just determined whether that value and functionality was used or not (in effect, making it nullable).

However, after realizing during further testing that the 1,000 us value didn’t work past 60 back-to-back tests and needed to be bumped to 2,000 us, I added the ability to parameterize the value at the driver level and figured that quirk flags were valuable enough a construct to stay as-is where future quirk flags might be added.

More abstractly, I like that the quick flag makes the value explicitly nullable. I tend to not otherwise like “magic” sentinel values (in this case, zero).

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

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

[Grant Erickson]

On Feb 12, 2025, at 11:29 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> 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.

Noted. I’ll coalesce these in a v2 patch set.

>> 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?

I’d originally had this in a pre-submit draft. That’s a good optimization; noted for v2.

>> + 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.

Noted. I’d debated a timeout / timer approach; however, it seemed much more complicated in terms of its impact on the send / dequeue path, so I went with this approach first to validate the concept.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/

Re: [PATCH 09/10] udevng: Set the QMI minimum service request period for Quectel BG96 modems.

[Grant Erickson]

On Feb 12, 2025, at 11:30 AM, Denis Kenzior <denkenz@gmail.com> wrote:
> On 2/11/25 11:52 PM, Grant Erickson wrote:
>> 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..e7cf494a70ff 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 (g_strcmp0(modem->vendor, "2c7c") == 0) {
> 
> Please use ell for all new code.  l_streq0 or so.
> 
>> + if (g_strcmp0(modem->model, "0296") == 0) {
>> + ofono_modem_set_integer(modem->modem, "QMIMinReqPeriodUs", 2000);
>> + }
>> + }
> 
> The {} are unnecessary.

Denis,

Noted; will include both in a v2.

Best,

Grant

-- 
Principal
Nuovations

gerickson@nuovations.com
https://www.nuovations.com/