From: Soumyajit Deb <debsoumyajit100@gmail.com>
To: Stefano Brivio <sbrivio@redhat.com>, outreachy-kernel@googlegroups.com
Subject: Re: [Outreachy kernel] [Outreachy] [PATCH 2/2] staging: rtl8188eu: Line over 80 characters
Date: Tue, 31 Mar 2020 20:04:02 +0530 [thread overview]
Message-ID: <20200331143356.GA12680@ubuntu> (raw)
In-Reply-To: <20200330201701.719b8c0f@elisabeth>
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.
Should I add this new patch to the v2 of this patchset or should I send
as a seperate new patch?
> >
> > psta->keep_alive_trycnt++;
> > if (ret == _SUCCESS) {
> > - DBG_88E("asoc check, sta(%pM) is alive\n", (psta->hwaddr));
> > + DBG_88E("asoc check, sta(%pM) is alive\n",
> > + (psta->hwaddr));
>
> Further cleanup (unrelated with this patch): excess parentheses.
>
> > psta->expire_to = pstapriv->expire_to;
> > psta->keep_alive_trycnt = 0;
> > continue;
> > @@ -315,11 +321,13 @@ void expire_timeout_chk(struct adapter *padapter)
> >
> > psta->keep_alive_trycnt = 0;
> >
> > - DBG_88E("asoc expire %pM, state = 0x%x\n", (psta->hwaddr), psta->state);
> > + DBG_88E("asoc expire %pM, state = 0x%x\n",
> > + (psta->hwaddr), psta->state);
> > spin_lock_bh(&pstapriv->asoc_list_lock);
> > list_del_init(&psta->asoc_list);
> > pstapriv->asoc_list_cnt--;
> > - updated = ap_free_sta(padapter, psta, true, WLAN_REASON_DEAUTH_LEAVING);
> > + updated = ap_free_sta(padapter, psta, true,
> > + WLAN_REASON_DEAUTH_LEAVING);
> > spin_unlock_bh(&pstapriv->asoc_list_lock);
> > }
> >
> > @@ -431,7 +439,8 @@ static void update_bmc_sta(struct adapter *padapter)
> > supportRateNum = rtw_get_rateset_len((u8 *)&pcur_network->SupportedRates);
> > network_type = rtw_check_network_type((u8 *)&pcur_network->SupportedRates);
> >
> > - memcpy(psta->bssrateset, &pcur_network->SupportedRates, supportRateNum);
> > + memcpy(psta->bssrateset, &pcur_network->SupportedRates,
> > + supportRateNum);
> > psta->bssratelen = supportRateNum;
> >
> > /* b/g mode ra_bitmap */
> > @@ -445,7 +454,8 @@ static void update_bmc_sta(struct adapter *padapter)
> > tx_ra_bitmap = 0xf;
> >
> > raid = networktype_to_raid(network_type);
> > - init_rate = get_highest_rate_idx(tx_ra_bitmap & 0x0fffffff) & 0x3f;
> > + init_rate = get_highest_rate_idx(tx_ra_bitmap & 0x0fffffff) &
> > + 0x3f;
>
> This is misleading, it looks like 0x3f somehow belongs to the argument
> of get_highest_rate_idx(). It shouldn't be there, it should be aligned
> under the start of the first & operand.
>
Okay, I will do that. To be honest, I didn't notice that earlier but now
you have mentioned it, I can see the confusion current alignment is
creating.
> >
> > /* ap mode */
> > rtw_hal_set_odm_var(padapter, HAL_ODM_STA_INFO, psta, true);
> > @@ -456,7 +466,8 @@ static void update_bmc_sta(struct adapter *padapter)
> > arg = psta->mac_id & 0x1f;
> > arg |= BIT(7);
> > tx_ra_bitmap |= ((raid << 28) & 0xf0000000);
> > - DBG_88E("%s, mask = 0x%x, arg = 0x%x\n", __func__, tx_ra_bitmap, arg);
> > + DBG_88E("%s, mask = 0x%x, arg = 0x%x\n", __func__,
> > + tx_ra_bitmap, arg);
> >
> > /* bitmap[0:27] = tx_rate_bitmap */
> > /* bitmap[28:31]= Rate Adaptive id */
> > @@ -647,7 +658,8 @@ static void start_bss_network(struct adapter *padapter, u8 *pbuf)
> > rtw_hal_set_hwreg(padapter, HW_VAR_SEC_CFG, (u8 *)(&val8));
> >
> > /* Beacon Control related register */
> > - rtw_hal_set_hwreg(padapter, HW_VAR_BEACON_INTERVAL, (u8 *)(&bcn_interval));
> > + rtw_hal_set_hwreg(padapter, HW_VAR_BEACON_INTERVAL,
> > + (u8 *)(&bcn_interval));
> >
> > UpdateBrateTbl(padapter, pnetwork->SupportedRates);
> > rtw_hal_set_hwreg(padapter, HW_VAR_BASIC_RATE, pnetwork->SupportedRates);
> > @@ -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.
> > 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?
> --
> Stefano
>
Should I add these new chnages as new patches to the v2 of this patchset
or should I send a altogether new patchset?
Thank you
next prev parent reply other threads:[~2020-03-31 14:34 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 8:20 [Outreachy] [PATCH 0/2] staging: rtl8188eu: Resolve various warnings reported by checkpatch.pl Soumyajit Deb
2020-03-30 8:20 ` [Outreachy] [PATCH 1/2] staging: rtl8188eu: Properly structure the multiline comment Soumyajit Deb
2020-03-30 8:20 ` [Outreachy] [PATCH 2/2] staging: rtl8188eu: Line over 80 characters Soumyajit Deb
2020-03-30 18:17 ` [Outreachy kernel] " Stefano Brivio
2020-03-31 14:34 ` Soumyajit Deb [this message]
2020-03-31 14:35 ` Julia Lawall
2020-03-31 14:49 ` Soumyajit Deb
2020-03-31 14:49 ` Stefano Brivio
2020-03-31 15:06 ` Soumyajit Deb
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200331143356.GA12680@ubuntu \
--to=debsoumyajit100@gmail.com \
--cc=outreachy-kernel@googlegroups.com \
--cc=sbrivio@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.