[RFC 0/6] mmc: Field Firmware Update

[Holger Schurig]

There have been some attempts to add FFU (field firmware update). The last
AFAIK in Nov 2014, http://www.spinics.net/lists/linux-mmc/msg29324.html

But it seems that the committers weren't persistent enought.

I took the liberty to take Avi's patch and make it hopefully
maintainer-review friendly.

The first 5 patches just move functions out of mmc_test.c into core.c. Those
functions will later be used by both mmc_test.c and mmc_ffu.c. Contrary to
Avi's patch I didn't add static helper functions to mmc_test.c, e.g.
there's no mmc_test_prepare_mrq() that calls mmc_prepare_mrq(). It's
simpler to call mmc_prepare_mrq() directly. It's just one more dereference
from *mmc_card to *mmc_test_card anyway.

The patch [6/6] is http://www.spinics.net/lists/linux-mmc/msg29326.html, but
with less checkpatch warnings. And it doesn't use mmc_send_ext_csd()
anymore, which has been deleted since November.

I'm sending this patch as RFC now. It compiles (for me). But I get the
firmware update file from Kingston only next Tuesday. That means that so
far I haven't been testing it. It won't do anything without the proper
user-space command in mmc-utils anyway :-)

Comments welcome (I intent to get this patch into the kernel)

The patch is against Linux GIT (v4.3-11748-g46d862b).

Holger

drivers/mmc/card/Kconfig | 11 +
drivers/mmc/card/Makefile | 1 +
drivers/mmc/card/block.c | 5 +
drivers/mmc/card/mmc_ffu.c | 489 ++++++++++++++++++++++++++++++++++++++++++++
drivers/mmc/card/mmc_test.c | 235 +++++----------------
drivers/mmc/core/core.c | 134 ++++++++++++
drivers/mmc/core/mmc_ops.c | 4 +-
include/linux/mmc/card.h | 1 +
include/linux/mmc/core.h | 41 ++++
include/linux/mmc/mmc.h | 6 +
10 files changed, 739 insertions(+), 188 deletions(-)
create mode 100644 drivers/mmc/card/mmc_ffu.c

--
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Holger Schurig]

This function can be used to send ext_csd data towards the chip, which is
needed in the (upcoming) firmware update patch.

Signed-off-by: Holger Schurig <holger...@gmail.com>
---
drivers/mmc/core/mmc_ops.c | 4 ++--
include/linux/mmc/core.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 1f44426..d1d6abc 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -287,8 +287,7 @@ mmc_send_cxd_native(struct mmc_host *host, u32 arg, u32 *cxd, int opcode)
* NOTE: void *buf, caller for the buf is required to use DMA-capable
* buffer or on-stack buffer (with some overhead in callee).
*/
-static int
-mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
+int mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
u32 opcode, void *buf, unsigned len)
{
struct mmc_request mrq = {NULL};
@@ -336,6 +335,7 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,

return 0;
}
+EXPORT_SYMBOL(mmc_send_cxd_data);

int mmc_send_csd(struct mmc_card *card, u32 *csd)
{
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 774846f..b0e0f15 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -162,6 +162,9 @@ extern void mmc_start_bkops(struct mmc_card *card, bool from_exception);
extern int mmc_switch(struct mmc_card *, u8, u8, u8, unsigned int);
extern int mmc_send_tuning(struct mmc_host *host, u32 opcode, int *cmd_error);
extern int mmc_get_ext_csd(struct mmc_card *card, u8 **new_ext_csd);
+extern int mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
+ u32 opcode, void *buf, unsigned len);
+

#define MMC_ERASE_ARG 0x00000000
#define MMC_SECURE_ERASE_ARG 0x80000000

[Holger Schurig]

Currently this function is used inside the mmc test driver. But it is also
usable in the (upcoming) firmware update patch. So move this function out
of mmc_test.c into core.c.

Signed-off-by: Holger Schurig <holger...@gmail.com>
---

