On 3/28/25 17:53, Joseph Huang wrote:

On 3/27/2025 6:52 PM, Nikolay Aleksandrov wrote:

quoted

On 3/27/25 00:38, Joseph Huang wrote:

quoted

On 3/21/2025 4:19 AM, Nikolay Aleksandrov wrote:

quoted

quoted

@@ -516,11 +513,14 @@ static void br_switchdev_mdb_complete(struct 

net_device *dev, int err, void *pri
           pp = &p->next) {
          if (p->key.port != port)
              continue;
-        p->flags |= MDB_PG_FLAGS_OFFLOAD;
+
+        if (err)
+            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
+        else
+            p->flags |= MDB_PG_FLAGS_OFFLOAD;

These two should be mutually exclusive, either it's offloaded or it 
failed an offload,
shouldn't be possible to have both set. I'd recommend adding some 
helper that takes
care of that.

It is true that these two are mutually exclusive, but strictly 
speaking there are four types of entries:

1. Entries which are not offload-able (i.e., the ports are not backed 
by switchdev)
2. Entries which are being offloaded, but results yet unknown
3. Entries which are successfully offloaded, and
4. Entries which failed to be offloaded

Even if we ignore the ones which are being offloaded (type 2 is 
transient), we still need two flags, otherwise we won't be able to 
tell type 1 from type 4 entries.

If we need two flags anyway, having separate flags for type 3 and 
type 4 simplifies the logic.

Or did I misunderstood your comments?

Thanks,
Joseph

I think you misunderstood me, I don't mind having the two flags. :)

Got it. Thanks.

quoted

My point is that they must be managed correctly and shouldn't be allowed
to be set simultaneously.

Cheers,
  Nik

Helper function like this?

+static void set_mdb_pg_offload_flags(bool err, u8 *flags)
+{
+    *flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
+    *flags |= (err ? MDB_PG_FLAGS_OFFLOAD_FAILED : MDB_PG_FLAGS_OFFLOAD);
+}

This could work, but I have to see how it aligns with the rest of the
code to be able to answer well. Also why not just pass the pg?
Please also choose another helper name, e.g.
br_multicast_set_pg_offload_flags() or something in these lines. You
can check br_private.h for other helpers to get an idea.

and then from the call site

-        p->flags |= MDB_PG_FLAGS_OFFLOAD;
+        set_mdb_pg_offload_flags(err, &p->flags);

?

Or simply clearing the flags in-line:

-        p->flags |= MDB_PG_FLAGS_OFFLOAD;
+        p->flags &= ~(MDB_PG_FLAGS_OFFLOAD | MDB_PG_FLAGS_OFFLOAD_FAILED);
+
+        if (err)
+            p->flags |= MDB_PG_FLAGS_OFFLOAD_FAILED;
+        else
+            p->flags |= MDB_PG_FLAGS_OFFLOAD;

?

I'd prefer using a  helper. Thanks.

Thanks,
Joseph

Cheers,
  Nik