Re: [net-next PATCH v3 4/8] net: dsa: realtek: rtl8365mb: add table lookup interface

[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.
---
net: dsa: realtek: rtl8365mb: add table lookup interface

This commit adds a generic table lookup interface for the RTL8365MB switch
family via rtl8365mb_table_query(), centralizing access to the internal
ACL, CVLAN, L2 and IGMP tables behind a single helper.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_table.c b/drivers/net/dsa/realtek/rtl8365mb_table.c
> new file mode 100644
> index 000000000000..df312769b0be
> --- /dev/null
> +++ b/drivers/net/dsa/realtek/rtl8365mb_table.c

[ ... ]

> +int rtl8365mb_table_query(struct realtek_priv *priv,
> +			  enum rtl8365mb_table table,
> +			  enum rtl8365mb_table_op op, u16 *addr,
> +			  enum rtl8365mb_table_l2_method method,
> +			  u16 port, u16 *data, size_t size)
> +{
> +	bool addr_as_input = true;
> +	bool write_data = false;
> +	int ret = 0;
> +	u32 cmd;
> +	u32 val;
> +	u32 hit;
> +
> +	/* Prepare target table and operation (read or write) */

[Medium]
Is the size argument validated anywhere in this function?  The kerneldoc
for rtl8365mb_table_query() in rtl8365mb_table.h states:

    * @size must match the size of the target table entry, expressed in 16-bit
    * words. This function only validates that it is non-zero and fits in the
    * available register space.

but no check of the form

    if (!size || size > RTL8365MB_TABLE_ENTRY_MAX_SIZE)
            return -EINVAL;

appears to exist.  All in-tree callers introduced by subsequent patches
pass compile-time-constant sizes well within bounds, so nothing breaks
today, but should the kerneldoc be adjusted to make the caller
responsible, or should the validation it promises be added at the top of
the function?

[ ... ]

> +	/* Write entry data if writing to the table (or L2_METHOD_MAC) */
> +	if (write_data) {
> +		/* bulk write data up to 9th byte */
> +		ret = regmap_bulk_write(priv->map_nolock,
> +					RTL8365MB_TABLE_WRITE_BASE,
> +					data,
> +					min_t(size_t, size,
> +					      RTL8365MB_TABLE_ENTRY_MAX_SIZE -
> +						      1));

[Low]
Does the comment "bulk write data up to 9th byte" match the code?  The
data parameter is declared as u16 *, the kerneldoc defines @size as
"size of @data in 16-bit words", and regmap_bulk_write() counts in
register-sized units (2 bytes here).  The min_t() clamp of
RTL8365MB_TABLE_ENTRY_MAX_SIZE - 1 is 9 words (18 bytes), not 9 bytes.

> +		if (ret)
> +			goto out;
> +
> +		/* 10th register uses only 4 less significant bits */

[Low]
Should "4 less significant bits" read "4 least significant bits" here?
RTL8365MB_TABLE_10TH_DATA_MASK is GENMASK(3, 0), i.e. the four least
significant bits.

> +		if (size == RTL8365MB_TABLE_ENTRY_MAX_SIZE) {
> +			val = FIELD_PREP(RTL8365MB_TABLE_10TH_DATA_MASK,
> +					 data[size - 1]);
> +			ret = regmap_update_bits(priv->map_nolock,
> +						 RTL8365MB_TABLE_WRITE_10TH_REG,
> +						 RTL8365MB_TABLE_10TH_DATA_MASK,
> +						 val);
> +		}
> +
> +		if (ret)
> +			goto out;
> +	}

[ ... ]

> +	/* Finally, get the table entry if we were reading */
> +	if (op == RTL8365MB_TABLE_OP_READ) {
> +		ret = regmap_bulk_read(priv->map_nolock,
> +				       RTL8365MB_TABLE_READ_BASE,
> +				       data, size);
> +
> +		/* For the biggest table entries, the uppermost table
> +		 * entry register has space for only one nibble. Mask
> +		 * out the remainder bits. Empirically I saw nothing
> +		 * wrong with omitting this mask, but it may prevent
> +		 * unwanted behaviour. FYI.
> +		 */
> +		if (size == RTL8365MB_TABLE_ENTRY_MAX_SIZE) {
> +			val = FIELD_GET(RTL8365MB_TABLE_10TH_DATA_MASK,
> +					data[size - 1]);
> +			data[size - 1] = val;
> +		}
> +	}

[Low, Medium]
Should the return value of regmap_bulk_read() be checked before falling
through to the data[size - 1] masking block?  Every other regmap call in
this function bails out with "if (ret) goto out;".  The ret is still
propagated to the caller via the out: label and the kerneldoc notes that
@data is undefined on error, so there is no functional bug with the
current callers, but the asymmetry stands out.

Relatedly, since size is not validated up front, regmap_bulk_read() is
called with the raw size here while the write path used
min_t(size_t, size, RTL8365MB_TABLE_ENTRY_MAX_SIZE - 1).  If a future
caller passed size > RTL8365MB_TABLE_ENTRY_MAX_SIZE, reads would spill
past the RTL8365MB_TABLE_READ_BASE register window and into whatever
follows, and could write past the caller's buffer too.  Would an
explicit bounds check make the contract enforceable?

> +
> +out:
> +	mutex_unlock(&priv->map_lock);
> +
> +	return ret;
> +}