| Message ID | 20170822112550.60311-14-ian@mnementh.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 22-08-17 13:25, Ian Molton wrote:
> Whilst this if () statement is technically correct, it lacks clarity.
I don't see the unclarity here. In my opinion people reading the code
should have a good level in C language and a decent level of curiosity
when they come across a function/macro like skb_queue_walk().
Regards,
Arend
On 30/08/17 20:11, Arend van Spriel wrote: >> Whilst this if () statement is technically correct, it lacks clarity. > > I don't see the unclarity here. In my opinion people reading the code > should have a good level in C language and a decent level of curiosity > when they come across a function/macro like skb_queue_walk(). I thought it to be part of the general codingstyle for the kernel that multi-line ifs and elses should be in braces, although I accept that technically the if-clause is a single block-level statement. Having said that, *this* specific example falls into a grey area in the codingstyle, which covers multi-statement, multi-line if() clauses and single-statement, single-line ones. It does not cover single-statement, multi-line examples such as the one here. Whilst I can't therefore definitively justify my position, I can show, for example, line 999 in net/mac80211/iface.c where a for() statement uses braces around the skb_queue_walk() for (i = 0; i < IEEE80211_MAX_QUEUES; i++) { skb_queue_walk_safe(&local->pending[i], skb, tmp) { struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb); if (info->control.vif == &sdata->vif) { __skb_unlink(skb, &local->pending[i]); ieee80211_free_txskb(&local->hw, skb); } } } And the following in ath9k_htc_tx_cleanup_queue() if (process) { skb_queue_walk_safe(&queue, skb, tmp) { __skb_unlink(skb, &queue); ath9k_htc_tx_process(priv, skb, NULL); } } So I feel that we should do the same. -Ian
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index d5d2b2222cc1..257655d7fce4 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -745,16 +745,17 @@ int brcmf_sdiod_send_pkt(struct brcmf_sdio_dev *sdiodev, if (err) return err; - if (pktq->qlen == 1 || !sdiodev->sg_support) + if (pktq->qlen == 1 || !sdiodev->sg_support) { skb_queue_walk(pktq, skb) { err = brcmf_sdiod_buff_write(sdiodev, SDIO_FUNC_2, addr, skb); if (err) break; } - else + } else { err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, true, addr, pktq); + } return err; }
Whilst this if () statement is technically correct, it lacks clarity. Signed-off-by: Ian Molton <ian@mnementh.co.uk> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)