core:driver implementation NXP CAAM Driver - RNG/HASH by cneveux · Pull Request #3259 · OP-TEE/optee_os

cneveux · 2019-09-10T11:01:34Z

Add the NXP CAAM drivers:

Random generator (instantiation and random generation)
Hash

etienne-lms · 2019-09-10T15:09:46Z

@cneveux. I've been through the whole P-R, which is pretty sane since preview review. Yet I still found few issues to tackle, and many many minor issues (parentheses, printf %d on unsigned, ...).
The whole makes about 80 comments. Sorry, that's huge, I know. Hope my review helps in some way...
I'll push the comments soon.

cneveux · 2019-09-16T13:32:41Z

@cneveux. I've been through the whole P-R, which is pretty sane since preview review. Yet I still found few issues to tackle, and many many minor issues (parentheses, printf %d on unsigned, ...).
The whole makes about 80 comments. Sorry, that's huge, I know. Hope my review helps in some way...
I'll push the comments soon.

@etienne-lms, are you going to submit your comments soon or do you still doing a review?

etienne-lms · 2019-09-10T11:39:24Z

+#include <caam_hal_cfg.h>
+#include <caam_hal_clk.h>
+#include <caam_hal_ctrl.h>
+#ifdef CFG_CRYPTO_HASH_HW


minor: can remove #ifdef/#endif

I prefer to make sure that the include is used only when HASH HW is enabled

As you like. Including even unused file prevents from useless pollution of #ifdef/#endif in the include list.

remove at all place in the file and define a static inline function for hash init

etienne-lms · 2019-09-10T11:51:35Z

+	return GET_CHANUM_MS_JRNUM(val);
+}
+
+#ifdef CFG_CRYPTO_HASH_HW


I think this #ifdef/#endif is useless.

yes and no, this function doesn't have to be present if not used. Prefer to keep as is

I don't think it is a good idea to keep this #ifdef for two reasons:

As a general rule, #ifdefs should be avoided as much as possible in .c files. We have too many of them already IMO. They make the code harder to read for no benefit over the recommended way which is to always prefer declaring stubs in the header file (static inline function that does nothing)

Even if you leave this function and it is never called, it should be removed from the final link due to --gc-sections so why bother?

well, I'll remove it

etienne-lms · 2019-09-10T11:54:16Z

+# and enable the associated CFG_CRYPTO_DRV_xxx Crypto driver
+# API
+#
+# Example: Enable CFG_CRYPTO_DRV_HASH qnd CFG_CRYPTO_HASH_HW


CFG_CRYPTO_HASH_HW seems CAAM specific and redundant with CFG_CRYPTO_DRV_HASH && CFG_NXP_CAAM.

Suggest to replace CFG_CRYPTO_HASH_HW with CFG_CRYPTO_DRV_HASH in this P-R, in core/drivers/crypto/caam/caam_ctrl.c and core/drivers/crypto/caam/sub.mk and fully remove all CFG_CRYPTO_xxx_HW.

As you said this is CFG_CRYPTO_HASH_HW and CFG_NXP_CAAM are CAAM NXP specific to include the CAAM HW driver and also the HASH feature of the CAAM.
The CFG_CRYPTO_DRV_HASH is to include any HW Hash driver.
For the moment we would like to keep the possibility to enable the CAAM HASH HW driver (or any other CAAM HW feature) without enabling the CFG_CRYPTO_DRV_HASH.
I prefer to keep the current implementation that is not affecting the TEE Generic Crypto

I get your point. Yet I think you introduce a generic CFG_CRYPTO_HASH_HW to configure a platform specific feature: "Embed CAAM hash HW driver".
I would suggest to use i.e. CFG_NXP_CAAM_HASH_DRV=y for embedding your driver,
and CFG_CRYPTO_DRV_HASH=y as currently to plug CAAM hash in core generic crypto library.

ok rename CFG_CRYPTO_HASH_HW to CFG_NXP_CAAM_HASH_DRV
same for RNG

etienne-lms · 2019-09-10T11:56:13Z

+#include <caam_desc_helper.h>
+#include <caam_io.h>
+#include <types_ext.h>
+#include <trace.h>


minor: swap 2 lines

etienne-lms · 2019-09-10T12:01:03Z

