[PATCH 0/5] Qualcomm SMD updates

[PATCH 0/5] Qualcomm SMD updates

[Bjorn Andersson]

A series of fixes for a few issues reported or seen with SMD.

The first two fixes changes the trigger for creating devices for channels
found, which should cause the rpm-smd driver to probe on 8909 and 8994 devices.

The third patch fixes an issue when multiple callers end up waiting on events.

The forth fix moves the check for a closed channel out of the tx-full loop, to
fail non-full send requests on a closed channel as well.

The last fix improves the handling of concurrent send-requests, allowing
concurrent sends, in particular try_send, to progress even though we're
sleeping waiting for room in the tx fifo.

Bjorn Andersson (5):
  rpmsg: smd: Perform handshake during open
  rpmsg: smd: Create device for all channels
  rpmsg: smd: Wake up all waiters
  rpmsg: smd: Fail send on a closed channel
  rpmsg: smd: Don't hold the tx lock during wait

 drivers/rpmsg/qcom_smd.c | 60 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 47 insertions(+), 13 deletions(-)

-- 
2.15.0

[PATCH 1/5] rpmsg: smd: Perform handshake during open

[Bjorn Andersson]

Validate the the remote side is opening the channel that we've found by
performing a handshake when opening the channel.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index b01774e9fac0..58dd44493420 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -200,6 +200,7 @@ struct qcom_smd_channel {
 	char *name;
 	enum smd_channel_state state;
 	enum smd_channel_state remote_state;
+	wait_queue_head_t state_change_event;
 
 	struct smd_channel_info_pair *info;
 	struct smd_channel_info_word_pair *info_word;
@@ -570,6 +571,8 @@ static bool qcom_smd_channel_intr(struct qcom_smd_channel *channel)
 	if (remote_state != channel->remote_state) {
 		channel->remote_state = remote_state;
 		need_state_scan = true;
+
+		wake_up_interruptible_all(&channel->state_change_event);
 	}
 	/* Indicate that we have seen any state change */
 	SET_RX_CHANNEL_FLAG(channel, fSTATE, 0);
@@ -786,7 +789,9 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data,
 static int qcom_smd_channel_open(struct qcom_smd_channel *channel,
 				 rpmsg_rx_cb_t cb)
 {
+	struct qcom_smd_edge *edge = channel->edge;
 	size_t bb_size;
+	int ret;
 
 	/*
 	 * Packets are maximum 4k, but reduce if the fifo is smaller
@@ -798,9 +803,33 @@ static int qcom_smd_channel_open(struct qcom_smd_channel *channel,
 
 	qcom_smd_channel_set_callback(channel, cb);
 	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENING);
+
+	/* Wait for remote to enter opening or opened */
+	ret = wait_event_interruptible_timeout(channel->state_change_event,
+			channel->remote_state == SMD_CHANNEL_OPENING ||
+			channel->remote_state == SMD_CHANNEL_OPENED,
+			HZ);
+	if (!ret) {
+		dev_err(&edge->dev, "remote side did not enter opening state\n");
+		goto out_close_timeout;
+	}
+
 	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENED);
 
+	/* Wait for remote to enter opened */
+	ret = wait_event_interruptible_timeout(channel->state_change_event,
+			channel->remote_state == SMD_CHANNEL_OPENED,
+			HZ);
+	if (!ret) {
+		dev_err(&edge->dev, "remote side did not enter open state\n");
+		goto out_close_timeout;
+	}
+
 	return 0;
+
+out_close_timeout:
+	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSED);
+	return -ETIMEDOUT;
 }
 
 /*
@@ -1055,6 +1084,7 @@ static struct qcom_smd_channel *qcom_smd_create_channel(struct qcom_smd_edge *ed
 	mutex_init(&channel->tx_lock);
 	spin_lock_init(&channel->recv_lock);
 	init_waitqueue_head(&channel->fblockread_event);
+	init_waitqueue_head(&channel->state_change_event);
 
 	info = qcom_smem_get(edge->remote_pid, smem_info_item, &info_size);
 	if (IS_ERR(info)) {
-- 
2.15.0

[PATCH 2/5] rpmsg: smd: Create device for all channels

[Bjorn Andersson]

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(-)

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;
 
-- 
2.15.0

[PATCH 3/5] rpmsg: smd: Wake up all waiters

[Bjorn Andersson]

It's possible to have multiple contexts waiting for new channel events
and with an upcoming change it's possible to have multiple contexts
waiting for a full FIFO. As such we need to wake them all up.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 1beddea6f087..0993e95bf0f5 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -579,7 +579,7 @@ static bool qcom_smd_channel_intr(struct qcom_smd_channel *channel)
 
 	/* Signal waiting qcom_smd_send() about the interrupt */
 	if (!GET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR))
