Date:   Sat, 11 Mar 2023 22:31:33 +0500
From:   Khadija Kamran <kamrankhadijadj@gmail.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     outreachy@lists.linux.dev, linux-staging@lists.linux.dev,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments
Message-ID: <ZAy69T5sYZiyL4TJ@khadija-virtual-machine>
References: <ZAxsyX8n7G92nlJh@khadija-virtual-machine>
 <ZAxzfiOoW0SfW5qD@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZAxzfiOoW0SfW5qD@kroah.com>
Precedence: bulk

On Sat, Mar 11, 2023 at 01:26:38PM +0100, Greg Kroah-Hartman wrote:
> On Sat, Mar 11, 2023 at 04:58:01PM +0500, Khadija Kamran wrote:
> > In file drivers/staging/axis-fifo/axis-fifo.c, in line 386 and 529, the
> > last argument is indented as if it were an argument of the second
> > argument. Remove tabs to align the arguments.
> > 
> > Signed-off-by: Khadija Kamran <kamrankhadijadj@gmail.com>
> > ---
> > Changes in v3:
> >  - Do not align the line 530 since it is not part of the last argument.
> > 
> >  drivers/staging/axis-fifo/axis-fifo.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> > index dfd2b357f484..b119cec25a60 100644
> > --- a/drivers/staging/axis-fifo/axis-fifo.c
> > +++ b/drivers/staging/axis-fifo/axis-fifo.c
> > @@ -384,9 +384,9 @@ static ssize_t axis_fifo_read(struct file *f, char __user *buf,
> >  		mutex_lock(&fifo->read_lock);
> >  		ret = wait_event_interruptible_timeout(fifo->read_queue,
> >  			ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> > -				 (read_timeout >= 0) ?
> > -				  msecs_to_jiffies(read_timeout) :
> > -				  MAX_SCHEDULE_TIMEOUT);
> > +			(read_timeout >= 0) ?
> > +			msecs_to_jiffies(read_timeout) :
> > +			MAX_SCHEDULE_TIMEOUT);
> 
> People have been trying to "polish" this mess for a long time, and I
> think it's better to step back and see what is really needed here.
> 
> There is a module parameter, read_timeout, that can only be set at
> loading time.  As it can only be modified once, why are we doing an if
> statement each and every time it is read from?
> 
> Instead, in the module probe function, how about doing something like:
> 	if (read_timeout >= 0)
> 		read_timeout = msecs_to_jiffies(read_timeout);
> 	else
> 		read_timeout = MAX_SCHEDULE_TIMEOUT;
>


Hi Greg! 
Thank you for the reply. Before sending the patch I just wanted to
confirm if I have understood this right. Should I write the above
mentioned code before the wait_event_interruptible_timeout() call, and
pass read_timeout as the last argument to wait_event_interruptible()?
And same for write_timeout.


> and then only ever use "read_timeout" here in the
> wait_event_interruptiable() call?  That should simplify this much more
> overall, and hopefully allow us to just get rid of the module parameter
> eventually as that's not how drivers should be working at all anymore.
> 
> Same goes for write_timeout.
> 
> Overall the code should be much simpler and easier to understand, which
> is the end goal here.
> 
> Can you try doing that instead?
> 
> thanks,
> 
> greg k-h