Message-ID: <20210914042453.ttbf2atkzymqt5yl@sidraya-laptopU>
Date:   Tue, 14 Sep 2021 09:54:55 +0530
From:   Sidraya Jayagond <sidraya.bj@...hpartnertech.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     gregkh@...uxfoundation.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, prashanth.ka@...hpartnertech.com,
        praneeth@...com, mchehab@...nel.org, linux-media@...r.kernel.org,
        praveen.ap@...hpartnertech.com
Subject: Re: [PATCH 09/30] v4l: vxd-dec: Add idgen api modules

On Tue, Aug 24, 2021 at 05:00:06PM +0300, Dan Carpenter wrote:
> 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@...hpartnertech.com 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
> 

I will make change for void *ahhandles[1] with void *ahhandles[];,
will use "struct_size" while allocating memory.

I will replaced all Error macros with directly linux Error macros.

About customized data structure API e.g-list,queue, I am currently working on
replacing them with Linux kernel generic dtata structure API's.
Thank you for reviewing.

-- 






This
message contains confidential information and is intended only 
for the
individual(s) named. If you are not the intended
recipient, you are 
notified that disclosing, copying, distributing or taking any
action in 
reliance on the contents of this mail and attached file/s is strictly
prohibited. Please notify the
sender immediately and delete this e-mail 
from your system. E-mail transmission
cannot be guaranteed to be secured or 
error-free as information could be
intercepted, corrupted, lost, destroyed, 
arrive late or incomplete, or contain
viruses. The sender therefore does 
not accept liability for any errors or
omissions in the contents of this 
message, which arise as a result of e-mail
transmission.

Powered by blists - more mailing lists