linux-kernel - Re: [PATCH] staging: octeon-usb: Fix 'should not end with '(''

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: <8536ca17c2945cb77c506d99eff5ccc42e71f2f3.camel@perches.com>
Date:   Sun, 06 Feb 2022 09:17:45 -0800
From:   Joe Perches <joe@...ches.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "'joelcchangg@...il.com'" <joelcchangg@...il.com>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.com" <gregkh@...uxfoundation.com>
Subject: Re: [PATCH] staging: octeon-usb: Fix 'should not end with '(''

On Sun, 2022-02-06 at 14:16 +0000, David Laight wrote:
> From: joelcchangg@...il.com
> > Sent: 06 February 2022 07:46
> > 
> > This patch fixes the checkpatch.pl check:
> > 
> > CHECK: Line should not end with '('
[]
> > diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
[]
> > @@ -1792,23 +1792,18 @@ static void cvmx_usb_start_channel(struct octeon_hcd *usb, int channel,
> >  	case CVMX_USB_TRANSFER_INTERRUPT:
> >  		break;
> >  	case CVMX_USB_TRANSFER_ISOCHRONOUS:
> > -		if (!cvmx_usb_pipe_needs_split(usb, pipe)) {
> > +		if (!cvmx_usb_pipe_needs_split(usb, pipe) &&
> > +		    pipe->transfer_dir == CVMX_USB_DIRECTION_OUT) {
> >  			/*
> >  			 * ISO transactions require different PIDs depending on
> >  			 * direction and how many packets are needed
> >  			 */
> > -			if (pipe->transfer_dir == CVMX_USB_DIRECTION_OUT) {
> > -				if (pipe->multi_count < 2) /* Need DATA0 */
> > -					USB_SET_FIELD32(
> > -						CVMX_USBCX_HCTSIZX(channel,
> > -								   usb->index),
> > +			if (pipe->multi_count < 2) /* Need DATA0 */
> > +				USB_SET_FIELD32(CVMX_USBCX_HCTSIZX(channel, usb->index),
> >  						cvmx_usbcx_hctsizx, pid, 0);
> > -				else /* Need MDATA */
> > -					USB_SET_FIELD32(
> > -						CVMX_USBCX_HCTSIZX(channel,
> > -								   usb->index),
> > +			else /* Need MDATA */
> > +				USB_SET_FIELD32(CVMX_USBCX_HCTSIZX(channel, usb->index),
> >  						cvmx_usbcx_hctsizx, pid, 3);
> > -			}
> >  		}
> >  		break;
> >  	}
> 
> What has that hunk got to do with the patch description?

Nothing and this should be revisited or explained much more
as it seems to change the logic.

> > @@ -2121,17 +2116,17 @@ static void cvmx_usb_complete(struct octeon_hcd *usb,
> >   *
> >   * Returns: Transaction or NULL on failure.
> >   */
> > -static struct cvmx_usb_transaction *cvmx_usb_submit_transaction(
> > -				struct octeon_hcd *usb,
> > -				struct cvmx_usb_pipe *pipe,
> > -				enum cvmx_usb_transfer type,
> > -				u64 buffer,
> > -				int buffer_length,
> > -				u64 control_header,
> > -				int iso_start_frame,
> > -				int iso_number_packets,
> > -				struct cvmx_usb_iso_packet *iso_packets,
> > -				struct urb *urb)
> > +static struct cvmx_usb_transaction *cvmx_usb_submit_transaction(struct octeon_hcd *usb,
> > +								struct cvmx_usb_pipe *pipe,
> > +								enum cvmx_usb_transfer type,
> > +								u64 buffer,
> > +								int buffer_length,
> > +								u64 control_header,
> > +								int iso_start_frame,
> > +								int iso_number_packets,
> > +								struct cvmx_usb_iso_packet
> > +									*iso_packets,
> > +								struct urb *urb)
> 
> Those lines are now far longer than is 'nice'.

True.

This _might_ be better with the return type on a separate line

Something like:

static struct cvmx_usb_transaction *
cvmx_usb_submit_transaction(struct octeon_hcd *usb, struct cvmx_usb_pipe *pipe,
			    enum cvmx_usb_transfer type,
			    u64 buffer, int buffer_length,
			    u64 control_header, int iso_start_frame,
			    int iso_number_packets,
			    struct cvmx_usb_iso_packet *iso_packets,
			    struct urb *urb)