drivers/mmc/card/mmc_test.c | 46 ++++-----------------------------------------
drivers/mmc/core/core.c | 38 +++++++++++++++++++++++++++++++++++++
include/linux/mmc/core.h | 1 +
3 files changed, 43 insertions(+), 42 deletions(-)

diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 69b6c45..d1c2d22 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -184,44 +184,6 @@ static int mmc_test_set_blksize(struct mmc_test_card *test, unsigned size)
return mmc_set_blocklen(test->card, size);
}

-static int mmc_test_busy(struct mmc_command *cmd)
-{
- return !(cmd->resp[0] & R1_READY_FOR_DATA) ||
- (R1_CURRENT_STATE(cmd->resp[0]) == R1_STATE_PRG);
-}
-
-/*
- * Wait for the card to finish the busy state
- */
-static int mmc_test_wait_busy(struct mmc_test_card *test)
-{
- int ret, busy;
- struct mmc_command cmd = {0};
-
- busy = 0;
- do {
- memset(&cmd, 0, sizeof(struct mmc_command));
-
- cmd.opcode = MMC_SEND_STATUS;
- cmd.arg = test->card->rca << 16;
- cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
-
- ret = mmc_wait_for_cmd(test->card->host, &cmd, 0);
- if (ret)
- break;
-
- if (!busy && mmc_test_busy(&cmd)) {
- busy = 1;
- if (test->card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
- pr_info("%s: Warning: Host did not "
- "wait for busy state to end.\n",
- mmc_hostname(test->card->host));
- }
- } while (mmc_test_busy(&cmd));
-
- return ret;
-}
-
/*
* Transfer a single sector of kernel addressable data
*/
@@ -250,7 +212,7 @@ static int mmc_test_buffer_transfer(struct mmc_test_card *test,
if (data.error)
return data.error;

- return mmc_test_wait_busy(test);
+ return mmc_wait_busy(test->card);
}

static void mmc_test_free_mem(struct mmc_test_mem *mem)
@@ -675,7 +637,7 @@ static int mmc_test_check_result_async(struct mmc_card *card,
struct mmc_test_async_req *test_async =
container_of(areq, struct mmc_test_async_req, areq);

- mmc_test_wait_busy(test_async->test);
+ mmc_wait_busy(test_async->test->card);

return mmc_test_check_result(test_async->test, areq->mrq);
}
@@ -812,7 +774,7 @@ static int mmc_test_simple_transfer(struct mmc_test_card *test,

mmc_wait_for_req(test->card->host, &mrq);

- mmc_test_wait_busy(test);
+ mmc_wait_busy(test->card);

return mmc_test_check_result(test, &mrq);
}
@@ -841,7 +803,7 @@ static int mmc_test_broken_transfer(struct mmc_test_card *test,

mmc_wait_for_req(test->card->host, &mrq);

- mmc_test_wait_busy(test);
+ mmc_wait_busy(test->card);

return mmc_test_check_broken_result(test, &mrq);
}
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 2b5e398..df5a61c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2856,6 +2856,44 @@ void mmc_prepare_mrq(struct mmc_card *card,
}
EXPORT_SYMBOL(mmc_prepare_mrq);

+static int mmc_test_busy(struct mmc_command *cmd)
+{
+ return !(cmd->resp[0] & R1_READY_FOR_DATA) ||
+ (R1_CURRENT_STATE(cmd->resp[0]) == R1_STATE_PRG);
+}
+
+/*
+ * Wait for the card to finish the busy state
+ */
+int mmc_wait_busy(struct mmc_card *card)
+{
+ int ret, busy;
+ struct mmc_command cmd = {0};
+
+ busy = 0;
+ do {
+ memset(&cmd, 0, sizeof(struct mmc_command));
+
+ cmd.opcode = MMC_SEND_STATUS;
+ cmd.arg = card->rca << 16;
+ cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
+
+ ret = mmc_wait_for_cmd(card->host, &cmd, 0);
+ if (ret)
+ break;
+
+ if (!busy && mmc_test_busy(&cmd)) {
+ busy = 1;
+ if (card->host->caps & MMC_CAP_WAIT_WHILE_BUSY)
+ pr_info("%s: Warning: Host did not wait for busy state to end.\n",
+ mmc_hostname(card->host));
+ }
+ } while (mmc_test_busy(&cmd));
+
+ return ret;
+}
+EXPORT_SYMBOL(mmc_wait_busy);
+
/**
* mmc_init_context_info() - init synchronization context
* @host: mmc host
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index f8db960..50e37e1 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -199,6 +199,7 @@ extern int mmc_detect_card_removed(struct mmc_host *host);
extern void mmc_prepare_mrq(struct mmc_card *card,
struct mmc_request *mrq, struct scatterlist *sg, unsigned sg_len,
unsigned dev_addr, unsigned blocks, unsigned blksz, int write);
+extern int mmc_wait_busy(struct mmc_card *card);

/**
* mmc_claim_host - exclusively claim a host

[Holger Schurig]

Currently this function is used inside the mmc test driver. But it is also

usable in the (upcoming) firmware update patch. So move this function out
of mmc_test.c into core.c.

Signed-off-by: Holger Schurig <holger...@gmail.com>
---

drivers/mmc/card/mmc_test.c | 38 ++++++--------------------------------
drivers/mmc/core/core.c | 27 +++++++++++++++++++++++++++
include/linux/mmc/core.h | 3 +++
3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/mmc/card/mmc_test.c b/drivers/mmc/card/mmc_test.c
index 1d9d997..94298fa 100644
--- a/drivers/mmc/card/mmc_test.c
+++ b/drivers/mmc/card/mmc_test.c
@@ -721,32 +721,6 @@ err:
}

/*
- * Tests a basic transfer with certain parameters
- */
-static int mmc_test_simple_transfer(struct mmc_test_card *test,
- struct scatterlist *sg, unsigned sg_len, unsigned dev_addr,
- unsigned blocks, unsigned blksz, int write)
-{
- struct mmc_request mrq = {0};

- struct mmc_command cmd = {0};

- struct mmc_command stop = {0};
- struct mmc_data data = {0};
-
- mrq.cmd = &cmd;
- mrq.data = &data;
- mrq.stop = &stop;
-
- mmc_prepare_mrq(test->card, &mrq, sg, sg_len, dev_addr,
- blocks, blksz, write);
-
- mmc_wait_for_req(test->card->host, &mrq);
-
- mmc_wait_busy(test->card);
-
- return mmc_check_result(&mrq);
-}
-
-/*
* Tests a transfer where the card will fail completely or partly
*/

static int mmc_test_broken_transfer(struct mmc_test_card *test,

@@ -801,7 +775,7 @@ static int mmc_test_transfer(struct mmc_test_card *test,
if (ret)
return ret;

- ret = mmc_test_simple_transfer(test, sg, sg_len, dev_addr,
+ ret = mmc_simple_transfer(test->card, sg, sg_len, dev_addr,
blocks, blksz, write);
if (ret)
return ret;
@@ -875,7 +849,7 @@ static int mmc_test_basic_write(struct mmc_test_card *test)

sg_init_one(&sg, test->buffer, 512);

- return mmc_test_simple_transfer(test, &sg, 1, 0, 1, 512, 1);
+ return mmc_simple_transfer(test->card, &sg, 1, 0, 1, 512, 1);
}

static int mmc_test_basic_read(struct mmc_test_card *test)
@@ -889,7 +863,7 @@ static int mmc_test_basic_read(struct mmc_test_card *test)

sg_init_one(&sg, test->buffer, 512);

- return mmc_test_simple_transfer(test, &sg, 1, 0, 1, 512, 0);
+ return mmc_simple_transfer(test->card, &sg, 1, 0, 1, 512, 0);
}

static int mmc_test_verify_write(struct mmc_test_card *test)
@@ -898,7 +872,7 @@ static int mmc_test_verify_write(struct mmc_test_card *test)

sg_init_one(&sg, test->buffer, 512);

- return mmc_test_transfer(test, &sg, 1, 0, 1, 512, 1);
+ return mmc_simple_transfer(test->card, &sg, 1, 0, 1, 512, 1);
}

static int mmc_test_verify_read(struct mmc_test_card *test)
@@ -907,7 +881,7 @@ static int mmc_test_verify_read(struct mmc_test_card *test)

sg_init_one(&sg, test->buffer, 512);

- return mmc_test_transfer(test, &sg, 1, 0, 1, 512, 0);
+ return mmc_simple_transfer(test->card, &sg, 1, 0, 1, 512, 0);
}

