Re: [PATCH v3 net-next 3/9] net: mscc: ocelot: serialize access to the MAC table

On Fri, Oct 22, 2021 at 09:04:47PM +0200, Andrew Lunn wrote:

On Fri, Oct 22, 2021 at 09:00:52PM +0300, Vladimir Oltean wrote:

quoted

On Fri, Oct 22, 2021 at 10:34:04AM -0700, Florian Fainelli wrote:

quoted

On 10/22/21 10:27 AM, Vladimir Oltean wrote:

quoted

DSA would like to remove the rtnl_lock from its
SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE handlers, and the felix driver uses
the same MAC table functions as ocelot.

This means that the MAC table functions will no longer be implicitly
serialized with respect to each other by the rtnl_mutex, we need to add
a dedicated lock in ocelot for the non-atomic operations of selecting a
MAC table row, reading/writing what we want and polling for completion.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 53 +++++++++++++++++++++++-------
 include/soc/mscc/ocelot.h          |  3 ++
 2 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 4e5ae687d2e2..72925529b27c 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c

@@ -20,11 +20,13 @@ struct ocelot_mact_entry {
 	enum macaccess_entry_type type;
 };
 
+/* Must be called with &ocelot->mact_lock held */

I don't know if the sparse annotations: __must_hold() would work here,
but if they do, they serve as both comment and static verification,
might as well use them?

I've never come across that annotation before, thanks.
I'll fix this and the other issue and resend once the build tests for
this series finish.

If sparse cannot figure it out, mv88e6xxx has:

static void assert_reg_lock(struct mv88e6xxx_chip *chip)
{
        if (unlikely(!mutex_is_locked(&chip->reg_lock))) {
                dev_err(chip->dev, "Switch registers lock not held!\n");
                dump_stack();
        }
}

which is a bit heavier in weight, but the MDIO bus transaction will
dominate the time for such operations, not checking a mutex.

Yes, and then there's also lockdep_assert_held. I knew about those.

Truth be told, I thought sparse would be smarter. But I tested with this
program:

#include <stdio.h>

# define __must_hold(x)	__attribute__((context(x,1,1)))
# define __acquires(x)	__attribute__((context(x,0,1)))
# define __releases(x)	__attribute__((context(x,1,0)))
# define __acquire(x)	__context__(x,1)
# define __release(x)	__context__(x,-1)

static void __acquires(a) lock(int a)
{
	__acquire(a);
}

static void __releases(a) unlock(int a)
{
	__release(a);
}

static void __must_hold(a) fn(int a)
{
	printf("%s: %d\n", __func__, a);
}

int main(int argc, char **argv)
{
	int a = 0;

	unlock(a);
	fn(a);
	lock(a);

	return 0;
}

and it doesn't see any problem whatsoever. It's only good to detect
context imbalances that aren't annotated.

Then I noticed Johannes Berg's sparse commit 2479d0f7819b ("Revert the
context tracking code"), and this discussion:
https://www.spinics.net/lists/linux-sparse/msg03934.html

so.... yeah. It's just about as useful as a very pretentious comment.

I don't know, I can send another version that replaces the __must_hold
with something else, but I'd rather not sprinkle around lockdep_assert_held()
calls the same I did with a static analyzer attribute like it's nothing...