Date:   Sat, 11 Mar 2023 19:15:33 +0100 (CET)
From:   Julia Lawall <julia.lawall@inria.fr>
To:     Khadija Kamran <kamrankhadijadj@gmail.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        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
In-Reply-To: <ZAy69T5sYZiyL4TJ@khadija-virtual-machine>
Message-ID: <alpine.DEB.2.22.394.2303111914050.2802@hadrien>
References: <ZAxsyX8n7G92nlJh@khadija-virtual-machine> <ZAxzfiOoW0SfW5qD@kroah.com> <ZAy69T5sYZiyL4TJ@khadija-virtual-machine>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk


On Sat, 11 Mar 2023, Khadija Kamran wrote:

> 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.

Greg suggests to do the initialization of read_timeout only once in the
probe function.  That is the function in which the driver figures out some
information about the environment in which it is running. It is only
called once, at startup time.  Then there will not be this if on every
wait_event_interruptible_timeout call.

julia


>
>
> > 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
>
>