Re: [RFC PATCH net-next 1/6] net: dsa: make dp->bridge_num one-based

On 11/11/21 13:45, Vladimir Oltean wrote:

On Thu, Nov 11, 2021 at 12:24:47PM +0000, Alvin Šipraga wrote:

quoted

On 10/26/21 18:26, Vladimir Oltean wrote:

quoted

I have seen too many bugs already due to the fact that we must encode an
invalid dp->bridge_num as a negative value, because the natural tendency
is to check that invalid value using (!dp->bridge_num). Latest example
can be seen in commit 1bec0f05062c ("net: dsa: fix bridge_num not
getting cleared after ports leaving the bridge").

Convert the existing users to assume that dp->bridge_num == 0 is the
encoding for invalid, and valid bridge numbers start from 1.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---

Reviewed-by: Alvin Šipraga <redacted>

Thanks for the review.

quoted

Small remark inline.

quoted

-int dsa_bridge_num_get(const struct net_device *bridge_dev, int max)
+unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max)
   {
-	int bridge_num = dsa_bridge_num_find(bridge_dev);
+	unsigned int bridge_num = dsa_bridge_num_find(bridge_dev);
   
-	if (bridge_num < 0) {
+	if (!bridge_num) {
   		/* First port that offloads TX forwarding for this bridge */

Perhaps you want to update this comment in patch 2/6, since bridge_num
is no longer just about TX forwarding offload.

quoted

-		bridge_num = find_first_zero_bit(&dsa_fwd_offloading_bridges,
-						 DSA_MAX_NUM_OFFLOADING_BRIDGES);
+		bridge_num = find_next_zero_bit(&dsa_fwd_offloading_bridges,
+						DSA_MAX_NUM_OFFLOADING_BRIDGES,
+						1);

I will update this comment in patch 2 to say "First port that requests
FDB isolation or TX forwarding offload for this bridge". Sounds ok?

Yes sounds good - that's also what I had in mind.