linux-kernel - Re: [PATCH] staging: axis-fifo: alignment should match opening parenthesis in axis-fifo.c

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: <aebfe9ab-3fec-d718-6e7-1cddac2b20a4@inria.fr>
Date:   Tue, 7 Mar 2023 09:07:37 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Dan Carpenter <error27@...il.com>
cc:     Khadija <kamrankhadijadj@...il.com>, outreachy@...ts.linux.dev,
        linux-staging@...ts.linux.dev, gregkh@...uxfoundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: axis-fifo: alignment should match opening
 parenthesis in axis-fifo.c



On Tue, 7 Mar 2023, Dan Carpenter wrote:

> On Tue, Mar 07, 2023 at 08:49:55AM +0100, Julia Lawall wrote:
> >
> >
> > On Tue, 7 Mar 2023, Khadija wrote:
> >
> > > Hey Julia! Thank you for the feedback. I will make the following changes and
> > > resend the patch:
> > > 1. Correct the patch description that is right under the subject line (make
> > > it precise and imperative) and make sure that it does not have more than 70
> > > characters per line.
> > > 2. Adjust all the arguments of wait_event_interruptible_timeout so that they
> > > are lined up. All of them should begin right under ( .
> >
> > The problem here is that the ( is really far to the right.  My opinion is
> > that the position of the second argument (ie the first one that is on a
> > line of its own) is ok in this case.  So you can leave that one where it
> > is and line up the other one.
> >
>
> I kind of like lining things up like this.  I think if you can't align
> things with the parens, then it's nice to at least use two tabs.  It's
> not kernel style or anyone's style explicitly, but I kind of like it.
>
> It doesn't make checkpatch happy.
>
> I guess I probably wouldn't bother sending this patch.  To controversial.
> I'd just move on to something else.  It's not like there is a shortage
> of stuff to do.  One idea in this file is that you could use
> sysfs_emit() in sysfs_read() and get rid of char tmp[32]; buffer.

Dan, the problem is not that the argument is to the left of the (

The problem is that the last argument is indented exactly as though it
were an argument of the second argument.  But it's not.  You have to count
the parentheses to see that.  It's very misleading.

julia

>
> regards,
> dan carpenter
>
> diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
> index dfd2b357f484..0bf180cf44a6 100644
> --- a/drivers/staging/axis-fifo/axis-fifo.c
> +++ b/drivers/staging/axis-fifo/axis-fifo.c
> @@ -383,10 +383,10 @@ 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);
> +				ioread32(fifo->base_addr + XLLF_RDFO_OFFSET),
> +				(read_timeout >= 0) ?
> +					msecs_to_jiffies(read_timeout) :
> +					MAX_SCHEDULE_TIMEOUT);
>
>  		if (ret <= 0) {
>  			if (ret == 0) {
>