-		wake_up_interruptible(&channel->fblockread_event);
+		wake_up_interruptible_all(&channel->fblockread_event);
 
 	/* Don't consume any data until we've opened the channel */
 	if (channel->state != SMD_CHANNEL_OPENED)
@@ -1191,7 +1191,7 @@ static void qcom_channel_scan_worker(struct work_struct *work)
 			dev_dbg(&edge->dev, "new channel found: '%s'\n", channel->name);
 			set_bit(i, edge->allocated[tbl]);
 
-			wake_up_interruptible(&edge->new_channel_event);
+			wake_up_interruptible_all(&edge->new_channel_event);
 		}
 	}
 
-- 
2.15.0

[PATCH 4/5] rpmsg: smd: Fail send on a closed channel

[Bjorn Andersson]

Move the check for a closed channel out from the tx-full loop to fail
any send request on a non-open channel.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 0993e95bf0f5..ed167ab52a68 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -743,17 +743,13 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data,
 	if (ret)
 		return ret;
 
-	while (qcom_smd_get_tx_avail(channel) < tlen) {
+	while (qcom_smd_get_tx_avail(channel) < tlen &&
+	       channel->state == SMD_CHANNEL_OPENED) {
 		if (!wait) {
 			ret = -EAGAIN;
 			goto out;
 		}
 
-		if (channel->state != SMD_CHANNEL_OPENED) {
-			ret = -EPIPE;
-			goto out;
-		}
-
 		SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 0);
 
 		ret = wait_event_interruptible(channel->fblockread_event,
@@ -765,6 +761,12 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data,
 		SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 1);
 	}
 
+	/* Fail if the channel was closed */
+	if (channel->state != SMD_CHANNEL_OPENED) {
+		ret = -EPIPE;
+		goto out;
+	}
+
 	SET_TX_CHANNEL_FLAG(channel, fTAIL, 0);
 
 	qcom_smd_write_fifo(channel, hdr, sizeof(hdr));
-- 
2.15.0

[PATCH 5/5] rpmsg: smd: Don't hold the tx lock during wait

[Bjorn Andersson]

Holding the tx lock while waiting for tx-drain events from the remote
side blocks try_send requests from failing quickly, so temporarily drop
the tx lock while waiting.

While this allows try_send to fail quickly it also could allow a
subsequent send to succeed putting a smaller packet in the FIFO while
we're waiting for room for our large packet. But as this lock is per
channel we expect that clients with ordering concerns implements their
own ordering mechanism.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/rpmsg/qcom_smd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index ed167ab52a68..10870189c0c8 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -752,12 +752,19 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data,
 
 		SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 0);
 
+		/* Wait without holding the tx_lock */
+		mutex_unlock(&channel->tx_lock);
+
 		ret = wait_event_interruptible(channel->fblockread_event,
 				       qcom_smd_get_tx_avail(channel) >= tlen ||
 				       channel->state != SMD_CHANNEL_OPENED);
 		if (ret)
 			goto out;
 
+		ret = mutex_lock_interruptible(&channel->tx_lock);
+		if (ret)
+			goto out;
+
 		SET_TX_CHANNEL_FLAG(channel, fBLOCKREADINTR, 1);
 	}
 
-- 
2.15.0

Re: [PATCH 2/5] rpmsg: smd: Create device for all channels

[Jeremy McNicoll]

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
> 

Re: [PATCH 1/5] rpmsg: smd: Perform handshake during open

[Jeremy McNicoll]

