[PATCH 0/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

[PATCH 0/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

[Bin Meng]

After command is sent and before card response shows up on the line,
there is a variable number of clock cycles in between called Ncr.
The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.

Current logic in mmc_spi_sendcmd() has a flaw that it could only work
with certain SD cards with their Ncr being just 1 byte.

When resp_match is false, the codes try to receive only 1 byte from
the SD card. On the other hand when resp_match is true, the logic
happens to be no problem as it loops until timeout to receive as many
bytes as possible to see a match of the expected resp_match_value.
However not every call to mmc_spi_sendcmd() is made with resp_match
being true hence this exposes a potential issue with SD cards that
have a larger Ncr value.

Given no issue was reported as of today, we can reasonably conclude
that all cards being used on the supported boards happen to have a 1
byte Ncr timing requirement. But a broken case can be triggered by
utilizing QEMU to emulate a larger value of Ncr (by default 1 byte
Ncr is used on QEMU). This series fixes such potential spec violation
to improve the card compatibility.

[1] "Physical Layer Specification Version 8.00"
     chapter 7.5.1: Command / Response
     chapter 7.5.4: Timing Values


Bin Meng (3):
  mmc: mmc_spi: Move argument check to the beginning of
    mmc_spi_sendcmd()
  mmc: mmc_spi: Fix potential spec violation in receiving card response
  mmc: mmc_spi: Document the 3 local functions

 drivers/mmc/mmc_spi.c | 74 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH 1/3] mmc: mmc_spi: Move argument check to the beginning of mmc_spi_sendcmd()

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The argument check should happen before any transfer on the SPI lines.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 drivers/mmc/mmc_spi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
index b1edb6a..85a2818 100644
--- a/drivers/mmc/mmc_spi.c
+++ b/drivers/mmc/mmc_spi.c
@@ -83,6 +83,9 @@ static int mmc_spi_sendcmd(struct udevice *dev,
 	      __func__, cmdidx, cmdarg, resp_type,
 	      resp_size, resp_match, resp_match_value);
 
+	if (!resp || !resp_size)
+		return 0;
+
 	cmdo[0] = 0xff;
 	cmdo[1] = MMC_SPI_CMD(cmdidx);
 	cmdo[2] = cmdarg >> 24;
@@ -98,9 +101,6 @@ static int mmc_spi_sendcmd(struct udevice *dev,
 	if (ret)
 		return ret;
 
-	if (!resp || !resp_size)
-		return 0;
-
 	debug("%s: cmd%d", __func__, cmdidx);
 
 	if (resp_match) {
-- 
2.7.4

[PATCH 1/3] mmc: mmc_spi: Move argument check to the beginning of mmc_spi_sendcmd()

[Jaehoon Chung]

On 2/1/21 1:20 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> The argument check should happen before any transfer on the SPI lines.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  drivers/mmc/mmc_spi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
> index b1edb6a..85a2818 100644
> --- a/drivers/mmc/mmc_spi.c
> +++ b/drivers/mmc/mmc_spi.c
> @@ -83,6 +83,9 @@ static int mmc_spi_sendcmd(struct udevice *dev,
>  	      __func__, cmdidx, cmdarg, resp_type,
>  	      resp_size, resp_match, resp_match_value);
>  
> +	if (!resp || !resp_size)
> +		return 0;

I'm not sure..does it needs to locate more above? resp_size is referred in debug message.
Other thing looks good to me. 

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> +
>  	cmdo[0] = 0xff;
>  	cmdo[1] = MMC_SPI_CMD(cmdidx);
>  	cmdo[2] = cmdarg >> 24;
> @@ -98,9 +101,6 @@ static int mmc_spi_sendcmd(struct udevice *dev,
>  	if (ret)
>  		return ret;
>  
> -	if (!resp || !resp_size)
> -		return 0;
> -
>  	debug("%s: cmd%d", __func__, cmdidx);
>  
>  	if (resp_match) {
> 

[PATCH 2/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

After command is sent and before card response shows up on the line,
there is a variable number of clock cycles in between called Ncr.
The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.

Current logic in mmc_spi_sendcmd() has a flaw that it could only work
with certain SD cards with their Ncr being just 1 byte.

When resp_match is false, the codes try to receive only 1 byte from
the SD card. On the other hand when resp_match is true, the logic
happens to be no problem as it loops until timeout to receive as many
bytes as possible to see a match of the expected resp_match_value.
However not every call to mmc_spi_sendcmd() is made with resp_match
being true hence this exposes a potential issue with SD cards that
have a larger Ncr value.

Given no issue was reported as of today, we can reasonably conclude
that all cards being used on the supported boards happen to have a 1
byte Ncr timing requirement. But a broken case can be triggered by
utilizing QEMU to emulate a larger value of Ncr (by default 1 byte
Ncr is used on QEMU). This commit fixes such potential spec violation
to improve the card compatibility.

[1] "Physical Layer Specification Version 8.00"
     chapter 7.5.1: Command / Response
     chapter 7.5.4: Timing Values

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 drivers/mmc/mmc_spi.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
index 85a2818..5ec6b0f 100644
--- a/drivers/mmc/mmc_spi.c
+++ b/drivers/mmc/mmc_spi.c
@@ -103,29 +103,31 @@ static int mmc_spi_sendcmd(struct udevice *dev,
 
 	debug("%s: cmd%d", __func__, cmdidx);
 
-	if (resp_match) {
+	if (resp_match)
 		r = ~resp_match_value;
-		i = CMD_TIMEOUT;
-		while (i) {
-			ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
-			if (ret)
-				return ret;
-			debug(" resp%d=0x%x", rpos, r);
-			rpos++;
-			i--;
+	i = CMD_TIMEOUT;
+	while (i) {
+		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
+		if (ret)
+			return ret;
+		debug(" resp%d=0x%x", rpos, r);
+		rpos++;
+		i--;
 
+		if (resp_match) {
 			if (r == resp_match_value)
 				break;
+		} else {
+			if (!(r & 0x80))
+				break;
 		}
-		if (!i && (r != resp_match_value))
+
+		if (!i)
 			return -ETIMEDOUT;
 	}
 
-	for (i = 0; i < resp_size; i++) {
-		if (i == 0 && resp_match) {
-			resp[i] = resp_match_value;
-			continue;
-		}
+	resp[0] = r;
+	for (i = 1; i < resp_size; i++) {
 		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
 		if (ret)
 			return ret;
-- 
2.7.4

[PATCH 2/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

[Jaehoon Chung]

Hi Bin,

On 2/1/21 1:20 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> After command is sent and before card response shows up on the line,
> there is a variable number of clock cycles in between called Ncr.
> The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.
> 
> Current logic in mmc_spi_sendcmd() has a flaw that it could only work
> with certain SD cards with their Ncr being just 1 byte.
> 
> When resp_match is false, the codes try to receive only 1 byte from
> the SD card. On the other hand when resp_match is true, the logic
> happens to be no problem as it loops until timeout to receive as many
> bytes as possible to see a match of the expected resp_match_value.
> However not every call to mmc_spi_sendcmd() is made with resp_match
> being true hence this exposes a potential issue with SD cards that
> have a larger Ncr value.
> 
> Given no issue was reported as of today, we can reasonably conclude
> that all cards being used on the supported boards happen to have a 1
> byte Ncr timing requirement. But a broken case can be triggered by
> utilizing QEMU to emulate a larger value of Ncr (by default 1 byte
> Ncr is used on QEMU). This commit fixes such potential spec violation
> to improve the card compatibility.
> 
> [1] "Physical Layer Specification Version 8.00"
>      chapter 7.5.1: Command / Response
>      chapter 7.5.4: Timing Values

After checking spec, i will reply for this patch. :)

Best Regards,
Jaehoon Chung

> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>  drivers/mmc/mmc_spi.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
> index 85a2818..5ec6b0f 100644
> --- a/drivers/mmc/mmc_spi.c
> +++ b/drivers/mmc/mmc_spi.c
> @@ -103,29 +103,31 @@ static int mmc_spi_sendcmd(struct udevice *dev,
>  
>  	debug("%s: cmd%d", __func__, cmdidx);
>  
> -	if (resp_match) {
> +	if (resp_match)
>  		r = ~resp_match_value;
> -		i = CMD_TIMEOUT;
> -		while (i) {
> -			ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
> -			if (ret)
> -				return ret;
> -			debug(" resp%d=0x%x", rpos, r);
> -			rpos++;
> -			i--;
> +	i = CMD_TIMEOUT;
> +	while (i) {
> +		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
> +		if (ret)
> +			return ret;
> +		debug(" resp%d=0x%x", rpos, r);
> +		rpos++;
> +		i--;
>  
> +		if (resp_match) {
>  			if (r == resp_match_value)
>  				break;
> +		} else {
> +			if (!(r & 0x80))
> +				break;
>  		}
> -		if (!i && (r != resp_match_value))
> +
> +		if (!i)
>  			return -ETIMEDOUT;
>  	}
>  
> -	for (i = 0; i < resp_size; i++) {
> -		if (i == 0 && resp_match) {
> -			resp[i] = resp_match_value;
> -			continue;
> -		}
> +	resp[0] = r;
> +	for (i = 1; i < resp_size; i++) {
>  		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
>  		if (ret)
>  			return ret;
> 

[PATCH 2/3] mmc: mmc_spi: Fix potential spec violation in receiving card response

[Jaehoon Chung]

On 2/1/21 1:20 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> After command is sent and before card response shows up on the line,
> there is a variable number of clock cycles in between called Ncr.
> The spec [1] says the minimum is 1 byte and the maximum is 8 bytes.
> 
> Current logic in mmc_spi_sendcmd() has a flaw that it could only work
> with certain SD cards with their Ncr being just 1 byte.
> 
> When resp_match is false, the codes try to receive only 1 byte from
> the SD card. On the other hand when resp_match is true, the logic
> happens to be no problem as it loops until timeout to receive as many
> bytes as possible to see a match of the expected resp_match_value.
> However not every call to mmc_spi_sendcmd() is made with resp_match
> being true hence this exposes a potential issue with SD cards that
> have a larger Ncr value.
> 
> Given no issue was reported as of today, we can reasonably conclude
> that all cards being used on the supported boards happen to have a 1
> byte Ncr timing requirement. But a broken case can be triggered by
> utilizing QEMU to emulate a larger value of Ncr (by default 1 byte
> Ncr is used on QEMU). This commit fixes such potential spec violation
> to improve the card compatibility.
> 
> [1] "Physical Layer Specification Version 8.00"
>      chapter 7.5.1: Command / Response
>      chapter 7.5.4: Timing Values
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  drivers/mmc/mmc_spi.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
> index 85a2818..5ec6b0f 100644
> --- a/drivers/mmc/mmc_spi.c
> +++ b/drivers/mmc/mmc_spi.c
> @@ -103,29 +103,31 @@ static int mmc_spi_sendcmd(struct udevice *dev,
>  
>  	debug("%s: cmd%d", __func__, cmdidx);
>  
> -	if (resp_match) {
> +	if (resp_match)
>  		r = ~resp_match_value;
> -		i = CMD_TIMEOUT;
> -		while (i) {
> -			ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
> -			if (ret)
> -				return ret;
> -			debug(" resp%d=0x%x", rpos, r);
> -			rpos++;
> -			i--;
> +	i = CMD_TIMEOUT;
> +	while (i) {
> +		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
> +		if (ret)
> +			return ret;
> +		debug(" resp%d=0x%x", rpos, r);
> +		rpos++;
> +		i--;
>  
> +		if (resp_match) {
>  			if (r == resp_match_value)
>  				break;
> +		} else {
> +			if (!(r & 0x80))
> +				break;
>  		}
> -		if (!i && (r != resp_match_value))
> +
> +		if (!i)
>  			return -ETIMEDOUT;
>  	}
>  
> -	for (i = 0; i < resp_size; i++) {
> -		if (i == 0 && resp_match) {
> -			resp[i] = resp_match_value;
> -			continue;
> -		}
> +	resp[0] = r;
> +	for (i = 1; i < resp_size; i++) {
>  		ret = dm_spi_xfer(dev, 1 * 8, NULL, &r, 0);
>  		if (ret)
>  			return ret;
> 

[PATCH 3/3] mmc: mmc_spi: Document the 3 local functions

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

mmc_spi_sendcmd(), mmc_spi_readdata() and mmc_spi_writedata() are
currently undocumented. Add comment blocks to explain the arguments
and the return value.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 drivers/mmc/mmc_spi.c | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
index 5ec6b0f..6d4f205 100644
--- a/drivers/mmc/mmc_spi.c
+++ b/drivers/mmc/mmc_spi.c
@@ -37,7 +37,8 @@
 #define SPI_RESPONSE_CRC_ERR		((5 << 1)|1)
 #define SPI_RESPONSE_WRITE_ERR		((6 << 1)|1)
 
-/* Read and write blocks start with these tokens and end with crc;
+/*
+ * Read and write blocks start with these tokens and end with crc;
  * on error, read tokens act like a subset of R2_SPI_* values.
  */
 /* single block write multiblock read */
@@ -70,6 +71,20 @@ struct mmc_spi_priv {
 	struct spi_slave *spi;
 };
 
+/**
+ * mmc_spi_sendcmd() - send a command to the SD card
+ *
+ * @dev:	mmc_spi device
+ * @cmdidx:	command index
+ * @cmdarg:	command argument
+ * @resp_type:	card response type
+ * @resp:	buffer to store the card response
+ * @resp_size:	size of the card response
+ * @resp_match:	if true, compare each of received bytes with @resp_match_value
+ * @resp_match_value:	a value to be compared with each of received bytes
+ * @r1b:	if true, receive additional bytes for busy signal token
+ * @return 0 if OK, -ETIMEDOUT if no card response is received, -ve on error
+ */
 static int mmc_spi_sendcmd(struct udevice *dev,
 			   ushort cmdidx, u32 cmdarg, u32 resp_type,
 			   u8 *resp, u32 resp_size,
@@ -159,6 +174,15 @@ static int mmc_spi_sendcmd(struct udevice *dev,
 	return 0;
 }
 
+/**
+ * mmc_spi_readdata() - read data block(s) from the SD card
+ *
+ * @dev:	mmc_spi device
+ * @xbuf:	buffer of the actual data (excluding token and crc) to read
+ * @bcnt:	number of data blocks to transfer
+ * @bsize:	size of the actual data (excluding token and crc) in bytes
+ * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors
+ */
 static int mmc_spi_readdata(struct udevice *dev,
 			    void *xbuf, u32 bcnt, u32 bsize)
 {
@@ -207,6 +231,16 @@ static int mmc_spi_readdata(struct udevice *dev,
 	return ret;
 }
 
+/**
+ * mmc_spi_writedata() - write data block(s) to the SD card
+ *
+ * @dev:	mmc_spi device
+ * @xbuf:	buffer of the actual data (excluding token and crc) to write
+ * @bcnt:	number of data blocks to transfer
+ * @bsize:	size of actual data (excluding token and crc) in bytes
+ * @multi:	indicate a transfer by multiple block write command (CMD25)
+ * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors
+ */
 static int mmc_spi_writedata(struct udevice *dev, const void *xbuf,
 			     u32 bcnt, u32 bsize, int multi)
 {
-- 
2.7.4

[PATCH 3/3] mmc: mmc_spi: Document the 3 local functions

[Jaehoon Chung]

On 2/1/21 1:20 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> mmc_spi_sendcmd(), mmc_spi_readdata() and mmc_spi_writedata() are
> currently undocumented. Add comment blocks to explain the arguments
> and the return value.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
>  drivers/mmc/mmc_spi.c | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/mmc_spi.c b/drivers/mmc/mmc_spi.c
> index 5ec6b0f..6d4f205 100644
> --- a/drivers/mmc/mmc_spi.c
> +++ b/drivers/mmc/mmc_spi.c
> @@ -37,7 +37,8 @@
>  #define SPI_RESPONSE_CRC_ERR		((5 << 1)|1)
>  #define SPI_RESPONSE_WRITE_ERR		((6 << 1)|1)
>  
> -/* Read and write blocks start with these tokens and end with crc;
> +/*
> + * Read and write blocks start with these tokens and end with crc;
>   * on error, read tokens act like a subset of R2_SPI_* values.
>   */
>  /* single block write multiblock read */
> @@ -70,6 +71,20 @@ struct mmc_spi_priv {
>  	struct spi_slave *spi;
>  };
>  
> +/**
> + * mmc_spi_sendcmd() - send a command to the SD card
> + *
> + * @dev:	mmc_spi device
> + * @cmdidx:	command index
> + * @cmdarg:	command argument
> + * @resp_type:	card response type
> + * @resp:	buffer to store the card response
> + * @resp_size:	size of the card response
> + * @resp_match:	if true, compare each of received bytes with @resp_match_value
> + * @resp_match_value:	a value to be compared with each of received bytes
> + * @r1b:	if true, receive additional bytes for busy signal token
> + * @return 0 if OK, -ETIMEDOUT if no card response is received, -ve on error
> + */
>  static int mmc_spi_sendcmd(struct udevice *dev,
>  			   ushort cmdidx, u32 cmdarg, u32 resp_type,
>  			   u8 *resp, u32 resp_size,
> @@ -159,6 +174,15 @@ static int mmc_spi_sendcmd(struct udevice *dev,
>  	return 0;
>  }
>  
> +/**
> + * mmc_spi_readdata() - read data block(s) from the SD card
> + *
> + * @dev:	mmc_spi device
> + * @xbuf:	buffer of the actual data (excluding token and crc) to read
> + * @bcnt:	number of data blocks to transfer
> + * @bsize:	size of the actual data (excluding token and crc) in bytes
> + * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors
> + */
>  static int mmc_spi_readdata(struct udevice *dev,
>  			    void *xbuf, u32 bcnt, u32 bsize)
>  {
> @@ -207,6 +231,16 @@ static int mmc_spi_readdata(struct udevice *dev,
>  	return ret;
>  }
>  
> +/**
> + * mmc_spi_writedata() - write data block(s) to the SD card
> + *
> + * @dev:	mmc_spi device
> + * @xbuf:	buffer of the actual data (excluding token and crc) to write
> + * @bcnt:	number of data blocks to transfer
> + * @bsize:	size of actual data (excluding token and crc) in bytes
> + * @multi:	indicate a transfer by multiple block write command (CMD25)
> + * @return 0 if OK, -ECOMM if crc error, -ETIMEDOUT on other errors
> + */
>  static int mmc_spi_writedata(struct udevice *dev, const void *xbuf,
>  			     u32 bcnt, u32 bsize, int multi)
>  {
>