issue with set_vlan_mode in starfire driver

issue with set_vlan_mode in starfire driver

[Colin Ian King]

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@redhat.com>
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