Of course this is all staging code and we normally don't review staging
code too hard when it's first sent because we understand that staging
code needs a lot of work before it's acceptable.
One of the rules of staging is that you can't touch code outside of
staging.
But since I happened to glance at a some of this code then here are
some tiny comments:
On Wed, Aug 18, 2021 at 07:40:16PM +0530, sidraya.bj@xxxxxxxxxxxxxxxxxxx wrote:
> +/*
> + * Structure contains the ID context.
> + */
> +struct idgen_hdblk {
> + void **link; /* to be part of single linked list */
> + /* Array of handles in this block. */
> + void *ahhandles[1];
Don't use 1 element flex arrays. Do it like this:
void *ahhandles[];
You will have to adjust all you math. But you should be using the
"struct_size(hdblk, ahhandles, nr)" macro anyway to prevent integer
overflows.
> +int idgen_createcontext(unsigned int maxid, unsigned int blksize,
> + int incid, void **idgenhandle)
> +{
> + struct idgen_context *idcontext;
> +
> + /* Create context structure */
> + idcontext = kzalloc(sizeof(*idcontext), GFP_KERNEL);
> + if (!idcontext)
> + return IMG_ERROR_OUT_OF_MEMORY;
We need to get rid of all these weird error codes? You can add that
to the TODO file.
> +int idgen_destroycontext(void *idgenhandle)
> +{
> + struct idgen_context *idcontext = (struct idgen_context *)idgenhandle;
> + struct idgen_hdblk *hdblk;
> +
> + if (!idcontext)
> + return IMG_ERROR_INVALID_PARAMETERS;
> +
> + /* If incrementing Ids, free the List of Incrementing Ids */
> + if (idcontext->incids) {
> + struct idgen_id *id;
> + unsigned int i = 0;
> +
> + for (i = 0; i < idcontext->blksize; i++) {
> + id = lst_removehead(&idcontext->incidlist[i]);
You're not allowed to invent your own list API. :P Generally there just
seems like too much extra helper functions and wrappers. Removing this
kind of stuff is standard for staging drivers so that's normall. Add it
to the TODO.
regards,
dan carpenter
