> +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.
> /* 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.
Andrew
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
> 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.
That is not good. Please take a look around and see if any other
driver offloads TBF, and what they do with burst.
> 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.
We don't really want a custom application, since we want users to use
TC to set this up.
Looking at the 6390 datasheet, Queue Counter Registers, mode 8 gives
the number of egress buffers for a port. You could validate that the
switch has at least the requested number of buffers assigned to the
port? There is quite a bit you can configure, so maybe there is a way
to influence the number of buffers, so you can actually implement the
burst parameter?
Andrew
On 6/9/23 19:16, Andrew Lunn wrote:
>> 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.
>
> That is not good. Please take a look around and see if any other
> driver offloads TBF, and what they do with burst.
>
>> 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.
>
> We don't really want a custom application, since we want users to use
> TC to set this up.
>
> Looking at the 6390 datasheet, Queue Counter Registers, mode 8 gives
> the number of egress buffers for a port. You could validate that the
> switch has at least the requested number of buffers assigned to the
> port? There is quite a bit you can configure, so maybe there is a way
> to influence the number of buffers, so you can actually implement the
> burst parameter?
Thanks for the pointers. I will check the egress buffers configuration and see
if I can come up with something better
>
> Andrew
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com