+	struct ptr_addr *ptr_addr = (struct ptr_addr *)(uintptr_t)last;
+
+	reg_pair_from_64(ptr, &ptr_addr->high, &ptr_addr->low);
+	inc++;


isn't something missing here? some caam_write_val(last, ptr_addr);

no, this is reg_pair_from_64 that is used here. We will get:
*ptr_addr->high = ptr >> 32;
*ptr_addr->low = ptr;
and ptr_addr->high is last[0], ptr_addr->low is last[1] or the opposite if CAAM addressing mode is Little endian,

Ok, i was wrong. Yet, check that inner 32bit endianness is satisfied by reg_pair_from_64() when CFG_CAAM_BIG_ENDIAN=y?

Yes, you're right.

etienne-lms · 2019-09-10T14:53:02Z

+#define DRV_DUMPDESC(desc) dump_desc(desc)
+
+#define DRV_DUMPBUF(title, buf, len)                                           \
+	({                                                                     \


for consistency, prefer do { and } while (0) as bounds.

etienne-lms · 2019-09-10T14:56:25Z

+#define __CAAM_UTILS_SGT_H__
+
+#include <utee_types.h>
+


#include <caam_common.h> for struct caamsgtbuf

etienne-lms · 2019-09-10T14:59:57Z

+	size_t alloc_size = 0;
+	uint32_t cacheline_size = 0;
+
+	MEM_TRACE("alloc %zu bytes of type %d", size, type);


etienne-lms · 2019-09-10T15:01:31Z

+
+	info = OF_MEM_INFO(ptr);
+	MEM_TRACE("free %p info %p", ptr, info);
+	MEM_TRACE("free @%p - %zu bytes of type %d", info->addr, info->size,


etienne-lms · 2019-09-10T15:01:42Z

+	size_t alloc_size = size;
+	uint32_t cacheline_size = 0;
+
+	MEM_TRACE("alloc (normal) %zu bytes of type %d", size, type);


etienne-lms · 2019-09-17T15:33:56Z

sorry, i've left that over and forgot to push them. here they are.

ricardosalveti · 2019-09-23T21:51:40Z

+	if (enable)
+		io_caam_write32(pcc2_base + PCC_CAAM, PCC_ENABLE_CLOCK);
+	else
+		io_caam_write32(pcc2_base + PCC_CAAM, PCC_DISABLE_CLOCK);


Looks like this pull request is missing the definitions of PCC_ENABLE/DISABLE_CLOCK, which is provided by the imx7ulp-crm_regs.h file in the imx fork.

Thanks for highlighting it. This is a fix that will be submitted with another patch when this PR will be merged.

Isn't it just easier and better to send a patch adding that header file first (just the header)? Otherwise users trying to enable it will face a build failure.

Major:
Since this P-R expects PCC_{ENABLE|DISABLE}_CLOCK be defined, please define these in this P-R, before or at least in the commit that introduces this code.
If you wait for a later fixup commit, the build will likely be broken for few commits, which is not a good idea.

This CAAM Driver is not build for this platform 7ULP. I'll see if the file can be added in a new comnit even if not necessary for this current PR

This CAAM Driver is not build for this platform 7ULP

Then why is the code even here inside #if defined(CFG_MX7ULP)/#endif?

Also we said on many occasions that we prefer to avoid #ifdefs in .c files if possible. It seems to me you could create one file per platform and use the pattern src-$(CFG_XYZ) += xyz.c here, no?

is it so mandatory for this case?
The code is already in a hal folder specific for a category of device. For a so small function that is not complex we didn't estimate to have a file per specific iMX define.

Mandatory, no, but it's just more convenient. For instance it's easier to navigate with grep or Vim ctags or whatever -- I'm showing my age here, perhaps you are using a more modern IDE, but anyway ;-) Say you're looking for the iMX6 clock enable function:

$ git grep caam_hal_clk_enable hal_clk.c:void caam_hal_clk_enable(bool enable) hal_clk.c:void caam_hal_clk_enable(bool enable) hal_clk.c:void caam_hal_clk_enable(bool enable)

Which one would you pick?

Compare with:

$ git grep caam_hal_clk_enable hal_clk_mx6_mx6ul.c:void caam_hal_clk_enable(bool enable) hal_clk_mx7.c:void caam_hal_clk_enable(bool enable) hal_clk_mx7ulp.c:void caam_hal_clk_enable(bool enable)

On the other hand, what's the advantage of having a single file?

etienne-lms

@cneveux, few more comments, mainly minor things but also few things that should be clean.

etienne-lms · 2019-09-20T10:53:29Z

+#
+$(call force, CFG_NXP_CAAM_RNG_DRV, y)
+
+ifeq ($(CFG_NXP_CAAM_RNG_DRV), y)


Remove this: always true since previous line.

etienne-lms · 2019-09-20T10:56:26Z

+	 * enable, hence relax the JR used for the CAAM configuration to
+	 * the Non-Secure
+	 */
+	if (jrcfg.base)


minor: suggest to move this before exit_init:: this sequence is not needed when function fails.

etienne-lms · 2019-09-24T06:55:40Z

+#define CCTRL_ULOAD_PKHA_B     BIT32(27)
+
+/* CCB NFIFO */
+#define NFIFO_CLASS(cla)       SHIFT_U32((NFIFO_CLASS_##cla & 0x3), 30)


can drop inner parentheses. same for lines below.

etienne-lms · 2019-09-24T06:56:53Z

+
+/* Configuration */
+#define JRX_JRCFGR_LS			0x0054
+#define JRX_JRCFGR_LS_ICTT(val)		SHIFT_U32(((val) & 0xFFFF), 16)


can drop inner parentheses, here and at line below.

etienne-lms · 2019-09-25T13:30:44Z

+
+	/*
+	 * If the configuration already locked, check that is the configuration
+	 * that we want. If not return in error.


minor: rephrase (sounds strange). I would suggest to remove this one and update the one at line 57 which says about the same: Configuration is locked: check it is the expected config.

etienne-lms · 2019-09-25T14:46:30Z

+
+#define MEM_TYPE_DEFAULT	MEM_TYPE_ZEROED
+
+#define CFG_CAAM_MEM_INFO


please avoid definin CFG_ config in source files. These should be defining from makefile scripts.

etienne-lms · 2019-09-25T14:47:15Z

+
+	if (type & MEM_TYPE_ALIGN) {
+		/*
+		 * In case of the buffer must be aligned on a cache


remove of

etienne-lms · 2019-09-25T14:48:21Z

+		 * In case of the buffer must be aligned on a cache
+		 * line, ensure that the size is aligned on a cache line
+		 *
+		 * Buffer address returned is moved up to a cache start offset,


s/cache start offset/cache line start offset/

etienne-lms · 2019-09-25T14:51:46Z

+	enum caam_status ret = CAAM_FAILURE;
+	size_t cpy_size = 0;
+
+	/* Check if the temporary buffer is allocted, else allocate it */


s/allocted/allocated/

etienne-lms · 2019-09-25T14:54:55Z

+
+/*
+ * Allocate normal memory.
+ * Depending on the MEM_TYPE_DEFAULT value initialized it to 0's.


I still think this is not nice. Looks like the API is not relaiable and can evolve.
I would strongly suggest that MEM_TYPE_DEFAULT relates to an immutable description.

ok. I'll change to define X_calloc_Y functions to make thing clearer and remove the MEM_TYPE_DEFAULT

etienne-lms

@cneveux. Here is a last round of review, at least for what I can tell.
Mostly saw still typos in inline comments (many inline comments => many review comments)

Yet, there are 3 major things to be fixed. I've prefixed my comments with Major: so that you can easily find then among the fifty minor comments.

etienne-lms · 2019-09-30T11:34:14Z

+
+driver_init(crypto_driver_init);
+
+/* Crypto driver late initialization to complete on-going CAAM operation */


nitpicking: s/operation/operations/

etienne-lms · 2019-09-30T11:37:19Z

+#endif /* CFG_CAAM_BIG_ENDIAN */
+}
+
+#else


minor: add here /* CFG_CAAM_64BIT */ and remove empty line above.

etienne-lms · 2019-09-30T11:38:14Z

+#include <caam_desc_helper.h>
+#include <caam_io.h>
+#include <caam_jr.h>
+#include <caam_hal_jr.h>


minor: move this one before #include <caam_io.h>

etienne-lms · 2019-09-30T11:38:55Z

+	uint32_t desc;   /* Physical address of the descriptor */
+	uint32_t status; /* Status of the executed job */
+};
+#endif


add /* CFG_CAAM_64BIT */

etienne-lms · 2019-09-30T11:44:14Z

+	    jr_privdata->nb_jobs) {
+		/*
+		 * Invalidate all circular job buffer because some
+		 * job ring completed are at the beginning of the buffer


s/all circular job buffer/the whole circular job buffer/
s/some job ring completed are/some completed job rings are/

etienne-lms · 2019-09-30T15:04:08Z

+	else
+		retstatus = CAAM_NO_ERROR;
+
+	/* Erase local callback function */


etienne-lms · 2019-09-30T15:05:41Z

+	struct rngdata *rng = NULL;
+	uint32_t wait_jobs = 0;
+	unsigned int idx = 0;
+	unsigned int loop = 4;


ok (resolved)

etienne-lms · 2019-09-30T15:05:50Z

+ * Returns the requested random data
+ *
+ * @len  number of bytes to returns
+ * @buf  [out] data buffer


etienne-lms · 2019-09-30T15:08:24Z

+		if (jr_offset == jrcfg->offset)
+			continue;
+#endif
+		status = caam_hal_jr_setowner(jrcfg->base, jr_offset,


So what can happen here if setowner fails? will CAAM be in a good state after resuming?

etienne-lms · 2019-09-30T15:13:38Z

+#define __CAAM_UTILS_STATUS_H__
+
+#include <tee_api_types.h>
+#include <stdint.h>


etienne-lms · 2019-09-30T16:54:41Z

+#endif
+	value = CTR_WORD_SIZE <<
+		((value >> CTR_DMINLINE_SHIFT) & CTR_DMINLINE_MASK);
+	MEM_TRACE("System Cache Line size = %u bytes", value);


hmm, i've tested few builds. It seems, at least gcc-8.3, is not too touchy on signed/unsigned and use of u32/x32 when applicable. Yet, if the code is to run on 32bit and 64bit machines, better be careful with vsprintf and typed argument extracted from the stack.

I guess I should not be too strict on log formats between signed and unsigned. As per int/long, prefer use the appropriate format.

Note: the maintainers suggested using PRI+* glued to strings, i.e. here:
(not mandatory, but may helps you preventing line breaks in indented traces.)
MEM_TRACE("System Cache Line size = %"PRIu32" bytes", value);

etienne-lms · 2019-09-30T17:04:31Z

+	 * also touching it
+	 */
+	cpu_spin_lock(&jr_privdata->callers_lock);
+	for (idx_jr = 0; idx_jr < jr_privdata->nb_jobs && !found; idx_jr++) {


nitpicking: here extracting !found from the loop control would be nice. Prefer a break at line 363.

etienne-lms

comment on the 2 last commits.

@cneveux, could you squash all fixup commit, remove Jens' r-b tag (doesn't apply since code has quite changed), and push a new clean commit series, for final review, hopefully tagging.

etienne-lms · 2019-10-01T14:02:50Z

+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright 2017-2019 NXP
+ *


minor: remove this line

etienne-lms · 2019-10-01T14:05:08Z

+ */
+#include <caam_hal_clk.h>
+#include <caam_io.h>
+#include <mm/core_memprot.h>


need to include platform_config.h to get CCM_BASE?

it was building without but I'll add it.

etienne-lms · 2019-10-01T14:08:58Z

 incdirs-y += .

-srcs-y += hal_clk.c
+ifneq (, $(filter y, $(CFG_MX6) $(CFG_MX6UL)))


srcs-$(CFG_MX6) += hal_clk_mx6_mx6ul.c seems enough, all mx6* flavours are CFG_MX6=y

correct. And so I'll rename the file to hal_clk_mx6.c

etienne-lms

Final round... several bad types in traces to fix.

Also please push the mx7 registers header file commit first, then the CAAM driver. Otherwise build is broken on CAAM driver commit.

etienne-lms · 2019-10-01T15:08:22Z

+				caller->pdesc = 0;
+				caller->job_id = JR_JOB_FREE;
+				found = true;
+				JR_TRACE("Free space #%d in the callers array",


s/%d/%"PRId16"/

etienne-lms · 2019-10-01T15:09:14Z

+				if (caller->job_id & wait_job_ids)
+					ret_job_id |= caller->job_id;
+
+				JR_TRACE("JR id=%d, context @0x%08" PRIxVA,


s/%d/%"PRId32"/

etienne-lms · 2019-10-01T15:10:15Z

+			jobctx->callback(jobctx);
+		}
+
+	} while (--nb_jobs_done);


prefer nb_jobs_done--

In fact change to a for loop because this change it's breaking to execution.

etienne-lms · 2019-10-01T15:12:20Z

+	cpu_spin_lock(&jr_privdata->callers_lock);
+	for (idx_jr = 0; idx_jr < jr_privdata->nb_jobs; idx_jr++) {
+		if (jr_privdata->callers[idx_jr].job_id == JR_JOB_FREE) {
+			JR_TRACE("Found a space #%d free in the callers array",


s/%d/%"PRId8"/

etienne-lms · 2019-10-01T15:14:32Z

+		goto end_enqueue;
+	}
+
+	JR_TRACE("Push id=%d, job (0x%08x) context @0x%08" PRIxVA,


s/%d/%"PRId16"/
s/%08x/%08"PRIx32"/

etienne-lms · 2019-10-01T15:39:34Z

+
+	retstatus =
+		caam_hal_jr_setowner(jrcfg->base, jrcfg->offset, JROWN_ARM_S);
+	JR_TRACE("JR setowner returned 0x%" PRIx32, retstatus);


s/%08" PRIx32/%08x"/
(retstatus is an enum and will evaluate as an int or unsigned int, not explicitly a 32bit value)

etienne-lms · 2019-10-01T15:40:30Z

+		atomic_store_u32(&rng->status, DATA_ONGOING);
+		ret = CAAM_NO_ERROR;
+	} else {
+		RNG_TRACE("RNG Job Ring Error 0x%" PRIx32, ret);


s/%" PRIx32/%08x"/
(ret is an enum and will evaluate as an int or unsigned int, not explicitly a 32bit value)

etienne-lms · 2019-10-01T15:41:25Z

+		prepare_inst_desc(nb_sh, sh_status, desc);
+
+		retstatus = caam_jr_enqueue(&jobctx, NULL);
+		RNG_TRACE("RNG Job returned 0x%08" PRIx32, retstatus);


s/%08" PRIx32/%08x"/

etienne-lms · 2019-10-01T15:41:44Z

+
+	caam_free_desc(&desc);
+
+	RNG_TRACE("RNG Instantiation return 0x%08" PRIx32, retstatus);


s/%08" PRIx32/%08x"/

etienne-lms · 2019-10-01T15:43:44Z

+	retresult = TEE_SUCCESS;
+exit_init:
+	if (retresult != TEE_SUCCESS) {
+		EMSG("CAAM Driver initialization (0x%x)", retresult);


s/%x/%x"PRIx32"/

etienne-lms · 2019-10-02T09:02:09Z

Acked-by: Etienne Carriere <[email protected]> for the 2 commits.

cneveux · 2019-10-02T10:26:48Z

Acked-by: Etienne Carriere <[email protected]> for the 2 commits.

There are 3 commits. Do you acked for all and I do a squash the 2 last commits?

etienne-lms · 2019-10-02T10:45:04Z

Sorry, I meant the 3 commits. But please squash them into 2 commits (first mx7 header then CAAM rng/hash) and apply my A-b tag on each.

Add imx7ulp CRM registers in a header file. Signed-off-by: Clement Faure <[email protected]> Acked-by: Etienne Carriere <[email protected]>

jforissier · 2019-10-03T08:12:01Z

@cneveux @etienne-lms thanks!

@cneveux I have one last nitpick...: I'd like the subject of the main commit to be "drivers: implement NXP CAAM driver", for consistency ("drivers:" is the most often used prefix for drivers, we always use imperative tense, and drop the unwanted capital D). I can change it myself and merge manually, but I'd rather have you do it otherwise the PR won't show as "merged". Thanks :)

cneveux · 2019-10-03T08:21:13Z

@cneveux @etienne-lms thanks!

@cneveux I have one last nitpick...: I'd like the subject of the main commit to be "drivers: implement NXP CAAM driver", for consistency ("drivers:" is the most often used prefix for drivers, we always use imperative tense, and drop the unwanted capital D). I can change it myself and merge manually, but I'd rather have you do it otherwise the PR won't show as "merged". Thanks :)

@jforissier
no problem I'm doing that.

jforissier · 2019-10-03T08:24:59Z

s/implementation/implement/

Add the NXP CAAM drivers: - Random generator (instantiation and random generation) - Hash Signed-off-by: Cedric Neveux <[email protected]> Acked-by: Etienne Carriere <[email protected]>

jforissier · 2019-10-03T09:10:38Z

@cneveux you still managed to get the subject wrong one more time (s/driver/drivers) but I'm throwing the towel ;-) Edit: you even dropped "NXP"... :-/

No offense but I think you should pay more attention to review comments in general. It looks like you're trying to push a bunch of code upstream as quickly as possible and frankly that won't work well I think.

cneveux · 2019-10-03T09:13:25Z

@cneveux you still managed to get the subject wrong one more time (s/driver/drivers) but I'm throwing the towel ;-)

No offense but I think you should pay more attention to review comments in general. It looks like you're trying to push a bunch of code upstream as quickly as possible and frankly that won't work well I think.

@jforissier
Very sorry. Thanks for the advice I'll pay more attention next time.

cneveux mentioned this pull request Sep 10, 2019

core:driver implementation NXP CAAM Driver - RNG/HASH #3228

Closed

cneveux force-pushed the feature/patch_caam_hash_3 branch 3 times, most recently from 6088911 to 7f09e73 Compare September 10, 2019 11:16

etienne-lms reviewed Sep 17, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_3 branch 2 times, most recently from d01028b to 350f720 Compare September 23, 2019 08:39

ricardosalveti reviewed Sep 23, 2019

View reviewed changes

etienne-lms reviewed Sep 25, 2019

View reviewed changes

etienne-lms reviewed Sep 30, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_3 branch from ebceb98 to 0ce2d92 Compare October 1, 2019 09:20

etienne-lms reviewed Oct 1, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_3 branch from 0ce2d92 to 28600ed Compare October 1, 2019 14:52

etienne-lms reviewed Oct 1, 2019

View reviewed changes

cneveux force-pushed the feature/patch_caam_hash_3 branch 2 times, most recently from 29d5968 to 7c5ca8c Compare October 2, 2019 06:39

cneveux force-pushed the feature/patch_caam_hash_3 branch from 7c5ca8c to ba7c306 Compare October 2, 2019 10:53

core: imx: add imx7ulp CRM registers

d12eb6f

Add imx7ulp CRM registers in a header file. Signed-off-by: Clement Faure <[email protected]> Acked-by: Etienne Carriere <[email protected]>

etienne-lms approved these changes Oct 3, 2019

View reviewed changes

etienne-lms mentioned this pull request Oct 3, 2019

Adding hardware accelerated crypto #3251

Closed

cneveux force-pushed the feature/patch_caam_hash_3 branch from ba7c306 to d7447e1 Compare October 3, 2019 08:22

cneveux force-pushed the feature/patch_caam_hash_3 branch from d7447e1 to 5db2ace Compare October 3, 2019 08:28

driver: implement CAAM driver

5db2ace

Add the NXP CAAM drivers: - Random generator (instantiation and random generation) - Hash Signed-off-by: Cedric Neveux <[email protected]> Acked-by: Etienne Carriere <[email protected]>

jforissier merged commit 2d7a896 into OP-TEE:master Oct 3, 2019

jockebech mentioned this pull request Oct 7, 2019

Update CHANGELOG for 3.7.0 #3309

Merged

etienne-lms mentioned this pull request Apr 2, 2020

HMAC implementation for CAAM driver #3689

Merged

cneveux deleted the feature/patch_caam_hash_3 branch October 13, 2020 12:55


		#define MEM_TYPE_DEFAULT MEM_TYPE_ZEROED

		#define CFG_CAAM_MEM_INFO


		driver_init(crypto_driver_init);

		/* Crypto driver late initialization to complete on-going CAAM operation */


		caam_free_desc(&desc);

		RNG_TRACE("RNG Instantiation return 0x%08" PRIx32, retstatus);

Conversation

cneveux commented Sep 10, 2019

Uh oh!

etienne-lms commented Sep 10, 2019

Uh oh!

cneveux commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cneveux Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etienne-lms commented Sep 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cneveux Sep 18, 2019 •

edited

Loading