[PATCH v3] staging: axis-fifo: remove tabs to align arguments

[PATCH v3] staging: axis-fifo: remove tabs to align arguments

[Khadija Kamran]

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);
 
 		if (ret <= 0) {
 			if (ret == 0) {
@@ -528,9 +528,9 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
 		ret = wait_event_interruptible_timeout(fifo->write_queue,
 			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
 				 >= words_to_write,
-				 (write_timeout >= 0) ?
-				  msecs_to_jiffies(write_timeout) :
-				  MAX_SCHEDULE_TIMEOUT);
+			(write_timeout >= 0) ?
+			msecs_to_jiffies(write_timeout) :
+			MAX_SCHEDULE_TIMEOUT);
 
 		if (ret <= 0) {
 			if (ret == 0) {
-- 
2.34.1

Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments

[Julia Lawall]

On Sat, 11 Mar 2023, 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.

You don't need the file name or the line numbers.  That information is
sufficiently obvious in the patch itself.  On the other hand, it could be
helpful rto point out that these are calls to
wait_event_interruptible_timeout and indeed that it is the timeout
argument that is misplaced.  That would help the reader know what to look
for in reading the patch.

julia

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

You can keep the entire history here.

julia

>  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);
>
>  		if (ret <= 0) {
>  			if (ret == 0) {
> @@ -528,9 +528,9 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
>  		ret = wait_event_interruptible_timeout(fifo->write_queue,
>  			ioread32(fifo->base_addr + XLLF_TDFV_OFFSET)
>  				 >= words_to_write,
> -				 (write_timeout >= 0) ?
> -				  msecs_to_jiffies(write_timeout) :
> -				  MAX_SCHEDULE_TIMEOUT);
> +			(write_timeout >= 0) ?
> +			msecs_to_jiffies(write_timeout) :
> +			MAX_SCHEDULE_TIMEOUT);
>
>  		if (ret <= 0) {
>  			if (ret == 0) {
> --
> 2.34.1
>
>
>

Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments

[Greg Kroah-Hartman]

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;

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

Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments

[Khadija Kamran]

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

Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments

[Julia Lawall]

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