[PATCH 0/3] soundwire: cadence: Fix oversized FIFO size define

[PATCH 0/3] soundwire: cadence: Fix oversized FIFO size define

[Richard Fitzgerald]

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

-- 
2.30.2

[PATCH 1/3] soundwire: cadence: Don't overflow the command FIFOs

[Richard Fitzgerald]

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

Re: [PATCH 1/3] soundwire: cadence: Don't overflow the command FIFOs

[Pierre-Louis Bossart]

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)

[PATCH 2/3] soundwire: cadence: Remove wasted space in response_buf

[Richard Fitzgerald]

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

Re: [PATCH 2/3] soundwire: cadence: Remove wasted space in response_buf

[Pierre-Louis Bossart]

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

Re: [PATCH 2/3] soundwire: cadence: Remove wasted space in response_buf

[Richard Fitzgerald]

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

[PATCH 3/3] soundwire: cadence: Drain the RX FIFO after an IO timeout

[Richard Fitzgerald]

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)
 {
-- 
2.30.2

Re: [PATCH 3/3] soundwire: cadence: Drain the RX FIFO after an IO timeout

[Pierre-Louis Bossart]

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