[PATCH] rpmsg: qcom: glink: support waking up on channel rx

[PATCH] rpmsg: qcom: glink: support waking up on channel rx

[Caleb Connolly]

Configure all channels as wakeup capable and report a wakeup event
when data is received.

This allows userspace to "subscribe" to a particular channel where
it is useful to wake up to process new data. The expected usecase
is to allow for handling incoming SMS or phone calls where the only
notification mechanism is via QRTR on the IPCRTR glink channel.

As this behaviour is likely undesirable for most users, this patch
doesn't enable a wakeup_source for any channels by default.

Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
 drivers/rpmsg/qcom_glink_native.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 115c0a1eddb1..1a96a7ae23bb 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -914,6 +914,9 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
 		channel->buf = NULL;
 
 		qcom_glink_rx_done(glink, channel, intent);
+
+		pm_wakeup_ws_event(channel->ept.rpdev->dev.power.wakeup, 0,
+				   true);
 	}
 
 advance_rx:
@@ -1510,6 +1513,17 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 		if (ret)
 			goto rcid_remove;
 
+		/*
+		 * Declare all channels as wakeup capable, but don't enable
+		 * waking up by default.
+		 *
+		 * Userspace may wish to be woken up for incoming messages on a
+		 * specific channel, for example to handle incoming calls or SMS
+		 * messages on the IPCRTR channel. This can be done be enabling
+		 * the wakeup source via sysfs.
+		 */
+		device_set_wakeup_capable(&rpdev->dev, true);
+
 		channel->rpdev = rpdev;
 	}
 
-- 
2.39.0

Re: [PATCH] rpmsg: qcom: glink: support waking up on channel rx

[Bjorn Andersson]

On Tue, Jan 17, 2023 at 02:24:13PM +0000, Caleb Connolly wrote:
> Configure all channels as wakeup capable and report a wakeup event
> when data is received.
> 
> This allows userspace to "subscribe" to a particular channel where
> it is useful to wake up to process new data. The expected usecase
> is to allow for handling incoming SMS or phone calls where the only
> notification mechanism is via QRTR on the IPCRTR glink channel.
> 
> As this behaviour is likely undesirable for most users, this patch
> doesn't enable a wakeup_source for any channels by default.
> 

This looks reasonable to me.

One suggestion that came up as we discussed this problem (elsewhere) a
while ago was the idea of using EPOLLWAKEUP to control the wakeup source
on a per-socket basis.

Would that suite your userspace?

Regards,
Bjorn

> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 115c0a1eddb1..1a96a7ae23bb 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -914,6 +914,9 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
>  		channel->buf = NULL;
>  
>  		qcom_glink_rx_done(glink, channel, intent);
> +
> +		pm_wakeup_ws_event(channel->ept.rpdev->dev.power.wakeup, 0,
> +				   true);
>  	}
>  
>  advance_rx:
> @@ -1510,6 +1513,17 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>  		if (ret)
>  			goto rcid_remove;
>  
> +		/*
> +		 * Declare all channels as wakeup capable, but don't enable
> +		 * waking up by default.
> +		 *
> +		 * Userspace may wish to be woken up for incoming messages on a
> +		 * specific channel, for example to handle incoming calls or SMS
> +		 * messages on the IPCRTR channel. This can be done be enabling
> +		 * the wakeup source via sysfs.
> +		 */
> +		device_set_wakeup_capable(&rpdev->dev, true);
> +
>  		channel->rpdev = rpdev;
>  	}
>  
> -- 
> 2.39.0
> 

Re: [PATCH] rpmsg: qcom: glink: support waking up on channel rx

[Caleb Connolly]

On 18/01/2023 18:01, Bjorn Andersson wrote:
> On Tue, Jan 17, 2023 at 02:24:13PM +0000, Caleb Connolly wrote:
>> Configure all channels as wakeup capable and report a wakeup event
>> when data is received.
>>
>> This allows userspace to "subscribe" to a particular channel where
>> it is useful to wake up to process new data. The expected usecase
>> is to allow for handling incoming SMS or phone calls where the only
>> notification mechanism is via QRTR on the IPCRTR glink channel.
>>
>> As this behaviour is likely undesirable for most users, this patch
>> doesn't enable a wakeup_source for any channels by default.
>>
> 
> This looks reasonable to me.
> 
> One suggestion that came up as we discussed this problem (elsewhere) a
> while ago was the idea of using EPOLLWAKEUP to control the wakeup source
> on a per-socket basis.
> 
> Would that suite your userspace?

I haven't been able to get any feedback from ModemManager, however
personally I'm more inclined to go with a global switch for the sake of
simplicity. At least until we have some idea of how to tie this together
in userpsace. This is the approach taken by IPA right now too.

If I understand correctly, this wakeup is always enabled on downstream,
and the RIL stack is expected to configure the modem to not send
spurious messages while the AP is in suspend.

Given that on phones the only time we don't want to wake up for
calls/sms is when on aeroplane mode (or some other explicit user
decision) I'm not sure that tying it to the socket is the best option,
at least initially.

I expect there to be some teething problems on the userspace side, and
some folks are going to want to disable this feature until those are
resolved. Making it a sysfs control means that we aren't reliant on
ModemManager to implement support for it.

Lastly (apologies this is getting a little long...), the sensor DSP also
utilises this mechanism to be able to wake the device on significant
motion. Similarly decoupling the wakeup control from whatever daemon is
responsible for handling this feels simpler to me, just poke sysfs and
the device will stop waking up when you move it.

=== Related bug ===

We've been shipping this patch in postmarketOS for a few months now, and
I've come across a very rare null pointer splat where channel->ept.rpdev
is null inside qcom_glink_rx_data(). I'm unsure if this is a bug in my
patch or in glink and I wondered if you have any idea.
> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
>> ---
>>  drivers/rpmsg/qcom_glink_native.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
>> index 115c0a1eddb1..1a96a7ae23bb 100644
>> --- a/drivers/rpmsg/qcom_glink_native.c
>> +++ b/drivers/rpmsg/qcom_glink_native.c
>> @@ -914,6 +914,9 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail)
>>  		channel->buf = NULL;
>>  
>>  		qcom_glink_rx_done(glink, channel, intent);
>> +
>> +		pm_wakeup_ws_event(channel->ept.rpdev->dev.power.wakeup, 0,
>> +				   true);
>>  	}
>>  
>>  advance_rx:
>> @@ -1510,6 +1513,17 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
>>  		if (ret)
>>  			goto rcid_remove;
>>  
>> +		/*
>> +		 * Declare all channels as wakeup capable, but don't enable
>> +		 * waking up by default.
>> +		 *
>> +		 * Userspace may wish to be woken up for incoming messages on a
>> +		 * specific channel, for example to handle incoming calls or SMS
>> +		 * messages on the IPCRTR channel. This can be done be enabling
>> +		 * the wakeup source via sysfs.
>> +		 */
>> +		device_set_wakeup_capable(&rpdev->dev, true);
>> +
>>  		channel->rpdev = rpdev;
>>  	}
>>  
>> -- 
>> 2.39.0
>>

-- 
// Caleb (they/them)