| Message ID | 20171212235857.10432-3-bjorn.andersson@linaro.org |
|---|---|
| State | Accepted |
| Commit | c12fc4519f607f83b6874a5388bb4df0759f687c |
| Headers | show |
| Series | [1/5] rpmsg: smd: Perform handshake during open | expand |
On Tue, Dec 12, 2017 at 03:58:54PM -0800, Bjorn Andersson wrote: > Rather than selectively creating devices only for the channels that the > remote have moved to "opening" state let's create devices for all > channels found. The driver model will match drivers to the ones we care > about and attempt to open these. > > The one case where this fails is if the user loads a firmware that lacks > a particular channel of the previous firmware that was running, in which > case we would find the old channel and attempt to probe it. The channel > opening handshake will ensure this will result in a graceful failure. Another case is that on the 8992/8994 there is no call to create_device due to the RPM always leaving the RX channel in the CLOSING state. I believe this may be happening in the LK based on this message: "[270] Waiting for the RPM to populate smd channel table " The exact same behaviour has been observed on the downstream kernel (initial state needing to be ignored in order). Stated another way, downstream _BLINDLY_ calls platform_device_register in smd_alloc_channel regardless of the value returned by ch->half_ch->get_state(ch->recv)); In the afore mentioned case _ALL_ of the images (RPM, bootloader: BZ11h, LK) used on the Nexus 5X/6P under test were unmodified as provided by MSM and/or Google. > > The result of this patch is that we will actively open the RPM channel > even though it's left in a state other than "opening" after the boot > loader's closing of the channel. > Its been a while since I looked at this closely but, isn't the result of this patch that we now will create a channel / register a platform device if the state of the channel is left in any state. > Reported-by: Jeremy McNicoll <jmcnicol@redhat.com> .....and Suggested-by: Jeremy McNicoll <jeremymc@redhat.com> > Reported-by: Will Newton <will.newton@gmail.com> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/rpmsg/qcom_smd.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c > index 58dd44493420..1beddea6f087 100644 > --- a/drivers/rpmsg/qcom_smd.c > +++ b/drivers/rpmsg/qcom_smd.c > @@ -1225,11 +1225,6 @@ static void qcom_channel_state_worker(struct work_struct *work) > if (channel->state != SMD_CHANNEL_CLOSED) > continue; > > - remote_state = GET_RX_CHANNEL_INFO(channel, state); > - if (remote_state != SMD_CHANNEL_OPENING && > - remote_state != SMD_CHANNEL_OPENED) > - continue; > - The code looks good, and the change is _VERY_ familiar. -jeremy > if (channel->registered) > continue; > > -- > 2.15.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 26 Jan 17:01 PST 2018, Jeremy McNicoll wrote: > On Tue, Dec 12, 2017 at 03:58:54PM -0800, Bjorn Andersson wrote: [..] > > > > The result of this patch is that we will actively open the RPM channel > > even though it's left in a state other than "opening" after the boot > > loader's closing of the channel. > > > > Its been a while since I looked at this closely but, isn't the result > of this patch that we now will create a channel / register a platform > device if the state of the channel is left in any state. > Correct, we will create devices for all channels found (on the specific edge), rather than just the ones that are in opening state. Looking through a few platforms does however indicate that the two cases where channels appear but are not in this state are: 1) The rpm_request channel when LK has been communicating with the RPM before jumping to the kernel. 2) You stop a remote processor, switch firmware to one with less features and the start it again. Any channels not used by the new firmware will still be found and we will fail to open them - as described above. I would be surprised if this would happen in reality. So with the added handshake mechanism we're supporting #1 and we deal with #2 - if it ever would happen. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c index 58dd44493420..1beddea6f087 100644 --- a/drivers/rpmsg/qcom_smd.c +++ b/drivers/rpmsg/qcom_smd.c @@ -1225,11 +1225,6 @@ static void qcom_channel_state_worker(struct work_struct *work) if (channel->state != SMD_CHANNEL_CLOSED) continue; - remote_state = GET_RX_CHANNEL_INFO(channel, state); - if (remote_state != SMD_CHANNEL_OPENING && - remote_state != SMD_CHANNEL_OPENED) - continue; - if (channel->registered) continue;
Rather than selectively creating devices only for the channels that the remote have moved to "opening" state let's create devices for all channels found. The driver model will match drivers to the ones we care about and attempt to open these. The one case where this fails is if the user loads a firmware that lacks a particular channel of the previous firmware that was running, in which case we would find the old channel and attempt to probe it. The channel opening handshake will ensure this will result in a graceful failure. The result of this patch is that we will actively open the RPM channel even though it's left in a state other than "opening" after the boot loader's closing of the channel. Reported-by: Jeremy McNicoll <jmcnicol@redhat.com> Reported-by: Will Newton <will.newton@gmail.com> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- drivers/rpmsg/qcom_smd.c | 5 ----- 1 file changed, 5 deletions(-) -- 2.15.0 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html