Re: [PATCH net-next 03/15] net: dsa: mv88e6xxx: Replace usage of found with dedicated iterator

[Vladimir Oltean]

Hi Jakob,

On Thu, Apr 07, 2022 at 12:28:48PM +0200, 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@xxxxxxxxxxxxxx/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@xxxxxxxxx>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 64f4fdd02902..f254f537c357 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1381,28 +1381,27 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds, int port,
>  /* Mask of the local ports allowed to receive frames from a given fabric port */
>  static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  {
> +	struct dsa_port *dp = NULL, *iter, *other_dp;
>  	struct dsa_switch *ds = chip->ds;
>  	struct dsa_switch_tree *dst = ds->dst;
> -	struct dsa_port *dp, *other_dp;
> -	bool found = false;
>  	u16 pvlan;
>  
>  	/* dev is a physical switch */
>  	if (dev <= dst->last_switch) {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			if (dp->ds->index == dev && dp->index == port) {
> -				/* dp might be a DSA link or a user port, so it
> +		list_for_each_entry(iter, &dst->ports, list) {
> +			if (iter->ds->index == dev && iter->index == port) {
> +				/* iter might be a DSA link or a user port, so it
>  				 * might or might not have a bridge.
> -				 * Use the "found" variable for both cases.
> +				 * Set the "dp" variable for both cases.
>  				 */
> -				found = true;
> +				dp = iter;
>  				break;
>  			}
>  		}
>  	/* dev is a virtual bridge */
>  	} else {
> -		list_for_each_entry(dp, &dst->ports, list) {
> -			unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> +		list_for_each_entry(iter, &dst->ports, list) {
> +			unsigned int bridge_num = dsa_port_bridge_num_get(iter);
>  
>  			if (!bridge_num)
>  				continue;
> @@ -1410,13 +1409,13 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
>  			if (bridge_num + dst->last_switch != dev)
>  				continue;
>  
> -			found = true;
> +			dp = iter;
>  			break;
>  		}
>  	}
>  
>  	/* Prevent frames from unknown switch or virtual bridge */
> -	if (!found)
> +	if (!dp)
>  		return 0;
>  
>  	/* Frames from DSA links and CPU ports can egress any local port */
> -- 
> 2.25.1
> 

Let's try to not make convoluted code worse. Do the following 2 patches
achieve what you are looking for? Originally I had a single patch (what
is now 2/2) but I figured it would be cleaner to break out the unrelated
change into what is now 1/2.

If you want I can submit these changes separately.

-----------------------------[ cut here ]-----------------------------