[PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[rogerable]

From: Roger Tseng <rogerable@realtek.com>

Current code erroneously fill the last byte of R2 response with an undefined
value. In addition, it is impossible to obtain the real values since the
controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
response. This could cause mmc stack to obtain inconsistent CID from the same
card after resume and misidentify it as a different card.

Fix by assigning a dummy value 0x01 to the last byte of R2 response.

Signed-off-by: Roger Tseng <rogerable@realtek.com>
---
 drivers/mmc/host/rtsx_pci_sdmmc.c |    1 +
 drivers/mmc/host/rtsx_usb_sdmmc.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
index dfde4a2..54849d8 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -412,6 +412,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
 	}
 
 	if (rsp_type == SD_RSP_TYPE_R2) {
+		ptr[16] = 1;
 		for (i = 0; i < 4; i++) {
 			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 5d3766e..ca08df1 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -435,6 +435,7 @@ static void sd_send_cmd_get_rsp(struct rtsx_usb_sdmmc *host,
 	}
 
 	if (rsp_type == SD_RSP_TYPE_R2) {
+		ptr[16] = 1;
 		for (i = 0; i < 4; i++) {
 			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
-- 
1.7.10.4

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Dan Carpenter]

On Mon, Aug 11, 2014 at 04:32:16PM +0800, rogerable@realtek.com wrote:
> From: Roger Tseng <rogerable@realtek.com>
> 
> Current code erroneously fill the last byte of R2 response with an undefined
> value. In addition, it is impossible to obtain the real values since the
> controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
> response. This could cause mmc stack to obtain inconsistent CID from the same
> card after resume and misidentify it as a different card.
> 
> Fix by assigning a dummy value 0x01 to the last byte of R2 response.
> 
> Signed-off-by: Roger Tseng <rogerable@realtek.com>
> ---
>  drivers/mmc/host/rtsx_pci_sdmmc.c |    1 +
>  drivers/mmc/host/rtsx_usb_sdmmc.c |    1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index dfde4a2..54849d8 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -412,6 +412,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
>  	}
>  
>  	if (rsp_type == SD_RSP_TYPE_R2) {
> +		ptr[16] = 1;

Avoid magic numbers like 16 and 0x1.

This is subtle enough that it deserves a comment.

		ptr[stat_idx] = 0x1 /* 0x1 chosen randomly */

>  		for (i = 0; i < 4; i++) {
>  			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
>  			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",

There are a lot of magic numbers in this function.  We could get rid
of this i < 4 loop but doing:

		memcpy(cmd->resp, ptr + 1, resp_len);

Currently we don't use resp_len and the resp_len = 5 assignment is off
by one...  It should be resp_len = 4.  This function is quite ugly.

regards,
dan carpenter

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Roger]

On 08/11/2014 09:02 PM, Dan Carpenter wrote:
> On Mon, Aug 11, 2014 at 04:32:16PM +0800, rogerable@realtek.com wrote:
>> From: Roger Tseng <rogerable@realtek.com>
>>
>> Current code erroneously fill the last byte of R2 response with an undefined
>> value. In addition, it is impossible to obtain the real values since the
>> controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
>> response. This could cause mmc stack to obtain inconsistent CID from the same
>> card after resume and misidentify it as a different card.
>>
>> Fix by assigning a dummy value 0x01 to the last byte of R2 response.
>>
>> Signed-off-by: Roger Tseng <rogerable@realtek.com>
>> ---
>>   drivers/mmc/host/rtsx_pci_sdmmc.c |    1 +
>>   drivers/mmc/host/rtsx_usb_sdmmc.c |    1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
>> index dfde4a2..54849d8 100644
>> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
>> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
>> @@ -412,6 +412,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
>>   	}
>>
>>   	if (rsp_type == SD_RSP_TYPE_R2) {
>> +		ptr[16] = 1;
>
> Avoid magic numbers like 16 and 0x1.
>
> This is subtle enough that it deserves a comment.
>
> 		ptr[stat_idx] = 0x1 /* 0x1 chosen randomly */
>
The 0x1 consists of 7-bit dummy zero CRC and stop bit 1, described in SD 
card. Anyway, I'll give a comment to this in the next version.
>>   		for (i = 0; i < 4; i++) {
>>   			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
>>   			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
>
> There are a lot of magic numbers in this function.  We could get rid
> of this i < 4 loop but doing:
>
> 		memcpy(cmd->resp, ptr + 1, resp_len);
>
> Currently we don't use resp_len and the resp_len = 5 assignment is off
> by one...  It should be resp_len = 4.  This function is quite ugly.

I can remove the unused rsp_len in this function. But I'm afraid the 
loop is still required. The destination cmd->resp is cpu-endian, but the 
raw response from SD card in the buffer (pointed by ptr) is big-endian.

> regards,
> dan carpenter
>
>
> ------Please consider the environment before printing this e-mail.
> .
>

Best regards,
Roger Tseng

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Dan Carpenter]

On Tue, Aug 12, 2014 at 03:19:12PM +0800, Roger wrote:
> I can remove the unused rsp_len in this function. But I'm afraid the
> loop is still required. The destination cmd->resp is cpu-endian, but
> the raw response from SD card in the buffer (pointed by ptr) is
> big-endian.

Oh, yes.  Of course.  Sorry for the noise.

regards,
dan carpenter

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Ulf Hansson]

On 11 August 2014 10:32,  <rogerable@realtek.com> wrote:
> From: Roger Tseng <rogerable@realtek.com>
>
> Current code erroneously fill the last byte of R2 response with an undefined
> value. In addition, it is impossible to obtain the real values since the
> controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
> response. This could cause mmc stack to obtain inconsistent CID from the same
> card after resume and misidentify it as a different card.
>
> Fix by assigning a dummy value 0x01 to the last byte of R2 response.
>
> Signed-off-by: Roger Tseng <rogerable@realtek.com>

Thanks! Queued for 3.18.

I guess this should go for stable as well?

Kind regards
Uffe


> ---
>  drivers/mmc/host/rtsx_pci_sdmmc.c |    1 +
>  drivers/mmc/host/rtsx_usb_sdmmc.c |    1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c
> index dfde4a2..54849d8 100644
> --- a/drivers/mmc/host/rtsx_pci_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
> @@ -412,6 +412,7 @@ static void sd_send_cmd_get_rsp(struct realtek_pci_sdmmc *host,
>         }
>
>         if (rsp_type == SD_RSP_TYPE_R2) {
> +               ptr[16] = 1;
>                 for (i = 0; i < 4; i++) {
>                         cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
>                         dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
> diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c b/drivers/mmc/host/rtsx_usb_sdmmc.c
> index 5d3766e..ca08df1 100644
> --- a/drivers/mmc/host/rtsx_usb_sdmmc.c
> +++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
> @@ -435,6 +435,7 @@ static void sd_send_cmd_get_rsp(struct rtsx_usb_sdmmc *host,
>         }
>
>         if (rsp_type == SD_RSP_TYPE_R2) {
> +               ptr[16] = 1;
>                 for (i = 0; i < 4; i++) {
>                         cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
>                         dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
> --
> 1.7.10.4
>

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Roger Tseng]

On Wed, 2014-08-13 at 17:09 +0200, Ulf Hansson wrote:
> On 11 August 2014 10:32,  <rogerable@realtek.com> wrote:
> > From: Roger Tseng <rogerable@realtek.com>
> >
> > Current code erroneously fill the last byte of R2 response with an undefined
> > value. In addition, it is impossible to obtain the real values since the
> > controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
> > response. This could cause mmc stack to obtain inconsistent CID from the same
> > card after resume and misidentify it as a different card.
> >
> > Fix by assigning a dummy value 0x01 to the last byte of R2 response.
> >
> > Signed-off-by: Roger Tseng <rogerable@realtek.com>
> 
> Thanks! Queued for 3.18.
> 
> I guess this should go for stable as well?
Yes. However, since rtsx_usb* is present in 3.16 and later, this patch
will not apply on 3.15.y or older. Should I separately send an adapted
version to stable?

By the way, according to Dan's comment I would like to add a few word
to explain the code. Would you help fix it up by following diff?

diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c
b/drivers/mmc/host/rtsx_pci_sdmmc.c
index 54849d8..ca31279 100644
--- a/drivers/mmc/host/rtsx_pci_sdmmc.c
+++ b/drivers/mmc/host/rtsx_pci_sdmmc.c
@@ -412,7 +412,13 @@ static void sd_send_cmd_get_rsp(struct
realtek_pci_sdmmc *host,
 	}
 
 	if (rsp_type == SD_RSP_TYPE_R2) {
+		/*
+		 * The controller offloads the last byte {CRC-7, stop bit 1'b1}
+		 * of response type R2. Assign a dummy CRC, 0, and stop bit to
+		 * the byte(ptr[16], goes into the LSB of resp[3] later).
+		 */
 		ptr[16] = 1;
+
 		for (i = 0; i < 4; i++) {
 			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",
diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c
b/drivers/mmc/host/rtsx_usb_sdmmc.c
index ca08df1..727a88d 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -435,7 +435,13 @@ static void sd_send_cmd_get_rsp(struct
rtsx_usb_sdmmc *host,
 	}
 
 	if (rsp_type == SD_RSP_TYPE_R2) {
+		/*
+		 * The controller offloads the last byte {CRC-7, stop bit 1'b1}
+		 * of response type R2. Assign a dummy CRC, 0, and stop bit to
+		 * the byte(ptr[16], goes into the LSB of resp[3] later).
+		 */
 		ptr[16] = 1;
+
 		for (i = 0; i < 4; i++) {
 			cmd->resp[i] = get_unaligned_be32(ptr + 1 + i * 4);
 			dev_dbg(sdmmc_dev(host), "cmd->resp[%d] = 0x%08x\n",

-- 
Best regards,
Roger Tseng

Re: [PATCH] mmc: rtsx: fix incorrect last byte in R2 response

[Ulf Hansson]

On 14 August 2014 08:06, Roger Tseng <rogerable@realtek.com> wrote:
> On Wed, 2014-08-13 at 17:09 +0200, Ulf Hansson wrote:
>> On 11 August 2014 10:32,  <rogerable@realtek.com> wrote:
>> > From: Roger Tseng <rogerable@realtek.com>
>> >
>> > Current code erroneously fill the last byte of R2 response with an undefined
>> > value. In addition, it is impossible to obtain the real values since the
>> > controller actually 'offloads' the last byte(CRC7, end bit) while receiving R2
>> > response. This could cause mmc stack to obtain inconsistent CID from the same
>> > card after resume and misidentify it as a different card.
>> >
>> > Fix by assigning a dummy value 0x01 to the last byte of R2 response.
>> >
>> > Signed-off-by: Roger Tseng <rogerable@realtek.com>
>>
>> Thanks! Queued for 3.18.
>>
>> I guess this should go for stable as well?
> Yes. However, since rtsx_usb* is present in 3.16 and later, this patch
> will not apply on 3.15.y or older. Should I separately send an adapted
> version to stable?

I haven't pushed this externally yet. I will drop the patch from my 3.18 branch.

Then, let's split the patch into two, one for usb and one for pci -
that should simplify patch management.

Additionally, you should include a Cc tag with proper hashmark telling
which kernel each patch should be included into.

Kind regards
Uffe