issue with set_vlan_mode in starfire driver

From: Colin Ian King
Date: Fri Jul 09 2021 - 09:46:16 EST


Hi,

Static analysis with Coverity on the starfire driver has detected an
issue introduced in function set_vlan_mode with the following commit:

commit 5da96be53a16a62488316810d0c7c5d58ce3ee4f
Author: Jiri Pirko <jpirko@xxxxxxxxxx>
Date: Wed Jul 20 04:54:31 2011 +0000

starfire: do vlan cleanup

The analysis is as follows:

1743 static u32 set_vlan_mode(struct netdev_private *np)
1744 {
1745 u32 ret = VlanMode;
1746 u16 vid;
1747 void __iomem *filter_addr = np->base + HashTable + 8;
1748 int vlan_count = 0;
1749
1750 for_each_set_bit(vid, np->active_vlans, VLAN_N_VID) {
1751 if (vlan_count == 32)
1752 break;
1753 writew(vid, filter_addr);
1754 filter_addr += 16;
1755 vlan_count++;
1756 }

cond_const: Condition vlan_count == 32, taking true branch. Now the
value of vlan_count is equal to 32.

1757 if (vlan_count == 32) {
1758 ret |= PerfectFilterVlan;

const: At condition vlan_count < 32, the value of vlan_count must be
equal to 32.
dead_error_condition: The condition vlan_count < 32 cannot be true.

1759 while (vlan_count < 32) {

Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: writew(0,
filter_addr);.

1760 writew(0, filter_addr);
1761 filter_addr += 16;
1762 vlan_count++;
1763 }
1764 }
1765 return ret;
1766 }

Looking at commit 5da96be53a16a62488316810d0c7c5d58ce3ee4f it appears
that the check if (vlan_count == 32) should be if (vid == VLAN_N_VID) if
I understand things correctly.

However, I'm not sure about the setting of ret |= PerfectFilterVlan -
should that be set if the vlan_count reaches 32 or if vid reaches
VLAN_N_VID. I don't understand the semantics of setting the
PerfectFilterVlan bit so I'm a bit stuck at figuring out an appropriate fix.

Thought had better flag this up as an issue since I can't resolve it.

Colin