On Tue, Dec 12, 2017 at 03:58:53PM -0800, Bjorn Andersson wrote:
> Validate the the remote side is opening the channel that we've found by
> performing a handshake when opening the channel.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/rpmsg/qcom_smd.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> index b01774e9fac0..58dd44493420 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -200,6 +200,7 @@ struct qcom_smd_channel {
>  	char *name;
>  	enum smd_channel_state state;
>  	enum smd_channel_state remote_state;
> +	wait_queue_head_t state_change_event;
>  
>  	struct smd_channel_info_pair *info;
>  	struct smd_channel_info_word_pair *info_word;
> @@ -570,6 +571,8 @@ static bool qcom_smd_channel_intr(struct qcom_smd_channel *channel)
>  	if (remote_state != channel->remote_state) {
>  		channel->remote_state = remote_state;
>  		need_state_scan = true;
> +
> +		wake_up_interruptible_all(&channel->state_change_event);
>  	}
>  	/* Indicate that we have seen any state change */
>  	SET_RX_CHANNEL_FLAG(channel, fSTATE, 0);
> @@ -786,7 +789,9 @@ static int __qcom_smd_send(struct qcom_smd_channel *channel, const void *data,
>  static int qcom_smd_channel_open(struct qcom_smd_channel *channel,
>  				 rpmsg_rx_cb_t cb)
>  {
> +	struct qcom_smd_edge *edge = channel->edge;
>  	size_t bb_size;
> +	int ret;
>  
>  	/*
>  	 * Packets are maximum 4k, but reduce if the fifo is smaller
> @@ -798,9 +803,33 @@ static int qcom_smd_channel_open(struct qcom_smd_channel *channel,
>  
>  	qcom_smd_channel_set_callback(channel, cb);
>  	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENING);
> +
> +	/* Wait for remote to enter opening or opened */
> +	ret = wait_event_interruptible_timeout(channel->state_change_event,
> +			channel->remote_state == SMD_CHANNEL_OPENING ||
> +			channel->remote_state == SMD_CHANNEL_OPENED,
> +			HZ);
> +	if (!ret) {
> +		dev_err(&edge->dev, "remote side did not enter opening state\n");
> +		goto out_close_timeout;
> +	}
> +
>  	qcom_smd_channel_set_state(channel, SMD_CHANNEL_OPENED);
>  
> +	/* Wait for remote to enter opened */
> +	ret = wait_event_interruptible_timeout(channel->state_change_event,
> +			channel->remote_state == SMD_CHANNEL_OPENED,
> +			HZ);
> +	if (!ret) {
> +		dev_err(&edge->dev, "remote side did not enter open state\n");
> +		goto out_close_timeout;
> +	}
> +
>  	return 0;
> +
> +out_close_timeout:
> +	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSED);

Why not do something like this, 

out_close_timeout:
	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSING);
	wait_for_a_little_bit();
	qcom_smd_channel_set_state(channel, SMD_CHANNEL_CLOSED);

Doing something like that may get the remote back into a better
state if say for example it was stuck, rather than jumping directly
to CLOSED.

The reason why I am suggesting this is that whilst investigating
the problem of the SMD channel not getting created correctly, due
to the initial state of the remote being in an unexpected state I did
observe that if the host transistions from CLOSING to CLOSED it
takes far less time for the host and remote to get back into
sync.  It could be that it was a side effect of some of the changes
that I made, although it is worth considering. 

We could run some experiments to see how long it takes for host
and remote to get back into sync when going directly to CLOSED vs.
CLOSING, CLOSED.

-jeremy

> +	return -ETIMEDOUT;
>  }
>  
>  /*
> @@ -1055,6 +1084,7 @@ static struct qcom_smd_channel *qcom_smd_create_channel(struct qcom_smd_edge *ed
>  	mutex_init(&channel->tx_lock);
>  	spin_lock_init(&channel->recv_lock);
>  	init_waitqueue_head(&channel->fblockread_event);
> +	init_waitqueue_head(&channel->state_change_event);
>  
>  	info = qcom_smem_get(edge->remote_pid, smem_info_item, &info_size);
>  	if (IS_ERR(info)) {
> -- 
> 2.15.0
> 

Re: [PATCH 0/5] Qualcomm SMD updates

[Jeremy McNicoll]

On Tue, Dec 12, 2017 at 03:58:52PM -0800, Bjorn Andersson wrote:
> A series of fixes for a few issues reported or seen with SMD.
> 
> The first two fixes changes the trigger for creating devices for channels
> found, which should cause the rpm-smd driver to probe on 8909 and 8994 devices.
> 

.... and 8992.

> The third patch fixes an issue when multiple callers end up waiting on events.
> 
> The forth fix moves the check for a closed channel out of the tx-full loop, to
> fail non-full send requests on a closed channel as well.
> 
> The last fix improves the handling of concurrent send-requests, allowing
> concurrent sends, in particular try_send, to progress even though we're
> sleeping waiting for room in the tx fifo.
> 
> Bjorn Andersson (5):
>   rpmsg: smd: Perform handshake during open
>   rpmsg: smd: Create device for all channels
>   rpmsg: smd: Wake up all waiters
>   rpmsg: smd: Fail send on a closed channel
>   rpmsg: smd: Don't hold the tx lock during wait
> 
>  drivers/rpmsg/qcom_smd.c | 60 +++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 47 insertions(+), 13 deletions(-)
>


Please add my tested by on this series as I have verified them on both
my Nexus 5X and 6P.  

-jeremy

Re: [PATCH 2/5] rpmsg: smd: Create device for all channels

[Bjorn Andersson]

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