| Message ID | 20230117142414.983946-1-caleb.connolly@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rpmsg: qcom: glink: support waking up on channel rx | expand |
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 >
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 >>
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; }
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(+)