On Fri, 11 Jan 2019 at 15:09, Daniel Thompson <daniel.thompson@xxxxxxxxxx> wrote: > > 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). > Will rename it to device_shm instead. > > > > + 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. > Oh okay, will use "for" loop instead. -Sumit > > Daniel.