static int mmc_test_multi_write(struct mmc_test_card *test)
@@ -1268,7 +1242,7 @@ static int mmc_test_area_transfer(struct mmc_test_card *test,
{
struct mmc_test_area *t = &test->area;

- return mmc_test_simple_transfer(test, t->sg, t->sg_len, dev_addr,
+ return mmc_simple_transfer(test->card, t->sg, t->sg_len, dev_addr,
t->blocks, 512, write);
}

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3254bce..fbc59ad 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2922,6 +2922,33 @@ int mmc_check_result(struct mmc_request *mrq)
}
EXPORT_SYMBOL(mmc_check_result);

+/*
+ * Run a basic transfer
+ */
+int mmc_simple_transfer(struct mmc_card *card,
+ struct scatterlist *sg, unsigned sg_len, unsigned dev_addr,
+ unsigned blocks, unsigned blksz, int write)
+{
+ struct mmc_request mrq = {0};

+ struct mmc_command cmd = {0};

+ struct mmc_command stop = {0};
+ struct mmc_data data = {0};
+
+ mrq.cmd = &cmd;
+ mrq.data = &data;
+ mrq.stop = &stop;
+
+ mmc_prepare_mrq(card, &mrq, sg, sg_len, dev_addr,
+ blocks, blksz, write);
+
+ mmc_wait_for_req(card->host, &mrq);
+
+ mmc_wait_busy(card);
+
+ return mmc_check_result(&mrq);
+}
+EXPORT_SYMBOL(mmc_simple_transfer);

+
/**
* mmc_init_context_info() - init synchronization context
* @host: mmc host
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h

index a5ad2d8..774846f 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -208,6 +208,9 @@ extern void mmc_prepare_mrq(struct mmc_card *card,

unsigned dev_addr, unsigned blocks, unsigned blksz, int write);

extern int mmc_wait_busy(struct mmc_card *card);
extern int mmc_check_result(struct mmc_request *mrq);
+extern int mmc_simple_transfer(struct mmc_card *test,
+ struct scatterlist *sg, unsigned sg_len, unsigned dev_addr,
+ unsigned blocks, unsigned blksz, int write);

/**
* mmc_claim_host - exclusively claim a host

[Alan Cooper]

The newly added ioctl MMC_IOC_MULTI_CMD allows user space to send
multiple commands
atomically, so mmu-utils may be a better place for this functionality.

Al

[Avi Shchislowski]

Hi Holger,

Thank you for resubmitting the FFU patches again.
We did couple of fixes since our last submission (in [RFC 6/6]), so we will post them soon.
Also, we would like to leave the original "signed-off-by" names in your FFU patch commits.
Could you please add the original:
Signed-off-by: Avi Shchislowski <avi.shch...@sandisk.com>
Signed-off-by: Alex Lemberg <Alex.L...@sandisk.com>

Thanks,
Avi & Alex

[Ulf Hansson]

+Chris, Baolin

On 25 November 2015 at 09:56, Holger Schurig <holger...@gmail.com> wrote:
> We had to modify the last patch a bit and was able to update a Kingston
> device from Firmware 0xba to 0xbd. But when doing this in user-space only is
> now the way to go, I should probably not submit it anymore?
>
> BTW, for this Kingston "EMMC16G-WL110-EBAU" the argument in the ext_csd is
> bogus. You need to use the "ffu_arg" hack from the mmc-utils patch and use
> argument 0x0000ffff. Sigh. They can't even stick to the standard.

>
>> Could you please add the original:
>

> Sorry, yes I could. But I won't, because if this can now be done reliably
> from user-space, there is no need to do this in kernel. So I guess the
> chances that a kernel solution get's applies is next to zero.

Hi Holger,

Sorry for the delay.

My advise right now is to try this out via the mmc ioctl in userspace,
yes. Although, if you encounter any issues with that approach that it
might not be reliable, I am open to look into the in-kernel solution
again.

Regarding mmc-utils as where I recommend you to implement this, I have
been thinking of moving this tool into the tools directory in the
kernel.

Baolin Wang, at Linaro is currently looking into this to see if it's
feasible. Ideas/comments are usual greatly appreciated.

Kind regards
Uffe

[Holger Schurig]

> Sorry for the delay.

No problem, I was busy with many other projects as well.

> My advise right now is to try this out via the mmc ioctl in userspace,
> yes. Although, if you encounter any issues with that approach that it
> might not be reliable, I am open to look into the in-kernel solution
> again.

I managed to update my Kingston eMMCs with a slighly modified patch to
what I submitted. I however didn't bother to submit this, as I saw no
chance of getting it applied.

Also I once asked in the mailing list if there is some user-space
example of how to use the multi-block feature that is supposed to enable
this, but I haven't gotten an answer.

> Regarding mmc-utils as where I recommend you to implement this, I have
> been thinking of moving this tool into the tools directory in the
> kernel.

Sounds good to me.



Remotely related:

Do you know that some google people made their own version of mmc-utils
for ChromeOS? And the don't seem to give much effort in unification of
them? As if the world wouldn't know how hard it can be to re-unite
things again, Google should know from their custom kernels they use on
Android ... Sigh.


So you can

* git clone git://git.kernel.org/pub/scm/linux/kernel/git/cjb/mmc-utils.git
* git clone https://chromium.googlesource.com/chromiumos/third_party/mmc-utils

currently.

[Ulf Hansson]

+ Olof (forgot to add him last time)

On 22 December 2015 at 09:55, Ulf Hansson <ulf.h...@linaro.org> wrote:
> + Gwendal, Grant, Olof, Seshagiri, Jon

> Thanks for sharing the information. Some Chromium folkz has certainly
> been helpful in upstreaming activities, but you are right that there
> is more to be done for mmc utils.
>
> Perhaps by moving the code into the kernel tools dir and finding
> someone who actively wants to maintain it, will enable improved
> upstream activities!?
>
> Kind regards
> Uffe

[Ulf Hansson]

+ Gwendal, Grant, Olof, Seshagiri, Jon

On 22 December 2015 at 09:15, Holger Schurig <holger...@gmail.com> wrote:
>

Thanks for sharing the information. Some Chromium folkz has certainly
been helpful in upstreaming activities, but you are right that there
is more to be done for mmc utils.

Perhaps by moving the code into the kernel tools dir and finding
someone who actively wants to maintain it, will enable improved
upstream activities!?

Kind regards
Uffe

[Olof Johansson]

[sigh, in cleartext this time.]

2015-12-22 0:57 GMT-08:00 Ulf Hansson <ulf.h...@linaro.org>:
> + Olof (forgot to add him last time)
>
> On 22 December 2015 at 09:55, Ulf Hansson <ulf.h...@linaro.org> wrote:
>> + Gwendal, Grant, Olof, Seshagiri, Jon
>>
>> On 22 December 2015 at 09:15, Holger Schurig <holger...@gmail.com> wrote:
>>>
>>>> Sorry for the delay.
>>>
>>> No problem, I was busy with many other projects as well.
>>>
>>>> My advise right now is to try this out via the mmc ioctl in userspace,
>>>> yes. Although, if you encounter any issues with that approach that it
>>>> might not be reliable, I am open to look into the in-kernel solution
>>>> again.
>>>
>>> I managed to update my Kingston eMMCs with a slighly modified patch to
>>> what I submitted. I however didn't bother to submit this, as I saw no
>>> chance of getting it applied.
>>>
>>> Also I once asked in the mailing list if there is some user-space
>>> example of how to use the multi-block feature that is supposed to enable
>>> this, but I haven't gotten an answer.
>>>
>>>> Regarding mmc-utils as where I recommend you to implement this, I have
>>>> been thinking of moving this tool into the tools directory in the
>>>> kernel.
>>>
>>> Sounds good to me.
>>>
>>>
>>>
>>> Remotely related:
>>>
>>> Do you know that some google people made their own version of mmc-utils
>>> for ChromeOS?

"Make their own version" makes it sound like we've rewritten it. We
haven't. We've applied some patches locally over the last two years
based on work we've done for our products. It's not a deep fork.

We'd be happy to upstream them if there's interest from other users
and maintainers.

>> Perhaps by moving the code into the kernel tools dir and finding
>> someone who actively wants to maintain it, will enable improved
>> upstream activities!?

Hmm. If there's a well-defined userspace interface there's little
reason to bundle it directly, and I think that's the case here.

Chris has been busy with other stuff the last couple of years, is
there a lot of mmc-utils code that's out there that hasn't been picked
up?


-Olof

[Alex Lemberg]

Hi Ulf,

We succeeded to run FFU via new mmc multi-command ioctl without any code modification,
but only by using Single Sector commands (CMD24).

From running the FFU and from code review, we see two minor issues in this way of running FFU:
1. There is no support for Multiple Block write commands (CMD25) in existing IOCTL implementation -
seems like there is no polling for the card status on data transfer completion.
(The kernel FFU implementation supports FFU using Multiple Block Write commands).
2. As you probably remember, there are two ways to install the new FW in the end of FFU process -
In case MODE_OPERATION_CODES field is not supported by the device, the host sets to NORMAL state
and initiates a CMD0/HW_Reset/Power cycle to install the new firmware.
This sequence cannot be done via multi-command ioctl, and requires manual reset of the device/platform.
(The kernel FFU implementation supports both FW install methods).

For running FFU via new mmc multi-command ioctl, we have modified mmc-utils and add new functionality for FFU.
Please let us know if you want us to submit the patch for mmc-utils FFU functionality via multi-command ioctl.

Best regards,

Alex

> -----Original Message-----
> From: Holger Schurig [mailto:holger...@gmail.com]

[Ulf Hansson]

On 22 December 2015 at 19:51, Chris Ball <ch...@printf.net> wrote:
> Hi,

>
>> Chris has been busy with other stuff the last couple of years, is there a
>> lot of mmc-utils code that's out there that hasn't been picked up?

I don't really know.

Although, I wanted to make sure that the tool still was still being
maintained as otherwise people sooner or later will not care to send
patches.

>
> I've been applying mmc-utils patches as I get them -- last push was five
> weeks ago. So I don't have a backlog, but if people are sending mmc-utils
> patches to lists rather than to me directly, I'm unlikely to see them there.
>

I haven't bothered much at looking at mmc utils patches, although I
have noticed some being posted without any response or being picked
up.
I browsed the history of these patches and noticed that it's exactly
as you say. These haven't been sent directly you or they were sent to
an incorrect email address.

I apologize if I was giving the wrong impression that you didn't
maintain mmc utils very well, you certainly are!

Perhaps we should add some information about it in the mmc kernel doc,
to make it more visible.

Kind regards
Uffe

[Ulf Hansson]

On 28 December 2015 at 15:12, Alex Lemberg <Alex.L...@sandisk.com> wrote:
> Hi Ulf,
>
> We succeeded to run FFU via new mmc multi-command ioctl without any code modification,
> but only by using Single Sector commands (CMD24).
>
> From running the FFU and from code review, we see two minor issues in this way of running FFU:
> 1. There is no support for Multiple Block write commands (CMD25) in existing IOCTL implementation -

That's right. But I guess we cope without the multiple block support!?

Although, I wonder how hard it would be to add it...

> seems like there is no polling for the card status on data transfer completion.

We should fix that!

In the rpmb case, we check the status so we can probably trigger that
code to run for CMD24/25 as well.

> (The kernel FFU implementation supports FFU using Multiple Block Write commands).
> 2. As you probably remember, there are two ways to install the new FW in the end of FFU process -
> In case MODE_OPERATION_CODES field is not supported by the device, the host sets to NORMAL state

Before starting the update, you can find out which mode that is
supported and take relevant actions, right?

> and initiates a CMD0/HW_Reset/Power cycle to install the new firmware.

Yes, but that's fragile - as discussed earlier.

What we really need to do is to also remove the "card" device from the
system, as otherwise we may have invalid data in its member variables
and who knows what issues that can cause to upper levels.

> This sequence cannot be done via multi-command ioctl, and requires manual reset of the device/platform.

Yepp, it seems so at least for now. Perhaps we can think of a way to
improve this?

> (The kernel FFU implementation supports both FW install methods).
>
> For running FFU via new mmc multi-command ioctl, we have modified mmc-utils and add new functionality for FFU.
> Please let us know if you want us to submit the patch for mmc-utils FFU functionality via multi-command ioctl.

Yes please. Don't forget to send this to Chris as well!

[...]

Kind regards
Uffe

[Gwendal Grignou]

I am arriving after the battle, but I have finally rebased the eMMC
FFU kernel ffu code to 4.x. It is based on what Avi and Alex have
written.
As stated earlier, the advantage over using MMC_MUTLI_CMD is we can
force a reset and rescan of the card without asking the user to reboot
their machine.
Also, by only sending a firmware name over the ioctl, we can use Kees'
work for firmware validation (https://lwn.net/Articles/605432/).
To prevent downloading firmware from unknown source, we would reject
some commands (like SWITCH with FFU_MODE) in the kernel
MMC_IOC/MULTI_CMD ioctl handler.

Gwendal.
>
> [...]
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in

[Ulf Hansson]

No matter what, I think the problem is how you would *safely* deal
with the reset. Especially in the case when the eMMC already has an
mounted file system on it.

Just doing something that *might* work, isn't good enough to me.

> Also, by only sending a firmware name over the ioctl, we can use Kees'
> work for firmware validation (https://lwn.net/Articles/605432/).

The request_firmware() interface would indeed be good to use. Although
unless we can figure out a way on how to safely deal with reset, we
will have to live without request_firmware().

> To prevent downloading firmware from unknown source, we would reject
> some commands (like SWITCH with FFU_MODE) in the kernel
> MMC_IOC/MULTI_CMD ioctl handler.

I don't follow, can you elaborate on this please.

Kind regards
Uffe

[Gwendal Grignou]

On Mon, Apr 4, 2016 at 4:50 AM, Ulf Hansson <ulf.h...@linaro.org> wrote:
>
> On 2 April 2016 at 02:23, Gwendal Grignou <gwe...@chromium.org> wrote:
> > On Thu, Jan 14, 2016 at 5:16 AM, Ulf Hansson <ulf.h...@linaro.org> wrote:
> >> On 28 December 2015 at 15:12, Alex Lemberg <Alex.L...@sandisk.com> wrote:
>
> > I am arriving after the battle, but I have finally rebased the eMMC
> > FFU kernel ffu code to 4.x. It is based on what Avi and Alex have
> > written.
> > As stated earlier, the advantage over using MMC_MUTLI_CMD is we can
> > force a reset and rescan of the card without asking the user to reboot
> > their machine.
>
> No matter what, I think the problem is how you would *safely* deal
> with the reset. Especially in the case when the eMMC already has an
> mounted file system on it.

Assuming the firmware is not wiping data or resizing the available
space, the data in the flash is readable after the upgrade.
For the host point of view, a firmware update and a reset is
equivalent to a reset, that could happen during error recovery.
The only change are in the cid/csd//extcsd registers the firmware may
have updated.
The stack has to assume these registers are not constant and can
change after reset.

When looking into the mmc stack, AFAICT, the code that needs to get
device specifics always rely on fields that are re-generated by
mmc_card_inif() (card->ext_csd, output of mmc_decode_csd()/cid(() and
so on).

>
>
> Just doing something that *might* work, isn't good enough to me.
>
> > Also, by only sending a firmware name over the ioctl, we can use Kees'
> > work for firmware validation (https://lwn.net/Articles/605432/).
>
> The request_firmware() interface would indeed be good to use. Although
> unless we can figure out a way on how to safely deal with reset, we
> will have to live without request_firmware().
>
> > To prevent downloading firmware from unknown source, we would reject
> > some commands (like SWITCH with FFU_MODE) in the kernel
> > MMC_IOC/MULTI_CMD ioctl handler.
>
> I don't follow, can you elaborate on this please.

Today, an attacker with root access could break the chain of trust by
writing a firmware in the eMMC that corrupts data on the fly and
return infected code to the host after verification.
One way is to use firmware signed by the manufacturer, a stronger
approach is to enforce that the firmware is part of the root
partition.
To prevent a bad firmware from being downloaded, we have to make sure
downloading firmware using raw single or multi commands ioctls does
not work.

Gwendal.
>
>
> Kind regards
> Uffe

[Ulf Hansson]

On 14 April 2016 at 00:33, Gwendal Grignou <gwe...@chromium.org> wrote:
> On Mon, Apr 4, 2016 at 4:50 AM, Ulf Hansson <ulf.h...@linaro.org> wrote:
>>
>> On 2 April 2016 at 02:23, Gwendal Grignou <gwe...@chromium.org> wrote:
>> > On Thu, Jan 14, 2016 at 5:16 AM, Ulf Hansson <ulf.h...@linaro.org> wrote:
>> >> On 28 December 2015 at 15:12, Alex Lemberg <Alex.L...@sandisk.com> wrote:
>>
>> > I am arriving after the battle, but I have finally rebased the eMMC
>> > FFU kernel ffu code to 4.x. It is based on what Avi and Alex have
>> > written.
>> > As stated earlier, the advantage over using MMC_MUTLI_CMD is we can
>> > force a reset and rescan of the card without asking the user to reboot
>> > their machine.
>>
>> No matter what, I think the problem is how you would *safely* deal
>> with the reset. Especially in the case when the eMMC already has an
>> mounted file system on it.
>
> Assuming the firmware is not wiping data or resizing the available
> space, the data in the flash is readable after the upgrade.

This is exactly my point. You can no* assume anything about the card
after a firmware upgrade.

> For the host point of view, a firmware update and a reset is
> equivalent to a reset, that could happen during error recovery.
> The only change are in the cid/csd//extcsd registers the firmware may
> have updated.

Is that defined by the spec and are all eMMC vendors conforming to
your above statement?

I clearly see the benefit of using request_firmware() and I open to
adopt an in-kernel FFU solution that uses it, as long as a safe reset
can be managed.

However, whether it's more safe to hackers has nothing to do with it.
If a hacker becomes root on a device they can do all kind of magic
things, for example replacing a firmware in rootfs or sending ioctl
commands to a device node that has root permissions. To achieve
security, verification of a signatures are needed and currently the
request_firmware() API doesn't support this and nor does the eMMC
device itself (at least to my knowledge).

Regarding the safe reset, the only way I see how to deal with this, is
to force a reboot and prevent serving new read/write request after a
firmware upgrade. Although, perhaps you can think of something more
clever.

Kind regards
Uffe