[PATCH v2] staging: wlan-ng: Fix third argument going over 80 characters

[PATCH v2] staging: wlan-ng: Fix third argument going over 80 characters

[John B. Wyatt IV]

Create a new 'status' variable to store the value of a long argument
that goes over 80 characters. The status variable is also used for
an if check. Replacing that long statement in both places makes the
code much easier to read.

Note: the status variable is assigned after a needed byte order
conversion for usbin->rxfrm.desc.status, which uses a reference.

Issue reported by checkpatch.

Suggested-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
---
v2: Resubmitting after git rebase and comments from Julia Lawall 
<julia.lawall@inria.fr>

 drivers/staging/wlan-ng/hfa384x_usb.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
index f8485601aead..9016e6b32efc 100644
--- a/drivers/staging/wlan-ng/hfa384x_usb.c
+++ b/drivers/staging/wlan-ng/hfa384x_usb.c
@@ -3252,15 +3252,16 @@ static void hfa384x_usbin_rx(struct wlandevice *wlandev, struct sk_buff *skb)
 	struct hfa384x *hw = wlandev->priv;
 	int hdrlen;
 	struct p80211_rxmeta *rxmeta;
-	u16 data_len;
-	u16 fc;
+	u16 data_len, fc, status;
 
 	/* Byte order convert once up front. */
 	le16_to_cpus(&usbin->rxfrm.desc.status);
 	le32_to_cpus(&usbin->rxfrm.desc.time);
 
 	/* Now handle frame based on port# */
-	switch (HFA384x_RXSTATUS_MACPORT_GET(usbin->rxfrm.desc.status)) {
+	status = HFA384x_RXSTATUS_MACPORT_GET(usbin->rxfrm.desc.status);
+
+	switch (status) {
 	case 0:
 		fc = le16_to_cpu(usbin->rxfrm.desc.frame_control);
 
@@ -3319,7 +3320,7 @@ static void hfa384x_usbin_rx(struct wlandevice *wlandev, struct sk_buff *skb)
 	default:
 		netdev_warn(hw->wlandev->netdev,
 			    "Received frame on unsupported port=%d\n",
-			    HFA384x_RXSTATUS_MACPORT_GET(usbin->rxfrm.desc.status));
+			    status);
 		break;
 	}
 }
-- 
2.25.1

Re: [PATCH v2] staging: wlan-ng: Fix third argument going over 80 characters

[Greg Kroah-Hartman]

On Fri, Mar 20, 2020 at 06:39:05PM -0700, John B. Wyatt IV wrote:
> Create a new 'status' variable to store the value of a long argument
> that goes over 80 characters. The status variable is also used for
> an if check. Replacing that long statement in both places makes the
> code much easier to read.
> 
> Note: the status variable is assigned after a needed byte order
> conversion for usbin->rxfrm.desc.status, which uses a reference.
> 
> Issue reported by checkpatch.
> 
> Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
> ---
> v2: Resubmitting after git rebase and comments from Julia Lawall 
> <julia.lawall@inria.fr>
> 
>  drivers/staging/wlan-ng/hfa384x_usb.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> index f8485601aead..9016e6b32efc 100644
> --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> @@ -3252,15 +3252,16 @@ static void hfa384x_usbin_rx(struct wlandevice *wlandev, struct sk_buff *skb)
>  	struct hfa384x *hw = wlandev->priv;
>  	int hdrlen;
>  	struct p80211_rxmeta *rxmeta;
> -	u16 data_len;
> -	u16 fc;
> +	u16 data_len, fc, status;

No need to change data_len, or fc definitions, just add a new line here
instead for status.

thanks,

greg k-h

Re: [Outreachy kernel] Re: [PATCH v2] staging: wlan-ng: Fix third argument going over 80 characters

[Stefano Brivio]

On Sat, 21 Mar 2020 10:34:02 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Mar 20, 2020 at 06:39:05PM -0700, John B. Wyatt IV wrote:
> > Create a new 'status' variable to store the value of a long argument
> > that goes over 80 characters. The status variable is also used for
> > an if check. Replacing that long statement in both places makes the
> > code much easier to read.
> > 
> > Note: the status variable is assigned after a needed byte order
> > conversion for usbin->rxfrm.desc.status, which uses a reference.
> > 
> > Issue reported by checkpatch.
> > 
> > Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
> > ---
> > v2: Resubmitting after git rebase and comments from Julia Lawall 
> > <julia.lawall@inria.fr>
> > 
> >  drivers/staging/wlan-ng/hfa384x_usb.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> > index f8485601aead..9016e6b32efc 100644
> > --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> > +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> > @@ -3252,15 +3252,16 @@ static void hfa384x_usbin_rx(struct wlandevice *wlandev, struct sk_buff *skb)
> >  	struct hfa384x *hw = wlandev->priv;
> >  	int hdrlen;
> >  	struct p80211_rxmeta *rxmeta;
> > -	u16 data_len;
> > -	u16 fc;
> > +	u16 data_len, fc, status;  
> 
> No need to change data_len, or fc definitions, just add a new line here
> instead for status.

My bad, I suggested that John would change that because to me:

	u16 data_len;
	u16 fc;
	u16 status;

looks only slightly worse than:

	u16 data_len;
	u16 fc;

but still it's horrible. If the idea is to avoid unrelated changes,
yeah, I get it :)

-- 
Stefano

Re: [Outreachy kernel] Re: [PATCH v2] staging: wlan-ng: Fix third argument going over 80 characters

[Greg Kroah-Hartman]

On Sat, Mar 21, 2020 at 10:46:04AM +0100, Stefano Brivio wrote:
> On Sat, 21 Mar 2020 10:34:02 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Fri, Mar 20, 2020 at 06:39:05PM -0700, John B. Wyatt IV wrote:
> > > Create a new 'status' variable to store the value of a long argument
> > > that goes over 80 characters. The status variable is also used for
> > > an if check. Replacing that long statement in both places makes the
> > > code much easier to read.
> > > 
> > > Note: the status variable is assigned after a needed byte order
> > > conversion for usbin->rxfrm.desc.status, which uses a reference.
> > > 
> > > Issue reported by checkpatch.
> > > 
> > > Suggested-by: Stefano Brivio <sbrivio@redhat.com>
> > > Signed-off-by: John B. Wyatt IV <jbwyatt4@gmail.com>
> > > ---
> > > v2: Resubmitting after git rebase and comments from Julia Lawall 
> > > <julia.lawall@inria.fr>
> > > 
> > >  drivers/staging/wlan-ng/hfa384x_usb.c | 9 +++++----
> > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> > > index f8485601aead..9016e6b32efc 100644
> > > --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> > > +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> > > @@ -3252,15 +3252,16 @@ static void hfa384x_usbin_rx(struct wlandevice *wlandev, struct sk_buff *skb)
> > >  	struct hfa384x *hw = wlandev->priv;
> > >  	int hdrlen;
> > >  	struct p80211_rxmeta *rxmeta;
> > > -	u16 data_len;
> > > -	u16 fc;
> > > +	u16 data_len, fc, status;  
> > 
> > No need to change data_len, or fc definitions, just add a new line here
> > instead for status.
> 
> My bad, I suggested that John would change that because to me:
> 
> 	u16 data_len;
> 	u16 fc;
> 	u16 status;
> 
> looks only slightly worse than:
> 
> 	u16 data_len;
> 	u16 fc;
> 
> but still it's horrible. If the idea is to avoid unrelated changes,
> yeah, I get it :)

The idea is to avoid unrelated changes :)