Re: [PATCH v3] qmimodem: support wake on SMS receive

[Tom Nguyen <tomirq@earthlink.net>]

[-- Attachment #1: Type: text/plain, Size: 4191 bytes --]

Hi Denis,

>From: Tom Nguyen
>
>Hi Denis,
>
>>From: Denis Kenzior 
>>
>>Hi Tom,
>>
>>On 05/23/2019 01:15 PM, Tom Nguyen wrote:
>>> Some modems, like Quectel EC25, support waking the host platform from a suspend state upon an SMS reception. Current SMS handling configures the modem to not save incoming messages. On platforms where the modem's interfaces (eg, cdc-wdm, wwan, etc) disconnect on suspend and reconnect on resume, oFono will re-initialize. This can cause lost messages upon resume because the QMI indication is sent before oFono is ready.
>>
>>Note, the commit description should be broken up with no line longer 
>>than 72 characters.
>>
>>> 
>>> Changes to support suspend/resume with wake on SMS:
>>> - On startup:
>>>    1. Configure modem to save messages to NV and notify for class NONE.
>>>    2. Delete all processed messages in modem memory to free space.
>>>    3. Get list of unread messages, then get and delete each.
>>> - After startup:
>>>    1. Process message upon event indication and delete.
>>>    2. Then check for possibly more messages to process.
>>> ---
>>>   drivers/qmimodem/sms.c | 327 ++++++++++++++++++++++++++++++++++++++++---------
>>>   drivers/qmimodem/wms.h |  77 +++++++++---
>>>   2 files changed, 332 insertions(+), 72 deletions(-)
>>> 

<snip>

>>>   static void event_notify(struct qmi_result *result, void *user_data) @@ -367,66 +575,72 @@ static void event_notify(struct qmi_result *result, void *user_data)
>>>   	struct ofono_sms *sms = user_data;
>>>   	struct sms_data *data = ofono_sms_get_data(sms);
>>>   	const struct qmi_wms_result_new_msg_notify *notify;
>>> -	const struct qmi_wms_result_message *message;
>>> -	uint16_t len;
>>>   
>>>   	DBG("");
>>>   
>>> -	notify = qmi_result_get(result, QMI_WMS_RESULT_NEW_MSG_NOTIFY, &len);
>>> +	notify = qmi_result_get(result, QMI_WMS_RESULT_NEW_MSG_NOTIFY, NULL);
>>>   	if (notify) {
>>> -		DBG("storage type %d index %d", notify->storage_type,
>>> -				GUINT32_FROM_LE(notify->storage_index));
>>> -	}
>>> +		/* route is store and notify */
>>> +		if (!qmi_result_get_uint8(result, QMI_WMS_RESULT_MSG_MODE,
>>> +				&data->msg_mode))
>>> +			DBG("msg mode not found, use mode %d", data->msg_mode);
>>> +
>>> +		DBG("msg type %d ndx %d mode %d", notify->storage_type,
>>> +			GUINT32_FROM_LE(notify->storage_index), data->msg_mode);
>>> +
>>> +		/* don't read if list is being processed, will find this msg */
>>> +		if (!data->msg_list_chk)
>>> +			raw_read(sms, notify->storage_type,
>>> +					GUINT32_FROM_LE(notify->storage_index));
>>
>>So I'm not familiar with QMI enough to truly know what is happening. 
>>But it looks like you are changing the logic slightly in this function. 
>>Before it would attempt to grab the QMI_WMS_RESULT_NEW_MSG_NOTIFY 
>>structure only for debug purposes, and instead attempt to grab the 
>>QMI_WMS_RESULT_MESSAGE in all cases.  If the QMI_WMS_RESULT_MESSAGE was 
>>not present, then the raw-read stuff would be triggered.  You're doing 
>>it slightly differently here.  It may be worth a comment.
>>
>>Also, it may be clearer if the introduction of raw_read and the 
>>refactoring of this function are done as a separate commit.
>>
>
>Hmm, looking at the "Quectel EC21" comment again, looks like the original code was done to address a quirk (ie, bug???) with that modem. I'll revert to reading both.
>
>But, per spec, the 2 message TLVs are mutually exclusive, depending on the route action configuration. I'm referring to setting "new_list->route[0].action =" in get_routes_cb(). So, we shouldn't need to always read both since one is guaranteed to fail.
>
>Also, the original code, if somehow both "notify" and "message" are NULL, it'll erroneously use a null notify. I'll address this in the revert.
>

On 2nd thought, my change is still ok per that "Quectel EC21" comment. I'll keep my changes and add comment to explain the 2 TLV that are being queried. Also, I introduced raw_read because I need to pull QMI_WMS_RAW_READ out of event_notify to service both event_notify and the message list. So, I'd like to keep this in the same commit.

Thanks,
Tom