[PATCH 0/3] soundwire: cadence: Fix oversized FIFO size define
As determined by experimentation and asking a hardware person, the FIFO in the Cadence IP is actually only 8 entries long, not 32. This is fixed in patch #1.
As a bonus, patches #2 and #3 fix two other things I noticed while debugging this.
Richard Fitzgerald (3): soundwire: cadence: Don't overflow the command FIFOs soundwire: cadence: Remove wasted space in response_buf soundwire: cadence: Drain the RX FIFO after an IO timeout
drivers/soundwire/cadence_master.c | 45 +++++++++++++++++++----------- drivers/soundwire/cadence_master.h | 2 +- 2 files changed, 29 insertions(+), 18 deletions(-)
The command FIFOs are 8 entries long, so change CDNS_MCP_CMD_LEN to 8.
CDNS_MCP_CMD_LEN was originally 32, which would lead to cdns_xfer_msg() writing up to 32 commands into the FIFO, so any message longer than 8 commands would fail.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 2f52a5177caa ("soundwire: cdns: Add cadence library") --- drivers/soundwire/cadence_master.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a1de363eba3f..27699f341f2c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -127,7 +127,8 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask");
#define CDNS_MCP_CMD_BASE 0x80 #define CDNS_MCP_RESP_BASE 0x80 -#define CDNS_MCP_CMD_LEN 0x20 +/* FIFO can hold 8 commands */ +#define CDNS_MCP_CMD_LEN 8 #define CDNS_MCP_CMD_WORD_LEN 0x4
#define CDNS_MCP_CMD_SSP_TAG BIT(31)
On 12/1/22 07:48, Richard Fitzgerald wrote:
The command FIFOs are 8 entries long, so change CDNS_MCP_CMD_LEN to 8.
CDNS_MCP_CMD_LEN was originally 32, which would lead to cdns_xfer_msg() writing up to 32 commands into the FIFO, so any message longer than 8 commands would fail.
The change is correct for all instances of SoundWire on Intel platforms. That said, maybe we should capture that the Cadence IP can handle 4/8/16/32 entries - this is a hardware configuration option.
We should also mention that so far we have not sent multiple commands so far so the code is only broken when grouping commands.
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com Fixes: 2f52a5177caa ("soundwire: cdns: Add cadence library")
drivers/soundwire/cadence_master.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index a1de363eba3f..27699f341f2c 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -127,7 +127,8 @@ MODULE_PARM_DESC(cdns_mcp_int_mask, "Cadence MCP IntMask");
#define CDNS_MCP_CMD_BASE 0x80 #define CDNS_MCP_RESP_BASE 0x80 -#define CDNS_MCP_CMD_LEN 0x20 +/* FIFO can hold 8 commands */ +#define CDNS_MCP_CMD_LEN 8 #define CDNS_MCP_CMD_WORD_LEN 0x4
#define CDNS_MCP_CMD_SSP_TAG BIT(31)
The response_buf was declared much larger (128 entries) than the number of responses that could ever be written into it (maximum 8).
Reduce response_buf to 8 entries and add checking in cdns_read_response() to prevent overflowing reponse_buf if CDNS_MCP_RX_FIFO_AVAIL contains an unexpectedly large number.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/cadence_master.c | 6 ++++++ drivers/soundwire/cadence_master.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 27699f341f2c..95c84d9f0775 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -774,8 +774,14 @@ static void cdns_read_response(struct sdw_cdns *cdns) u32 num_resp, cmd_base; int i;
+ BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN); + num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); num_resp &= CDNS_MCP_RX_FIFO_AVAIL; + if (num_resp > ARRAY_SIZE(cdns->response_buf)) { + dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); + num_resp = CDNS_MCP_CMD_LEN; + }
cmd_base = CDNS_MCP_CMD_BASE;
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 0434d70d4b1f..c2d817e8e22a 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -117,7 +117,7 @@ struct sdw_cdns { struct sdw_bus bus; unsigned int instance;
- u32 response_buf[0x80]; + u32 response_buf[8]; struct completion tx_complete; struct sdw_defer *defer;
On 12/1/22 07:48, Richard Fitzgerald wrote:
The response_buf was declared much larger (128 entries) than the number of responses that could ever be written into it (maximum 8).
Indeed I don't know why we used 128 entries. This is a magic value that doesn't appear in any specs I've looked at.
Note that there's 'sniffer' mode when each response takes two consecutive 32-words in the FIFO. we've never used this mode though so it's not really an issue.
It's also possible that this is related to the automatic command retry, where a failed command can be re-issued 15 times. However in that case the worst case would be 32 commands * 15 = 480. The value of 128 makes no sense at all, unless it was an upper bound for 8 * 15. We don't use this hardware retry btw.
See more below...
Reduce response_buf to 8 entries and add checking in cdns_read_response() to prevent overflowing reponse_buf if CDNS_MCP_RX_FIFO_AVAIL contains an unexpectedly large number.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/cadence_master.c | 6 ++++++ drivers/soundwire/cadence_master.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 27699f341f2c..95c84d9f0775 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -774,8 +774,14 @@ static void cdns_read_response(struct sdw_cdns *cdns) u32 num_resp, cmd_base; int i;
- BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN);
- num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); num_resp &= CDNS_MCP_RX_FIFO_AVAIL;
- if (num_resp > ARRAY_SIZE(cdns->response_buf)) {
dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp);num_resp = CDNS_MCP_CMD_LEN;
.... this is different from what the hardware documentation tells me. The range of values to RX_FIFO_AVAIL is 0..RX_FIFO_DEPTH + 2.
I don't understand the +2, but we should maybe be more cautious and use u32 response_buf[CDNS_MCP_CMD_LEN + 2];
}
cmd_base = CDNS_MCP_CMD_BASE;
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 0434d70d4b1f..c2d817e8e22a 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -117,7 +117,7 @@ struct sdw_cdns { struct sdw_bus bus; unsigned int instance;
- u32 response_buf[0x80];
- u32 response_buf[8]; struct completion tx_complete; struct sdw_defer *defer;
On 01/12/2022 18:12, Pierre-Louis Bossart wrote:
On 12/1/22 07:48, Richard Fitzgerald wrote:
The response_buf was declared much larger (128 entries) than the number of responses that could ever be written into it (maximum 8).
Indeed I don't know why we used 128 entries. This is a magic value that doesn't appear in any specs I've looked at.
Note that there's 'sniffer' mode when each response takes two consecutive 32-words in the FIFO. we've never used this mode though so it's not really an issue.
It's also possible that this is related to the automatic command retry, where a failed command can be re-issued 15 times. However in that case the worst case would be 32 commands * 15 = 480. The value of 128 makes no sense at all, unless it was an upper bound for 8 * 15. We don't use this hardware retry btw.
See more below...
Reduce response_buf to 8 entries and add checking in cdns_read_response() to prevent overflowing reponse_buf if CDNS_MCP_RX_FIFO_AVAIL contains an unexpectedly large number.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/cadence_master.c | 6 ++++++ drivers/soundwire/cadence_master.h | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 27699f341f2c..95c84d9f0775 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -774,8 +774,14 @@ static void cdns_read_response(struct sdw_cdns *cdns) u32 num_resp, cmd_base; int i;
- BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN);
- num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); num_resp &= CDNS_MCP_RX_FIFO_AVAIL;
- if (num_resp > ARRAY_SIZE(cdns->response_buf)) {
dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp);num_resp = CDNS_MCP_CMD_LEN;.... this is different from what the hardware documentation tells me. The range of values to RX_FIFO_AVAIL is 0..RX_FIFO_DEPTH + 2.
I don't understand the +2, but we should maybe be more cautious and use u32 response_buf[CDNS_MCP_CMD_LEN + 2];
Probably we wouldn't know what to do with those extra 2 responses. _cdns_xfer_msg() tells cdns_fill_msg_resp() to extract the same number of responses as the original message length. So it would only extract 8 maximum. But anyway yes I can add the mysterious +2 into this. Thinking about it, that's probably a good idea anyay for the next patch that drains the RX FIFO after an error.
}
cmd_base = CDNS_MCP_CMD_BASE;
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h index 0434d70d4b1f..c2d817e8e22a 100644 --- a/drivers/soundwire/cadence_master.h +++ b/drivers/soundwire/cadence_master.h @@ -117,7 +117,7 @@ struct sdw_cdns { struct sdw_bus bus; unsigned int instance;
- u32 response_buf[0x80];
- u32 response_buf[8]; struct completion tx_complete; struct sdw_defer *defer;
If wait_for_completion_timeout() times-out in _cdns_xfer_msg() it is possible that something could have been written to the RX FIFO. In this case, we should drain the RX FIFO so that anything in it doesn't carry over and mess up the next transfer.
Obviously, if we got to this state something went wrong, and we don't really know the state of everything. The cleanup in this situation cannot be bullet-proof but we should attempt to avoid breaking future transaction, if only to reduce the amount of error noise when debugging the failure from a kernel log.
Note that this patch only implements the draining for blocking (non-deferred) transfers. The deferred API doesn't have any proper handling of error conditions and would need some re-design before implementing cleanup. That is a task for a separate patch...
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/soundwire/cadence_master.c | 48 ++++++++++++++++-------------- 1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 95c84d9f0775..6bffecf3d61a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -555,6 +555,28 @@ cdns_fill_msg_resp(struct sdw_cdns *cdns, return SDW_CMD_OK; }
+static void cdns_read_response(struct sdw_cdns *cdns) +{ + u32 num_resp, cmd_base; + int i; + + BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN); + + num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); + num_resp &= CDNS_MCP_RX_FIFO_AVAIL; + if (num_resp > ARRAY_SIZE(cdns->response_buf)) { + dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); + num_resp = CDNS_MCP_CMD_LEN; + } + + cmd_base = CDNS_MCP_CMD_BASE; + + for (i = 0; i < num_resp; i++) { + cdns->response_buf[i] = cdns_readl(cdns, cmd_base); + cmd_base += CDNS_MCP_CMD_WORD_LEN; + } +} + static enum sdw_command_response _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, int offset, int count, bool defer) @@ -596,6 +618,10 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, dev_err(cdns->dev, "IO transfer timed out, cmd %d device %d addr %x len %d\n", cmd, msg->dev_num, msg->addr, msg->len); msg->len = 0; + + /* Drain anything in the RX_FIFO */ + cdns_read_response(cdns); + return SDW_CMD_TIMEOUT; }
@@ -769,28 +795,6 @@ EXPORT_SYMBOL(cdns_read_ping_status); * IRQ handling */
-static void cdns_read_response(struct sdw_cdns *cdns) -{ - u32 num_resp, cmd_base; - int i; - - BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN); - - num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT); - num_resp &= CDNS_MCP_RX_FIFO_AVAIL; - if (num_resp > ARRAY_SIZE(cdns->response_buf)) { - dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp); - num_resp = CDNS_MCP_CMD_LEN; - } - - cmd_base = CDNS_MCP_CMD_BASE; - - for (i = 0; i < num_resp; i++) { - cdns->response_buf[i] = cdns_readl(cdns, cmd_base); - cmd_base += CDNS_MCP_CMD_WORD_LEN; - } -} - static int cdns_update_slave_status(struct sdw_cdns *cdns, u64 slave_intstat) {
On 12/1/22 07:48, Richard Fitzgerald wrote:
If wait_for_completion_timeout() times-out in _cdns_xfer_msg() it is possible that something could have been written to the RX FIFO. In this case, we should drain the RX FIFO so that anything in it doesn't carry over and mess up the next transfer.
Obviously, if we got to this state something went wrong, and we don't really know the state of everything. The cleanup in this situation cannot be bullet-proof but we should attempt to avoid breaking future transaction, if only to reduce the amount of error noise when debugging the failure from a kernel log.
Note that this patch only implements the draining for blocking (non-deferred) transfers. The deferred API doesn't have any proper handling of error conditions and would need some re-design before implementing cleanup. That is a task for a separate patch...
It's nearly impossible to deal with error conditions with deferred transfers, specifically in the case where deferred transfers deal with bank switches to synchronize changes across multiple links. The NAK is visible only in the scope of a link, and it could happen that a bank switch happens on one link and not the other. We don't have any means to recover at this point.
That said, draining the FIFO on timeouts for regular commands is a good idea - or it cannot hurt, so
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com
drivers/soundwire/cadence_master.c | 48 ++++++++++++++++-------------- 1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c index 95c84d9f0775..6bffecf3d61a 100644 --- a/drivers/soundwire/cadence_master.c +++ b/drivers/soundwire/cadence_master.c @@ -555,6 +555,28 @@ cdns_fill_msg_resp(struct sdw_cdns *cdns, return SDW_CMD_OK; }
+static void cdns_read_response(struct sdw_cdns *cdns) +{
- u32 num_resp, cmd_base;
- int i;
- BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN);
- num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT);
- num_resp &= CDNS_MCP_RX_FIFO_AVAIL;
- if (num_resp > ARRAY_SIZE(cdns->response_buf)) {
dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp);num_resp = CDNS_MCP_CMD_LEN;- }
- cmd_base = CDNS_MCP_CMD_BASE;
- for (i = 0; i < num_resp; i++) {
cdns->response_buf[i] = cdns_readl(cdns, cmd_base);cmd_base += CDNS_MCP_CMD_WORD_LEN;- }
+}
static enum sdw_command_response _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, int offset, int count, bool defer) @@ -596,6 +618,10 @@ _cdns_xfer_msg(struct sdw_cdns *cdns, struct sdw_msg *msg, int cmd, dev_err(cdns->dev, "IO transfer timed out, cmd %d device %d addr %x len %d\n", cmd, msg->dev_num, msg->addr, msg->len); msg->len = 0;
/* Drain anything in the RX_FIFO */cdns_read_response(cdns);- return SDW_CMD_TIMEOUT; }
@@ -769,28 +795,6 @@ EXPORT_SYMBOL(cdns_read_ping_status);
- IRQ handling
*/
-static void cdns_read_response(struct sdw_cdns *cdns) -{
- u32 num_resp, cmd_base;
- int i;
- BUILD_BUG_ON(ARRAY_SIZE(cdns->response_buf) < CDNS_MCP_CMD_LEN);
- num_resp = cdns_readl(cdns, CDNS_MCP_FIFOSTAT);
- num_resp &= CDNS_MCP_RX_FIFO_AVAIL;
- if (num_resp > ARRAY_SIZE(cdns->response_buf)) {
dev_warn(cdns->dev, "RX AVAIL %d too long\n", num_resp);num_resp = CDNS_MCP_CMD_LEN;- }
- cmd_base = CDNS_MCP_CMD_BASE;
- for (i = 0; i < num_resp; i++) {
cdns->response_buf[i] = cdns_readl(cdns, cmd_base);cmd_base += CDNS_MCP_CMD_WORD_LEN;- }
-}
static int cdns_update_slave_status(struct sdw_cdns *cdns, u64 slave_intstat) {
participants (2)
-
Pierre-Louis Bossart -
Richard Fitzgerald