Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Unsafe conversion of atomic command register triggers into multiple sequential Read-Modify-Write (RMW) operations.
- [High] Hardware staging registers for FDB and VLAN data will retain stale or garbage bits due to the switch from full-register overwrites to field-based RMWs.
- [High] `mt7530_port_enable()` incorrectly applies `PCR_MATRIX_MASK` to an unshifted port matrix value, isolating the port and dropping all traffic.
- [High] Missing `reg_fields` initialization for `ID_EN7528` leads to a NULL pointer dereference during port mirroring.
- [High] mt7530_lib_port_bridge_flags() disables MAC address learning when it is supposed to be enabled, and vice versa.
- [High] mt7530_lib_fdb_read() swaps the last two bytes of the MAC address when reading FDB entries.
- [High] Unchecked regmap_field_read() failures lead to the use of uninitialized stack variables, potentially leaking stack memory to userspace or corrupting hardware state.
- [Medium] `mt7530_lib_mib_reset()` inadvertently disables the hardware MIB counters for octets and breaks the flush sequence by writing 0 instead of 1.
--
--- Patch [9]: [PATCH net-next v20 09/10] net: dsa: mt7530: generalize and move common function to lib module ---
commit 302309065c4dc5c4e0f17f164c9e39732602b582
Author: Christian Marangi <[email protected]>
net: dsa: mt7530: generalize and move common function to lib module
This commit moves common functions for the MT7530 switch into a separate
library module to prepare for Airoha support. It refactors register
accesses to use the regmap reg_field API instead of raw read/writes. A
new library struct is introduced to house the shared dependencies and
register fields.
> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> new file mode 100644
> index 0000000000000..01ed941efc892
> --- /dev/null
> +++ b/drivers/net/dsa/mt7530-lib.c
> @@ -0,0 +1,1106 @@
[ ... ]
> +void mt7530_lib_mib_reset(struct mt7530_lib_priv *priv)
> +{
> + regmap_field_write(priv->fields[MT7530_CCR_MIB_ENABLE], 0);
> + regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_GOOD], 0);
> + regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_BAD], 0);
> + regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_GOOD], 0);
> + regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_BAD], 0);
[Severity: Medium]
Does this correctly clear the counters? The original code wrote a 1 to flush
and clear the counters. Writing 0 to a write-1-to-clear field might fail to
reset them.
[ ... ]
> +int mt7530_lib_fdb_cmd(struct mt7530_lib_priv *priv, enum mt7530_fdb_cmd cmd,
> + u32 mat, u32 *rsp)
> +{
> + u32 val;
> + int ret;
> +
> + ret = regmap_field_write(priv->fields[MT7530_ATC_MAT], mat);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_write(priv->fields[MT7530_ATC_CMD], cmd);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_write(priv->fields[MT7530_ATC_BUSY], 1);
[Severity: High]
Can splitting the FDB command execution into multiple read-modify-writes
corrupt the MAC table operations? The previous implementation constructed the
32-bit value and wrote it atomically, preventing intermediate hardware states.
[ ... ]
> +static void mt7530_lib_fdb_write(struct mt7530_lib_priv *priv, u16 vid,
> + u8 port_mask, const u8 *mac,
> + u8 aging, u8 type)
> +{
> + regmap_field_write(priv->fields[MT7530_ATWD_CVID], vid);
> + regmap_field_write(priv->fields[MT7530_ATWD_IVL], 1);
[Severity: High]
Could this approach leak uninitialized or stale data into the FDB table?
Writing fields individually performs a read-modify-write, preserving undefined
bits from prior operations, whereas the old code explicitly cleared
unspecified bits by writing fully initialized arrays directly.
[ ... ]
> +void mt7530_lib_fdb_read(struct mt7530_lib_priv *priv, struct mt7530_fdb *fdb)
> +{
> + u32 val;
> +
> + regmap_field_read(priv->fields[MT7530_ATRD_CVID], &val);
> + fdb->vid = val;
[Severity: High]
If regmap_field_read fails, could we end up assigning uninitialized stack
memory from val to the fdb fields? This might leak kernel stack data to
userspace during an FDB dump.
[ ... ]
> +int mt7530_lib_port_mirror_add(struct mt7530_lib_priv *priv, int port,
> + struct dsa_mall_mirror_tc_entry *mirror,
> + bool ingress, struct netlink_ext_ack *extack)
> +{
> + int monitor_port;
> + u32 val;
> +
> + /* Check for existent entry */
> + if ((ingress ? priv->mirror_rx : priv->mirror_tx) & BIT(port))
> + return -EEXIST;
> +
> + regmap_field_read(priv->fields[MT7530_MIRROR_EN], &val);
[Severity: High]
Is it possible for priv->fields to lack an initialization here? The
ID_EN7528 entry in mt753x_table appears to be missing its reg_fields
setup, which could lead to a NULL pointer dereference.
[Severity: High]
Could a failure in regmap_field_read leave val and monitor_port
uninitialized? Using these uninitialized values in the subsequent condition
might cause unpredictable branch behavior.
[ ... ]
> +int mt7530_lib_port_bridge_flags(struct mt7530_lib_priv *priv, int port,
> + struct switchdev_brport_flags flags,
> + struct netlink_ext_ack *extack)
> +{
> + struct dsa_switch *ds = priv->ds;
> +
> + if (flags.mask & BR_LEARNING)
> + regmap_fields_write(priv->fields[MT7530_PSC_SA_DIR],
> + port, !!(flags.val & BR_LEARNING));
[Severity: High]
Does this invert the learning logic? The hardware bit SA_DIS disables
learning, so passing the boolean value of BR_LEARNING directly might
disable learning when it is supposed to be enabled, and vice versa.
[ ... ]
> +static int mt7530_vlan_cmd(struct mt7530_lib_priv *priv, enum mt7530_vlan_cmd cmd,
> + u16 vid)
> +{
> + u32 val;
> + int ret;
> +
> + ret = regmap_field_write(priv->fields[MT7530_VTCR_FUNC], cmd);
> + if (ret)
> + return ret;
> +
> + ret = regmap_field_write(priv->fields[MT7530_VTCR_VID], vid);
[Severity: High]
Will breaking this command register access into multiple read-modify-write
operations cause issues with the hardware state machine? The original code
wrote the entire VTCR register in a single atomic operation.
[ ... ]
> +static void mt7530_hw_vlan_add(struct mt7530_lib_priv *priv,
> + struct mt7530_hw_vlan_entry *entry)
> +{
> + struct dsa_port *dp = dsa_to_port(priv->ds, entry->port);
> + u8 new_members;
> + u32 val;
> +
> + new_members = entry->old_members | BIT(entry->port);
> +
> + /* Validate the entry with independent learning, create egress tag per
> + * VLAN and joining the port as one of the port members.
> + */
> + regmap_field_write(priv->fields[MT7530_VAWD_IVL_MAC], 1);
[Severity: High]
Might this leave stale or garbage bits in the VAWD1 register? The original
logic performed a full 32-bit register overwrite, explicitly clearing
unspecified bits like EG_CON. Using sequential regmap_field_write operations
performs read-modify-writes, which retains any previous unmapped bits.
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index d01772cfe3a2d..f313a7a7bfef1 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
[ ... ]
> @@ -24,10 +24,159 @@
> #include <net/pkt_cls.h>
>
> #include "mt7530.h"
> +#include "mt7530-lib.h"
>
> #define MT7530_STATS_POLL_INTERVAL (1 * HZ)
> #define MT7530_STATS_RATE_LIMIT (HZ / 10)
>
> +static const struct mt7530_reg_field mt753x_fields[] = {
[ ... ]
> + { MT7530_ATRD_ENT_STATUS, REG_FIELD(MT7530_ATRD, 2, 3), },
> + { MT7530_ATRD_MAC_BYTE_5, REG_FIELD(MT7530_TSRA2, 24, 31), },
> + { MT7530_ATRD_MAC_BYTE_4, REG_FIELD(MT7530_TSRA2, 16, 23), },
[Severity: High]
Are the bit ranges for MAC_BYTE_4 and MAC_BYTE_5 swapped here? The original
logic mapped MAC_BYTE_4 to bits 24-31 and MAC_BYTE_5 to bits 16-23 in the
TSRA2 register, causing corrupted MAC addresses on FDB read.
[ ... ]
> @@ -1373,7 +1141,7 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
> if (dsa_port_is_user(dp)) {
> struct dsa_port *cpu_dp = dp->cpu_dp;
>
> - priv->ports[port].pm |= PCR_MATRIX(BIT(cpu_dp->index));
> + priv->ports[port].pm |= BIT(cpu_dp->index);
> }
> priv->ports[port].enable = true;
> mt7530_rmw(priv, MT7530_PCR_P(port), PCR_MATRIX_MASK,
> priv->ports[port].pm);
[Severity: High]
Does this effectively isolate the port by dropping all traffic? The bitmask
in pm is now constructed without the PCR_MATRIX() shift, but it is passed
directly into mt7530_rmw() where PCR_MATRIX_MASK is applied. Since the
unshifted bits fall outside the mask, this will write zero to the matrix
field.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.