netdev - Re: [PATCH v4 net-next 3/5] net: dsa: Link aggregation support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Dec 2020 20:47:59 +0100
From:   Tobias Waldekranz <tobias@...dekranz.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        j.vosburgh@...il.com, vfalico@...il.com, andy@...yhouse.net,
        netdev@...r.kernel.org
Subject: Re: [PATCH v4 net-next 3/5] net: dsa: Link aggregation support

On Wed, Dec 16, 2020 at 18:22, Vladimir Oltean <olteanv@...il.com> wrote:
> On Wed, Dec 16, 2020 at 05:00:54PM +0100, Tobias Waldekranz wrote:
>> Monitor the following events and notify the driver when:
>> 
>> - A DSA port joins/leaves a LAG.
>> - A LAG, made up of DSA ports, joins/leaves a bridge.
>> - A DSA port in a LAG is enabled/disabled (enabled meaning
>>   "distributing" in 802.3ad LACP terms).
>> 
>> When a LAG joins a bridge, the DSA subsystem will treat that as each
>> individual port joining the bridge. The driver may look at the port's
>> LAG device pointer to see if it is associated with any LAG, if that is
>> required. This is analogue to how switchdev events are replicated out
>> to all lower devices when reaching e.g. a LAG.
>> 
>> Drivers can optionally request that DSA maintain a linear mapping from
>> a LAG ID to the corresponding netdev by setting ds->num_lag_ids to the
>> desired size.
>> 
>> In the event that the hardware is not capable of offloading a
>> particular LAG for any reason (the typical case being use of exotic
>> modes like broadcast), DSA will take a hands-off approach, allowing
>> the LAG to be formed as a pure software construct. This is reported
>> back through the extended ACK, but is otherwise transparent to the
>> user.
>> 
>> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
>> ---
>> 
>> I tried in vain to win checkpatch's approval of the foreach macros, no
>> matter how many parentheses I added. Looking at existing macros, this
>> style seems to be widely accepted. Is this a known issue?
>>
>>  include/net/dsa.h  | 60 +++++++++++++++++++++++++++++++++++
>>  net/dsa/dsa2.c     | 74 +++++++++++++++++++++++++++++++++++++++++++
>>  net/dsa/dsa_priv.h | 36 +++++++++++++++++++++
>>  net/dsa/port.c     | 79 ++++++++++++++++++++++++++++++++++++++++++++++
>>  net/dsa/slave.c    | 70 ++++++++++++++++++++++++++++++++++++----
>>  net/dsa/switch.c   | 50 +++++++++++++++++++++++++++++
>>  6 files changed, 362 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 4e60d2610f20..9092c711a37c 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -149,8 +149,41 @@ struct dsa_switch_tree {
>>  
>>  	/* List of DSA links composing the routing table */
>>  	struct list_head rtable;
>> +
>> +	/* Maps offloaded LAG netdevs to a zero-based linear ID for
>> +	 * drivers that need it.
>> +	 */
>> +	struct net_device **lags;
>> +	unsigned int lags_len;
>>  };
>>  
>> +#define dsa_lags_foreach_id(_id, _dst)				\
>> +	for ((_id) = 0; (_id) < (_dst)->lags_len; (_id)++)	\
>> +		if ((_dst)->lags[_id])
>> +
>> +#define dsa_lag_foreach_port(_dp, _dst, _lag)	       \
>> +	list_for_each_entry(_dp, &(_dst)->ports, list) \
>> +		if ((_dp)->lag_dev == (_lag))
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #86: FILE: include/net/dsa.h:160:
> +#define dsa_lags_foreach_id(_id, _dst)                         \
> +       for ((_id) = 0; (_id) < (_dst)->lags_len; (_id)++)      \
> +               if ((_dst)->lags[_id])
>                                  ~~~
>                                  missing parentheses
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #90: FILE: include/net/dsa.h:164:
> +#define dsa_lag_foreach_port(_dp, _dst, _lag)         \
> +       list_for_each_entry(_dp, &(_dst)->ports, list) \
>                             ~~~
>                             missing parentheses
> +               if ((_dp)->lag_dev == (_lag))

Please see my comment just before the diffstat. I tried adding these
parentheses, and about a gagillion other ones. But no matter what I did
I could not make checkpatch happy.

This is where I gave up:

ERROR: Macros with complex values should be enclosed in parentheses
#160: FILE: include/net/dsa.h:160:
+#define dsa_lags_foreach_id(_id, _dst)                         \
+       for (((_id) = 0); ((_id) < ((_dst)->lags_len)); ((_id)++))      \
+               if ((_dst)->lags[(_id)])

ERROR: Macros with complex values should be enclosed in parentheses
#164: FILE: include/net/dsa.h:164:
+#define dsa_lag_foreach_port(_dp, _dst, _lag)         \
+       list_for_each_entry((_dp), &((_dst)->ports), list)      \
+               if (((_dp)->lag_dev) == (_lag))