Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: implement egress tbf qdisc for 6393x family

[Alexis Lothoré]

Hi Andrew, thanks for the review,

On 6/9/23 16:53, Andrew Lunn wrote:
>> +int mv88e6393x_tbf_add(struct mv88e6xxx_chip *chip, int port,
>> +		       struct tc_tbf_qopt_offload_replace_params *replace_params)
>> +{
>> +	int rate_kbps = DIV_ROUND_UP(replace_params->rate.rate_bytes_ps * 8, 1000);
>> +	int overhead = DIV_ROUND_UP(replace_params->rate.overhead, 4);
>> +	int rate_step, decrement_rate, err;
>> +	u16 val;
>> +
>> +	if (rate_kbps < MV88E6393X_PORT_EGRESS_RATE_MIN_KBPS ||
>> +	    rate_kbps >= MV88E6393X_PORT_EGRESS_RATE_MAX_KBPS)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (replace_params->rate.overhead > MV88E6393X_PORT_EGRESS_MAX_OVERHEAD)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* Switch supports only max rate configuration. There is no
>> +	 * configurable burst/max size nor latency.
> 
> Can you return -EOPNOTSUPP if these values are not 0? That should make
> it clear to the user they are not supported.

Yes, I can do that (or maybe -EINVAL to match Vladimir's comment ?). I think
it's worth mentioning that I encountered an issue regarding those values during
tests: I use tc program to set the tbf, and I observed that tc does not even
reach kernel to set the qdisc if we pass no burst/latency value OR if we set it
to 0. So tc enforces right on userspace side non-zero value for those
parameters, and I have passed random values and ignored them on kernel side.
Checking available doc about tc-tbf makes me feel like that indeed a TBF qdisc
command without burst or latency value makes no sense, except my use case can
not have such values. That's what I struggled a bit to find a proper qdisc to
match hardware cap. I may fallback to a custom netlink program to improve testing.
> 
>>  /* Offset 0x09: Egress Rate Control */
>> -#define MV88E6XXX_PORT_EGRESS_RATE_CTL1		0x09
>> +#define MV88E6XXX_PORT_EGRESS_RATE_CTL1				0x09
>> +#define MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_64_KBPS		0x1E84
>> +#define MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_1_MBPS		0x01F4
>> +#define MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_10_MBPS		0x0032
>> +#define MV88E6XXX_PORT_EGRESS_RATE_CTL1_STEP_100_MBPS		0x0005
>> +#define MV88E6XXXw_PORT_EGRESS_RATE_CTL1_FRAME_OVERHEAD_SHIFT	8
> 
> Are they above values specific to the 6393? Or will they also work for
> other families? You use the MV88E6XXX prefix which means they should
> be generic across all devices.

I have no idea about EGRESS_RATE_CTL1 and EGRESSE_RATE_CTL2 registers layout or
features for other switches supported in mv88e6xxx, and it is likely risky to
assume it is identical, so indeed I will rename those defines to make them
specific to 6393 (+ nasty typo in
MV88E6XXXw_PORT_EGRESS_RATE_CTL1_FRAME_OVERHEAD_SHIFT)

Thanks,
Alexis

> 
> 	Andrew

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com