When the list iterator loop does not exit early the list iterator variable
contains a type-confused pointer to a 'bogus' list element computed based
on the head [1].
Often a 'found' variable is used to ensure the list iterator
variable is only accessed after the loop body if the loop did exit early
(using a break or goto).
In other cases that list iterator variable is used in
combination to access the list member which reverses the invocation of
container_of() and brings back a "safe" pointer to the head of the list.
Since, due to this code patten, there were quite a few bugs discovered [2],
Linus concluded that the rule should be to never use the list iterator
after the loop and introduce a dedicated pointer for that [3].
With the new gnu11 standard, it will now be possible to limit the scope
of the list iterator variable to the traversal loop itself by defining
the variable within the for loop.
This, however, requires to remove all uses of the list iterator after
the loop.
Based on input from Paolo Abeni [4], Vinicius Costa Gomes [5], and
Jakub Kicinski [6], I've splitted all the net-next related changes into
two patch sets, where this is part 1.
v3->v4:
- fix build issue in efx_alloc_rss_context_entry() (Jakub Kicinski)
v2->v3:
- fix commit authors and signed-off order regarding Vladimir's patches
(Sorry about that, wasn't intentional.)
v1->v2:
- Fixed commit message for PATCH 14/18 and used dedicated variable
pointing to the position (Edward Cree)
- Removed redundant check in mv88e6xxx_port_vlan() (Vladimir Oltean)
- Refactor mv88e6xxx_port_vlan() using separate list iterator functions
(Vladimir Oltean)
- Refactor sja1105_insert_gate_entry() to use separate list iterator
functions (Vladimir Oltean)
- Allow early return in sja1105_insert_gate_entry() if
sja1105_first_entry_longer_than() didn't find any element
(Vladimir Oltean)
- Use list_add_tail() instead of list_add() in sja1105_insert_gate_entry()
(Jakub Kicinski)
- net: netcp: also use separate 'pos' variable instead of duplicating list_add()
Link: https://lwn.net/Articles/887097/ [1]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [2]
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [3]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [4]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [5]
Link: https://lore.kernel.org/linux-kernel/[email protected]/ [6]
From: Vladimir Oltean <[email protected]>
When passed a non-head list element, list_add_tail() actually adds the
new element to its left, which is what we want. Despite the slightly
confusing name, use the dedicated function which does the same thing as
the open-coded list_add(pos->prev).
Suggested-by: Jakub Kicinski <[email protected]>
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_vl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index e5ea8eb9ec4e..7fe9b18f1cbd 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -49,7 +49,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
e->rule = rule;
e->gate_state = gate_state;
e->interval = entry_time;
- list_add(&e->list, pos->prev);
+ list_add_tail(&e->list, pos);
gating_cfg->num_entries++;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
.../microchip/sparx5/sparx5_mactable.c | 25 +++++++++----------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index a5837dbe0c7e..bb8d9ce79ac2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -362,8 +362,7 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
unsigned char mac[ETH_ALEN],
u16 vid, u32 cfg2)
{
- struct sparx5_mact_entry *mact_entry;
- bool found = false;
+ struct sparx5_mact_entry *mact_entry = NULL, *iter;
u16 port;
if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_TYPE_GET(cfg2) !=
@@ -378,28 +377,28 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
return;
mutex_lock(&sparx5->mact_lock);
- list_for_each_entry(mact_entry, &sparx5->mact_entries, list) {
- if (mact_entry->vid == vid &&
- ether_addr_equal(mac, mact_entry->mac)) {
- found = true;
- mact_entry->flags |= MAC_ENT_ALIVE;
- if (mact_entry->port != port) {
+ list_for_each_entry(iter, &sparx5->mact_entries, list) {
+ if (iter->vid == vid &&
+ ether_addr_equal(mac, iter->mac)) {
+ iter->flags |= MAC_ENT_ALIVE;
+ if (iter->port != port) {
dev_warn(sparx5->dev, "Entry move: %d -> %d\n",
- mact_entry->port, port);
- mact_entry->port = port;
- mact_entry->flags |= MAC_ENT_MOVED;
+ iter->port, port);
+ iter->port = port;
+ iter->flags |= MAC_ENT_MOVED;
}
/* Entry handled */
+ mact_entry = iter;
break;
}
}
mutex_unlock(&sparx5->mact_lock);
- if (found && !(mact_entry->flags & MAC_ENT_MOVED))
+ if (mact_entry && !(mact_entry->flags & MAC_ENT_MOVED))
/* Present, not moved */
return;
- if (!found) {
+ if (!mact_entry) {
/* Entry not found - now add */
mact_entry = alloc_mact_entry(sparx5, mac, vid, port);
if (!mact_entry)
--
2.25.1
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using &next->list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/ti/netcp_core.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c b/drivers/net/ethernet/ti/netcp_core.c
index 16507bff652a..f25104b5a31b 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -471,6 +471,7 @@ struct netcp_hook_list {
int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = &netcp_priv->txhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -485,10 +486,12 @@ int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
spin_lock_irqsave(&netcp_priv->lock, flags);
list_for_each_entry(next, &netcp_priv->txhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = &next->list;
break;
+ }
}
- __list_add(&entry->list, next->list.prev, &next->list);
+ list_add_tail(&entry->list, pos);
spin_unlock_irqrestore(&netcp_priv->lock, flags);
return 0;
@@ -520,6 +523,7 @@ EXPORT_SYMBOL_GPL(netcp_unregister_txhook);
int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = &netcp_priv->rxhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -534,10 +538,12 @@ int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
spin_lock_irqsave(&netcp_priv->lock, flags);
list_for_each_entry(next, &netcp_priv->rxhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = &next->list;
break;
+ }
}
- __list_add(&entry->list, next->list.prev, &next->list);
+ list_add_tail(&entry->list, pos);
spin_unlock_irqrestore(&netcp_priv->lock, flags);
return 0;
--
2.25.1
From: Vladimir Oltean <[email protected]>
We know that "dev > dst->last_switch" in the "else" block.
In other words, that "dev - dst->last_switch" is > 0.
dsa_port_bridge_num_get(dp) can be 0, but the check
"if (bridge_num + dst->last_switch != dev) continue", rewritten as
"if (bridge_num != dev - dst->last_switch) continue", aka
"if (bridge_num != something which cannot be 0) continue",
makes it redundant to have the extra "if (!bridge_num) continue" logic,
since a bridge_num of zero would have been skipped anyway.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..b3aa0e5bc842 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
list_for_each_entry(dp, &dst->ports, list) {
unsigned int bridge_num = dsa_port_bridge_num_get(dp);
- if (!bridge_num)
- continue;
-
if (bridge_num + dst->last_switch != dev)
continue;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
Since "found" and "p_ent" need to be equal, "found" should be used
consistently to limit the scope of "p_ent" to the list traversal in
the future.
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index d01b9245f811..cbaa2abed660 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -934,10 +934,10 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
u8 fw_return_code,
union event_ring_data *p_data)
{
+ struct qed_spq_entry *found = NULL;
struct qed_spq *p_spq;
- struct qed_spq_entry *p_ent = NULL;
+ struct qed_spq_entry *p_ent;
struct qed_spq_entry *tmp;
- struct qed_spq_entry *found = NULL;
if (!p_hwfn)
return -EINVAL;
@@ -980,7 +980,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
"Complete EQE [echo %04x]: func %p cookie %p)\n",
le16_to_cpu(echo),
- p_ent->comp_cb.function, p_ent->comp_cb.cookie);
+ found->comp_cb.function, found->comp_cb.cookie);
if (found->comp_cb.function)
found->comp_cb.function(p_hwfn, found->comp_cb.cookie, p_data,
fw_return_code);
--
2.25.1
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or skip the iteration
(if the previous iteration was complete) list_prepare_entry() is used.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/team/team.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b07dde6f0abf..688c4393f099 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2425,17 +2425,17 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
int flags, team_nl_send_func_t *send_func,
struct list_head *sel_opt_inst_list)
{
+ struct team_option_inst *opt_inst, *tmp = NULL;
struct nlattr *option_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_option_inst *opt_inst;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- opt_inst = list_first_entry(sel_opt_inst_list,
- struct team_option_inst, tmp_list);
+ tmp = list_first_entry(sel_opt_inst_list,
+ struct team_option_inst, tmp_list);
start_again:
err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2456,7 +2456,9 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
goto nla_put_failure;
i = 0;
+ opt_inst = list_prepare_entry(tmp, sel_opt_inst_list, tmp_list);
incomplete = false;
+ tmp = NULL;
list_for_each_entry_from(opt_inst, sel_opt_inst_list, tmp_list) {
err = team_nl_fill_one_option_get(skb, team, opt_inst);
if (err) {
@@ -2464,6 +2466,7 @@ static int team_nl_send_options_get(struct team *team, u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = opt_inst;
break;
}
goto errout;
@@ -2707,14 +2710,14 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
struct nlattr *port_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_port *port;
+ struct team_port *port, *tmp = NULL;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- port = list_first_entry_or_null(&team->port_list,
- struct team_port, list);
+ tmp = list_first_entry_or_null(&team->port_list,
+ struct team_port, list);
start_again:
err = __send_and_alloc_skb(&skb, team, portid, send_func);
@@ -2744,7 +2747,9 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
err = team_nl_fill_one_port_get(skb, one_port);
if (err)
goto errout;
- } else if (port) {
+ } else {
+ port = list_prepare_entry(tmp, &team->port_list, list);
+ tmp = NULL;
list_for_each_entry_from(port, &team->port_list, list) {
err = team_nl_fill_one_port_get(skb, port);
if (err) {
@@ -2752,6 +2757,7 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = port;
break;
}
goto errout;
--
2.25.1
From: Vladimir Oltean <[email protected]>
The link below explains that there is a desire to syntactically change
list_for_each_entry() and list_for_each() such that it becomes
impossible to use the iterator variable outside the scope of the loop.
Although sja1105_insert_gate_entry() makes legitimate use of the
iterator pointer when it breaks out, the pattern it uses may become
illegal, so it needs to change.
It is deemed acceptable to use a copy of the loop iterator, and
sja1105_insert_gate_entry() only needs to know the list_head element
before which the list insertion should be made. So let's profit from the
occasion and refactor the list iteration to a dedicated function.
An additional benefit is given by the fact that with the helper function
in place, we no longer need to special-case the empty list, since it is
equivalent to not having found any gating entry larger than the
specified interval in the list. We just need to insert at the tail of
that list (list_add vs list_add_tail on an empty list does the same
thing).
Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#24810127
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_vl.c | 46 ++++++++++++++++++----------
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..369be2ac3587 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -7,6 +7,27 @@
#define SJA1105_SIZE_VL_STATUS 8
+static struct list_head *
+sja1105_first_entry_longer_than(struct list_head *entries,
+ s64 interval,
+ struct netlink_ext_ack *extack)
+{
+ struct sja1105_gate_entry *p;
+
+ list_for_each_entry(p, entries, list) {
+ if (p->interval == interval) {
+ NL_SET_ERR_MSG_MOD(extack, "Gate conflict");
+ return ERR_PTR(-EBUSY);
+ }
+
+ if (interval < p->interval)
+ return &p->list;
+ }
+
+ /* Empty list, or specified interval is largest within the list */
+ return entries;
+}
+
/* Insert into the global gate list, sorted by gate action time. */
static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
struct sja1105_rule *rule,
@@ -14,6 +35,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
struct netlink_ext_ack *extack)
{
struct sja1105_gate_entry *e;
+ struct list_head *pos;
int rc;
e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -24,25 +46,15 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
e->gate_state = gate_state;
e->interval = entry_time;
- if (list_empty(&gating_cfg->entries)) {
- list_add(&e->list, &gating_cfg->entries);
- } else {
- struct sja1105_gate_entry *p;
-
- list_for_each_entry(p, &gating_cfg->entries, list) {
- if (p->interval == e->interval) {
- NL_SET_ERR_MSG_MOD(extack,
- "Gate conflict");
- rc = -EBUSY;
- goto err;
- }
-
- if (e->interval < p->interval)
- break;
- }
- list_add(&e->list, p->list.prev);
+ pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
+ e->interval, extack);
+ if (IS_ERR(pos)) {
+ rc = PTR_ERR(pos);
+ goto err;
}
+ list_add(&e->list, pos->prev);
+
gating_cfg->num_entries++;
return 0;
--
2.25.1
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or start a new
iteration (if the previous iteration was complete) list_prepare_entry()
is used.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ipvlan/ipvlan_main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 696e245f6d00..063d7c30e944 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -9,7 +9,7 @@
static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
struct netlink_ext_ack *extack)
{
- struct ipvl_dev *ipvlan;
+ struct ipvl_dev *ipvlan, *tmp = NULL;
unsigned int flags;
int err;
@@ -26,8 +26,10 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
flags & ~IFF_NOARP,
extack);
}
- if (unlikely(err))
+ if (unlikely(err)) {
+ tmp = ipvlan;
goto fail;
+ }
}
if (nval == IPVLAN_MODE_L3S) {
/* New mode is L3S */
@@ -43,6 +45,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
return 0;
fail:
+ ipvlan = list_prepare_entry(tmp, &port->ipvlans, pnode);
/* Undo the flags changes that have been done so far. */
list_for_each_entry_continue_reverse(ipvlan, &port->ipvlans, pnode) {
flags = ipvlan->dev->flags;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 ++++++++++-----------
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 1d1d4caad680..198c9321bf51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -1630,38 +1630,36 @@ static struct qed_iwarp_listener *
qed_iwarp_get_listener(struct qed_hwfn *p_hwfn,
struct qed_iwarp_cm_info *cm_info)
{
- struct qed_iwarp_listener *listener = NULL;
+ struct qed_iwarp_listener *listener = NULL, *iter;
static const u32 ip_zero[4] = { 0, 0, 0, 0 };
- bool found = false;
- list_for_each_entry(listener,
+ list_for_each_entry(iter,
&p_hwfn->p_rdma_info->iwarp.listen_list,
list_entry) {
- if (listener->port == cm_info->local_port) {
- if (!memcmp(listener->ip_addr,
+ if (iter->port == cm_info->local_port) {
+ if (!memcmp(iter->ip_addr,
ip_zero, sizeof(ip_zero))) {
- found = true;
+ listener = iter;
break;
}
- if (!memcmp(listener->ip_addr,
+ if (!memcmp(iter->ip_addr,
cm_info->local_ip,
sizeof(cm_info->local_ip)) &&
- (listener->vlan == cm_info->vlan)) {
- found = true;
+ iter->vlan == cm_info->vlan) {
+ listener = iter;
break;
}
}
}
- if (found) {
+ if (listener)
DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener found = %p\n",
listener);
- return listener;
- }
+ else
+ DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- return NULL;
+ return listener;
}
static int
--
2.25.1
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..3d167e37e654 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
{
struct qede_dev *edev = netdev_priv(dev);
- struct qede_vlan *vlan;
+ struct qede_vlan *vlan = NULL;
+ struct qede_vlan *iter;
int rc = 0;
DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);
/* Find whether entry exists */
__qede_lock(edev);
- list_for_each_entry(vlan, &edev->vlan_list, list)
- if (vlan->vid == vid)
+ list_for_each_entry(iter, &edev->vlan_list, list)
+ if (iter->vid == vid) {
+ vlan = iter;
break;
+ }
- if (list_entry_is_head(vlan, &edev->vlan_list, list)) {
+ if (!vlan) {
DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
"Vlan isn't configured\n");
goto out;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
net/dsa/dsa.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89c6c86e746f..645522c4dd4a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,22 +112,21 @@ const struct dsa_device_ops *dsa_find_tagger_by_name(const char *buf)
const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol)
{
- struct dsa_tag_driver *dsa_tag_driver;
+ struct dsa_tag_driver *dsa_tag_driver = NULL, *iter;
const struct dsa_device_ops *ops;
- bool found = false;
request_module("%s%d", DSA_TAG_DRIVER_ALIAS, tag_protocol);
mutex_lock(&dsa_tag_drivers_lock);
- list_for_each_entry(dsa_tag_driver, &dsa_tag_drivers_list, list) {
- ops = dsa_tag_driver->ops;
+ list_for_each_entry(iter, &dsa_tag_drivers_list, list) {
+ ops = iter->ops;
if (ops->proto == tag_protocol) {
- found = true;
+ dsa_tag_driver = iter;
break;
}
}
- if (found) {
+ if (dsa_tag_driver) {
if (!try_module_get(dsa_tag_driver->owner))
ops = ERR_PTR(-ENOPROTOOPT);
} else {
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/connector/cn_queue.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 996f025eb63c..ed77599b0b25 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -92,20 +92,19 @@ int cn_queue_add_callback(struct cn_queue_dev *dev, const char *name,
void cn_queue_del_callback(struct cn_queue_dev *dev, const struct cb_id *id)
{
- struct cn_callback_entry *cbq, *n;
- int found = 0;
+ struct cn_callback_entry *cbq = NULL, *iter, *n;
spin_lock_bh(&dev->queue_lock);
- list_for_each_entry_safe(cbq, n, &dev->queue_list, callback_entry) {
- if (cn_cb_equal(&cbq->id.id, id)) {
- list_del(&cbq->callback_entry);
- found = 1;
+ list_for_each_entry_safe(iter, n, &dev->queue_list, callback_entry) {
+ if (cn_cb_equal(&iter->id.id, id)) {
+ list_del(&iter->callback_entry);
+ cbq = iter;
break;
}
}
spin_unlock_bh(&dev->queue_lock);
- if (found)
+ if (cbq)
cn_queue_release_callback(cbq);
}
--
2.25.1
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using &new->list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/sfc/rx_common.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
index 1b22c7be0088..716847ba7038 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -556,6 +556,7 @@ efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
{
struct list_head *head = &efx->rss_context.list;
+ struct list_head *pos = head;
struct efx_rss_context *ctx, *new;
u32 id = 1; /* Don't use zero, that refers to the master RSS context */
@@ -563,8 +564,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
/* Search for first gap in the numbering */
list_for_each_entry(ctx, head, list) {
- if (ctx->user_id != id)
+ if (ctx->user_id != id) {
+ pos = &ctx->list;
break;
+ }
id++;
/* Check for wrap. If this happens, we have nearly 2^32
* allocated RSS contexts, which seems unlikely.
@@ -582,7 +585,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
/* Insert the new entry into the gap */
new->user_id = id;
- list_add_tail(&new->list, &ctx->list);
+ list_add_tail(&new->list, pos);
return new;
}
--
2.25.1
From: Vladimir Oltean <[email protected]>
sja1105_first_entry_longer_than() does not make use of the full struct
sja1105_gate_entry *e, just of e->interval which is set from the passed
entry_time.
This means that if there is a gate conflict, we have allocated e for
nothing, just to free it later. Reorder the memory allocation and the
function call, to avoid that and simplify the error unwind path.
Signed-off-by: Vladimir Oltean <[email protected]>
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/dsa/sja1105/sja1105_vl.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c
index 369be2ac3587..e5ea8eb9ec4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
{
struct sja1105_gate_entry *e;
struct list_head *pos;
- int rc;
+
+ pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
+ entry_time, extack);
+ if (IS_ERR(pos))
+ return PTR_ERR(pos);
e = kzalloc(sizeof(*e), GFP_KERNEL);
if (!e)
@@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
e->rule = rule;
e->gate_state = gate_state;
e->interval = entry_time;
-
- pos = sja1105_first_entry_longer_than(&gating_cfg->entries,
- e->interval, extack);
- if (IS_ERR(pos)) {
- rc = PTR_ERR(pos);
- goto err;
- }
-
list_add(&e->list, pos->prev);
gating_cfg->num_entries++;
return 0;
-err:
- kfree(e);
- return rc;
}
/* The gate entries contain absolute times in their e->interval field. Convert
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qede/qede_rdma.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_rdma.c b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
index 6304514a6f2c..2eb03ffe2484 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
@@ -246,18 +246,17 @@ static void qede_rdma_change_mtu(struct qede_dev *edev)
static struct qede_rdma_event_work *
qede_rdma_get_free_event_node(struct qede_dev *edev)
{
- struct qede_rdma_event_work *event_node = NULL;
- bool found = false;
+ struct qede_rdma_event_work *event_node = NULL, *iter;
- list_for_each_entry(event_node, &edev->rdma_info.rdma_event_list,
+ list_for_each_entry(iter, &edev->rdma_info.rdma_event_list,
list) {
- if (!work_pending(&event_node->work)) {
- found = true;
+ if (!work_pending(&iter->work)) {
+ event_node = iter;
break;
}
}
- if (!found) {
+ if (!event_node) {
event_node = kzalloc(sizeof(*event_node), GFP_ATOMIC);
if (!event_node) {
DP_NOTICE(edev,
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 672480c9d195..e920e7dcf66a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -174,7 +174,7 @@ int qed_db_recovery_add(struct qed_dev *cdev,
int qed_db_recovery_del(struct qed_dev *cdev,
void __iomem *db_addr, void *db_data)
{
- struct qed_db_recovery_entry *db_entry = NULL;
+ struct qed_db_recovery_entry *db_entry = NULL, *iter;
struct qed_hwfn *p_hwfn;
int rc = -EINVAL;
@@ -190,12 +190,13 @@ int qed_db_recovery_del(struct qed_dev *cdev,
/* Protect the list */
spin_lock_bh(&p_hwfn->db_recovery_info.lock);
- list_for_each_entry(db_entry,
+ list_for_each_entry(iter,
&p_hwfn->db_recovery_info.list, list_entry) {
/* search according to db_data addr since db_addr is not unique (roce) */
- if (db_entry->db_data == db_data) {
- qed_db_recovery_dp_entry(p_hwfn, db_entry, "Deleting");
- list_del(&db_entry->list_entry);
+ if (iter->db_data == db_data) {
+ qed_db_recovery_dp_entry(p_hwfn, iter, "Deleting");
+ list_del(&iter->list_entry);
+ db_entry = iter;
rc = 0;
break;
}
--
2.25.1
On 4/15/2022 5:29 AM, Jakob Koschel wrote:
> To move the list iterator variable into the list_for_each_entry_*()
> macro in the future it should be avoided to use the list iterator
> variable after the loop body.
>
> To *never* use the list iterator variable after the loop it was
> concluded to use a separate iterator variable instead of a
> found boolean [1].
>
> This removes the need to use a found variable and simply checking if
> the variable was set, can determine if the break/goto was hit.
>
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/15/2022 5:29 AM, Jakob Koschel wrote:
> From: Vladimir Oltean <[email protected]>
>
> The link below explains that there is a desire to syntactically change
> list_for_each_entry() and list_for_each() such that it becomes
> impossible to use the iterator variable outside the scope of the loop.
>
> Although sja1105_insert_gate_entry() makes legitimate use of the
> iterator pointer when it breaks out, the pattern it uses may become
> illegal, so it needs to change.
>
> It is deemed acceptable to use a copy of the loop iterator, and
> sja1105_insert_gate_entry() only needs to know the list_head element
> before which the list insertion should be made. So let's profit from the
> occasion and refactor the list iteration to a dedicated function.
>
> An additional benefit is given by the fact that with the helper function
> in place, we no longer need to special-case the empty list, since it is
> equivalent to not having found any gating entry larger than the
> specified interval in the list. We just need to insert at the tail of
> that list (list_add vs list_add_tail on an empty list does the same
> thing).
>
> Link: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/#24810127
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Jakob Koschel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/15/2022 5:29 AM, Jakob Koschel wrote:
> From: Vladimir Oltean <[email protected]>
>
> When passed a non-head list element, list_add_tail() actually adds the
> new element to its left, which is what we want. Despite the slightly
> confusing name, use the dedicated function which does the same thing as
> the open-coded list_add(pos->prev).
>
> Suggested-by: Jakub Kicinski <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Jakob Koschel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/15/2022 5:29 AM, Jakob Koschel wrote:
> From: Vladimir Oltean <[email protected]>
>
> We know that "dev > dst->last_switch" in the "else" block.
> In other words, that "dev - dst->last_switch" is > 0.
>
> dsa_port_bridge_num_get(dp) can be 0, but the check
> "if (bridge_num + dst->last_switch != dev) continue", rewritten as
> "if (bridge_num != dev - dst->last_switch) continue", aka
> "if (bridge_num != something which cannot be 0) continue",
> makes it redundant to have the extra "if (!bridge_num) continue" logic,
> since a bridge_num of zero would have been skipped anyway.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Jakob Koschel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On 4/15/2022 5:29 AM, Jakob Koschel wrote:
> From: Vladimir Oltean <[email protected]>
>
> sja1105_first_entry_longer_than() does not make use of the full struct
> sja1105_gate_entry *e, just of e->interval which is set from the passed
> entry_time.
>
> This means that if there is a gate conflict, we have allocated e for
> nothing, just to free it later. Reorder the memory allocation and the
> function call, to avoid that and simplify the error unwind path.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> Signed-off-by: Jakob Koschel <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
On Fri, Apr 15, 2022 at 02:29:43PM +0200, Jakob Koschel wrote:
> In preparation to limit the scope of a list iterator to the list
> traversal loop, use a dedicated pointer pointing to the location
> where the element should be inserted [1].
>
> Before, the code implicitly used the head when no element was found
> when using &new->list. The new 'pos' variable is set to the list head
> by default and overwritten if the list exits early, marking the
> insertion point for list_add().
>
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> drivers/net/ethernet/sfc/rx_common.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/sfc/rx_common.c b/drivers/net/ethernet/sfc/rx_common.c
> index 1b22c7be0088..716847ba7038 100644
> --- a/drivers/net/ethernet/sfc/rx_common.c
> +++ b/drivers/net/ethernet/sfc/rx_common.c
> @@ -556,6 +556,7 @@ efx_rx_packet_gro(struct efx_channel *channel, struct efx_rx_buffer *rx_buf,
> struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
> {
> struct list_head *head = &efx->rss_context.list;
> + struct list_head *pos = head;
This violates the reverse Xmas list policy. This definition should be
1 line further down.
Martin
> struct efx_rss_context *ctx, *new;
> u32 id = 1; /* Don't use zero, that refers to the master RSS context */
>
> @@ -563,8 +564,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>
> /* Search for first gap in the numbering */
> list_for_each_entry(ctx, head, list) {
> - if (ctx->user_id != id)
> + if (ctx->user_id != id) {
> + pos = &ctx->list;
> break;
> + }
> id++;
> /* Check for wrap. If this happens, we have nearly 2^32
> * allocated RSS contexts, which seems unlikely.
> @@ -582,7 +585,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
>
> /* Insert the new entry into the gap */
> new->user_id = id;
> - list_add_tail(&new->list, &ctx->list);
> + list_add_tail(&new->list, pos);
> return new;
> }
>
> --
> 2.25.1