| Message ID | 20240925072328.1163183-1-quic_deesin@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [V2] rpmsg: glink: Add abort_tx check in intent wait | expand |
On Wed, Sep 25, 2024 at 12:53:28PM +0530, Deepak Kumar Singh wrote: > From: Sarannya S <quic_sarannya@quicinc.com> > > On remote susbsystem restart rproc will stop glink subdev which will "When stopping or restarting a remoteproc the glink subdev stop will invoke qcom_glink_native_remove(). Any ..." > trigger qcom_glink_native_remove, any ongoing intent wait should be Please always have () on function names, to make clear they are indeed functions. s/intent wait/wait for intent request/ And I think "can" is a better word than "should" (we can abort the wait to not waiting for the intents that aren't expected to come). > aborted from there otherwise this wait delays glink send which potentially > delays glink channel removal as well. This further introduces delay in ssr > notification to other remote subsystems from rproc. > > Currently qcom_glink_native_remove is not setting channel->intent_received, This observation is correct, but I don't see a reason why it should. So express this in terms of the applicable logic (i.e. we have abort_tx to signal this scenario already, let's use it) > so any ongoing intent wait is not aborted on remote susbsystem restart. > abort_tx flag can be used as a condition to abort in such cases. > > Adding abort_tx flag check in intent wait, to abort intent wait from > qcom_glink_native_remove. More () please. > > Fixes: c05dfce0b89e ("rpmsg: glink: Wait for intent, not just request ack") > Cc: stable@vger.kernel.org I don't think the current code is broken, just suboptimal. And as such I don't think this is a bugfix. Perhaps I'm missing some case here? Otherwise, please drop the Fixes and Cc... > Signed-off-by: Sarannya S <quic_sarannya@quicinc.com> > Signed-off-by: Deepak Kumar Singh <quic_deesin@quicinc.com> > --- > drivers/rpmsg/qcom_glink_native.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c > index 82d460ff4777..ff828531c36f 100644 > --- a/drivers/rpmsg/qcom_glink_native.c > +++ b/drivers/rpmsg/qcom_glink_native.c > @@ -438,7 +438,6 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink, > > static void qcom_glink_intent_req_abort(struct glink_channel *channel) > { > - WRITE_ONCE(channel->intent_req_result, 0); > wake_up_all(&channel->intent_req_wq); > } > > @@ -1354,8 +1353,9 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, > goto unlock; > > ret = wait_event_timeout(channel->intent_req_wq, > - READ_ONCE(channel->intent_req_result) >= 0 && > - READ_ONCE(channel->intent_received), > + (READ_ONCE(channel->intent_req_result) >= 0 && > + READ_ONCE(channel->intent_received)) || > + glink->abort_tx, This looks good. But Chris and I talked about his patches posted yesterday, and it seems like a good idea to differentiate the cases of aborted and granted = 0. Chris' patch is fixing a real bug, so that should be backported, so let's conclude that discussion (with this patch included or in mind). Regards, Bjorn > 10 * HZ); > if (!ret) { > dev_err(glink->dev, "intent request timed out\n"); > -- > 2.34.1 >
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 82d460ff4777..ff828531c36f 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -438,7 +438,6 @@ static void qcom_glink_handle_intent_req_ack(struct qcom_glink *glink, static void qcom_glink_intent_req_abort(struct glink_channel *channel) { - WRITE_ONCE(channel->intent_req_result, 0); wake_up_all(&channel->intent_req_wq); } @@ -1354,8 +1353,9 @@ static int qcom_glink_request_intent(struct qcom_glink *glink, goto unlock; ret = wait_event_timeout(channel->intent_req_wq, - READ_ONCE(channel->intent_req_result) >= 0 && - READ_ONCE(channel->intent_received), + (READ_ONCE(channel->intent_req_result) >= 0 && + READ_ONCE(channel->intent_received)) || + glink->abort_tx, 10 * HZ); if (!ret) { dev_err(glink->dev, "intent request timed out\n");