On Fri, Jan 11, 2019 at 12:52:19PM +0530, Sumit Garg wrote: > On Thu, 10 Jan 2019 at 19:49, Daniel Thompson > <daniel.thompson@xxxxxxxxxx> wrote: > > > +static int get_devices(struct tee_context *ctx, u32 session, > > > + struct tee_shm *device_uuid, u32 *shm_size) > > > > Missing const on device_uuid? > > > > I don't think we should have a const for device_uuid here as this is > shared memory struct pointer which is dynamically allocated and used > to fetch device UUIDs. Agree. Perhaps device_uuid is misnamed though (part of the reason I misread this is that it is singular so I though it was a single UUID travelling into the TZ). > > > + rc = get_devices(ctx, sess_arg.session, device_shm, &shm_size); > > > + if (rc < 0) > > > + goto out_shm; > > > + > > > + device_uuid = tee_shm_get_va(device_shm, 0); > > > + if (IS_ERR(device_uuid)) { > > > + pr_err("tee_shm_get_va failed\n"); > > > + rc = PTR_ERR(device_uuid); > > > + goto out_shm; > > > + } > > > + > > > + while (idx < shm_size / sizeof(uuid_t)) { > > > > This is a very uncommon way to write a for loop ;-). > > > > Ok, will add "num_devices" variable. num_devices might add readability but that is not what I meant. The most idiomatic way to write somthing that loops for every valid index value is: for (i=0; i < limit; i++) You wrote it like this: int idx=0; /* lots of code between initializer and first use */ while (idx < limit) { /* more code */ idx++; } Sure, they are equivalent but the idiomatic form is easier to read. Daniel.