Re: [Outreachy kernel] [Outreachy] [PATCH 2/2] staging: rtl8188eu: Line over 80 characters

[Soumyajit Deb <debsoumyajit100@gmail.com>]

On Tue, Mar 31, 2020 at 04:49:47PM +0200, Stefano Brivio wrote:
> On Tue, 31 Mar 2020 20:04:02 +0530
> Soumyajit Deb <debsoumyajit100@gmail.com> wrote:
> 
> > On Mon, Mar 30, 2020 at 08:17:01PM +0200, Stefano Brivio wrote:
> > > On Mon, 30 Mar 2020 13:50:36 +0530
> > > Soumyajit Deb <debsoumyajit100@gmail.com> wrote:
> > >   
> > > > Break various lines into multiple lines to respect the 80 character
> > > > width limit.
> > > > Reported by checkpatch.pl
> > > > 
> > > > Signed-off-by: Soumyajit Deb <debsoumyajit100@gmail.com>
> > > > ---
> > > >  drivers/staging/rtl8188eu/core/rtw_ap.c | 62 +++++++++++++++++--------
> > > >  1 file changed, 42 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/rtl8188eu/core/rtw_ap.c b/drivers/staging/rtl8188eu/core/rtw_ap.c
> > > > index 5b2b4d1d56f6..94cfd45e3eaa 100644
> > > > --- a/drivers/staging/rtl8188eu/core/rtw_ap.c
> > > > +++ b/drivers/staging/rtl8188eu/core/rtw_ap.c
> > > > @@ -264,8 +264,10 @@ void expire_timeout_chk(struct adapter *padapter)
> > > >  			list_del_init(&psta->asoc_list);
> > > >  			pstapriv->asoc_list_cnt--;
> > > >  
> > > > -			DBG_88E("asoc expire %pM, state = 0x%x\n", (psta->hwaddr), psta->state);
> > > > -			updated = ap_free_sta(padapter, psta, true, WLAN_REASON_DEAUTH_LEAVING);
> > > > +			DBG_88E("asoc expire %pM, state = 0x%x\n",
> > > > +				(psta->hwaddr), psta->state);
> > > > +			updated = ap_free_sta(padapter, psta, true,
> > > > +					      WLAN_REASON_DEAUTH_LEAVING);
> > > >  		} else {
> > > >  			/* TODO: Aging mechanism to digest frames in sleep_q to avoid running out of xmitframe */
> > > >  			if (psta->sleepq_len > (NR_XMITFRAME / pstapriv->asoc_list_cnt) &&
> > > > @@ -294,16 +296,20 @@ void expire_timeout_chk(struct adapter *padapter)
> > > >  		for (i = 0; i < chk_alive_num; i++) {
> > > >  			int ret = _FAIL;
> > > >  
> > > > -			psta = rtw_get_stainfo_by_offset(pstapriv, chk_alive_list[i]);
> > > > +			psta = rtw_get_stainfo_by_offset(pstapriv,
> > > > +							 chk_alive_list[i]);
> > > >  
> > > >  			if (psta->state & WIFI_SLEEP_STATE)
> > > > -				ret = issue_nulldata(padapter, psta->hwaddr, 0, 1, 50);
> > > > +				ret = issue_nulldata(padapter, psta->hwaddr, 0,
> > > > +						     1, 50);
> > > >  			else
> > > > -				ret = issue_nulldata(padapter, psta->hwaddr, 0, 3, 50);
> > > > +				ret = issue_nulldata(padapter, psta->hwaddr, 0,
> > > > +						     3, 50);  
> > > 
> > > For both clauses, here, you need curly brackets, checkpatch.pl should
> > > report. Not because they are functionally needed, rather because they
> > > are visually helpful to group the statements and avoid that a second
> > > statement is inadvertently added without curly brackets.
> > >  
> > 
> > Okay, yes I will add the curly brackets to the clauses and add them as
> > new patches. 
> 
> No, wait, the curly brackets are needed as soon as you split those
> lines. They need to be in the same patch.
> 
> > Should I add this new patch to the v2 of this patchset or should I send
> > as a seperate new patch?
> 
> Yes, v2 of this patchset, but for this change specifically, it's not a
> new patch.
> 
> > [...]
> >
> > > > @@ -657,7 +669,10 @@ static void start_bss_network(struct adapter *padapter, u8 *pbuf)
> > > >  		Switch_DM_Func(padapter, DYNAMIC_ALL_FUNC_ENABLE, true);
> > > >  	}
> > > >  	/* set channel, bwmode */
> > > > -	p = rtw_get_ie((pnetwork->ies + sizeof(struct ndis_802_11_fixed_ie)), _HT_ADD_INFO_IE_, &ie_len, (pnetwork->ie_length - sizeof(struct ndis_802_11_fixed_ie)));
> > > > +	p = rtw_get_ie((pnetwork->ies + sizeof(struct ndis_802_11_fixed_ie)),
> > > > +		       HT_ADD_INFO_IE_, &ie_len,
> > > > +		       (pnetwork->ie_length -
> > > > +			sizeof(struct ndis_802_11_fixed_ie)));  
> > > 
> > > This looks correct, but still it's horrible, all those excess
> > > parentheses make it quite hard to read. If you plan to remove them,
> > > perhaps it's easier to have a change doing that first, in the same
> > > series, so that this change can be checked more easily.
> > 
> > Okay, I will remove the extra unnecessary parentheses.
> 
> Just for clarity: that needs to be done in a separate patch, in the
> same series.
> 
> > > >  	if (p && ie_len) {
> > > >  		pht_info = (struct HT_info_element *)(p + 2);
> > > >  
> > > > @@ -682,7 +697,8 @@ static void start_bss_network(struct adapter *padapter, u8 *pbuf)
> > > >  	 */
> > > >  	set_channel_bwmode(padapter, cur_channel, cur_ch_offset, cur_bwmode);
> > > >  
> > > > -	DBG_88E("CH =%d, BW =%d, offset =%d\n", cur_channel, cur_bwmode, cur_ch_offset);
> > > > +	DBG_88E("CH =%d, BW =%d, offset =%d\n", cur_channel, cur_bwmode,
> > > > +		cur_ch_offset);
> > > >  
> > > >  	/*  */
> > > >  	pmlmeext->cur_channel = cur_channel;
> > > > @@ -771,7 +787,8 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
> > > >  	cap = get_unaligned_le16(ie);
> > > >  
> > > >  	/* SSID */
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _SSID_IE_, &ie_len, (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _SSID_IE_, &ie_len,
> > > > +		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > >  	if (p && ie_len > 0) {
> > > >  		memset(&pbss_network->ssid, 0, sizeof(struct ndis_802_11_ssid));
> > > >  		memcpy(pbss_network->ssid.ssid, (p + 2), ie_len);
> > > > @@ -781,7 +798,8 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
> > > >  	/* channel */
> > > >  	channel = 0;
> > > >  	pbss_network->Configuration.Length = 0;
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _DSSET_IE_, &ie_len, (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _DSSET_IE_, &ie_len,
> > > > +		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > >  	if (p && ie_len > 0)
> > > >  		channel = *(p + 2);
> > > >  
> > > > @@ -789,14 +807,16 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
> > > >  
> > > >  	memset(supportRate, 0, NDIS_802_11_LENGTH_RATES_EX);
> > > >  	/*  get supported rates */
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _SUPPORTEDRATES_IE_, &ie_len, (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _SUPPORTEDRATES_IE_, &ie_len,
> > > > +		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > >  	if (p) {
> > > >  		memcpy(supportRate, p + 2, ie_len);
> > > >  		supportRateNum = ie_len;
> > > >  	}
> > > >  
> > > >  	/* get ext_supported rates */
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _EXT_SUPPORTEDRATES_IE_, &ie_len, pbss_network->ie_length - _BEACON_IE_OFFSET_);
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _EXT_SUPPORTEDRATES_IE_,
> > > > +		       &ie_len, pbss_network->ie_length - _BEACON_IE_OFFSET_);
> > > >  	if (p) {
> > > >  		memcpy(supportRate + supportRateNum, p + 2, ie_len);
> > > >  		supportRateNum += ie_len;
> > > > @@ -807,7 +827,8 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
> > > >  	rtw_set_supported_rate(pbss_network->SupportedRates, network_type);
> > > >  
> > > >  	/* parsing ERP_IE */
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _ERPINFO_IE_, &ie_len, (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _ERPINFO_IE_, &ie_len,
> > > > +		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > >  	if (p && ie_len > 0)
> > > >  		ERP_IE_handler(padapter, (struct ndis_802_11_var_ie *)p);
> > > >  
> > > > @@ -824,7 +845,8 @@ int rtw_check_beacon_data(struct adapter *padapter, u8 *pbuf,  int len)
> > > >  	pairwise_cipher = 0;
> > > >  	psecuritypriv->wpa2_group_cipher = _NO_PRIVACY_;
> > > >  	psecuritypriv->wpa2_pairwise_cipher = _NO_PRIVACY_;
> > > > -	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _RSN_IE_2_, &ie_len, (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > > +	p = rtw_get_ie(ie + _BEACON_IE_OFFSET_, _RSN_IE_2_, &ie_len,
> > > > +		       (pbss_network->ie_length - _BEACON_IE_OFFSET_));
> > > >  	if (p && ie_len > 0) {
> > > >  		if (rtw_parse_wpa2_ie(p, ie_len + 2, &group_cipher, &pairwise_cipher, NULL) == _SUCCESS) {
> > > >  			psecuritypriv->dot11AuthAlgrthm = dot11AuthAlgrthm_8021X;  
> > > 
> > > Same also for all these hunks.
> > >   
> > 
> > I did not understand what you mean by same for all these hunks.
> > Do you mean I should remove extra unnecessary parentheses from these
> > line too as suggested earlier?
> 
> Yes, see https://en.wiktionary.org/wiki/hunk#Noun in the computing
> sense.
> 
> > Should I add these new chnages as new patches to the v2 of this patchset
> > or should I send a altogether new patchset?
> 
> Some of it (removing ()) is a new patch that needs to be added as v2 of
> this patchset (also called "series"). Some of it are new versions of the
> patch you already posted. You need anyway to re-send the full patchset.
> That is:
> 
> - the current patchset has:
> 	- patch 1/2
> 	- patch 2/2
> 
> - the new patchset (v2) will have:
> 	- patch 1/3 v2 (no changes)
> 	- patch 2/3 v2 (for example, if you remove () here)
> 	- patch 3/3 v2 (this current patch with the changes I suggested)
> 
> -- 
> Stefano
>

Thank you, Stefano
I will make the changes you suggested and will add them in  v2 of this
patch series.

--
Soumyajit