From: Vladimir Oltean <olteanv@gmail.com> To: Hans S <schultz.hans@gmail.com> Cc: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org, Hans Schultz <schultz.hans+netdev@gmail.com>, Andrew Lunn <andrew@lunn.ch>, Vivien Didelot <vivien.didelot@gmail.com>, Florian Fainelli <f.fainelli@gmail.com>, Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, Jiri Pirko <jiri@resnulli.us>, Ivan Vecera <ivecera@redhat.com>, Roopa Prabhu <roopa@nvidia.com>, Nikolay Aleksandrov <razor@blackwall.org>, Shuah Khan <shuah@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Ido Schimmel <idosch@nvidia.com>, linux-kernel@vger.kernel.org, bridge@lists.linux-foundation.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation Date: Wed, 6 Jul 2022 11:55:59 +0300 [thread overview] Message-ID: <20220706085559.oyvzijcikivemfkg@skbuf> (raw) In-Reply-To: <CAKUejP7ugMB9d3MVX3m9Brw12_ocFoT+nuJJucYdQH70kzC7=w@mail.gmail.com> On Tue, Jun 28, 2022 at 02:26:43PM +0200, Hans S wrote: > > Dumb question: if you only flush the locked entries at fast age if the > > port is locked, then what happens with the existing locked entries if > > the port becomes unlocked before an FDB flush takes place? > > Shouldn't mv88e6xxx_port_set_lock() call mv88e6xxx_atu_locked_entry_flush() > > too? > > That was my first thought too, but the way the flags are handled with the mask etc, does so that > mv88e6xxx_port_set_lock() is called when other flags change. It could be done by the transition > from locked->unlocked by checking if the port is locked already. Why does mv88e6xxx_port_set_lock() get called when other flags change? > On the other hand, the timers will timeout and the entries will be removed anyhow. > > > +static void mv88e6xxx_atu_locked_entry_timer_work(struct atu_locked_entry *ale) > > > > Please find a more adequate name for this function. > > Any suggestions? Not sure. It depends on whether you leave just the logic to delete a locked ATU entry, or also the switchdev FDB_DEL_TO_BRIDGE notifier. In any case, pick a name that reflects what it does. Something with locked_entry_delete() can't be too wrong. > > From the discussion with Ido and Nikolay I get the impression that > > you're not doing the right thing here either, notifying a > > SWITCHDEV_FDB_DEL_TO_BRIDGE from what is effectively the > > SWITCHDEV_FDB_DEL_TO_DEVICE handler (port_fdb_del). > > Hmm, my experience tells me that much is opposite the normal > conventions when dealing with > locked ports, as there was never switchdev notifications from the > driver to the bridge before, but > that is needed to keep ATU and FDB entries in sync. On delete you mean? So the bridge signals switchdev a deletion of a locked FDB entry (as I pointed out, this function gets indirectly called from port_fdb_del), but it won't get deleted until switchdev signals it back, is what you're saying? > > Why is the rtnl_unlock() outside the switch statement but the rtnl_lock() inside? > > Not to mention, the dsa_port_to_bridge_port() call needs to be under rtnl_lock(). > > Just a small optimization as I also have another case of the switch > (only one switch case if > you didn't notice) belonging to the next patch set regarding dynamic > ATU entries. What kind of optimization are you even talking about? Please get rid of coding patterns like this, sorry. > > Please, no "if (chiplock) mutex_lock()" hacks. Just lockdep_assert_held(&chip->reg_lock), > > which serves both for documentation and for validation purposes, ensure > > the lock is always taken at the caller (which in this case is super easy) > > and move on. > > As I am calling the function in if statement checks, it would make > that code more messy, while with > this approach the function can be called from anywhere. I also looked > at having two functions, with > one being a wrapper function taking the lock and calling the other... There are many functions in mv88e6xxx that require the reg_lock to be held, there's nothing new or special here. > > > > > + > > > + if (mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, ®)) > > > + goto out; > > > > It would be good to actually propagate the error to the caller and > > "locked" via a pass-by-reference bool pointer argument, not just say > > that I/O errors mean that the port is unlocked. > > Again the wish to be able to call it from if statement checks,. > > > > + reg &= MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK; > > > + if (locked) { > > > + reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1; > > > > I'd suggest aligning these macros vertically. > > They are according to the Linux kernel coding standard wrt indentation afaik. Compare: reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1; with: reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
WARNING: multiple messages have this Message-ID (diff)
From: Vladimir Oltean <olteanv@gmail.com> To: Hans S <schultz.hans@gmail.com> Cc: Ivan Vecera <ivecera@redhat.com>, Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, Jiri Pirko <jiri@resnulli.us>, Daniel Borkmann <daniel@iogearbox.net>, netdev@vger.kernel.org, Nikolay Aleksandrov <razor@blackwall.org>, bridge@lists.linux-foundation.org, Eric Dumazet <edumazet@google.com>, Ido Schimmel <idosch@nvidia.com>, Vivien Didelot <vivien.didelot@gmail.com>, Hans Schultz <schultz.hans+netdev@gmail.com>, linux-kselftest@vger.kernel.org, Roopa Prabhu <roopa@nvidia.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>, "David S. Miller" <davem@davemloft.net>, linux-kernel@vger.kernel.org Subject: Re: [Bridge] [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation Date: Wed, 6 Jul 2022 11:55:59 +0300 [thread overview] Message-ID: <20220706085559.oyvzijcikivemfkg@skbuf> (raw) In-Reply-To: <CAKUejP7ugMB9d3MVX3m9Brw12_ocFoT+nuJJucYdQH70kzC7=w@mail.gmail.com> On Tue, Jun 28, 2022 at 02:26:43PM +0200, Hans S wrote: > > Dumb question: if you only flush the locked entries at fast age if the > > port is locked, then what happens with the existing locked entries if > > the port becomes unlocked before an FDB flush takes place? > > Shouldn't mv88e6xxx_port_set_lock() call mv88e6xxx_atu_locked_entry_flush() > > too? > > That was my first thought too, but the way the flags are handled with the mask etc, does so that > mv88e6xxx_port_set_lock() is called when other flags change. It could be done by the transition > from locked->unlocked by checking if the port is locked already. Why does mv88e6xxx_port_set_lock() get called when other flags change? > On the other hand, the timers will timeout and the entries will be removed anyhow. > > > +static void mv88e6xxx_atu_locked_entry_timer_work(struct atu_locked_entry *ale) > > > > Please find a more adequate name for this function. > > Any suggestions? Not sure. It depends on whether you leave just the logic to delete a locked ATU entry, or also the switchdev FDB_DEL_TO_BRIDGE notifier. In any case, pick a name that reflects what it does. Something with locked_entry_delete() can't be too wrong. > > From the discussion with Ido and Nikolay I get the impression that > > you're not doing the right thing here either, notifying a > > SWITCHDEV_FDB_DEL_TO_BRIDGE from what is effectively the > > SWITCHDEV_FDB_DEL_TO_DEVICE handler (port_fdb_del). > > Hmm, my experience tells me that much is opposite the normal > conventions when dealing with > locked ports, as there was never switchdev notifications from the > driver to the bridge before, but > that is needed to keep ATU and FDB entries in sync. On delete you mean? So the bridge signals switchdev a deletion of a locked FDB entry (as I pointed out, this function gets indirectly called from port_fdb_del), but it won't get deleted until switchdev signals it back, is what you're saying? > > Why is the rtnl_unlock() outside the switch statement but the rtnl_lock() inside? > > Not to mention, the dsa_port_to_bridge_port() call needs to be under rtnl_lock(). > > Just a small optimization as I also have another case of the switch > (only one switch case if > you didn't notice) belonging to the next patch set regarding dynamic > ATU entries. What kind of optimization are you even talking about? Please get rid of coding patterns like this, sorry. > > Please, no "if (chiplock) mutex_lock()" hacks. Just lockdep_assert_held(&chip->reg_lock), > > which serves both for documentation and for validation purposes, ensure > > the lock is always taken at the caller (which in this case is super easy) > > and move on. > > As I am calling the function in if statement checks, it would make > that code more messy, while with > this approach the function can be called from anywhere. I also looked > at having two functions, with > one being a wrapper function taking the lock and calling the other... There are many functions in mv88e6xxx that require the reg_lock to be held, there's nothing new or special here. > > > > > + > > > + if (mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_CTL0, ®)) > > > + goto out; > > > > It would be good to actually propagate the error to the caller and > > "locked" via a pass-by-reference bool pointer argument, not just say > > that I/O errors mean that the port is unlocked. > > Again the wish to be able to call it from if statement checks,. > > > > + reg &= MV88E6XXX_PORT_ASSOC_VECTOR_PAV_MASK; > > > + if (locked) { > > > + reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | > > > + MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1; > > > > I'd suggest aligning these macros vertically. > > They are according to the Linux kernel coding standard wrt indentation afaik. Compare: reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1; with: reg |= MV88E6XXX_PORT_ASSOC_VECTOR_IGNORE_WRONG | MV88E6XXX_PORT_ASSOC_VECTOR_LOCKED_PORT | MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT | MV88E6XXX_PORT_ASSOC_VECTOR_HOLD_AT_1;
next prev parent reply other threads:[~2022-07-06 8:56 UTC|newest] Thread overview: 108+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-24 15:21 [PATCH V3 net-next 0/4] Extend locked port feature with FDB locked flag (MAC-Auth/MAB) Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 15:21 ` [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 15:39 ` Nikolay Aleksandrov 2022-05-24 15:39 ` [Bridge] " Nikolay Aleksandrov 2022-05-24 16:08 ` Hans Schultz 2022-05-24 16:08 ` [Bridge] " Hans Schultz 2022-05-24 16:21 ` Hans Schultz 2022-05-24 16:21 ` [Bridge] " Hans Schultz 2022-05-25 8:06 ` Nikolay Aleksandrov 2022-05-25 8:06 ` [Bridge] " Nikolay Aleksandrov 2022-05-25 8:34 ` Hans Schultz 2022-05-25 8:34 ` [Bridge] " Hans Schultz 2022-05-25 8:38 ` Nikolay Aleksandrov 2022-05-25 8:38 ` [Bridge] " Nikolay Aleksandrov 2022-05-25 9:11 ` Hans Schultz 2022-05-25 9:11 ` [Bridge] " Hans Schultz 2022-05-25 10:18 ` Nikolay Aleksandrov 2022-05-25 10:18 ` [Bridge] " Nikolay Aleksandrov 2022-07-06 18:13 ` Vladimir Oltean 2022-07-06 18:13 ` [Bridge] " Vladimir Oltean 2022-07-06 19:38 ` Nikolay Aleksandrov 2022-07-06 19:38 ` [Bridge] " Nikolay Aleksandrov 2022-07-06 20:21 ` Vladimir Oltean 2022-07-06 20:21 ` [Bridge] " Vladimir Oltean 2022-07-06 21:01 ` Nikolay Aleksandrov 2022-07-06 21:01 ` [Bridge] " Nikolay Aleksandrov 2022-07-07 14:08 ` Nikolay Aleksandrov 2022-07-07 14:08 ` [Bridge] " Nikolay Aleksandrov 2022-07-07 17:15 ` Vladimir Oltean 2022-07-07 17:15 ` [Bridge] " Vladimir Oltean 2022-07-07 17:26 ` Nikolay Aleksandrov 2022-07-07 17:26 ` [Bridge] " Nikolay Aleksandrov 2022-07-08 6:38 ` Hans S 2022-07-08 6:38 ` [Bridge] " Hans S 2022-05-26 14:13 ` Ido Schimmel 2022-05-26 14:13 ` [Bridge] " Ido Schimmel 2022-05-27 8:52 ` Hans Schultz 2022-05-27 8:52 ` [Bridge] " Hans Schultz 2022-05-27 9:58 ` Ido Schimmel 2022-05-27 9:58 ` [Bridge] " Ido Schimmel 2022-05-27 16:00 ` Hans Schultz 2022-05-27 16:00 ` [Bridge] " Hans Schultz 2022-05-31 9:34 ` Hans Schultz 2022-05-31 9:34 ` [Bridge] " Hans Schultz 2022-05-31 14:23 ` Ido Schimmel 2022-05-31 14:23 ` [Bridge] " Ido Schimmel 2022-05-31 15:49 ` Hans Schultz 2022-05-31 15:49 ` [Bridge] " Hans Schultz 2022-06-02 9:17 ` Hans Schultz 2022-06-02 9:17 ` [Bridge] " Hans Schultz 2022-06-02 9:33 ` Nikolay Aleksandrov 2022-06-02 9:33 ` [Bridge] " Nikolay Aleksandrov 2022-06-02 10:17 ` Hans Schultz 2022-06-02 10:17 ` [Bridge] " Hans Schultz 2022-06-02 10:30 ` Nikolay Aleksandrov 2022-06-02 10:30 ` [Bridge] " Nikolay Aleksandrov 2022-06-02 10:39 ` Ido Schimmel 2022-06-02 10:39 ` [Bridge] " Ido Schimmel 2022-06-02 11:36 ` Hans Schultz 2022-06-02 11:36 ` [Bridge] " Hans Schultz 2022-06-02 11:55 ` Ido Schimmel 2022-06-02 11:55 ` [Bridge] " Ido Schimmel 2022-06-02 12:08 ` Hans Schultz 2022-06-02 12:08 ` [Bridge] " Hans Schultz 2022-06-02 12:18 ` Ido Schimmel 2022-06-02 12:18 ` [Bridge] " Ido Schimmel 2022-06-02 12:53 ` Hans S 2022-06-02 13:27 ` Hans S 2022-06-02 13:27 ` [Bridge] " Hans S 2022-05-24 15:21 ` [PATCH V3 net-next 2/4] net: switchdev: add support for offloading of fdb locked flag Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-06-27 16:06 ` Vladimir Oltean 2022-06-27 16:06 ` [Bridge] " Vladimir Oltean 2022-05-24 15:21 ` [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-24 21:36 ` kernel test robot 2022-06-27 12:58 ` Hans S 2022-06-27 12:58 ` [Bridge] " Hans S 2022-06-27 18:05 ` Vladimir Oltean 2022-06-27 18:05 ` [Bridge] " Vladimir Oltean 2022-06-28 12:26 ` Hans S 2022-06-28 12:26 ` [Bridge] " Hans S 2022-07-05 15:05 ` Hans S 2022-07-05 15:05 ` [Bridge] " Hans S 2022-07-06 13:28 ` Vladimir Oltean 2022-07-06 13:28 ` [Bridge] " Vladimir Oltean 2022-07-06 13:48 ` Hans S 2022-07-06 13:48 ` [Bridge] " Hans S 2022-07-06 8:55 ` Vladimir Oltean [this message] 2022-07-06 8:55 ` Vladimir Oltean 2022-07-06 10:12 ` Hans S 2022-07-06 10:12 ` [Bridge] " Hans S 2022-07-06 14:23 ` Hans S 2022-07-06 14:23 ` [Bridge] " Hans S 2022-07-06 14:33 ` Vladimir Oltean 2022-07-06 14:33 ` [Bridge] " Vladimir Oltean 2022-07-06 15:38 ` Hans S 2022-07-06 15:38 ` [Bridge] " Hans S 2022-07-07 6:54 ` Hans S 2022-07-07 6:54 ` [Bridge] " Hans S 2022-05-24 15:21 ` [PATCH V3 net-next 4/4] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests Hans Schultz 2022-05-24 15:21 ` [Bridge] " Hans Schultz 2022-05-26 14:27 ` Ido Schimmel 2022-05-26 14:27 ` [Bridge] " Ido Schimmel 2022-05-27 9:07 ` Hans Schultz 2022-05-27 9:07 ` [Bridge] " Hans Schultz
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220706085559.oyvzijcikivemfkg@skbuf \ --to=olteanv@gmail.com \ --cc=andrew@lunn.ch \ --cc=bridge@lists.linux-foundation.org \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=edumazet@google.com \ --cc=f.fainelli@gmail.com \ --cc=idosch@nvidia.com \ --cc=ivecera@redhat.com \ --cc=jiri@resnulli.us \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=pabeni@redhat.com \ --cc=razor@blackwall.org \ --cc=roopa@nvidia.com \ --cc=schultz.hans+netdev@gmail.com \ --cc=schultz.hans@gmail.com \ --cc=shuah@kernel.org \ --cc=vivien.didelot@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.