On Thu, Jul 10, 2014 at 4:40 PM, Alex Lemberg <Alex.Lemberg@xxxxxxxxxxx> wrote: > Thanks Gwendal! > We will test your changes. > > Are you still experiencing the "request_firmware" failure? no, since I added the mmc_put_card(). But enclosing request_firmware() betwen put()/get() is not to my opinion not the right thing to do: the caller called us with the lock held, we should not release it behind its back. We also need to revisit the eMMC reset, I had to drop the lock there as well. > > Alex > >> -----Original Message----- >> From: Gwendal Grignou [mailto:gwendal@xxxxxxxxxxxx] >> Sent: Thursday, July 10, 2014 4:08 PM >> To: Avi Shchislowski; chris@xxxxxxxxxx; ulf.hansson@xxxxxxxxxx >> Cc: olofj@xxxxxxxxxxxx; Alex Lemberg; linux-mmc@xxxxxxxxxxxxxxx; >> cjb@xxxxxxxxxx; grundler@xxxxxxxxxxxx; Yaniv Iarovici; Gwendal Grignou >> Subject: [PATCH] Fix on top of sandisk patches. >> >> To apply on top of >> [RFC PATCH 1/1 v7]mmc: Support-FFU-for-eMMC-v5.0 >> >> Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> >> --- >> >> Avi, >> >> As I mentioned earlier, I found several problems with your patch. >> Here is a patch that allows to go further. >> It is still not working reliably. The eMMC I am using has a problem being >> reseted after upgrade, and I am still hitting what appears to be memory >> corruptions. I believe the issues I am still having are due to the kernel code, >> not the eMMC I am testing. >> >> Gwendal. >> >> drivers/mmc/card/block.c | 3 +- >> drivers/mmc/card/ffu.c | 121 +++++++++++++++++++++++-------------------- >> ---- >> drivers/mmc/core/mmc.c | 8 ++++ >> include/linux/mmc/card.h | 1 + >> include/linux/mmc/ffu.h | 13 +---- >> 5 files changed, 71 insertions(+), 75 deletions(-) >> >> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c index >> fc65a6c..9d866fb 100644 >> --- a/drivers/mmc/card/block.c >> +++ b/drivers/mmc/card/block.c >> @@ -504,8 +504,7 @@ static int mmc_blk_ioctl_cmd(struct block_device >> *bdev, >> mmc_get_card(card); >> >> if (cmd.opcode == MMC_FFU_DOWNLOAD_OP) { >> - err = mmc_ffu_download(card, &cmd , idata->buf, >> - idata->buf_bytes); >> + err = mmc_ffu_download(card, idata->buf); >> goto cmd_rel_host; >> } >> >> diff --git a/drivers/mmc/card/ffu.c b/drivers/mmc/card/ffu.c index >> 04e07a5..0e85b2b 100644 >> --- a/drivers/mmc/card/ffu.c >> +++ b/drivers/mmc/card/ffu.c >> @@ -63,15 +63,13 @@ struct mmc_ffu_area { >> >> static void mmc_ffu_prepare_mrq(struct mmc_card *card, >> struct mmc_request *mrq, struct scatterlist *sg, unsigned int sg_len, >> - u32 arg, unsigned int blocks, unsigned int blksz, int write) { >> + u32 arg, unsigned int blocks, unsigned int blksz) { >> BUG_ON(!mrq || !mrq->cmd || !mrq->data || !mrq->stop); >> >> if (blocks > 1) { >> - mrq->cmd->opcode = write ? >> - MMC_WRITE_MULTIPLE_BLOCK : >> MMC_READ_MULTIPLE_BLOCK; >> + mrq->cmd->opcode = MMC_WRITE_MULTIPLE_BLOCK; >> } else { >> - mrq->cmd->opcode = write ? MMC_WRITE_BLOCK : >> - MMC_READ_SINGLE_BLOCK; >> + mrq->cmd->opcode = MMC_WRITE_BLOCK; >> } >> >> mrq->cmd->arg = arg; >> @@ -89,11 +87,12 @@ static void mmc_ffu_prepare_mrq(struct mmc_card >> *card, >> >> mrq->data->blksz = blksz; >> mrq->data->blocks = blocks; >> - mrq->data->flags = write ? MMC_DATA_WRITE : MMC_DATA_READ; >> + mrq->data->flags = MMC_DATA_WRITE; >> mrq->data->sg = sg; >> mrq->data->sg_len = sg_len; >> >> - mmc_set_data_timeout(mrq->data, card); } >> + mmc_set_data_timeout(mrq->data, card); } >> >> /* >> * Checks that a normal transfer didn't have any errors @@ -153,7 +152,7 >> @@ static int mmc_ffu_wait_busy(struct mmc_card *card) { >> */ >> static int mmc_ffu_simple_transfer(struct mmc_card *card, >> struct scatterlist *sg, unsigned int sg_len, u32 arg, >> - unsigned int blocks, unsigned int blksz, int write) { >> + unsigned int blocks, unsigned int blksz) { >> struct mmc_request mrq = {0}; >> struct mmc_command cmd = {0}; >> struct mmc_command stop = {0}; >> @@ -162,8 +161,7 @@ static int mmc_ffu_simple_transfer(struct mmc_card >> *card, >> mrq.cmd = &cmd; >> mrq.data = &data; >> mrq.stop = &stop; >> - mmc_ffu_prepare_mrq(card, &mrq, sg, sg_len, arg, blocks, blksz, >> - write); >> + mmc_ffu_prepare_mrq(card, &mrq, sg, sg_len, arg, blocks, blksz); >> mmc_wait_for_req(card->host, &mrq); >> >> mmc_ffu_wait_busy(card); >> @@ -175,19 +173,17 @@ static int mmc_ffu_simple_transfer(struct >> mmc_card *card, >> * Map memory into a scatterlist. >> */ >> static unsigned int mmc_ffu_map_sg(struct mmc_ffu_mem *mem, int size, >> - struct scatterlist *sglist, unsigned int max_segs, >> - unsigned int max_seg_sz) >> + struct scatterlist *sglist) >> { >> struct scatterlist *sg = sglist; >> unsigned int i; >> unsigned long sz = size; >> - unsigned int sctr_len = 0; >> unsigned long len; >> >> - sg_init_table(sglist, max_segs); >> + sg_init_table(sglist, mem->cnt); >> >> - for (i = 0; i < mem->cnt && sz; i++, sz -= len) { >> - len = PAGE_SIZE * (1 << mem->arr[i].order); >> + for (i = 0; i < mem->cnt && sz; i++, sg++, sz -= len) { >> + len = PAGE_SIZE << mem->arr[i].order; >> >> if (len > sz) { >> len = sz; >> @@ -195,12 +191,10 @@ static unsigned int mmc_ffu_map_sg(struct >> mmc_ffu_mem *mem, int size, >> } >> >> sg_set_page(sg, mem->arr[i].page, len, 0); >> - sg = sg_next(sg); >> - sctr_len += 1; >> } >> sg_mark_end(sg); >> >> - return sctr_len; >> + return mem->cnt; >> } >> >> static void mmc_ffu_free_mem(struct mmc_ffu_mem *mem) { @@ -277,6 >> +271,8 @@ static struct mmc_ffu_mem *mmc_ffu_alloc_mem(unsigned >> long min_sz, >> mem->arr[mem->cnt].page = page; >> mem->arr[mem->cnt].order = order; >> mem->cnt += 1; >> + pr_debug("FFU: cnt: %d - order %d\n", mem->cnt, order); >> + >> if (max_page_cnt <= (1UL << order)) >> break; >> max_page_cnt -= 1UL << order; >> @@ -298,11 +294,11 @@ out_free: >> * Copy the data to the allocated pages. >> */ >> static int mmc_ffu_area_init(struct mmc_ffu_area *area, struct mmc_card >> *card, >> - u8 *data, int size) >> + const u8 *data, int size) >> { >> int ret; >> int i; >> - int length = 0; >> + int length = 0, page_length; >> >> area->max_tfr = size; >> >> @@ -323,20 +319,20 @@ static int mmc_ffu_area_init(struct mmc_ffu_area >> *area, struct mmc_card *card, >> goto out_free; >> } >> >> + page_length = PAGE_SIZE << area->mem->arr[i].order; >> memcpy(page_address(area->mem->arr[i].page), data + >> length, >> - min(size - length, (int)area->max_seg_sz)); >> - length += area->max_seg_sz; >> + min(size - length, page_length)); >> + length += page_length; >> } >> >> area->sg = kmalloc(sizeof(struct scatterlist) * area->mem->cnt, >> - GFP_KERNEL); >> + GFP_KERNEL); >> if (!area->sg) { >> ret = -ENOMEM; >> goto out_free; >> } >> >> - area->sg_len = mmc_ffu_map_sg(area->mem, size, area->sg, >> - area->max_segs, area->mem->cnt); >> + area->sg_len = mmc_ffu_map_sg(area->mem, size, area->sg); >> >> return 0; >> >> @@ -345,7 +341,7 @@ out_free: >> return ret; >> } >> >> -static int mmc_ffu_write(struct mmc_card *card, u8 *src, u32 arg, >> +static int mmc_ffu_write(struct mmc_card *card, const u8 *src, u32 arg, >> int size) >> { >> int rc; >> @@ -370,7 +366,8 @@ static int mmc_ffu_write(struct mmc_card *card, u8 >> *src, u32 arg, >> goto exit; >> >> rc = mmc_ffu_simple_transfer(card, area.sg, area.sg_len, arg, >> - max_tfr / CARD_BLOCK_SIZE, CARD_BLOCK_SIZE, 1); >> + max_tfr / CARD_BLOCK_SIZE, CARD_BLOCK_SIZE); >> + mmc_ffu_area_cleanup(&area); >> if (rc != 0) >> goto exit; >> >> @@ -379,7 +376,6 @@ static int mmc_ffu_write(struct mmc_card *card, u8 >> *src, u32 arg, >> } while (size > 0); >> >> exit: >> - mmc_ffu_area_cleanup(&area); >> return rc; >> } >> >> @@ -406,47 +402,39 @@ exit: >> return err; >> } >> >> -int mmc_ffu_download(struct mmc_card *card, struct mmc_command >> *cmd, >> - u8 *data, int buf_bytes) >> +int mmc_ffu_download(struct mmc_card *card, const char *name) >> { >> u8 ext_csd[CARD_BLOCK_SIZE]; >> int err; >> int ret; >> - u8 *buf = NULL; >> + u32 arg; >> const struct firmware *fw; >> >> - /* Read the EXT_CSD */ >> - err = mmc_send_ext_csd(card, ext_csd); >> - if (err) { >> - pr_err("FFU: %s: error %d sending ext_csd\n", >> - mmc_hostname(card->host), err); >> + pr_debug("FFU: %s uploading firmware %.20s to device\n", >> + mmc_hostname(card->host), name); >> + >> + if (strlen(name) > 512) { >> + err = -EINVAL; >> + pr_err("FFU: %s: %.20s is not a valid argument\n", >> + mmc_hostname(card->host), name); >> goto exit; >> } >> >> - /* check if card is eMMC 5.0 or higher */ >> - if (card->ext_csd.rev < 7) >> - return -EINVAL; >> - >> /* Check if FFU is supported */ >> - if (!FFU_SUPPORTED_MODE(ext_csd[EXT_CSD_SUPPORTED_MODE]) >> || >> - FFU_CONFIG(ext_csd[EXT_CSD_FW_CONFIG])) { >> - err = -EINVAL; >> + if (!card->ext_csd.ffu_capable) { >> + err = -EOPNOTSUPP; >> pr_err("FFU: %s: error %d FFU is not supported\n", >> mmc_hostname(card->host), err); >> goto exit; >> } >> >> - /* setup FW data buffer */ >> - err = request_firmware(&fw, data, &card->dev); >> + /* setup FW name buffer */ >> + mmc_put_card(card); >> + err = request_firmware(&fw, name, &card->dev); >> + mmc_get_card(card); >> if (err) { >> pr_err("Firmware request failed %d\n", err); >> - goto exit_normal; >> - } >> - >> - buf = kmalloc(fw->size, GFP_KERNEL); >> - if (buf == NULL) { >> - pr_err("Allocating memory for firmware failed!\n"); >> - goto exit_normal; >> + goto exit; >> } >> >> if ((fw->size % CARD_BLOCK_SIZE)) { >> @@ -454,9 +442,16 @@ int mmc_ffu_download(struct mmc_card *card, >> struct mmc_command *cmd, >> mmc_hostname(card->host), fw->size); >> } >> >> - memcpy(buf, fw->data, fw->size); >> + /* Read the EXT_CSD */ >> + err = mmc_send_ext_csd(card, ext_csd); >> + if (err) { >> + pr_err("FFU: %s: error %d sending ext_csd\n", >> + mmc_hostname(card->host), err); >> + goto exit; >> + } >> >> /* set device to FFU mode */ >> + pr_debug("FFU: %s switch to FFU mode\n", mmc_hostname(card- >> >host)); >> err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> EXT_CSD_MODE_CONFIG, >> MMC_FFU_MODE_SET, card->ext_csd.generic_cmd6_time); >> if (err) { >> @@ -466,18 +461,18 @@ int mmc_ffu_download(struct mmc_card *card, >> struct mmc_command *cmd, >> } >> >> /* set CMD ARG */ >> - cmd->arg = ext_csd[EXT_CSD_FFU_ARG] | >> + arg = ext_csd[EXT_CSD_FFU_ARG] | >> ext_csd[EXT_CSD_FFU_ARG + 1] << 8 | >> ext_csd[EXT_CSD_FFU_ARG + 2] << 16 | >> ext_csd[EXT_CSD_FFU_ARG + 3] << 24; >> >> - err = mmc_ffu_write(card, buf, cmd->arg, (int)fw->size); >> + err = mmc_ffu_write(card, fw->data, arg, (int)fw->size); >> >> exit_normal: >> release_firmware(fw); >> - kfree(buf); >> >> /* host switch back to work in normal MMC Read/Write commands >> */ >> + pr_debug("FFU: %s switch to normal mode\n", mmc_hostname(card- >> >host)); >> ret = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL, >> EXT_CSD_MODE_CONFIG, MMC_FFU_MODE_NORMAL, >> card->ext_csd.generic_cmd6_time); >> @@ -495,6 +490,8 @@ int mmc_ffu_install(struct mmc_card *card) >> u32 ffu_data_len; >> u32 timeout; >> >> + pr_debug("FFU: %s installing firmware to device\n", >> + mmc_hostname(card->host)); >> err = mmc_send_ext_csd(card, ext_csd); >> if (err) { >> pr_err("FFU: %s: error %d sending ext_csd\n", @@ -503,9 >> +500,8 @@ int mmc_ffu_install(struct mmc_card *card) >> } >> >> /* Check if FFU is supported */ >> - if (!FFU_SUPPORTED_MODE(ext_csd[EXT_CSD_SUPPORTED_MODE]) >> || >> - FFU_CONFIG(ext_csd[EXT_CSD_FW_CONFIG])) { >> - err = -EINVAL; >> + if (!card->ext_csd.ffu_capable) { >> + err = -EOPNOTSUPP; >> pr_err("FFU: %s: error %d FFU is not supported\n", >> mmc_hostname(card->host), err); >> goto exit; >> @@ -514,19 +510,20 @@ int mmc_ffu_install(struct mmc_card *card) >> /* check mode operation */ >> if (!FFU_FEATURES(ext_csd[EXT_CSD_FFU_FEATURES])) { >> /* restart the eMMC */ >> + mmc_put_card(card); >> err = mmc_ffu_restart(card); >> + mmc_get_card(card); >> if (err) { >> pr_err("FFU: %s: error %d FFU install:\n", >> mmc_hostname(card->host), err); >> } >> } else { >> - >> ffu_data_len = >> ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG]| >> ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG + 1] << >> 8 | >> ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG + 2] << >> 16 | >> ext_csd[EXT_CSD_NUM_OF_FW_SEC_PROG + 3] << >> 24; >> >> - if (!ffu_data_len) { >> + if (ffu_data_len == 0) { >> err = -EPERM; >> return err; >> } >> diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index >> 4099424..ace9123 100644 >> --- a/drivers/mmc/core/mmc.c >> +++ b/drivers/mmc/core/mmc.c >> @@ -552,6 +552,14 @@ static int mmc_read_ext_csd(struct mmc_card >> *card, u8 *ext_csd) >> card->ext_csd.data_sector_size = 512; >> } >> >> + /* eMMC v5 or later */ >> + if (card->ext_csd.rev >= 7) { >> + card->ext_csd.ffu_capable = >> + ((ext_csd[EXT_CSD_SUPPORTED_MODE] & 1) == 1) >> && >> + ((ext_csd[EXT_CSD_FW_CONFIG] & 1) == 0); >> + } else { >> + card->ext_csd.ffu_capable = false; >> + } >> out: >> return err; >> } >> diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h index >> 6a5c754..6a7de8d 100644 >> --- a/include/linux/mmc/card.h >> +++ b/include/linux/mmc/card.h >> @@ -87,6 +87,7 @@ struct mmc_ext_csd { >> unsigned int data_tag_unit_size; /* DATA TAG UNIT size */ >> unsigned int boot_ro_lock; /* ro lock support */ >> bool boot_ro_lockable; >> + bool ffu_capable; /* support Firmware Field >> Upgrade */ >> u8 raw_exception_status; /* 54 */ >> u8 raw_partition_support; /* 160 */ >> u8 raw_rpmb_size_mult; /* 168 */ >> diff --git a/include/linux/mmc/ffu.h b/include/linux/mmc/ffu.h index >> f5dcedb..a0ade1b 100644 >> --- a/include/linux/mmc/ffu.h >> +++ b/include/linux/mmc/ffu.h >> @@ -34,19 +34,10 @@ >> #define MMC_FFU_INSTALL_SET 0x1 >> >> #ifdef CONFIG_MMC_FFU >> -#define MMC_FFU_ENABLE 0x0 >> -#define MMC_FFU_CONFIG 0x1 >> -#define MMC_FFU_SUPPORTED_MODES 0x1 >> #define MMC_FFU_FEATURES 0x1 >> +#define FFU_FEATURES(ffu_features) (ffu_features & MMC_FFU_FEATURES) >> >> -#define FFU_ENABLED(ffu_enable) (ffu_enable & MMC_FFU_CONFIG) >> -#define FFU_SUPPORTED_MODE(ffu_sup_mode) \ >> - (ffu_sup_mode && MMC_FFU_SUPPORTED_MODES) >> -#define FFU_CONFIG(ffu_config) (ffu_config & MMC_FFU_CONFIG) - >> #define FFU_FEATURES(ffu_fetures) (ffu_fetures & MMC_FFU_FEATURES) >> - >> -int mmc_ffu_download(struct mmc_card *card, struct mmc_command >> *cmd, >> - u8 *data, int buf_bytes); >> +int mmc_ffu_download(struct mmc_card *card, const char *name); >> int mmc_ffu_install(struct mmc_card *card); #else static inline int >> mmc_ffu_download(struct mmc_card *card, >> -- >> 2.0.0.526.g5318336 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
