Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: add support for credit based shaper

[Paolo Abeni]

From: AI Reviewer <ai@xxxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
net: dsa: mv88e6xxx: add support for credit based shaper

This commit adds support for credit-based shaping (802.1Qav) to several 
MV88E6xxx chip families (6352, 6390, 6393). It implements the UAPI 
structures allowing configuration of the idleslope and hicredit parameters 
per queue.

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 1bd0615ea95b1..7ab0733f49154 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -7170,6 +7197,101 @@ static int mv88e6xxx_crosschip_lag_leave(struct dsa_switch *ds, int sw_index,
[ ... ]
> +	if (!cbs->enable) {
[ ... ]
> +		goto unlock;
> +	}
> +
> +	hi_limit = cbs->hicredit & qav->hi_limit_mask;

[Severity: High]
The tc_cbs_qopt_offload UAPI structure specifies hicredit in bytes, but this
appears to pass the value directly to the hardware register without scaling.
If standard tooling passes a correct byte value, will this result in the
hardware allocating significantly more credits than intended due to hardware
block size multipliers?

> +	err = avb_ops->port_qav_write(chip, port, hilimit_reg, hi_limit);
> +	if (err)
> +		goto unlock;
> +
> +	err = avb_ops->port_qav_write(chip, port, rate_reg, rate);
> +	if (err)
> +		goto unlock;
> +
> +	err = ops->port_set_scheduling_mode(chip, port,
> +					    chip->info->num_tx_queues - 1);
> +	if (err) {
> +		avb_ops->port_qav_write(chip, port, rate_reg, 0);
> +		goto unlock;

[Severity: Medium]
If ops->port_set_scheduling_mode() fails, the rate register is cleared, but
the queue_bit is not removed from chip->ports[port].cbs_active_queues.
Could this cause the software state to incorrectly believe the queue is
still active, preventing the port from returning to standard scheduling
mode when other queues are later disabled?

> +	}
> +	chip->ports[port].cbs_active_queues |= queue_bit;
> +
> +unlock:
> +	mv88e6xxx_reg_unlock(chip);
> +
> +	return err;
> +}
[ ... ]
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ea1fab71968a0..cadbc664608ff 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1323,6 +1323,44 @@ int mv88e6097_port_egress_rate_limiting(struct mv88e6xxx_chip *chip, int port)
[ ... ]
> +int mv88e6390_port_set_scheduling_mode(struct mv88e6xxx_chip *chip, int port,
> +				       u8 mode)
> +{
> +	u16 reg;
> +
> +	if (mode > MV88E6390_PORT_QUEUE_CTL_SCHEDULE_MASK)
> +		return -EINVAL;
> +
> +	reg = MV88E6390_PORT_QUEUE_CTL_UPDATE |
> +	      (MV88E6390_PORT_QUEUE_CTL_SCHEDULE <<
> +	       MV88E6390_PORT_QUEUE_CTL_PTR_SHIFT) |
> +	      (mode & MV88E6390_PORT_QUEUE_CTL_SCHEDULE_MASK);
> +
> +	return mv88e6xxx_port_write(chip, port, MV88E6390_PORT_QUEUE_CTL,
> +				    reg);

[Severity: Medium]
This function sets the MV88E6390_PORT_QUEUE_CTL_UPDATE bit but returns
immediately. Should there be a wait or poll for the hardware to clear
this bit before returning, to prevent sequential configuration changes
from silently dropping updates while the hardware is still busy?

> +}
-- 
This is an AI-generated review.