Re: [net-next PATCH v3 5/8] net: dsa: realtek: rtl8365mb: add VLAN support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Note: The format of this review has been altered to use a free-form summary of the findings rather than inline code quotes due to recitation restrictions.

commit 2401182ef461b16c2b0ffe98288b5a310381d0f7
Author: Alvin Šipraga <alsi@bang-olufsen.dk>
Subject: net: dsa: realtek: rtl8365mb: add VLAN support

This commit adds VLAN support for the RTL8365MB switch family. It implements a dynamic allocation scheme to manage both the full 4096-entry VLAN4K table and the smaller 32-entry VLANMC table, keeping them synchronized and allocating VLANMC entries primarily for PVID assignment.

When configuring a port's PVID, rtl8365mb_vlan_pvid_port_add() allocates a new VLANMC entry. However, if a user changes a port's PVID, does the driver leak the old PVID's hardware slot? In rtl8365mb_vlan_pvid_port_del(), the driver reads the port's current PVID and returns early if it no longer matches the deleted VLAN. Since the hardware is strictly limited to 31 VLANMC entries, it seems repeatedly changing PVIDs could permanently exhaust the table and result in -E2BIG errors.

In rtl8365mb_vlan_mc_read(), the driver extracts the member, fid, priority, and evid fields, but appears to leave meteridx, policing_en, and priority_en uninitialized. When modifying an existing entry, rtl8365mb_vlan_mc_port_set() passes a partially zero-initialized stack structure to rtl8365mb_vlan_mc_write(). Does this unintentionally clear those fields in the hardware register during write-back?

The commit message states that the driver keeps membership configurations in sync when a corresponding entry exists in VLANMC. However, rtl8365mb_port_vlan_add() only calls the PVID add function if the pvid flag is true. If a port is added as a non-PVID member to a VLAN that already has an active MC entry, it seems the port is only added to the VLAN4K table. Does this leave the VLANMC member mask permanently desynchronized from the actual VLAN membership?

In rtl8365mb_port_vlan_filtering(), the driver iterates through available ports to set the transparent mode. If rtl8365mb_port_set_transparent() fails midway, the function returns immediately. A similar pattern exists in rtl8365mb_vlan_pvid_port_del(), where it modifies the port frame filter and PVID before calling rtl8365mb_vlan_mc_port_set(). Could these early returns leave the switch in an inconsistent state? It looks like they might benefit from proper error unwinding to revert the successful register modifications.

When searching for an empty VLANMC slot, rtl8365mb_vlan_mc_find() checks if the evid is 0x0. If a port is explicitly configured to use VLAN 0, perhaps for 802.1p priority tagging, could this active configuration be mistakenly identified as an empty slot? It seems this might cause a newly configured PVID to silently overwrite the active VLAN 0 configuration. Would checking the member mask alongside the evid help prevent this?

This isn't a bug, but in rtl8365mb_vlan_port_set_framefilter(), the accepted_frame argument is typed as an enum rtl8365mb_frame_ingress, but it is bit-shifted and reassigned to itself. Could this trigger static analyzer warnings about assigning a shifted integer back into an enum variable? Using a separate u32 variable for the shifted mask might be safer.