| Message ID | 20240814072934.2559911-11-avri.altman@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add SDUC Support | expand |
Hi Avri, 在 2024/8/14 15:29, Avri Altman 写道: > @@ -948,13 +949,20 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) > int err; > u32 result; > __be32 *blocks; > - > + u8 resp_sz; > struct mmc_request mrq = {}; > struct mmc_command cmd = {}; > struct mmc_data data = {}; > - > struct scatterlist sg; > > + /* > + * SD cards, specifically high volume cards, expect to be allowed with the > + * full 500msec busy period post write. Otherwise, they may not indicate > + * correctly the number of bytes written. > + */ > + if (mmc_card_is_sduc(card->host)) > + mmc_delay(500); Could you kindly point me to the right section of SD spec which states this 500ms before ACMD22 ? Is it the write busy time? And as you mentioned high volume cards, I am curious if 1TB sandisk MircoSDXC need 500ms delay as well?
Hi, Thanks for having a look. > Hi Avri, > > 在 2024/8/14 15:29, Avri Altman 写道: > > @@ -948,13 +949,20 @@ static int mmc_sd_num_wr_blocks(struct mmc_card > *card, u32 *written_blocks) > > int err; > > u32 result; > > __be32 *blocks; > > - > > + u8 resp_sz; > > struct mmc_request mrq = {}; > > struct mmc_command cmd = {}; > > struct mmc_data data = {}; > > - > > struct scatterlist sg; > > > > + /* > > + * SD cards, specifically high volume cards, expect to be allowed with the > > + * full 500msec busy period post write. Otherwise, they may not indicate > > + * correctly the number of bytes written. > > + */ > > + if (mmc_card_is_sduc(card->host)) > > + mmc_delay(500); > > Could you kindly point me to the right section of SD spec which states > this 500ms before ACMD22 ? Is it the write busy time? Yes. I encountered it while testing SDUC: If there are some phy errors (probably caused because the micro-to-SD adapter I was using), The first get-status response contains an error bit, and the recovery flow is entered immediately. Thus, violating the min 500msec allotted to the card write. > > And as you mentioned high volume cards, I am curious if 1TB sandisk > MircoSDXC need 500ms delay as well? Theoretically should be applied to all cards. But since this code is there since forever, and no issue observed thus far - I preferred limiting this to ultra capacity cards only. Thanks, Avri
Hi Avri, 在 2024/8/15 20:57, Avri Altman 写道: > Hi, > Thanks for having a look. > >> Hi Avri, >> >> 在 2024/8/14 15:29, Avri Altman 写道: >>> @@ -948,13 +949,20 @@ static int mmc_sd_num_wr_blocks(struct mmc_card >> *card, u32 *written_blocks) >>> int err; >>> u32 result; >>> __be32 *blocks; >>> - >>> + u8 resp_sz; >>> struct mmc_request mrq = {}; >>> struct mmc_command cmd = {}; >>> struct mmc_data data = {}; >>> - >>> struct scatterlist sg; >>> >>> + /* >>> + * SD cards, specifically high volume cards, expect to be allowed with the >>> + * full 500msec busy period post write. Otherwise, they may not indicate >>> + * correctly the number of bytes written. >>> + */ >>> + if (mmc_card_is_sduc(card->host)) >>> + mmc_delay(500); >> >> Could you kindly point me to the right section of SD spec which states >> this 500ms before ACMD22 ? Is it the write busy time? > Yes. I encountered it while testing SDUC: > If there are some phy errors (probably caused because the micro-to-SD adapter I was using), > The first get-status response contains an error bit, and the recovery flow is entered immediately. > Thus, violating the min 500msec allotted to the card write. I see. > >> >> And as you mentioned high volume cards, I am curious if 1TB sandisk >> MircoSDXC need 500ms delay as well? > Theoretically should be applied to all cards. > But since this code is there since forever, and no issue observed thus far - > I preferred limiting this to ultra capacity cards only. > So my biggest guess is the cards issue the busy indication after the first error block and the host drivers implemented either SW or HW wait busy mechanism, maybe before ACMD22 is issued, so no issue observed? > Thanks, > Avri >
> Hi Avri, > > 在 2024/8/15 20:57, Avri Altman 写道: > > Hi, > > Thanks for having a look. > > > >> Hi Avri, > >> > >> 在 2024/8/14 15:29, Avri Altman 写道: > >>> @@ -948,13 +949,20 @@ static int mmc_sd_num_wr_blocks(struct > >>> mmc_card > >> *card, u32 *written_blocks) > >>> int err; > >>> u32 result; > >>> __be32 *blocks; > >>> - > >>> + u8 resp_sz; > >>> struct mmc_request mrq = {}; > >>> struct mmc_command cmd = {}; > >>> struct mmc_data data = {}; > >>> - > >>> struct scatterlist sg; > >>> > >>> + /* > >>> + * SD cards, specifically high volume cards, expect to be allowed > >>> + with the > >>> + * full 500msec busy period post write. Otherwise, they may not > >>> + indicate > >>> + * correctly the number of bytes written. > >>> + */ > >>> + if (mmc_card_is_sduc(card->host)) > >>> + mmc_delay(500); > >> > >> Could you kindly point me to the right section of SD spec which > >> states this 500ms before ACMD22 ? Is it the write busy time? > > Yes. I encountered it while testing SDUC: > > If there are some phy errors (probably caused because the micro-to-SD > > adapter I was using), The first get-status response contains an error bit, and > the recovery flow is entered immediately. > > Thus, violating the min 500msec allotted to the card write. > > I see. > > > > >> > >> And as you mentioned high volume cards, I am curious if 1TB sandisk > >> MircoSDXC need 500ms delay as well? > > Theoretically should be applied to all cards. > > But since this code is there since forever, and no issue observed thus > > far - I preferred limiting this to ultra capacity cards only. > > > > So my biggest guess is the cards issue the busy indication after the first error > block and the host drivers implemented either SW or HW wait busy > mechanism, maybe before ACMD22 is issued, so no issue observed? I only observed it with SDUC, and on a setup with micro-to-SD adapter. On my other setups, the recovery flow wasn't triggered. What was happening is: mmc_blk_mq_req_done mmc_blk_mq_complete_prev_req mmc_blk_mq_poll_completion CMD13: 0: 00080900 00000000 00000000 00000000 = READY_FOR_DATA + ERROR mmc_blk_mq_rw_recovery mmc_sd_num_wr_blocks - bytes_xfered = 0 Consulting with our SD system guys, the 500msec must-have write timeout brought up, And fixed that. May I ask what exactly your concerns are? Thanks, Avri > > > Thanks, > > Avri > >
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 30cb8b0d5742..cc5ec94a4be1 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -48,6 +48,7 @@ #include <linux/mmc/sd.h> #include <linux/uaccess.h> +#include <asm/unaligned.h> #include "queue.h" #include "block.h" @@ -948,13 +949,20 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) int err; u32 result; __be32 *blocks; - + u8 resp_sz; struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {}; - struct scatterlist sg; + /* + * SD cards, specifically high volume cards, expect to be allowed with the + * full 500msec busy period post write. Otherwise, they may not indicate + * correctly the number of bytes written. + */ + if (mmc_card_is_sduc(card->host)) + mmc_delay(500); + err = mmc_app_cmd(card->host, card); if (err) return err; @@ -963,7 +971,14 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) cmd.arg = 0; cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC; - data.blksz = 4; + /* + * Normally, ACMD22 returns the number of written sectors as u32. + * SDUC, however, returns it as u64. This is not a superfluous + * requirement, because SDUC writes may exceed 2TB. + */ + resp_sz = mmc_card_is_sduc(card->host) ? 8 : 4; + + data.blksz = resp_sz; data.blocks = 1; data.flags = MMC_DATA_READ; data.sg = &sg; @@ -973,15 +988,31 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) mrq.cmd = &cmd; mrq.data = &data; - blocks = kmalloc(4, GFP_KERNEL); + blocks = kmalloc(resp_sz, GFP_KERNEL); if (!blocks) return -ENOMEM; - sg_init_one(&sg, blocks, 4); + sg_init_one(&sg, blocks, resp_sz); mmc_wait_for_req(card->host, &mrq); - result = ntohl(*blocks); + if (mmc_card_is_sduc(card->host)) { + u64 blocks_64 = get_unaligned_be64(blocks); + /* + * For Linux mmc however, the previously write operation could + * not be more than the block layer limits, thus just make room + * for a u64 and cast the response back to u32. + */ + + if (blocks_64 > UINT_MAX) { + /* avoid any test robot warnings */ + result = UINT_MAX; + } else { + result = (u32)blocks_64; + } + } else { + result = ntohl(*blocks); + } kfree(blocks); if (cmd.error || data.error)
ACMD22 is used to verify the previously write operation. Normally, it returns the number of written sectors as u32. SDUC, however, returns it as u64. This is not a superfluous requirement, because SDUC writes may exceeds 2TB. For Linux mmc however, the previously write operation could not be more than the block layer limits, thus we make room for a u64 and cast the returning value to u32. Moreover, SD cards expect to be allowed the full 500msec busy period post write operations. This is true for standard capacity SD, and even more so for high volume SD cards, specifically SDUC. If CMD13 return an error bit, the recovery flow is entered regardless of the busy period. Thus, better enforce the busy period for SDUC, otherwise it might return a bogus bytes written. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/mmc/core/block.c | 43 ++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-)