Re: [PATCH v2 net-next 7/9] net: netdevsim: mimic tc-taprio offload

[Vladimir Oltean]

On Wed, Jun 14, 2023 at 05:06:24PM -0700, Vinicius Costa Gomes wrote:
> > +static int nsim_setup_tc_taprio(struct net_device *dev,
> > +				struct tc_taprio_qopt_offload *offload)
> > +{
> > +	int err = 0;
> > +
> > +	switch (offload->cmd) {
> > +	case TAPRIO_CMD_REPLACE:
> > +	case TAPRIO_CMD_DESTROY:
> > +		break;
> 
> I was thinking about how useful would proper validation of the
> parameters be? Thinking that we could detect "driver API" breakages
> earlier, and we want it documented that the drivers should check for the
> things that it supports.
> 
> Makes sense?

Sorry, I lack imagination as to what the netdevsim driver may check for.
The taprio offload parameters should always be valid, properly speaking,
otherwise the Qdisc wouldn't be passing them on to the driver. At least
that would be the intention. The rest are hardware specific checks for
hardware specific limitations. Here there is no hardware.

The parameters passed to TAPRIO_CMD_REPLACE are:

struct tc_mqprio_qopt_offload mqprio:
	struct tc_mqprio_qopt qopt: validated by taprio_parse_mqprio_opt() for flags 0x2
	u16 mode: always set to TC_MQPRIO_MODE_DCB
	u16 shaper: always set to TC_MQPRIO_SHAPER_DCB
	u32 flags: always set to 0
	u64 min_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	u64 max_rate[TC_QOPT_MAX_QUEUE]: always set to [0,]
	unsigned long preemptible_tcs: always set to 0, because ethtool_dev_mm_supported() returns false

ktime_t base_time: any value is valid

u64 cycle_time: any value is valid

u64 cycle_time_extension: any value <= cycle_time is valid. According to 802.1Q
			  "Q.5 CycleTimeExtension variables", it's the maximum
			  amount by which the penultimate cycle can be extended
			  to avoid a very short cycle upon a ConfigChange event.
			  But if CycleTimeExtension is larger than one CycleTime,
			  then we're not even talking about the penultimate cycle
			  anymore, but about ones previous to that?! Maybe this
			  should be limited to 0 <= cycle_time_extension <= cycle_time
			  by taprio, certainly not by offloading drivers.

u32 max_sdu[TC_MAX_QUEUE]: limited to a value <= dev->max_mtu by taprio

size_t num_entries: any value is valid

struct tc_taprio_sched_entry entries[]:
	u8 command: will be either one of: TC_TAPRIO_CMD_SET_GATES, TC_TAPRIO_CMD_SET_AND_HOLD
		    or TC_TAPRIO_CMD_SET_AND_RELEASE. However 802.1Q "Table 8-7—Gate operations"
		    says "If frame preemption is not supported or not enabled (preemptionActive is
		    FALSE), this operation behaves the same as SetGateStates.". So I
		    see no reason to enforce any restriction here either?

	u32 gate_mask: technically can have bits set, which correspond
		       to traffic classes larger than dev->num_tc.
		       Taprio can enforce this, so I wouldn't see
		       drivers beginning to feel paranoid about it.
		       Actually I had a patch about this:
		       https://patchwork.kernel.org/project/netdevbpf/patch/20230130173145.475943-15-vladimir.oltean@xxxxxxx/
		       but I decided to drop it because I didn't have
		       any strong case for it.
	u32 interval: any value is valid. If the sum of entry intervals
		      is less than the cycle_time, again that's taprio's
		      problem to check for, in its netlink attribute
		      validation method rather than offloading drivers.

There are no parameters given to TAPRIO_CMD_DESTROY.