| Message ID | 20161125005432.1205-2-martin.blumenstingl@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 25/11/16 00:54, Martin Blumenstingl wrote: > The original code was relying on the fact that the SCPI firmware > responds with the same number of bytes (or more, all extra data would be > ignored in that case) as requested. > However, we have some pre-v1.0 SCPI firmwares which are responding with > less data for some commands (sensor_value.hi_val did not exist in the > old implementation). This means that some data from the previous > command's RX buffer was leaked into the current command (as the RX > buffer is re-used for all commands on the same channel). Clearing the > RX buffer before (re-) using it ensures we get a consistent result, even > if the SCPI firmware returns less bytes than requested. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index 70e1323..8c183d8 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -259,6 +259,7 @@ struct scpi_chan { > struct mbox_chan *chan; > void __iomem *tx_payload; > void __iomem *rx_payload; > + resource_size_t max_payload_len; > struct list_head rx_pending; > struct list_head xfers_list; > struct scpi_xfer *xfers; > @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) > if (t->rx_buf) { > if (!(++ch->token)) > ++ch->token; > + > + /* clear the RX buffer as it is shared across all commands on > + * the same channel (to make sure we're not leaking data from > + * the previous response into the current command if the SCPI > + * firmware writes less data than requested). > + * This is especially important for pre-v1.0 SCPI firmwares > + * where some fields in the responses do not exist (while they > + * exist but are optional in newer versions). One example for > + * this problem is sensor_value.hi_val, which would contain > + * ("leak") the second 4 bytes of the RX buffer from the > + * previous command. > + */ > + memset_io(ch->rx_payload, 0, ch->max_payload_len); > + This looks like a overkill to me. I prefer your first approach over this, if it's only this command that's affected. I am still not sure why Neil Armstrong mentioned that it worked for him with 64-bit read.
On Fri, 2016-11-25 at 01:54 +0100, Martin Blumenstingl wrote: > The original code was relying on the fact that the SCPI firmware > responds with the same number of bytes (or more, all extra data would be > ignored in that case) as requested. > However, we have some pre-v1.0 SCPI firmwares which are responding with > less data for some commands (sensor_value.hi_val did not exist in the > old implementation). This means that some data from the previous > command's RX buffer was leaked into the current command (as the RX > buffer is re-used for all commands on the same channel). Clearing the > RX buffer before (re-) using it ensures we get a consistent result, even > if the SCPI firmware returns less bytes than requested. > > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index 70e1323..8c183d8 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -259,6 +259,7 @@ struct scpi_chan { > struct mbox_chan *chan; > void __iomem *tx_payload; > void __iomem *rx_payload; > + resource_size_t max_payload_len; Ah, here's max_payload_len, sorry, I reviewed the patches in the wrong order. And reflecting on things, having the runtime > struct list_head rx_pending; > struct list_head xfers_list; > struct scpi_xfer *xfers; > @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) > if (t->rx_buf) { > if (!(++ch->token)) > ++ch->token; > + > + /* clear the RX buffer as it is shared across all commands on > + * the same channel (to make sure we're not leaking data from > + * the previous response into the current command if the SCPI > + * firmware writes less data than requested). > + * This is especially important for pre-v1.0 SCPI firmwares > + * where some fields in the responses do not exist (while they > + * exist but are optional in newer versions). One example for > + * this problem is sensor_value.hi_val, which would contain > + * ("leak") the second 4 bytes of the RX buffer from the > + * previous command. > + */ > + memset_io(ch->rx_payload, 0, ch->max_payload_len); > + Isn't the payload size specified in the header? In which case the bug you describe is due to the implementation writing 4 bytes and setting the length to 8. Anyway, this seems almost like a quirk of a specific implementation and perhaps should be handled as such, rather that doing this for all commands on all boards using SCPI. > ADD_SCPI_TOKEN(t->cmd, ch->token); > spin_lock_irqsave(&ch->rx_lock, flags); > list_add_tail(&t->node, &ch->rx_pending); > @@ -921,7 +936,9 @@ static int scpi_probe(struct platform_device *pdev) > ret = -EADDRNOTAVAIL; > goto err; > } > - pchan->tx_payload = pchan->rx_payload + (size >> 1); > + > + pchan->max_payload_len = size / 2; > + pchan->tx_payload = pchan->rx_payload + pchan->max_payload_len; > > cl->dev = dev; > cl->rx_callback = scpi_handle_remote_msg;
On Wed, Dec 7, 2016 at 7:17 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 25/11/16 00:54, Martin Blumenstingl wrote: >> The original code was relying on the fact that the SCPI firmware >> responds with the same number of bytes (or more, all extra data would be >> ignored in that case) as requested. >> However, we have some pre-v1.0 SCPI firmwares which are responding with >> less data for some commands (sensor_value.hi_val did not exist in the >> old implementation). This means that some data from the previous >> command's RX buffer was leaked into the current command (as the RX >> buffer is re-used for all commands on the same channel). Clearing the >> RX buffer before (re-) using it ensures we get a consistent result, even >> if the SCPI firmware returns less bytes than requested. >> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> --- >> drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++- >> 1 file changed, 18 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c >> index 70e1323..8c183d8 100644 >> --- a/drivers/firmware/arm_scpi.c >> +++ b/drivers/firmware/arm_scpi.c >> @@ -259,6 +259,7 @@ struct scpi_chan { >> struct mbox_chan *chan; >> void __iomem *tx_payload; >> void __iomem *rx_payload; >> + resource_size_t max_payload_len; >> struct list_head rx_pending; >> struct list_head xfers_list; >> struct scpi_xfer *xfers; >> @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) >> if (t->rx_buf) { >> if (!(++ch->token)) >> ++ch->token; >> + >> + /* clear the RX buffer as it is shared across all commands on >> + * the same channel (to make sure we're not leaking data from >> + * the previous response into the current command if the SCPI >> + * firmware writes less data than requested). >> + * This is especially important for pre-v1.0 SCPI firmwares >> + * where some fields in the responses do not exist (while they >> + * exist but are optional in newer versions). One example for >> + * this problem is sensor_value.hi_val, which would contain >> + * ("leak") the second 4 bytes of the RX buffer from the >> + * previous command. >> + */ >> + memset_io(ch->rx_payload, 0, ch->max_payload_len); >> + > > This looks like a overkill to me. I prefer your first approach over > this, if it's only this command that's affected. I am still not sure > why Neil Armstrong mentioned that it worked for him with 64-bit read. OK, I'm fine with that. I also had a look at the patch you posted in the cover-letter (I did not have time to test it yet though, I'll give feedback tomorrow) - this looks better than my v1. I think the explanation why it worked for Neil is pretty simple: it depends on the SCPI command which was executed before the "read sensor" command: if that command returned [4 bytes with any value]0x00000000[any number of bytes] then he got a valid result
diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c index 70e1323..8c183d8 100644 --- a/drivers/firmware/arm_scpi.c +++ b/drivers/firmware/arm_scpi.c @@ -259,6 +259,7 @@ struct scpi_chan { struct mbox_chan *chan; void __iomem *tx_payload; void __iomem *rx_payload; + resource_size_t max_payload_len; struct list_head rx_pending; struct list_head xfers_list; struct scpi_xfer *xfers; @@ -470,6 +471,20 @@ static void scpi_tx_prepare(struct mbox_client *c, void *msg) if (t->rx_buf) { if (!(++ch->token)) ++ch->token; + + /* clear the RX buffer as it is shared across all commands on + * the same channel (to make sure we're not leaking data from + * the previous response into the current command if the SCPI + * firmware writes less data than requested). + * This is especially important for pre-v1.0 SCPI firmwares + * where some fields in the responses do not exist (while they + * exist but are optional in newer versions). One example for + * this problem is sensor_value.hi_val, which would contain + * ("leak") the second 4 bytes of the RX buffer from the + * previous command. + */ + memset_io(ch->rx_payload, 0, ch->max_payload_len); + ADD_SCPI_TOKEN(t->cmd, ch->token); spin_lock_irqsave(&ch->rx_lock, flags); list_add_tail(&t->node, &ch->rx_pending); @@ -921,7 +936,9 @@ static int scpi_probe(struct platform_device *pdev) ret = -EADDRNOTAVAIL; goto err; } - pchan->tx_payload = pchan->rx_payload + (size >> 1); + + pchan->max_payload_len = size / 2; + pchan->tx_payload = pchan->rx_payload + pchan->max_payload_len; cl->dev = dev; cl->rx_callback = scpi_handle_remote_msg;
The original code was relying on the fact that the SCPI firmware responds with the same number of bytes (or more, all extra data would be ignored in that case) as requested. However, we have some pre-v1.0 SCPI firmwares which are responding with less data for some commands (sensor_value.hi_val did not exist in the old implementation). This means that some data from the previous command's RX buffer was leaked into the current command (as the RX buffer is re-used for all commands on the same channel). Clearing the RX buffer before (re-) using it ensures we get a consistent result, even if the SCPI firmware returns less bytes than requested. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> --- drivers/firmware/arm_scpi.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)