diff mbox

[v2,09/20] rpmsg: glink: Fix default case while handling received commands

Message ID 1503559302-3744-10-git-send-email-sricharan@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Sricharan Ramabadhran Aug. 24, 2017, 7:21 a.m. UTC
Currently if we receive a command that we still do not
support, then its simply discarded. While doing so, the
RX FIFO pointer also needs to be incremented. Fixing this.

Signed-off-by: Sricharan R <sricharan@codeaurora.org>
---
 drivers/rpmsg/qcom_glink_native.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Arun Kumar Neelakantam Aug. 28, 2017, 11:48 a.m. UTC | #1
On 8/24/2017 12:51 PM, Sricharan R wrote:
> Currently if we receive a command that we still do not
> support, then its simply discarded. While doing so, the
> RX FIFO pointer also needs to be incremented. Fixing this.
>
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
Acked-by: Arun Kumar Neelakantam <aneela@codeaurora.org>

Regards,
Arun N
> ---
>   drivers/rpmsg/qcom_glink_native.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 50a8008..9a58925 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -541,6 +541,7 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>   			ret = 0;
>   			break;
>   		default:
> +			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>   			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>   			ret = -EINVAL;
>   			break;
Bjorn Andersson Aug. 29, 2017, 8:57 p.m. UTC | #2
On Thu 24 Aug 00:21 PDT 2017, Sricharan R wrote:

> Currently if we receive a command that we still do not
> support, then its simply discarded. While doing so, the
> RX FIFO pointer also needs to be incremented. Fixing this.
> 

Messages are variable length, so when we hit a unrecognized message we
don't know how far to advance the rx tail, meaning that with this change
we're more likely to get out of sync than to actually find the next
message. Which like would make people debug side effects of this.

I believe a better way to handle this would be to acquire a reference to
the parent struct rproc and call rproc_report_crash() on this, if it
exist (i.e. it's not RPM).

Regards,
Bjorn

> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> ---
>  drivers/rpmsg/qcom_glink_native.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
> index 50a8008..9a58925 100644
> --- a/drivers/rpmsg/qcom_glink_native.c
> +++ b/drivers/rpmsg/qcom_glink_native.c
> @@ -541,6 +541,7 @@ static irqreturn_t qcom_glink_native_intr(int irq, void *data)
>  			ret = 0;
>  			break;
>  		default:
> +			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
>  			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
>  			ret = -EINVAL;
>  			break;
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
diff mbox

Patch

diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c
index 50a8008..9a58925 100644
--- a/drivers/rpmsg/qcom_glink_native.c
+++ b/drivers/rpmsg/qcom_glink_native.c
@@ -541,6 +541,7 @@  static irqreturn_t qcom_glink_native_intr(int irq, void *data)
 			ret = 0;
 			break;
 		default:
+			qcom_glink_rx_advance(glink, ALIGN(sizeof(msg), 8));
 			dev_err(glink->dev, "unhandled rx cmd: %d\n", cmd);
 			ret = -EINVAL;
 			break;