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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZAxzfiOoW0SfW5qD@kroah.com>
Date:   Sat, 11 Mar 2023 13:26:38 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Khadija Kamran <kamrankhadijadj@...il.com>
Cc:     outreachy@...ts.linux.dev, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] staging: axis-fifo: remove tabs to align arguments

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