[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 11 Nov 2021 12:45:28 +0000
From: Vladimir Oltean <vladimir.oltean@....com>
To: Alvin Šipraga <ALSI@...g-olufsen.dk>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Tobias Waldekranz <tobias@...dekranz.com>,
DENG Qingfang <dqfext@...il.com>
Subject: Re: [RFC PATCH net-next 1/6] net: dsa: make dp->bridge_num one-based
On Thu, Nov 11, 2021 at 12:24:47PM +0000, Alvin Šipraga wrote:
> On 10/26/21 18:26, Vladimir Oltean wrote:
> > 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@....com>
> > ---
>
> Reviewed-by: Alvin Šipraga <alsi@...g-olufsen.dk>
Thanks for the review.
> Small remark inline.
>
> > -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.
>
> > - 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?
Powered by blists - more mailing lists