Re: [PATCH net-next v8 1/2] net: dsa: qca8k: let regmap serialize register access

[Stanislaw Pal <kuncy7@gmail.com>]

On Fri, Nov 06, 2026 at 07:07:19PM +0100, Christian Marangi wrote:
> Both mgmt and mdio operation locks at the start and unlock at the end of
> the read/write or rmw operation.
> Using the regmap lock seems redundant and this is why I disabled
> originally.
> I would like to have more explaination for the
> "regmap formats the register and parses the value in the
> map's shared work_buf, outside of the callbacks, so two concurrent
> users can swap or corrupt each other's register and value."
> case.

The qca8k locks cover what runs inside the callbacks, but since the
map uses .read/.write, part of every access runs in the regmap core
before and after them, in map->work_buf, which there is one of per
map:

- regmap_read() goes through _regmap_bus_read() and _regmap_raw_read():
  format_reg() writes the register into map->work_buf, ->read() is
  called with reg_buf = map->work_buf and val_buf = work_buf +
  reg_bytes, and after it returns parse_val() reads the value back
  from work_buf + reg_bytes.
- regmap_write() goes through _regmap_bus_raw_write() and
  _regmap_raw_write_impl() (no format_write for 16-bit registers with
  32-bit values): format_val() puts the value into map->work_buf
  behind the register bytes, format_reg() puts the register in front
  of it, and ->write() is called on map->work_buf as one buffer.

qca8k_bulk_read() takes reg = *(u16 *)reg_buf before it locks
anything, and the value it stores is parsed after it has unlocked;
qca8k_bulk_write() likewise takes the register and the value out of
work_buf before its lock. With .disable_locking nothing orders two
callers around those steps, so caller A can format its register,
caller B format its own over it, and A's callback then reads or
writes B's register; or A's value is overwritten in work_buf before A
parses it.

Two callers that can meet there without rtnl: phylink's resolve work,
which holds only phylink's own mutexes when it calls
qca8k_phylink_mac_link_up() -> qca8k_write(PORT_STATUS), and the LEDs,
e.g. activating the netdev trigger, which calls ->hw_control_get() ->
regmap_read() under the trigger's locks only. ethtool -S and the bridge
callbacks hold rtnl, but the two above do not.

On Fri, Nov 06, 2026 at 02:38:37PM +0100, Andrew Lunn wrote:
> Do any of these operations which don't take RTNL do an open coded
> read/modify/write? Does the regmap read/modify/write operations hold
> the regmap mutex over the entire operation?

No open-coded RMW that I found: the phylink callbacks do plain writes
(and regmap_set_bits/clear_bits on link down), the LEDs use
regmap_update_bits() or plain reads. Whether regmap_update_bits() holds
a lock over the whole operation depends on the path. With REGCACHE_NONE
every register is volatile, so it goes to qca8k_regmap_update_bits().
Over MDIO that holds the bus lock across the read and the write. Over
the management frames it does not: qca8k_regmap_update_bits_eth() is
qca8k_read_eth() followed by qca8k_write_eth(), each taking
mgmt_eth_data->mutex on its own, so two updates of the same register
can interleave between the read and the write and one loses its bits.
The LED control registers hold the rule bits of ports 0-3 and port 4
in one register, and the LED callbacks run without rtnl, so two LEDs
can meet there. With the map lock enabled, regmap_update_bits_base()
holds it around the whole update, which closes that one too.

To be clear about where this comes from: I found it reading the regmap
path, not from corrupted registers on a board, and I have not seen it
happen. If that changes the case for net-next versus a lighter fix, I'm
happy to rework the commit message, or the patch, whichever you prefer.

Stanislaw